Add data plane fields w/ renamed Worker Versioning concepts #514

ShahabT · 2025-01-14T23:51:49Z

READ BEFORE MERGING: All PRs require approval by both Server AND SDK teams before merging! This is why the number of required approvals is "2" and not "1"--two reviewers from the same team is NOT sufficient. If your PR is not approved by someone in BOTH teams, it may be summarily reverted.

What changed?
The following concepts in Worker Versioning V3 is being renamed. This PR adds the data plane fields with new names so SDK can populate them like the old ones.
DeploymentSeries -> WorkerDeployment
Deployment -> WorkerDeploymentVersion

Also WorkerVersionCapabilities and its use_versioning boolean are replaced with WorkerDeploymentOptions and WorkflowVersioningMode enum to be more compatible with V3 changes and also extensible in the future.

RespondActivityTask*Request used to send deployment but that is now deprecated. Since Server does not have any anticipated use for these values, I did not add a replacement for them at the moment.

Why?
See ^^

Breaking changes
Not breaking, some things are deprecated that will be cleaned up at a later time with other old Versioning stuff.

Server PR
Server PR will follow soon. But these changes should not break server compile because methods are not touched.

cretz

General concerns about deprecating so many things a few weeks after merging it. I just want to make sure we understand the consequences of ever removing API.

But only change requested is to add deprecated options.

cretz · 2025-01-15T14:20:16Z

temporal/api/history/v1/message.proto

    // The deployment that completed this task. May or may not be set for unversioned workers,
    // depending on whether a value is sent by the SDK. This value updates workflow execution's
    // `versioning_info.deployment`.
+    // Deprecated. Replaced with `deployment_name` and `deployment_version`.


I admit I am a bit concerned about the idea of removing this later. This was just merged a couple months ago and now it's being deprecated.

It is very important we try our best not to make backwards incompatible changes to released API, even if we think it is unused. Customers build proxies and have code generators and in general have stability expectations on our API. We have broken users in the past by making incompatible changes to API (again, even if unused by our SDKs as our SDKs are not the only ones that expect API stability). This strive for API stability includes pre-release/public-preview API because unlike SDKs, we cannot make assumptions about downstream consumption.

Also, if this field is ever set, removing this will make history JSON incompatible and we strive to make history always backwards (and forwards) compatible even pre-release/public-preview histories.

I'm a bit surprised by this take because our Temporal product release stages guide guidance on API stability is "Experimental; API is subject to change" for Pre-release and "API breaking changes are kept to a minimum" for Public Preview (and then "API is stable" for GA). It seems appropriate for all parties to refer to that as their source of truth for how to plan around API changes.

There is a difference between SDK API contract stability, API behavior stability, and proto API contract stability. They are all different forms of API stability. Regardless of what we're technically allowed to do, we should always try for stability at every level. And we should be aware of the consequences of breaking compatibility to our protos, even if we technically say we're allowed to do so. You can hurt proxy users and general API proto consumers.

We're intentionally taking an iterative approach to worker versioning. Insisting that there are no breaking changes for pre-releases ever is a recipe for stasis and overthinking because every decision will seem to carry years of weight, even at the pre-release stage.
I'm okay with very cheap mechanisms to ensure backward compatibility here but overall it's a non-goal. Happy to chat offline to sort through any strategic misalignments.

I am just bringing up that there are consequences for removing something in the public API and we should strive to avoid this, especially in such short turnarounds. Sometimes for others making API changes to these protos, the consequences are non-obvious until complaints come about. We have had issues in the past where people treat master as their implementation playground to make changes a couple months apart on the same project. Granted this PR doesn't remove anything anyways, so just making sure the consequences are understood (I was waiting for the other comment to be addressed before approving).

There is not a world where it churn never happens (but there is a never-remove-only-deprecate world, but even that is too strict).

Let's just make sure we call it out clearly in the API release notes if/when we remove the fields, and let's also make sure that we have a Go SDK release at about the same time as that API release so at least our latest Go SDK works with the latest Go API (which is released at the same time as this repo). We have had issues in the past where compat-breaking changes to this repo were released in api-go with no associated Go SDK release causing those doing a go get -u to have compile breaks.

I am not sure we can ever remove history fields, we should have an internal discussion on that. Some people get workflow JSON including unset fields (unfortunately) and expect that JSON to work with replayers in perpetuity. But it may be safe in these cases.

let's also make sure that we have a Go SDK release at about the same time as that API release so at least our latest Go SDK works with the latest Go API

@cretz to clarify, you mean the release in which the deprecated fields are removed, not this one because it is not breaking anything, right?

Let's just make sure we call it out clearly in the API release notes if/when we remove the fields

