Worker Deployment Versioning #466

Sushisource · 2025-04-24T21:57:16Z

What was changed

Added the annotations and options for worker-deployment-based versioning

Why?

New feature in all SDKs

Checklist

Closes [Feature Request] Support New Worker Versioning API #437
How was this tested:
New & existing tests
Any docs updates needed?

Sushisource · 2025-04-30T23:58:58Z

Ready for review, but will have one small addition of version override options on start workflow after temporalio/api#579 is merged

src/Temporalio.Extensions.Hosting/TemporalWorkerServiceId.cs

src/Temporalio.Extensions.Hosting/TemporalWorkerService.cs

src/Temporalio.Extensions.Hosting/TemporalWorkerServiceId.cs

cretz · 2025-05-01T15:54:49Z

src/Temporalio/Worker/TemporalWorkerOptions.cs

        /// <c>Assembly.GetEntryAssembly().ManifestModule.ModuleVersionId</c>.
        /// </summary>
+        /// <remarks>Exclusive with <see cref="DeploymentOptions"/>.</remarks>
+        [Obsolete("Use DeploymentOptions instead")]


Is this obsolete? People may still want this build ID even without versioning, correct?

We're going to move people towards using a deployment, even if versioning is off.

I see. Then I think the AddHostedTemporalWorker(this IServiceCollection services, string taskQueue, string? buildId = null) overload of the DI creator should be delegating to the WorkerDeploymentOptions overload instead of setting this option.

It can't - it would have to make up a deployment name. I'm deprecating it per the other comment (let's try to keep related ones like that together on one thread).

That was my concern with deprecating BuildId, we're deprecating BuildId-only without a replacement. People use this field and now they're going to have to set their code to ignore deprecation because they don't have a dot in their build ID where they can convert. I'm not sure it's fair to make people make up a deployment name for no reason or benefit to them just to use build ID.

That's a product decision, I'm just implementing it. At the end of the day I agree with it though. Defining an application/deployment name isn't exactly a huge burden. You do it one time and you're done.

It's not whether it's a burden, it's whether it's necessary for us to retain the existing Temporal behavior they are used to that doesn't relate to this. I can bring this up internally, not blocking by any means.

src/Temporalio/Workflows/WorkflowDefinitionOptions.cs

src/Temporalio/Workflows/WorkflowDefinition.cs

cretz · 2025-05-01T18:28:21Z

tests/Temporalio.Tests/TestUtils.cs

    }
-}
+
+    public static async Task<Temporalio.Api.WorkflowService.V1.SetWorkerDeploymentRampingVersionResponse>


Arguably this utility and that last one doesn't provide much value vs inlining the call heh

cretz · 2025-05-01T18:29:26Z

tests/Temporalio.Tests/TestUtils.cs

+            });
+    }
+
+    public static async Task<Temporalio.Api.WorkflowService.V1.DescribeWorkerDeploymentResponse>


I'd personally toss this in AssertMore and call it EventuallyWorkerDeploymentPresentAsync or something, but no biggie

cretz

Added a few more comments based on recent updates, but older unaddressed comments may still apply

cretz · 2025-05-01T22:10:33Z

src/Temporalio.Extensions.Hosting/TemporalHostingServiceCollectionExtensions.cs

        public static ITemporalWorkerServiceOptionsBuilder AddHostedTemporalWorker(
            this IServiceCollection services, string taskQueue, string? buildId = null)
        {
+            // TODO: Review - We probably need to deprecate buildId param & add deployment version?


If the BuildId worker option is deprecated, this should be too IMO

cretz · 2025-05-01T22:12:48Z

src/Temporalio.Extensions.Hosting/TemporalHostingServiceCollectionExtensions.cs

+#pragma warning disable 0618
                    options.BuildId = buildId;
+#pragma warning restore 0618


If this is deprecated, does that mean that there's a way to use WorkerDeploymentOptions to represent "just build ID"? Otherwise, how do I represent just build ID in a non-deprecated way if I don't care about deployment name and won't ever enable versioning? And if I can do that, why does this not just call that overload?

Yes, it means that, just Build ID is deprecated. You use a fully-formed deployment name now

I think this is a bit rough on users who shouldn't have to care about deployment name to just have their existing build ID functionality remain

cretz · 2025-05-01T22:14:11Z

src/Temporalio.Extensions.Hosting/TemporalWorkerServiceOptions.cs

        /// </summary>
        /// <param name="taskQueue">Task queue.</param>
-        /// <param name="buildId">Build ID.</param>
+        /// <param name="version">Worker Depoyment version or Build ID.</param>


Suggested change

/// <param name="version">Worker Depoyment version or Build ID.</param>

/// <param name="version">Worker Deployment version or Build ID.</param>

cretz · 2025-05-01T22:14:49Z

src/Temporalio.Extensions.Hosting/TemporalWorkerService.cs

            IOptionsMonitor<TemporalWorkerServiceOptions> optionsMonitor,
            ITemporalClient? existingClient = null,
            ILoggerFactory? loggerFactory = null)
