-
Notifications
You must be signed in to change notification settings - Fork 23
Tentacle Abandon Script #1226
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Tentacle Abandon Script #1226
Changes from all commits
9073427
4f3fc4d
e797a67
06517d6
ffa5b82
d03dcfb
65a6db5
96babdd
23059e4
7ee0fdb
86ef03a
d4e85ba
5fd82a5
21b8d2a
d195d65
a2e283b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
Large diffs are not rendered by default.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -14,17 +14,20 @@ sealed class ObservingScriptOrchestrator | |
| readonly OnScriptStatusResponseReceived onScriptStatusResponseReceived; | ||
| readonly OnScriptCompleted onScriptCompleted; | ||
| readonly IScriptExecutor scriptExecutor; | ||
| readonly TimeSpan? abandonAfterCancellationPendingFor; | ||
|
|
||
| public ObservingScriptOrchestrator( | ||
| IScriptObserverBackoffStrategy scriptObserverBackOffStrategy, | ||
| OnScriptStatusResponseReceived onScriptStatusResponseReceived, | ||
| OnScriptCompleted onScriptCompleted, | ||
| IScriptExecutor scriptExecutor) | ||
| IScriptExecutor scriptExecutor, | ||
| TimeSpan? abandonAfterCancellationPendingFor = null) | ||
| { | ||
| this.scriptExecutor = scriptExecutor; | ||
| this.scriptObserverBackOffStrategy = scriptObserverBackOffStrategy; | ||
| this.onScriptStatusResponseReceived = onScriptStatusResponseReceived; | ||
| this.onScriptCompleted = onScriptCompleted; | ||
| this.abandonAfterCancellationPendingFor = abandonAfterCancellationPendingFor; | ||
| } | ||
|
|
||
| public async Task<ScriptExecutionResult> ExecuteScript(ExecuteScriptCommand command, CancellationToken scriptExecutionCancellationToken) | ||
|
|
@@ -80,12 +83,28 @@ async Task<ScriptOperationExecutionResult> ObserveUntilComplete( | |
| var iteration = 0; | ||
| var cancellationIteration = 0; | ||
| var lastResult = startScriptResult; | ||
| var stopwatch = new Stopwatch(); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: rename to something like cancelling duration |
||
|
|
||
| while (lastResult.ScriptStatus.State != ProcessState.Complete) | ||
| { | ||
| if (scriptExecutionCancellationToken.IsCancellationRequested) | ||
| { | ||
| lastResult = await scriptExecutor.CancelScript(lastResult.ContextForNextCommand).ConfigureAwait(false); | ||
| // Record when cancellation first fired so we can escalate to abandon after the threshold. | ||
| if (!stopwatch.IsRunning) | ||
| { | ||
| stopwatch.Start(); | ||
| } | ||
|
jimmyp marked this conversation as resolved.
|
||
|
|
||
| // Only escalate to abandon when the Tentacle advertised the abandon capability. Old V2 | ||
| // Tentacles (pre-abandon) and V1/Kubernetes don't, so we keep cancelling rather than | ||
| // calling a verb they don't have. | ||
| var shouldAbandon = lastResult.ContextForNextCommand.ScripServiceVersionUsed.SupportsAbandon | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wonder if we should drop the capabilities into the The CommandContext is stored in V2 messages, so we would need to consider the upgrade case (I think just upgrading to an empty set of capabilities would be sufficient) If we do decide to keep |
||
| && abandonAfterCancellationPendingFor.HasValue | ||
| && stopwatch.Elapsed >= abandonAfterCancellationPendingFor.Value; | ||
|
|
||
| lastResult = shouldAbandon | ||
| ? await scriptExecutor.AbandonScript(lastResult.ContextForNextCommand).ConfigureAwait(false) | ||
| : await scriptExecutor.CancelScript(lastResult.ContextForNextCommand).ConfigureAwait(false); | ||
| } | ||
| else | ||
| { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,11 +1,20 @@ | ||
| namespace Octopus.Tentacle.Client.Scripts | ||
| namespace Octopus.Tentacle.Client.Scripts | ||
| { | ||
| public record ScriptServiceVersion(string Value) | ||
| { | ||
| public static ScriptServiceVersion ScriptServiceVersion1 = new(nameof(ScriptServiceVersion1)); | ||
| public static ScriptServiceVersion ScriptServiceVersion2 = new(nameof(ScriptServiceVersion2)); | ||
| public static ScriptServiceVersion ScriptServiceVersion2WithAbandon = new(nameof(ScriptServiceVersion2WithAbandon)); | ||
| public static ScriptServiceVersion KubernetesScriptServiceVersion1 = new(nameof(KubernetesScriptServiceVersion1)); | ||
|
|
||
| // True for both V2 variants — they talk to the same ScriptServiceV2; ...WithAbandon just also | ||
| // advertised the abandon verb. | ||
| public bool IsV2 => Value == nameof(ScriptServiceVersion2) || Value == nameof(ScriptServiceVersion2WithAbandon); | ||
|
|
||
| // Only a V2 Tentacle that advertised the AbandonScript capability supports abandon. Old V2 | ||
| // Tentacles predate the verb, so they select ScriptServiceVersion2, not ...WithAbandon. | ||
| public bool SupportsAbandon => Value == nameof(ScriptServiceVersion2WithAbandon); | ||
|
|
||
| public override string ToString() => Value; | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -16,6 +16,7 @@ | |
| using Octopus.Tentacle.Contracts.Capabilities; | ||
| using Octopus.Tentacle.Contracts.Logging; | ||
| using Octopus.Tentacle.Contracts.Observability; | ||
| using Octopus.Tentacle.Contracts.ScriptServiceV2; | ||
| using ITentacleClientObserver = Octopus.Tentacle.Contracts.Observability.ITentacleClientObserver; | ||
|
|
||
| namespace Octopus.Tentacle.Client | ||
|
|
@@ -168,7 +169,8 @@ public async Task<ScriptExecutionResult> ExecuteScript(ExecuteScriptCommand exec | |
| OnScriptStatusResponseReceived onScriptStatusResponseReceived, | ||
| OnScriptCompleted onScriptCompleted, | ||
| ITentacleClientTaskLog logger, | ||
| CancellationToken scriptExecutionCancellationToken) | ||
| CancellationToken scriptExecutionCancellationToken, | ||
| TimeSpan? abandonAfterCancellationPendingFor = null) | ||
| { | ||
| using var activity = ActivitySource.StartActivity($"{nameof(TentacleClient)}.{nameof(ExecuteScript)}"); | ||
| activity?.AddTag("octopus.tentacle.script.files", string.Join(",", executeScriptCommand.Files.Select(f => f.Name))); | ||
|
|
@@ -188,7 +190,8 @@ public async Task<ScriptExecutionResult> ExecuteScript(ExecuteScriptCommand exec | |
| var orchestrator = new ObservingScriptOrchestrator(scriptObserverBackOffStrategy, | ||
| onScriptStatusResponseReceived, | ||
| onScriptCompleted, | ||
| scriptExecutor); | ||
| scriptExecutor, | ||
| abandonAfterCancellationPendingFor); | ||
|
|
||
| var result = await orchestrator.ExecuteScript(executeScriptCommand, scriptExecutionCancellationToken); | ||
|
|
||
|
|
@@ -260,6 +263,27 @@ public async Task<ScriptOperationExecutionResult> CancelScript(CommandContext co | |
| return await scriptExecutor.CancelScript(commandContext); | ||
| } | ||
|
|
||
| public async Task<ScriptStatusResponseV2> AbandonScript(ScriptTicket scriptTicket, ITentacleClientTaskLog logger, CancellationToken cancellationToken) | ||
| { | ||
| using var activity = ActivitySource.StartActivity($"{nameof(TentacleClient)}.{nameof(AbandonScript)}"); | ||
| activity?.AddTag("octopus.tentacle.script.ticket", scriptTicket.TaskId); | ||
|
|
||
| async Task<ScriptStatusResponseV2> AbandonScriptAction(CancellationToken ct) | ||
| { | ||
| var request = new AbandonScriptCommandV2(scriptTicket, lastLogSequence: 0); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think giving a lastLogSequence of 0 will ask for ALL logs to be returned from the very beginning of the script. I think cancel above is using the CommandContext to get the "tail" of the logs rather than all the logs. It also goes via the scriptExecutor. Why is this not following the same pattern? |
||
| return await allClients.ScriptServiceV2.AbandonScriptAsync(request, new HalibutProxyRequestOptions(ct)); | ||
| } | ||
|
|
||
| return await rpcCallExecutor.Execute( | ||
| retriesEnabled: clientOptions.RpcRetrySettings.RetriesEnabled, | ||
| RpcCall.Create<IScriptServiceV2>(nameof(IScriptServiceV2.AbandonScript)), | ||
| AbandonScriptAction, | ||
| logger, | ||
| // Abandon is a one-shot RPC; like CancelScript we don't track operation metrics for it. | ||
| ClientOperationMetricsBuilder.Start(), | ||
| cancellationToken).ConfigureAwait(false); | ||
| } | ||
|
|
||
| public async Task<ScriptStatus?> CompleteScript(CommandContext commandContext, ITentacleClientTaskLog logger, CancellationToken scriptExecutionCancellationToken) | ||
| { | ||
| using var activity = ActivitySource.StartActivity($"{nameof(TentacleClient)}.{nameof(CompleteScript)}"); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the names ever changes that would be a bug, since we would break compatibility.
No Action but the comment is perhaps misleading almost suggesting we can change it.