For if/when I'd like to wait until we're close to GA. We can then add a heads up to one of the release notes and specify a plan, and when the specified time comes after that, delete the fields with proper release note.

non-sdk users that may use automated tools to generate proxies, UI interfaces that interact directly with the API

Doing this on pre-release APIs in such a way that they cannot change should not be a supported scenario.

to clarify, you mean the release in which the deprecated fields are removed, not this one because it is not breaking anything, right?

Correct

Doing this on pre-release APIs in such a way that they cannot change should not be a supported scenario.

Automated protoc does not differentiate unstable API and stable API right next to each other in a stable version release. But the other issue is that we are using these to-be-deleted APIs in our SDKs. So even for people that don't care about versioning, once we remove the fields, if they upgrade go.temporal.io/api to latest but choose not to upgrade their Go SDK (which they can totally do with our version constraints), all of their compilations start breaking. So we just tell them "sorry, even though that SDK says it's compatible with API >= some-version, it's not" and ask them to upgrade both SDK and API.

cretz · 2025-01-15T14:27:28Z

temporal/api/history/v1/message.proto

    // The deployment that completed this task. May or may not be set for unversioned workers,
    // depending on whether a value is sent by the SDK. This value updates workflow execution's
    // `versioning_info.deployment`.
+    // Deprecated. Replaced with `deployment_name` and `deployment_version`.


Can you add [deprecated = true]; to all deprecated fields?

carlydf

approved with a few wording suggestions

temporal/api/enums/v1/workflow.proto

temporal/api/workflow/v1/message.proto

carlydf · 2025-01-15T22:41:13Z

temporal/api/workflow/v1/message.proto

+    // execution. Must be present if `behavior` is set. Absent value means no workflow task is
+    // completed, or the last workflow task was completed by a legacy worker who does not pass
+    // deployment_version.
+    // Note that if `versioning_override.behavior`.


did you mean to write more information here?

Co-authored-by: Carly de Frondeville <[email protected]>

antlai-temporal

before we had a temporal.api.deployment.v1.Deployment struct that encapsulated the name and the "version".
Now we are splitting in two separate fields deployment_name/deployment_version and we are inconsistent when we pass one or both, i.e., making assumptions that version is all it matters in some places.
I think we should use a DeploymentVersion object (or something like that) that always passes both, because in the future we may decide that the deployment_name is also important for routing too...

antlai-temporal · 2025-01-15T22:19:20Z

temporal/api/enums/v1/workflow.proto

+    // Version, in which case the activity will be sent to a different Deployment Version according
+    // to the activity's Task Queue situation.


This is too vague, the previous comment talked about the "current deployment", giving a hint how the Deployment version is chosen in that case...

antlai-temporal · 2025-01-15T22:35:51Z

temporal/api/workflow/v1/message.proto

+    // Note that if `versioning_override.behavior`.
+    string deployment_version = 6;


This comment is incomplete

antlai-temporal · 2025-01-15T22:38:57Z

temporal/api/workflow/v1/message.proto

+    // The target version of the transition. Absent means a so-far-versioned workflow is
+    // transitioning to unversioned workers.
+    string target_version = 1;


Do we need the deployment_name here?

if we allow transitions between deployments, which it sounds like we do:

// When present, indicates the workflow is transitioning to a different deployment version // (which may belong to the same deployment name or another).

Then it seems like we should have deployment_name there?

Since versions are unique within a namespace, I don't think specifying the deployment_name becomes necessary - I also am not sure if we are allowing deployment transitions to happen across namespaces but if we are not, I think having just the target_version should suffice

happy to be pointed in the right direction if my understanding is wrong!

oops I forgot that version is unique within a namespace, nevermind what I said

antlai-temporal · 2025-01-15T22:40:48Z

temporal/api/workflow/v1/message.proto

+    string pinned_version = 3;
 }


Do we need the deployment_name here, and not just the version?

I've only thought of use cases where someone would want to override the version, not the deployment, but I think if we only allow overriding the version within the same deployment (as we do here), then the comment above VersioningOverride needs to reflect that, so I suggested a change there

we only allow overriding the version within the same deployment

@carlydf Is this what we want though? I was thinking we'd allow to pin to any version in case users want to move pinned workflows to other deployments.

The idea is that user only passes the version and that's enough because server can lookup deployment name based on the version. IMHO, It'd be additional friction and more room for error if user has to pass both values.

yeah I think moving to other deployments is good! I forgot that version was unique within a namespace and was just trying to think flexibly

antlai-temporal · 2025-01-15T22:43:38Z

temporal/api/workflowservice/v1/request_response.proto

