New Worker Versioning API #337

ShahabT · 2023-12-12T19:24:16Z

What changed?

Deprecating UpdateWorkerBuildIdCompatibility and GetWorkerBuildIdCompatibility and replacing them with UpdateWorkerVersioningRules and ListWorkerVersioningRules.
Deprecating the concept of Version Set and "default Version Set", replacing them with more flexible BuildIdAssignmentRule and CompatibleBuildIdRedirectRule concepts.
The assignment rules assign a Build ID to new (Unassigned) Workflow Executions and Activities.
The redirect rules are only used in context of compatible Build IDs. These rules apply to tasks assigned to a particular Build ID (source_build_id) to redirect them to another compatible Build ID (target_build_id).

Why?

Be able to support Gradual rollout for Canary, Blue/Green, Rolling deployments (and alike)
- Rolling: ability to send new and/or existing executions to the new build based on the worker count of new vs old build.
- Blue/Green: ability to send a particular percentage of the new executions to the new build
- Canary: ability to filter new executions based on “VersioningHint” Search Attribute set by Starter
Version Set is not the right abstraction for the Build ID compatibility relationships, replaced it with CompatibleBuildIdRedirectRule which:
- Do not loose the directionality: a redirect is from a source Build ID to a target Build ID
- Are removable (useful in cases that the compatibility was claimed by mistake)
- Are more powerful: redirect rules support worker-ratio-based ramps. Other ramp types, dry run mode, and filter can be added in the future.
Making the API Incompatible-first: The primary beneficiary of WV are use cases with presumed-incompatible build IDs because WV frees them from caring about compatibility. They should be able to use WV without having to learn or think about Version Sets which currently takes the center stage of the API. Now these use cases can use BuildIdAssignmentRule with independent Build IDs.

Breaking changes
The UpdateWorkerBuildIdCompatibility and GetWorkerBuildIdCompatibility become deprecated, but supported for one version beside the new API.

bergundy · 2023-12-12T20:12:51Z

temporal/api/common/v1/message.proto

@@ -92,6 +92,12 @@ message ActivityType {
    string name = 1;
 }

