Event driven script orchestrator #1058

sburmanoctopus · 2025-01-21T01:53:44Z

[sc-101635]

Background

Currently we only allow calling into tentacle client to run a script via the TentacleClient.ExecuteScript method. Although extremely simple it is not re-entrant. When we talk about re-entrancy we are considering the case the client gets turned off and on and wants to continue on. The deficiencies are:

when using SSv1 could start a script a second time
it will re-fetch all logs, rather than where or close to the point we left off from.
it doesn't work well from an event driven orchestrator.

Results

This PR was a continuation of the spike started here

In summary the changes are:

The ObservingOrchestrator is no longer abstract and becomes a thing that "just" knows how to run the life cycle of the script.
A new IScriptExecutor interface is created which allows driving script life cycle
- This is now used by the ObservingOrchestrator
- Could be used by an event driven orchestrator.
The ScriptService Orchestrators which inherited from ObservingOrchestrator, now implement IScriptExecutor. They now focus on implementing the main stages of script execution following a single interface.
- They are also responsible from converting from types the client of the client might use to the types that go over the wire.
- They are responsible for their own "quirks" providing a straight forward interface to interact with.
A ScriptExecutor is created which:
- On start script determines which of the other ScriptServices to use.
- On the other methods uses the given CommandContext to determine which script service to use.
- In doing so it can deal with calling the correct service.
The ObservingOrchestrator is given a ScriptExecutor
- Which is called by TentacleClient.ExecuteScript
- In this way the existing tests are able to exercise the ScriptExecutor which is what an event driven orchestrator would call.
- This reduces what an event driven orchestrator needs to implement to just what is done in ObservingOrchestrator

# Conflicts: # source/Octopus.Tentacle.Client/Scripts/KubernetesScriptServiceV1AlphaOrchestrator.cs # source/Octopus.Tentacle.Client/Scripts/ScriptOrchestratorFactory.cs # source/Octopus.Tentacle.Client/TentacleClient.cs

LukeButters · 2025-01-21T02:39:13Z

source/Octopus.Tentacle.Client/EventDriven/CommandContext.cs