+    // `WorkerDeploymentOptions`.
+    string deployment_version = 10;


Do we need the deployment_name?

we don't strictly need it but now I'm passing a struct containing both version and name.

antlai-temporal · 2025-01-15T22:47:36Z

temporal/api/workflowservice/v1/request_response.proto

+    // Deprecated.
    temporal.api.deployment.v1.Deployment deployment = 6;


I wonder if for future-proofing we should add deployment_options on the activity task responses/failures even if
the current server does not use it...

We've had this since V1 always thinking "it might become useful" but it only added churn to the APIs during the iterations. I think if want to add them in future for a known purpose we'd then add them to the next SDK release. Better not to be speculative.

Added depoyment_version now.

carlydf · 2025-01-16T00:55:17Z

temporal/api/workflow/v1/message.proto

    temporal.api.enums.v1.VersioningBehavior behavior = 1;

    // Required if behavior is `PINNED`. Must be null if behavior is `AUTO_UPGRADE`.
    // Identifies the worker deployment to pin the workflow to.


Suggested change

// Identifies the worker deployment to pin the workflow to.

// Identifies the worker deployment version to pin the workflow to.

cretz · 2025-01-16T13:33:49Z

Can we wait on the implementation being basically complete and nearer to a server release (even RC/cloud release) before merging this to master? Just want to make sure we give as long of time needed to change our minds during implementation.

Shivs11

no blockers - just some nit comments

Shivs11 · 2025-01-16T15:17:42Z

temporal/api/common/v1/message.proto

