Skip to content
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.

Sign up for GitHub

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

Fix versioning override with AutoUpgrade behavior #1765

Merged
merged 5 commits into from
Jan 8, 2025
+49 −5
Merged

Fix versioning override with AutoUpgrade behavior #1765

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
eace425
Fix versioning override with AutoUpgrade behavior
antlai-temporal Jan 8, 2025
5799109
Fix comment
antlai-temporal Jan 8, 2025
8d386d6
Add comment
antlai-temporal Jan 8, 2025
ee19542
Merge remote-tracking branch 'upstream/master' into fix-versioning-ov…
antlai-temporal Jan 8, 2025
da72344
Merge remote-tracking branch 'upstream/master' into fix-versioning-ov…
antlai-temporal Jan 8, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion contrib/tools/workflowcheck/determinism/testdata/src/a/ignore.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,9 @@ func IgnoreAboveLine() { // want IgnoreAboveLine:"calls non-deterministic functi
fmt.Println("Do not ignore this")
}

//workflowcheck:ignore
// IgnoreEntireFunction can have a Godoc comment too
//
//workflowcheck:ignore
func IgnoreEntireFunction() {
fmt.Print("Do not ignore this")
fmt.Printf("Ignore this")
Expand Down
9 changes: 9 additions & 0 deletions internal/internal_workflow_execution_options.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,15 @@ func workflowExecutionOptionsMaskToProto(mask []string) *fieldmaskpb.FieldMask {
}

func workerDeploymentToProto(d Deployment) *deploymentpb.Deployment {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not make the source of this a pointer if unset is a valid state? Meaning, make VersioningOverride.Deployment accept a pointer.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought it was cleaner not to expose pointers and just use an empty struct. Otherwise you ended up with what "a pointer to an empty struct" means vs a nil pointer. It is also consistent with the other apis that use Deployment.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It sounds like the server treats an empty struct here as invalid and errors is that correct?

Copy link
Member

@cretz cretz Jan 8, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Otherwise you ended up with what "a pointer to an empty struct" means vs a nil pointer.

If there is a difference between empty and null in the server-side API and we need to represent that difference, we should do the same here. If the server errors on empty, good, that tells the user it was invalid like it might for anything else invalid. If the server wants to treat empty and null as the same, we can too.

It is also consistent with the other apis that use Deployment.

Are there other fields where the zero value of a struct means nil when converting to proto? I think we should consider fixing those too.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, other empty structs related to deployment are ok, but this one was not... They may eventually fix that, but this is
blocking the autoUpgrade override...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cretz There was a difference but it was not intentional, there should be no difference. It is just that the OSS server release is out, and it is better to fix it internally in the first SDK release without exposing it, and eventually change it in the server...
All the other values that I can think of would take either empty or nil. I mapped VersioningOverride{} to nil mostly to keep things more clear, but I don't think it is strictly needed.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add a comment explaining this so future readers understand why we are inconsistent here?

If the server will always treat nil and empty the same then I am fine using an empty struct instead of a pointer.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment added

// Server 1.26.2 requires a nil Deployment pointer, and not just a pointer to an empty Deployment,
// to indicate that there is no Deployment.
// It is a server error to override versioning behavior to AutoUpgrade while providing a Deployment,
// and we need to replace it by nil. See https://github.com/temporalio/sdk-go/issues/1764.
//
// Future server versions may relax this requirement.
if (Deployment{}) == d {
return nil
}
return &deploymentpb.Deployment{
SeriesName: d.SeriesName,
BuildId: d.BuildID,
Expand Down
6 changes: 3 additions & 3 deletions internal/workflow_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,8 +61,8 @@ func TestGetChildWorkflowOptions(t *testing.T) {
},
ParentClosePolicy: enums.PARENT_CLOSE_POLICY_REQUEST_CANCEL,
VersioningIntent: VersioningIntentDefault,
StaticSummary: "child workflow summary",
StaticDetails: "child workflow details",
StaticSummary: "child workflow summary",
StaticDetails: "child workflow details",
}

// Require test options to have non-zero value for each field. This ensures that we update tests (and the
Expand All @@ -84,7 +84,7 @@ func TestGetActivityOptions(t *testing.T) {
RetryPolicy: newTestRetryPolicy(),
DisableEagerExecution: true,
VersioningIntent: VersioningIntentDefault,
Summary: "activity summary",
Summary: "activity summary",
}

assertNonZero(t, opts)
Expand Down
1 change: 1 addition & 0 deletions mocks/Client.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

35 changes: 34 additions & 1 deletion test/deployment_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -344,10 +344,11 @@ func (ts *DeploymentTestSuite) TestUpdateWorkflowExecutionOptions() {
// Two workers:
// 1) 1.0 with WaitSignalToStartVersionedOne (setCurrent)
// 2) 2.0 with WaitSignalToStartVersionedTwo
// Three workflows:
// Four workflows:
// 1) started with "1.0", manual override to "2.0", finish "2.0"
// 2) started with "1.0", manual override to "2.0", remove override, finish "1.0"
// 3) started with "1.0", no override, finishes with "1.0" unaffected
// 4) started with "1.0", manual override to auto-upgrade, finishes with "2.0"

worker1 := worker.New(ts.client, ts.taskQueueName, worker.Options{
BuildID: "1.0",
Expand Down Expand Up @@ -401,6 +402,11 @@ func (ts *DeploymentTestSuite) TestUpdateWorkflowExecutionOptions() {
handle3, err := ts.client.ExecuteWorkflow(ctx, ts.startWorkflowOptions("3"), "WaitSignalToStartVersioned")
ts.NoError(err)

handle4, err := ts.client.ExecuteWorkflow(ctx, ts.startWorkflowOptions("4"), "WaitSignalToStartVersioned")
ts.NoError(err)

ts.waitForWorkflowRunning(ctx, handle4)

options, err := ts.client.UpdateWorkflowExecutionOptions(ctx, client.UpdateWorkflowExecutionOptionsRequest{
WorkflowId: handle1.GetID(),
RunId: handle1.GetRunID(),
Expand Down Expand Up @@ -445,9 +451,31 @@ func (ts *DeploymentTestSuite) TestUpdateWorkflowExecutionOptions() {
ts.NoError(err)
ts.Equal(options.VersioningOverride, client.VersioningOverride{})

// Add autoUpgrade to handle4
options, err = ts.client.UpdateWorkflowExecutionOptions(ctx, client.UpdateWorkflowExecutionOptionsRequest{
WorkflowId: handle4.GetID(),
RunId: handle4.GetRunID(),
WorkflowExecutionOptionsChanges: client.WorkflowExecutionOptionsChanges{
VersioningOverride: &client.VersioningOverride{
Behavior: workflow.VersioningBehaviorAutoUpgrade,
},
},
})
ts.NoError(err)
ts.Equal(options.VersioningOverride.Deployment.BuildID, "")

_, err = ts.client.DeploymentClient().SetCurrent(ctx, client.DeploymentSetCurrentOptions{
Deployment: client.Deployment{
BuildID: "2.0",
SeriesName: seriesName,
},
})
ts.NoError(err)

ts.NoError(ts.client.SignalWorkflow(ctx, handle1.GetID(), handle1.GetRunID(), "start-signal", "prefix"))
ts.NoError(ts.client.SignalWorkflow(ctx, handle2.GetID(), handle2.GetRunID(), "start-signal", "prefix"))
ts.NoError(ts.client.SignalWorkflow(ctx, handle3.GetID(), handle3.GetRunID(), "start-signal", "prefix"))
ts.NoError(ts.client.SignalWorkflow(ctx, handle4.GetID(), handle4.GetRunID(), "start-signal", "prefix"))

// Wait for all wfs to finish
var result string
Expand All @@ -462,6 +490,11 @@ func (ts *DeploymentTestSuite) TestUpdateWorkflowExecutionOptions() {
ts.NoError(handle3.Get(ctx, &result))
// no override
ts.True(IsVersionOne(result))

ts.NoError(handle4.Get(ctx, &result))
// override + autoUpgrade
ts.True(IsVersionTwo(result))

}

func (ts *DeploymentTestSuite) TestListDeployments() {
Expand Down
Loading