Test and fix Deployment SAs #7293
Conversation
| if ms.GetMostRecentWorkerVersionStamp().GetUseVersioning() && | ||
| //nolint:staticcheck // SA1019 deprecated stamp will clean up later | ||
| !request.GetWorkerVersionStamp().GetUseVersioning() && | ||
| request.GetDeploymentOptions().GetWorkerVersioningMode() != enumspb.WORKER_VERSIONING_MODE_VERSIONED { | ||
| // Mutable state wasn't changed yet and doesn't have to be cleared. | ||
| releaseLeaseWithError = false | ||
| return nil, serviceerror.NewInvalidArgument("Workflow using versioning must continue to use versioning.") |
There was a problem hiding this comment.
how does "ramp to unversioned" work with this? If someone uses versioning in a version stamp, they can't ramp to unversioned? (It makes sense to me that v1/v2 can't ramp to unversioned, but v3.1 should be able to, right?)
There was a problem hiding this comment.
yeah they could not in v1-2. now that I exclude v3.1 from the condition it should be possible for v3.1 even if stamp is not sent.
| oldVersioningUsed := handler.mutableState.GetMostRecentWorkerVersionStamp().GetUseVersioning() | ||
| oldVersioningUsed := handler.mutableState.GetMostRecentWorkerVersionStamp().GetUseVersioning() && | ||
| // for V3 versioning it's ok to dispatch eager activities | ||
| handler.mutableState.GetEffectiveVersioningBehavior() == enumspb.VERSIONING_BEHAVIOR_UNSPECIFIED | ||
| newVersioningUsed := handler.mutableState.GetExecutionInfo().GetAssignedBuildId() != "" |
There was a problem hiding this comment.
is newVersioningUsed correct? it seems like this only checks that the WF never used versioning v2.
There was a problem hiding this comment.
I think it is correct. once a wf uses v2 it cannot go back! in here I just want to exclude v3 from the check. not touching v1-2 suff.
| @@ -580,7 +581,7 @@ func (d *VersionWorkflowRunner) handleSyncState(ctx workflow.Context, args *depl | |||
|
|
|||
| // stopped accepting new workflows --> start drainage child wf | |||
| if wasAcceptingNewWorkflows && !isAcceptingNewWorkflows { | |||
| d.startDrainage(ctx, true) | |||
| d.startDrainage(ctx, false) | |||
| DeploymentOptions: &deploymentpb.WorkerDeploymentOptions{ | ||
| BuildId: tv.BuildID(), | ||
| DeploymentName: tv.DeploymentSeries(), | ||
| WorkerVersioningMode: enumspb.WORKER_VERSIONING_MODE_VERSIONED, |
There was a problem hiding this comment.
do you need a version stamp here too?
There was a problem hiding this comment.
well, I someone whats to check the SAs after wf completion yes but I didn't have that use and left it out for now because wanted to make sure the request goes through without stamp as well.
What changed?
Making sure the following SAs are populated correctly for Versioning v3.1
Why?
How did you test it?
Potential risks
Documentation
Is hotfix candidate?