+// Worker Deployment options set in SDK that need to be sent to server in every poll.
+message WorkerDeploymentOptions {
+    // Required. Worker deployment name.
+    string name = 1;


should we call this deployment_name? Might be too pedantic, I fear, but I like the extra clarity it brings - just my two cents!

I think the host message name makes it clear: WorkerDeploymentOptions.

Shivs11 · 2025-01-16T15:22:56Z

temporal/api/enums/v1/deployment.proto

+    // Deployment Versions with this mode can be set as the Current or Ramping
+    // Version of their Deployment and hence are the best option for Blue/Green
+    // and Rainbow strategies (but typically not suitable for Rolling upgrades.)
+    WORKFLOW_VERSIONING_MODE_VERSIONING_BEHAVIORS = 2;


In the case of WORKFLOW_VERSIONING_MODE_UNVERSIONED, the comments speak about the use-case of users requiring to use patching to keep things non-breaking. For long running unpinned workflows going through changes, users still are required to use patching to keep things non-breaking. The lack of a mention of patching requirements for WORKFLOW_VERSIONING_MODE_VERSIONING_BEHAVIORS almost made me think if I don't need to patch my workflows if I use this behaviour - Should we mention the fact that patching is still required for AutoUpgrade long running wfs?

good point I clarified

Shivs11 · 2025-01-16T15:43:20Z

temporal/api/enums/v1/workflow.proto

+    // execution to another Deployment Version.
+    // Activities of `PINNED` workflows are sent to the same Deployment Version. Exception to this
+    // would be when the activity Task Queue workers are not present in the workflow's Deployment
+    // Version, in which case the activity will be sent to a different Deployment Version according


nit: in which case the activity will be sent to the Deployment Version containing the activity's task-queues

Shivs11 · 2025-01-16T15:44:56Z

temporal/api/enums/v1/workflow.proto

+    // present in the workflow's Deployment Version, in which case, the activity will be sent to a
+    // different Deployment Version according to the activity's Task Queue situation.


nit: in which case the activity will be sent to the Deployment Version containing the activity's task-queues

Shivs11 · 2025-01-16T15:51:39Z

temporal/api/workflow/v1/message.proto

+    // The target version of the transition. Absent means a so-far-versioned workflow is
+    // transitioning to unversioned workers.
+    string target_version = 1;


Since versions are unique within a namespace, I don't think specifying the deployment_name becomes necessary - I also am not sure if we are allowing deployment transitions to happen across namespaces but if we are not, I think having just the target_version should suffice

happy to be pointed in the right direction if my understanding is wrong!

ShahabT · 2025-01-16T17:32:33Z

@cretz OK, I changed the base to a feature branch for now.

ShahabT · 2025-01-16T17:33:33Z

@antlai-temporal I added the WorkerDeploymentVersion struct and addressed you comments.

…PI's (#515) _**READ BEFORE MERGING:** All PRs require approval by both Server AND SDK teams before merging! This is why the number of required approvals is "2" and not "1"--two reviewers from the same team is NOT sufficient. If your PR is not approved by someone in BOTH teams, it may be summarily reverted._  **What changed?** - Updated protos to reflect changes based on the recent conclusion regarding naming conventions for versioning-3 - `Deployment` -> `WorkerDeploymentVersion` - Added comments to remove pre-release versioning-3 code. Note: This API does not update pre-release versioning-3 changes related to dataplane. That currently is being worked in separate [PR](#514). The aim of this PR is to update those protos which are required by the deployment entity workflows so that work on those can commence there ASAP. Additionally, the updates to the proto are not finalized and will bring in more additions. Right now, they contain the bare minimum fields/structs required to make the deployment entity workflows run. With that being said, care has been taken to not add fields here and delete them in the near future. In other words, future changes will only bring in additions to these new protos.  **Why?** - Versioning-3  **Breaking changes**  **Server PR** - The Server PR is currently being built. Note, the server PR will only contain renamings (to agree with the namings of this PR) so I don't think that should serve as a blocker for reviewing this. --------- Co-authored-by: Shahab Tajik <[email protected]>

…oyment-version # Conflicts: # openapi/openapiv2.json # openapi/openapiv3.yaml

Add data plane fields w/ renamed Worker Versioning concepts

d6dc1e6

ShahabT requested review from a team as code owners January 14, 2025 23:51

ShahabT requested review from antlai-temporal and carlydf January 14, 2025 23:52

missing renames

6ee6f71

cretz reviewed Jan 15, 2025

View reviewed changes

Shivs11 mentioned this pull request Jan 15, 2025

New Deployment protos to work with already-implemented versioning 3 API's #515

Merged

add [deprecated = true]

a8e0fbe

carlydf approved these changes Jan 15, 2025

View reviewed changes

ShahabT and others added 3 commits January 15, 2025 14:51

Update temporal/api/enums/v1/workflow.proto

babd1b2

Co-authored-by: Carly de Frondeville <[email protected]>

Update temporal/api/workflow/v1/message.proto

e4ab0fe

Co-authored-by: Carly de Frondeville <[email protected]>

Update temporal/api/workflow/v1/message.proto

592a0b4

Co-authored-by: Carly de Frondeville <[email protected]>

antlai-temporal reviewed Jan 15, 2025

View reviewed changes

cretz approved these changes Jan 15, 2025

View reviewed changes

carlydf reviewed Jan 16, 2025

View reviewed changes

Shivs11 approved these changes Jan 16, 2025

View reviewed changes

add WorkerDeploymentVersion message

0c83aa7

ShahabT changed the base branch from master to versioning-3.1 January 16, 2025 17:30

comment updates

42ba48a

ShahabT added 2 commits January 16, 2025 12:42

Merge remote-tracking branch 'origin/versioning-3.1' into shahab/depl…

a3facbb

…oyment-version # Conflicts: # openapi/openapiv2.json # openapi/openapiv3.yaml

make

4dea02c

ShahabT merged commit 3635656 into versioning-3.1 Jan 16, 2025
3 checks passed

ShahabT deleted the shahab/deployment-version branch January 16, 2025 20:43

		// Version, in which case the activity will be sent to a different Deployment Version according
		// to the activity's Task Queue situation.

		// Note that if `versioning_override.behavior`.
		string deployment_version = 6;

		// `WorkerDeploymentOptions`.
		string deployment_version = 10;

		// Deprecated.
		temporal.api.deployment.v1.Deployment deployment = 6;

	// Identifies the worker deployment to pin the workflow to.
	// Identifies the worker deployment version to pin the workflow to.

		// present in the workflow's Deployment Version, in which case, the activity will be sent to a
		// different Deployment Version according to the activity's Task Queue situation.

Add data plane fields w/ renamed Worker Versioning concepts #514

Add data plane fields w/ renamed Worker Versioning concepts #514

Uh oh!

Conversation

ShahabT commented Jan 14, 2025

Uh oh!

cretz left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cretz Jan 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cretz Jan 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

drewhoskins-temporal Jan 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cretz Jan 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cretz Jan 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ShahabT Jan 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cretz Jan 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

carlydf left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

antlai-temporal left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Shivs11 Jan 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

carlydf Jan 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

cretz left a comment •

edited

Loading

cretz Jan 15, 2025 •

edited

Loading

cretz Jan 15, 2025 •

edited

Loading

drewhoskins-temporal Jan 15, 2025 •

edited

Loading

cretz Jan 15, 2025 •

edited

Loading

cretz Jan 15, 2025 •

edited

Loading

ShahabT Jan 16, 2025 •

edited

Loading

cretz Jan 16, 2025 •

edited

Loading

Shivs11 Jan 16, 2025 •

edited

Loading

carlydf Jan 16, 2025 •

edited

Loading