+
+namespace Octopus.Tentacle.Client.EventDriven
+{
+    public class CommandContext


nit: consider documenting what this class is for.

Good idea. Done

LukeButters · 2025-01-21T02:40:35Z

source/Octopus.Tentacle.Client/Scripts/IScriptExecutor.cs

+{
+    public interface IScriptExecutor 
+    {
+        Task<ScriptOperationExecutionResult> StartScript(ExecuteScriptCommand command,


It might make sense to document that the resulting CommandContext should be used for the next call.

LukeButters · 2025-01-21T02:41:48Z

source/Octopus.Tentacle.Client/Scripts/IScriptExecutor.cs

+            StartScriptIsBeingReAttempted startScriptIsBeingReAttempted,
+            CancellationToken scriptExecutionCancellationToken);
+
+        Task<ScriptOperationExecutionResult> GetStatus(CommandContext commandContext, CancellationToken scriptExecutionCancellationToken);


nit: I think I actually preferred the tuple because the responses where not something that are really coupled AND I think it improved readability. ie it was clear each method was returning a CommandContext which might be enough to hint to the user to use that CommandContext for the next call.

nit: I think I actually preferred the tuple because the responses where not something that are really coupled
🤔 I feel that the fact we return both implies that they are related.

I think it improved readability. ie it was clear each method was returning a CommandContext which might be enough to hint to the user to use that CommandContext for the next call.

🤔 I would have thought the property named ContextForNextCommand would be good enough. Plus, adding the documentation from the previous comment should be more than enough.

I personally don't like tuples at all, and feel this is much easier to understand. For now, let's leave it the way it is, and if we find it unhelpful we can address it then.

LukeButters · 2025-01-21T02:49:31Z

source/Octopus.Tentacle.Client/Scripts/ScriptServiceVersion.cs

@@ -1,6 +1,7 @@
 namespace Octopus.Tentacle.Client.Scripts
 {
-    record ScriptServiceVersion(string Value)
+    // TODO should not be public.


I assume we will address this in a future

This will be addressed as part of the work to use a serialized version of command context.

But yes, we shouldn't be leaving these TODOs.

LukeButters · 2025-01-21T02:51:25Z

source/Octopus.Tentacle.Client/Scripts/UnsafeStartAttemptException.cs

+
+namespace Octopus.Tentacle.Client.Scripts
+{
+    public class UnsafeStartAttemptException : Exception


If this is used we should document what it is about.

LukeButters · 2025-01-21T02:52:15Z

source/Tentacle.sln.DotSettings

@@ -183,6 +183,7 @@
 	<s:Boolean x:Key="/Default/Environment/SettingsMigration/IsMigratorApplied/=JetBrains_002EReSharper_002EPsi_002ECSharp_002ECodeStyle_002ECSharpPlaceEmbeddedOnSameLineMigration/@EntryIndexedValue">True</s:Boolean>
 	<s:Boolean x:Key="/Default/Environment/SettingsMigration/IsMigratorApplied/=JetBrains_002EReSharper_002EPsi_002ECSharp_002ECodeStyle_002ECSharpRenamePlacementToArrangementMigration/@EntryIndexedValue">True</s:Boolean>
 	<s:Boolean x:Key="/Default/Environment/SettingsMigration/IsMigratorApplied/=JetBrains_002EReSharper_002EPsi_002ECSharp_002ECodeStyle_002ECSharpUseContinuousIndentInsideBracesMigration/@EntryIndexedValue">True</s:Boolean>
+	<s:Boolean x:Key="/Default/Environment/SettingsMigration/IsMigratorApplied/=JetBrains_002EReSharper_002EPsi_002ECSharp_002ECodeStyle_002EMemberReordering_002EMigrations_002ECSharpFileLayoutPatternRemoveIsAttributeUpgrade/@EntryIndexedValue">True</s:Boolean>


just dotnet things

LukeButters and others added 30 commits June 21, 2024 12:47

Try to make an event driven orchstrator

d470d9f

Drop start script type

c912d08

Start to remove TScriptStatusResponse

269dce8

Drop nullable

720b9c4

WIP

4336c4c

Use new IStructuredScriptOrchestrator interface

254bd55

Finish can return more logs

acac7fc

Change over ScriptServiceV2Orchestrator to use new interface

3f0dfac

Fix factory

d80bc62

Change KubernetesScriptServiceV1Orchestrator to use new interface

11b96d4

get building

6a4b1f6

WIP

e99bc2a

Revert back to using tuples

a5e0589

Update k8s and v1

e4ee43c

WIP

9c0f285

Fix ssv2

4796b05

Implement EventDrivenScriptExecutor

33bb1b4

minor refactor

9003b32

ssv2 fix

28f0ccc

.

87ccc83

Push knowledge around SSV1 into its executor

cd5086b

Create the AggregateScriptExecutor

d8c2f32

.

228af09

Merge branch 'main' into luke/event-driven-script-orchestrator

5bd1329

# Conflicts: # source/Octopus.Tentacle.Client/Scripts/KubernetesScriptServiceV1AlphaOrchestrator.cs # source/Octopus.Tentacle.Client/Scripts/ScriptOrchestratorFactory.cs # source/Octopus.Tentacle.Client/TentacleClient.cs

Merge fixes

0702813

Small tidy ups

9a00c66

Renaming (and fixing bug with Kubernetes cancelling)

af4d603

Tidy

e00a9d2

Adding result object

015fdb8

Using the same client object for executor

a754c9a

sburmanoctopus added 5 commits January 21, 2025 10:40

Keeping metrics when using TentacleClient

a377da4

Dealing with cancellation issues

3ed6087

Rename

d16a942

Better wording

a13b162

Fixing cancellation on builds

051a74d

sburmanoctopus marked this pull request as ready for review January 21, 2025 02:00

sburmanoctopus requested a review from a team as a code owner January 21, 2025 02:00

LukeButters reviewed Jan 21, 2025

View reviewed changes

LukeButters approved these changes Jan 21, 2025

View reviewed changes

PR fixes

8ba4e0f

sburmanoctopus merged commit 67b07ca into main Jan 21, 2025
51 of 52 checks passed

sburmanoctopus deleted the sast/event-driven-script-orchestrator branch January 21, 2025 05:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Event driven script orchestrator #1058

Event driven script orchestrator #1058

sburmanoctopus commented Jan 21, 2025 •

edited

Loading

LukeButters Jan 21, 2025

sburmanoctopus Jan 21, 2025

LukeButters Jan 21, 2025

sburmanoctopus Jan 21, 2025

LukeButters Jan 21, 2025

sburmanoctopus Jan 21, 2025

LukeButters Jan 21, 2025

sburmanoctopus Jan 21, 2025

LukeButters Jan 21, 2025

sburmanoctopus Jan 21, 2025

LukeButters Jan 21, 2025

Event driven script orchestrator #1058

Event driven script orchestrator #1058

Conversation

sburmanoctopus commented Jan 21, 2025 • edited Loading

Background

Results

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sburmanoctopus commented Jan 21, 2025 •

edited

Loading