+        : this(


If the BuildId worker option is deprecated, this constructor should be too IMO

cretz · 2025-05-01T22:17:02Z

src/Temporalio.Extensions.Hosting/TemporalWorkerService.cs

+            }
+            else
+            {
+                if (options.DeploymentOptions?.Version?.ToCanonicalString() != serviceId.Version)


Should there be a check to ensure options.BuildId is null and options.UseVersioning is false here? Meaning, if they are using new versioning, shouldn't they not be setting the old values away from their default?

I don't think there needs to be, because if the versions match, then they couldn't possibly have set the other options on options, because they're exclusive.

I think my concern was if I was using new deployment options and have manually set old build ID, but I guess worker creation throws for that, no worries

cretz · 2025-05-01T22:22:08Z

src/Temporalio/Workflows/IWorkflowContext.cs

+        /// Gets the options defined by the <see cref="WorkflowDynamicOptionsAttribute" /> annotated
+        /// method, if any.
+        /// </summary>
+        WorkflowDefinitionOptions? DynamicDefinitionOptions { get; }


I don't think we need to expose this to users

cretz · 2025-05-01T22:28:02Z

src/Temporalio/Worker/WorkflowInstance.cs

+                        Console.WriteLine("Established versioning behavior: " + establishedVersioningBehavior);
+                        Console.WriteLine("Dynamic versioning behavior: " + dynamicDefinitionOptions?.VersioningBehavior);


Leftover stuff

cretz · 2025-05-01T22:30:37Z

src/Temporalio/Workflows/WorkflowDefinitionOptions.cs

+        /// <see cref="Exceptions.FailureException" /> + cancellation and suspend via task failure
+        /// all others. But this default may change in the future.
+        /// </remarks>
+        public Type[]? FailureExceptionTypes { get; init; }


I think options should be get/set for those using builder patterns (otherwise records are better because at least you can with them)

cretz · 2025-05-01T22:32:41Z

src/Temporalio/Worker/WorkflowInstance.cs

        }

+        /// <inheritdoc />
+        public WorkflowDefinitionOptions? DynamicDefinitionOptions


I was thinking this would be more like:

internal WorkflowDefinitionOptions DefinitionOptions { get { if (definitionOptions == null) { definitionOptions = new() { FailureExceptionTypes = Definition.FailureExceptionTypes, VersioningBehavior = Definition.VersioningBehavior, }; if (Definition.DynamicOptionsMethod != null) { var dynOptions = Definition.DynamicOptionsMethod.Invoke(Instance); if (dynOptions.FailureExceptionTypes != null) { definitionOptions.FailureExceptionTypes = dynOptions.FailureExceptionTypes; } if (dynOptions.VersioningBehavior != VersioningBehavior.Unspecified) { definitionOptions.VersioningBehavior = dynOptions.VersioningBehavior; } } } return definitionOptions; } }

and then DefinitionOptions can be used everywhere in code here and elsewhere. Though I could understand concern of it being accessed before instance made available. I wasn't expecting it to be user-facing.

And if you're always going to be initializing this in one place, no need to make it lazy.

Ah, ok, I get what you mean now, use that as a bag for the things set on definition too, sure, makes sense.

It can't exactly work this way because the failure types on the definition can be used before initialization ever even happens, as in ExecuteWorkflowAsync_BadInput_CanFailWorkflow, which was fun to discover. The idea is pretty much the same still though.

cretz · 2025-05-01T22:35:26Z

src/Temporalio/Worker/WorkflowInstance.cs

+                // Dynamic options method needs to be invoked at this point, after initting the
+                // workflow instance but before performing any activations.
+                _ = DynamicDefinitionOptions;


If you're always running this anyways, then you don't need to make it lazy IMO, can just set the definition options right here. Before I didn't think you cared about when you called the dynamic definition method, but I can see now it is better to be predictable. My concern was what happens if the dynamic options method accesses Workflow.Instance per #466 (comment)?

Yeah, that was exactly the point of doing it inline there. It needs to happen at the right time

Sushisource · 2025-05-02T01:03:42Z

src/Temporalio.Extensions.Hosting/TemporalHostingServiceCollectionExtensions.cs

            string clientNamespace,
            string taskQueue,