+message KeywordFilter {


I would strongly recommend we use search attributes for this and would also prefer if we don't introduce this in the initial version of this API to minimize the scope.

we'll use search attributes. this is to be able to filter based on those search attributes. Given that the search attribute eliminates the extra SDK side work for this feature, I though we can add it to initial version to have a stronger message for supporting canary deployments. but I'm open to commenting it out rn and add it later.

Could we simplify this even more: each assignment rule has a single optional string, that's an exact match for the hint search attribute? No excludes, no multiple includes. You can get the same effect as multiple includes by adding multiple assignment rules.

temporal/api/workflowservice/v1/request_response.proto

temporal/api/common/v1/message.proto

temporal/api/taskqueue/v1/message.proto

temporal/api/workflowservice/v1/request_response.proto

dnr · 2023-12-12T20:58:49Z

temporal/api/common/v1/message.proto

@@ -92,6 +92,12 @@ message ActivityType {
    string name = 1;
 }

+message KeywordFilter {


Could we simplify this even more: each assignment rule has a single optional string, that's an exact match for the hint search attribute? No excludes, no multiple includes. You can get the same effect as multiple includes by adding multiple assignment rules.

temporal/api/taskqueue/v1/message.proto

temporal/api/workflowservice/v1/request_response.proto

cretz

NOTE: My approval is purely on syntax. Would appreciate not using it as the "SDK team approve" and get another, more-versioning-familiar SDK team member to provide logical approval.

bergundy · 2023-12-13T23:24:48Z

temporal/api/taskqueue/v1/message.proto

+// The subset is defined by:
+//    - Including or excluding particular values of the `VersioningHint` Search
+//      Attribute. This is useful for Canary deployments, for example, when
+//      only the executions with `versioning_hint="canary"` should be sent to


Looks like this comment should be updated.

bergundy · 2023-12-13T23:25:13Z

temporal/api/taskqueue/v1/message.proto

+
+    // Filter executions based on the "VersioningHint" Search Attribute set by
+    // the Starter or parent Workflow.
+    string hint_filter = 2;


Can you explain how this is supposed to be used? Is this a visibility query?

the idea is to have a pre-defined SA such as "VersioningHint" that user can use. History sends only that SA to matching (if present). If we support any SA, then History has to send all the SA's to matching.

bergundy · 2023-12-14T00:02:38Z

temporal/api/taskqueue/v1/message.proto

+message BuildIdAssignmentRule {
+    string target_build_id = 1;
+
+    // Filter executions based on the "VersioningHint" Search Attribute set by


I thought we want to filter on any search attribute. Not on a specific one.

Also a note against search attributes, we can use this to assign activity tasks.

I think this feature requires more thought and I would remove it from the API entirely for now.

We'll do this at the very end. in the meanwhile we can finalize the design. the current thought is to use a pre-defined SA specific for this purpose.

For activities, maybe we can send the wf's hint to matching automatically. not sure if there is any need to set a different hint for the activity when it comes to deployment use cases.

bergundy · 2023-12-14T00:06:49Z

temporal/api/workflowservice/v1/service.proto

+    rpc UpdateWorkerVersioningRules (UpdateWorkerVersioningRulesRequest) returns (UpdateWorkerVersioningRulesResponse) {}
+
+    // Fetches the Build ID assignment and redirect rules for a Task Queue
+    rpc ListWorkerVersioningRules (ListWorkerVersioningRulesRequest) returns (ListWorkerVersioningRulesResponse) {


I wonder if Get is a more appropriate verb than list here.
Our other list APIs imply pagination of identifiable objects.

I don't have an opinion. will wait to see what others have to say.

bergundy · 2023-12-14T00:07:45Z

Not sure if mentioned before, please do not merge this until the feature is implemented.

ShahabT · 2023-12-14T01:06:10Z

temporal/api/taskqueue/v1/message.proto

+        // is automatically set as the ratio of the target Build ID's active
+        // workers over all active workers for any Build ID used in the
+        // assignment rules.
+        RampByWorkerRatio worker_ratio_ramp = 4;


Changed this part to support a ramp based on worker count instead of the contingent mode. Feeding target build ID as much as it can could have undesired consequences. The ramp based on ratio of new vs old workers is better in the following ways:
◦ does not overwhelm new workers when they are not tuned well
◦ when operating with a lot of headroom, ramp % still remains close to upgrade %
◦ you don’t end up in a situation that more wfs are moved to the new build than it can really handle. For example, if wf pollers are plenty but activity pollers are not (which may often be the case) you’d still end up moving a lot of workflows without having capacity to process their activities.

ShahabT · 2024-04-10T21:49:16Z

API Changes merged from versioning-2 feature branch to main in #393

New Worker Versioning API

68f772c

ShahabT requested review from a team as code owners December 12, 2023 19:24

ShahabT added 2 commits December 12, 2023 11:53

lint

1888f26

update hint_filter comment

9068239

bergundy reviewed Dec 12, 2023

View reviewed changes

cretz reviewed Dec 12, 2023

View reviewed changes

dnr reviewed Dec 12, 2023

View reviewed changes

ShahabT added 3 commits December 12, 2023 14:18

address comments

3c9cd7a

address comments

f98d019

fix hint comment

3a7ecad

cretz approved these changes Dec 13, 2023

View reviewed changes

bergundy reviewed Dec 14, 2023

View reviewed changes

replace contingent with RampByWorkerRatio

0a49b6f

ShahabT commented Dec 14, 2023

View reviewed changes

Merge branch 'master' into shahab/worker-versioning-2

5695b0c

ShahabT closed this Apr 10, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New Worker Versioning API #337

New Worker Versioning API #337

ShahabT commented Dec 12, 2023 •

edited

Loading

bergundy Dec 12, 2023

ShahabT Dec 12, 2023

dnr Dec 12, 2023 •

edited

Loading

dnr Dec 12, 2023 •

edited

Loading

cretz left a comment

bergundy Dec 13, 2023

bergundy Dec 13, 2023

ShahabT Dec 14, 2023

bergundy Dec 14, 2023

bergundy Dec 14, 2023

bergundy Dec 14, 2023

ShahabT Dec 14, 2023

bergundy Dec 14, 2023

ShahabT Dec 14, 2023

bergundy commented Dec 14, 2023

ShahabT Dec 14, 2023

ShahabT commented Apr 10, 2024

New Worker Versioning API #337

New Worker Versioning API #337

Conversation

ShahabT commented Dec 12, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dnr Dec 12, 2023 • edited Loading

Choose a reason for hiding this comment

dnr Dec 12, 2023 • edited Loading

Choose a reason for hiding this comment

cretz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bergundy commented Dec 14, 2023

Choose a reason for hiding this comment

ShahabT commented Apr 10, 2024

ShahabT commented Dec 12, 2023 •

edited

Loading

dnr Dec 12, 2023 •

edited

Loading

dnr Dec 12, 2023 •

edited

Loading