-
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 7 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,27 @@ 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 script service can actually abandon. On versions | ||
| // that can't (V1, Kubernetes) 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,17 @@ | ||
| 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 KubernetesScriptServiceVersion1 = new(nameof(KubernetesScriptServiceVersion1)); | ||
|
|
||
| // Only ScriptServiceV2 has the AbandonScript verb, so the orchestrator checks this before it | ||
| // escalates a stuck cancel to abandon and never calls abandon where it can't work. This assumes | ||
| // a V2 tentacle advertises AbandonScript (true from this build forward); the server doesn't enable | ||
| // abandon against older tentacles, so we don't re-check the capability per call here. | ||
|
Contributor
Author
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. You can remove this comment |
||
| public bool SupportsAbandon => Value == nameof(ScriptServiceVersion2); | ||
|
jimmyp marked this conversation as resolved.
Outdated
|
||
|
|
||
| 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)}"); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,17 @@ | ||
| using System; | ||
|
|
||
| namespace Octopus.Tentacle.Contracts.ScriptServiceV2 | ||
| { | ||
| public class AbandonScriptCommandV2 | ||
| { | ||
| public AbandonScriptCommandV2(ScriptTicket ticket, long lastLogSequence) | ||
| { | ||
| Ticket = ticket; | ||
| LastLogSequence = lastLogSequence; | ||
| } | ||
|
|
||
| public ScriptTicket Ticket { get; } | ||
|
|
||
| public long LastLogSequence { get; } | ||
| } | ||
| } |
Uh oh!
There was an error while loading. Please reload this page.