Fix replication issues with nexus operations and cancelation requests #6457
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
bergundy
force-pushed
the
nexus-operation-cancel-requested-replication-fix
branch
from
5336d15 to
02efd41
Compare
yux0
approved these changes
Aug 28, 2024
| Message: "operation hasn't started yet, dropping cancel request", | ||
| Response: &http.Response{ | ||
| // Make up a non retryable error code to consider this error non-retryable. | ||
| StatusCode: http.StatusPreconditionFailed, |
There was a problem hiding this comment.
TBH it doesn't matter much, it's not recorded so any non-retryable error code would do.
pdoerner
approved these changes
Aug 28, 2024
yycptt
approved these changes
Aug 28, 2024
bergundy
force-pushed
the
nexus-operation-cancel-requested-replication-fix
branch
from
02efd41 to
37cadea
Compare
yycptt
pushed a commit
that referenced
this pull request
Aug 29, 2024
…#6457) ## What changed? Changed the operation cancelation logic to not cancel the operation if the operation hasn't been started yet. Before this change canceling an operation would add a NEXUS_OPERATION_CANCELED event and immediately transition to the `Canceled` state, which had issues with replication and buffered events. E.g. the `Started` event would be applied before the `CancelRequested` event on one cluster and then applied in reverse order when replicating to another cluster. Example history batch taken from a test cluster contains these events: ``` EVENT_TYPE_WORKFLOW_TASK_COMPLETED EVENT_TYPE_NEXUS_OPERATION_CANCEL_REQUESTED EVENT_TYPE_REQUEST_CANCEL_EXTERNAL_WORKFLOW_EXECUTION_INITIATED EVENT_TYPE_NEXUS_OPERATION_STARTED // <-- this had to be buffered EVENT_TYPE_WORKFLOW_TASK_SCHEDULED EVENT_TYPE_WORKFLOW_TASK_STARTED ``` The new code path simply creates the cancelation state machine but doesn't actually cancel it, instead it lets the task executor attempt to resolve the machine as canceled. Eventually we'll want to properly support cancel-before-started. At the moment, we may end up starting an operation and leave it oblivious of the cancelation request. ## How did you test it? I didn't test the replication issue but the new code path avoids ordering issues by not transitioning to canceled state. Added unit tests for the executor changes and modified the state machine tests. ## Is hotfix candidate? We should patch 120 and maybe even 119. The latter will be promoted to OSS 1.25.0 but since we explicitly state that Nexus in 1.25.0 isn't suited for multi cluster deployments, we could skip this patch.
alexshtin
pushed a commit
that referenced
this pull request
Nov 5, 2024
…#6457) Changed the operation cancelation logic to not cancel the operation if the operation hasn't been started yet. Before this change canceling an operation would add a NEXUS_OPERATION_CANCELED event and immediately transition to the `Canceled` state, which had issues with replication and buffered events. E.g. the `Started` event would be applied before the `CancelRequested` event on one cluster and then applied in reverse order when replicating to another cluster. Example history batch taken from a test cluster contains these events: ``` EVENT_TYPE_WORKFLOW_TASK_COMPLETED EVENT_TYPE_NEXUS_OPERATION_CANCEL_REQUESTED EVENT_TYPE_REQUEST_CANCEL_EXTERNAL_WORKFLOW_EXECUTION_INITIATED EVENT_TYPE_NEXUS_OPERATION_STARTED // <-- this had to be buffered EVENT_TYPE_WORKFLOW_TASK_SCHEDULED EVENT_TYPE_WORKFLOW_TASK_STARTED ``` The new code path simply creates the cancelation state machine but doesn't actually cancel it, instead it lets the task executor attempt to resolve the machine as canceled. Eventually we'll want to properly support cancel-before-started. At the moment, we may end up starting an operation and leave it oblivious of the cancelation request. I didn't test the replication issue but the new code path avoids ordering issues by not transitioning to canceled state. Added unit tests for the executor changes and modified the state machine tests. We should patch 120 and maybe even 119. The latter will be promoted to OSS 1.25.0 but since we explicitly state that Nexus in 1.25.0 isn't suited for multi cluster deployments, we could skip this patch.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What changed?
Changed the operation cancelation logic to not cancel the operation if the operation hasn't been started yet.
Before this change canceling an operation would add a NEXUS_OPERATION_CANCELED event and immediately transition to the
Canceledstate, which had issues with replication and buffered events. E.g. theStartedevent would be applied before theCancelRequestedevent on one cluster and then applied in reverse order when replicating to another cluster.Example history batch taken from a test cluster contains these events:
The new code path simply creates the cancelation state machine but doesn't actually cancel it, instead it lets the task executor attempt to resolve the machine as canceled.
Eventually we'll want to properly support cancel-before-started. At the moment, we may end up starting an operation and leave it oblivious of the cancelation request.
How did you test it?
I didn't test the replication issue but the new code path avoids ordering issues by not transitioning to canceled state.
Added unit tests for the executor changes and modified the state machine tests.
Is hotfix candidate?
We should patch 120 and maybe even 119. The latter will be promoted to OSS 1.25.0 but since we explicitly state that Nexus in 1.25.0 isn't suited for multi cluster deployments, we could skip this patch.