-            string? buildId = null) =>
+            string buildId) =>


In order to make these obsolete, I had to make them not take null for the last param, and instead have that happen on the new one. I believe that should be a compatible change still

With C# named parameters, it's technically not compatible since a user could have had buildId: null. Can you leave the string? type here (can still make the parameter required I believe).

cretz

Only piddly things remain I think then we're good

cretz · 2025-05-02T13:49:52Z

src/Temporalio/Worker/WorkflowInstance.cs

        private WorkflowUpdateDefinition? dynamicUpdate;
        private bool workflowInitialized;
        private bool applyModernEventLoopLogic;
+        private WorkflowDefinitionOptions definitionOptions;


Suggested change

private WorkflowDefinitionOptions definitionOptions;

private readonly WorkflowDefinitionOptions definitionOptions;

(and move to the readonly set of fields, unless I'm missing something)

cretz · 2025-05-02T13:51:53Z

src/Temporalio/Workflows/WorkflowDefinitionOptions.cs

+    /// Additional options for a workflow definition that may be returned by
+    /// a function annotated with <see cref="WorkflowDynamicOptionsAttribute"/>.
+    /// </summary>
+    public class WorkflowDefinitionOptions


Can this be cloneable? Not really important, just a pattern we've been doing for "options"

cretz · 2025-05-02T13:52:11Z

src/Temporalio/Worker/WorkflowInstance.cs

                            RunOnce(checkConditions);
                        }
+
+                        if (definitionOptions?.VersioningBehavior != null)


I don't think this condition could ever evaluate to false now

cretz · 2025-05-02T13:52:58Z

src/Temporalio/Worker/WorkflowInstance.cs

            e is FailureException ||
                e is OperationCanceledException ||
-                Definition.FailureExceptionTypes?.Any(t => t.IsAssignableFrom(e.GetType())) == true ||
+                definitionOptions?.FailureExceptionTypes?.Any(t => t.IsAssignableFrom(e.GetType())) == true ||


Suggested change

definitionOptions?.FailureExceptionTypes?.Any(t => t.IsAssignableFrom(e.GetType())) == true ||

definitionOptions.FailureExceptionTypes?.Any(t => t.IsAssignableFrom(e.GetType())) == true ||

cretz · 2025-05-02T13:56:14Z

src/Temporalio/Worker/WorkflowWorker.cs

+
+                if (options.DeploymentOptions?.UseWorkerVersioning == true)
+                {
+                    if (options.DeploymentOptions?.DefaultVersioningBehavior is null or VersioningBehavior.Unspecified


Interesting, I have not used or in this project before, but it is C# 9, so works

cretz · 2025-05-02T14:01:08Z

src/Temporalio/Workflows/WorkflowDefinition.cs

+        /// <summary>
+        /// Gets the dynamic options method.
+        /// </summary>
+        public Func<object?, WorkflowDefinitionOptions>? DynamicOptionsMethod { get; private init; }


Arguably not a "method" like "runMethod", more like a DynamicOptionsGetter like "creator", but doesn't really matter

cretz

LGTM, thanks! The concern over deprecating build ID for those users that don't care about deployment name is not .NET specific, so doesn't really affect this PR specifically.

Sushisource · 2025-05-02T21:17:06Z

Like TS, want to get temporalio/api#579 in so I can use it in this before merging, so I don't have to go back immediately after

cretz · 2025-05-02T21:26:32Z

No prob, can leave this hanging open as long as needed.

src/Temporalio/Common/Priority.cs

cretz

Semantic things only

cretz · 2025-05-14T17:07:38Z

src/Temporalio/Common/VersioningOverride.cs

+        public sealed record PinnedVersioningOverride(
+            WorkerDeploymentVersion Version,
+            PinnedOverrideBehavior Behavior = PinnedOverrideBehavior.Pinned) : VersioningOverride;
+
+        /// <summary>
+        /// The workflow will auto-upgrade to the current deployment version on the next workflow
+        /// task.
+        /// </summary>
+        public sealed record AutoUpgradeVersioningOverride() : VersioningOverride;


Suggested change

public sealed record PinnedVersioningOverride(

WorkerDeploymentVersion Version,

PinnedOverrideBehavior Behavior = PinnedOverrideBehavior.Pinned) : VersioningOverride;

/// <summary>

/// The workflow will auto-upgrade to the current deployment version on the next workflow

/// task.

/// </summary>

public sealed record AutoUpgradeVersioningOverride() : VersioningOverride;

public sealed record Pinned(

WorkerDeploymentVersion Version,

PinnedOverrideBehavior Behavior = PinnedOverrideBehavior.Pinned) : VersioningOverride;

/// <summary>

/// The workflow will auto-upgrade to the current deployment version on the next workflow

/// task.

/// </summary>

public sealed record AutoUpgrade() : VersioningOverride;

Probably don't need the VersioningOverride suffix on these

cretz · 2025-05-14T17:08:53Z

src/Temporalio/Common/VersioningOverride.cs

+        /// <summary>
+        /// Specifies different sub‐types of pinned override.
+        /// </summary>
+        public enum PinnedOverrideBehavior


Could also move this to be an inner class of Pinned called Behavior, leaving the only direct inner types of VersioningOverride to be implementations of it. Usually .NET doesn't have so many nested types, but in this case it makes sense I think

Sushisource force-pushed the deployment-versioning branch 5 times, most recently from 8232611 to 495e196 Compare April 30, 2025 23:43

Sushisource marked this pull request as ready for review April 30, 2025 23:58

Sushisource requested a review from a team as a code owner April 30, 2025 23:58

Sushisource force-pushed the deployment-versioning branch 2 times, most recently from 5d7f03c to 567c662 Compare May 1, 2025 17:13

cretz reviewed May 1, 2025

View reviewed changes

Sushisource force-pushed the deployment-versioning branch from 0c196a5 to 6fa38e5 Compare May 1, 2025 23:29

Sushisource commented May 2, 2025

View reviewed changes

Sushisource force-pushed the deployment-versioning branch from a34bcc0 to 04bf08e Compare May 2, 2025 01:47

cretz reviewed May 2, 2025

View reviewed changes

cretz approved these changes May 2, 2025

View reviewed changes

Sushisource mentioned this pull request May 2, 2025

Add Priorities #470

Merged

cretz reviewed May 5, 2025

View reviewed changes

src/Temporalio/Common/Priority.cs Outdated Show resolved Hide resolved

Sushisource added 9 commits May 8, 2025 16:40

Update Core

e7d2523

Add initial options / core plumbing

3d095c5

Regenerate codegenned files

54c699d

Hook up versioning strategy to bridge

cb7eb76

Add tests / set VB properly

ffb8cab

Add dynamic getter

83b8d44

Fix required check

cb4780d

Test fixes

b7cc06a

Add support for hosted service

bb41736

Sushisource added 6 commits May 8, 2025 16:41

Record / empty options allowances

8371411

Review comments

52726e1

Review comments round 2

c58f8eb

Review 3

ddfc684

Add Priorities (#470)

5f1ee95

Don't re-init default priorty

944a82c

Sushisource force-pushed the deployment-versioning branch from 2844cf0 to 944a82c Compare May 8, 2025 23:49

Sushisource added 3 commits May 13, 2025 16:04

Update core for new API changes

e142d64

Update protos

3a46309

Add versioning override

f43760f

Sushisource force-pushed the deployment-versioning branch from dc17057 to f43760f Compare May 14, 2025 00:38

cretz approved these changes May 14, 2025

View reviewed changes

Last bit of review

3617779

Sushisource merged commit cbf20f9 into main May 14, 2025
13 checks passed

Sushisource deleted the deployment-versioning branch May 14, 2025 18:24

	/// <param name="version">Worker Depoyment version or Build ID.</param>
	/// <param name="version">Worker Deployment version or Build ID.</param>

		Console.WriteLine("Established versioning behavior: " + establishedVersioningBehavior);
		Console.WriteLine("Dynamic versioning behavior: " + dynamicDefinitionOptions?.VersioningBehavior);

	private WorkflowDefinitionOptions definitionOptions;
	private readonly WorkflowDefinitionOptions definitionOptions;

	definitionOptions?.FailureExceptionTypes?.Any(t => t.IsAssignableFrom(e.GetType())) == true \|\|
	definitionOptions.FailureExceptionTypes?.Any(t => t.IsAssignableFrom(e.GetType())) == true \|\|

Worker Deployment Versioning #466

Worker Deployment Versioning #466

Uh oh!

Conversation

Sushisource commented Apr 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What was changed

Why?

Checklist

Uh oh!

Sushisource commented Apr 30, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cretz May 1, 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!

Sushisource May 1, 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 May 2, 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 May 2, 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!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cretz 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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Sushisource May 2, 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!

Sushisource commented Apr 24, 2025 •

edited

Loading

cretz May 1, 2025 •

edited

Loading

Sushisource May 1, 2025 •

edited

Loading

cretz May 2, 2025 •

edited

Loading

cretz May 2, 2025 •

edited

Loading

Sushisource May 2, 2025 •

edited

Loading

cretz May 2, 2025 •

edited

Loading