Add Deployment Pre-Release APIs #479

ShahabT · 2024-11-17T21:22:13Z

What changed?
Added the following APIs:

DescribeDeployment
ListDeployments
GetCurrentDeployment
SetCurrentDeployment (low-level placeholder for Rollout API)
GetDeploymentReachability

Why?
Needed for Worker Versioning V3.

Breaking changes

Server PR

cretz

Mostly just small things, nothing major.

temporal/api/deployment/v1/message.proto

temporal/api/history/v1/message.proto

temporal/api/workflowservice/v1/request_response.proto

temporal/api/deployment/v1/message.proto

temporal/api/workflowservice/v1/request_response.proto

temporal/api/deployment/v1/message.proto

antlai-temporal · 2024-11-18T21:22:30Z

temporal/api/enums/v1/deployment.proto

+    // The deployment does not have a special status.
+    DEPLOYMENT_STATUS_NO_STATUS = 1;


Why do we need this? is UNSPECIFIED not enough? Clarify in the comments...

antlai-temporal · 2024-11-18T21:29:34Z

temporal/api/deployment/v1/message.proto

+
+// Used as part of Deployment write APIs to update metadata attached to a deployment.
+message UpdateDeploymentMetadata {
+    map<string, bytes> upsert_entries = 1;


Do we need values to be binary or just string. It could be more user friendly if they were all strings.

Ended up making them Payload similar to the memo in workflows and schedules.

temporal/api/deployment/v1/message.proto

temporal/api/enums/v1/deployment.proto

temporal/api/workflow/v1/message.proto

carlydf · 2024-11-18T21:30:14Z

temporal/api/workflowservice/v1/request_response.proto

+        deployment.v1.Deployment deployment = 1;
+        enums.v1.DeploymentStatus status = 2;
+        google.protobuf.Timestamp status_change_time = 3;
+    }


why redefine DeploymentInfo without metadata and task_queue_infos here?

so the list is smaller? is the idea that if the caller wants more detailed info they would subsequently call DescribeDeployment?

Also we don't want to make the list API expensive. For example, TQ info will not be immediately accessible from Visibility.

Can you call it something different to avoid confusion, like DeploymentSummary or DeploymentBriefs or something like that...

This also seems odd to me. Why do users care if this API is expensive?

@drewhoskins-temporal are you suggesting the list should return complete info of all deployments?

I can change the name to DeploymentListEntry or DeploymentSummary. Same pattern is used for ListSchedules where we return a abbreviated set of info in the list response:

api/temporal/api/schedule/v1/message.proto

Line 372 in a3ed90f

// ScheduleListInfo is an abbreviated set of values from Schedule and ScheduleInfo

Changed it to DeploymentListInfo similar to schedules.

temporal/api/workflow/v1/message.proto

temporal/api/workflowservice/v1/request_response.proto

temporal/api/workflowservice/v1/service.proto

mfateev · 2024-11-18T22:09:19Z

temporal/api/deployment/v1/message.proto

+        string name = 1;
+        temporal.api.enums.v1.TaskQueueType type = 2;
+        // When server saw the first poller for this task queue in this deployment.
+        google.protobuf.Timestamp first_poller_time = 3;


I would add last_poller_time if technically possible. Can be updated lazily.

👍 That's possible and I agree we should have it. Will create a task for it (can't do right away because of more technical work).

mfateev · 2024-11-18T22:10:35Z

temporal/api/deployment/v1/message.proto

+        string name = 1;
+        temporal.api.enums.v1.TaskQueueType type = 2;
+        // When server saw the first poller for this task queue in this deployment.
+        google.protobuf.Timestamp first_poller_time = 3;


I don't think it belongs here. As this time is per build id. Or this message should be renamed to the DeploymentTaskQueueInfo

The message is already nested in DeploymentInfo so I thought it's less verbose like this. But please lmk if you think it should still be renamed to DeploymentTaskQueueInfo.

Message names are top level. So someone looking at the name will not see where it is used.

@mfateev This message name is nested, not top level. Are suggesting we should not have nested message definitions and only top level?

mfateev · 2024-11-18T22:16:21Z

temporal/api/enums/v1/deployment.proto

+    // The deployment is the ramping deployment for the deployment series.
+    DEPLOYMENT_STATUS_RAMPING = 2;
+    // The deployment is the current deployment for the deployment series.
+    DEPLOYMENT_STATUS_CURRENT = 3;


Should we consider a future when we are able to use a different current deployment for different workflow types or task queues? To me it looks like we need a separate Table/Message/API to get all Start/ContinueAsNew records that point to deployments.

We've thought about some of these cases:

for task queues, at least on our current roadmap, we'll insist that users create a different deployment series if they want to rollout task queues separately. (Note that we will support the ability to move a task queue from one deployment series to another) We don't see this as being a big burden for such an advanced use case.

for workflow types, we've decided to lean on the UpdateWorkflowOptions for these power-user scenarios for now. (or they can create a different deployment series) But you could imagine adding more fine-grained control to the rollout API. (i.e. instead of a single % ramp, we could provide various ramps for different types. Shahab mentioned there could be multi-cursor problems to think about with such, though). In that world, we could keep the high-level status of DEPLOYMENT_STATUS_RAMPING and create a more fine-grained API for power users. So, it seems premature to think too deeply now.

Adding to @drewhoskins-temporal comments, basically we envision eventually adding task queue and workflow type granularity to Rollout so user can ramp them one-by-one. However, setting different current deployment for different workflow types or task queues is not currently on the road map and users should create new deployment series for such cases.

mfateev · 2024-11-18T22:19:34Z

temporal/api/workflow/v1/message.proto

-        // `behavior_override` is PINNED. Takes precedence over `deployment`.
-        temporal.api.common.v1.WorkerDeployment deployment_override = 4;
+        temporal.api.deployment.v1.Deployment deployment = 2;
+        // Manual override for execution's versioning behavior. Takes precedence over `behavior`


I don't like "Manual" as it can be done by a script through an API. May be call it something like "WorkflowSpecificOverride" or something similar?

WorkflowExecutionOverride maybe? I feel like "WorkflowSpecific" is a bit ambiguous because "workflow" could mean Workflow Execution or Workflow Type/Workflow Definition

Got rid of the "manual" in the field name and comment. Mentioned "execution-specific" in the comment. Renamed the message to VersioningOverride as discussed before. Also renamed the field name to be consistent to the message name.

mfateev · 2024-11-18T22:21:21Z

temporal/api/workflowservice/v1/request_response.proto

+    }
+}
+
+message SetCurrentDeploymentRequest {


I don't think this API should exist as all changes to a deployment should happen through a rollout.

@mfateev I've been meaning to mention to you--for pre-release, we're planning to ship this lower-level API. We're working on the design for the rollout API for public preview, at which time we will support ramp pre-deploy testing as part of a bigger rollout API. Then, we can deprecate this API.
The team is currently focused on execution, but expect to review the rollout API design in December.

I annotated this and other APIs as "Experimental" in service.proto so it's clear that they all can change or be removed in the next release.

As @drewhoskins-temporal said, this one in particular is merely a low-level placeholder until the rollout API arrives before public preview.

drewhoskins-temporal · 2024-11-19T03:54:56Z

temporal/api/deployment/v1/message.proto

+    string build_id = 2;
+}
+
+// Holds information about a worker deployment.


this comment doesn't add much information. Should define "deployment"

Added more information for both Deployment and DeploymentInfo.

drewhoskins-temporal · 2024-11-19T04:03:28Z

temporal/api/deployment/v1/message.proto

+message DeploymentInfo {
+    Deployment deployment = 1;
+    temporal.api.enums.v1.DeploymentStatus status = 2;
+    google.protobuf.Timestamp last_became_current_time = 3;


This is confusingly worded and perhaps the concept isn't quite right.
Would it be 0 if status is not equal to current?
And why do users care about that specific status transition, but not the others?

It's unclear to me where timestamps should go--on the rollout, on the deployment, or both?
For example, we might link to the associated rollouts, once we have that.

Do we need this for pre-release? Maybe cut for now, or just stick with the normal "created time" and we can add stuff later.

Initially it was more generic and was called status_change_time. But I realized for internal use while there is no rollout yet, we also need timestamp of the last time that a deployment becomes current. But you are right, that info should not go to the public API. Will revert this to status_change_time.

drewhoskins-temporal · 2024-11-19T04:07:40Z

temporal/api/deployment/v1/message.proto

+// Holds information about a worker deployment.
+message DeploymentInfo {
+    Deployment deployment = 1;
+    temporal.api.enums.v1.DeploymentStatus status = 2;


This seems like a denormalization of the rollout status. Which might be fine, but it's also blurring the lines between a rollout and a deployment in a way that's a bit confusing. (why isn't it RolloutStatus) ?
And it's making deployments stateful when perhaps rollouts are the stateful thing.
I'm okay thinking of this as a placeholder for pre-release, but let's flag this to revisit.

Thinking about this more, until we design the state machine, maybe best is just to expose an is_current boolean that we can choose to deprecate. It might be more challenging and confusing to evolve if we put in this enum, which might be wrong in some way. (wrong name, wrong states, sitting on the wrong object)

I suspected that in the UI, for example, we would want to label statuses such as RAMPING and CURRENT so the Describe and List APIs might want to return these info in some format. It does not necessarily mean that the Deployment itself is stateful, as the status could be logically populated based on the Rollout records.

For now, I can get rid of the status and the change timestamp if you prefer. That means for now the only way for the user to get currentness of a deployment is to compare it with the result of GetCurrentDeployment.

Yeah, I buy the user scenario, and I think this design may well be valid, but it just needs a design pass.
I think users would like to see a created timestamp on this object so they know when the command to create the deploy got run (and ideally, by whom, for auditing purposes). I presume that would be the sort order and it's a fairly straightforward field that doesn't have any design considerations, and would give people an intuition for these rollouts until we figure out what we want to do with state tracking.

Up to you if you want to completely remove the status/changed status fields for now. I'm okay adding the bool for pre-release.

cretz

Only one additional minor suggestion, otherwise LGTM (knowing it's going into non master and may be adjusted as implementation dictates)

cretz · 2024-11-19T13:31:33Z

temporal/api/deployment/v1/message.proto

+    google.protobuf.Timestamp last_became_current_time = 3;
+    // A user-defined set of key-values. Can be updated as part of write operations to the
+    // deployment, such as `SetCurrentDeployment`.
+    map<string, string> metadata = 4;


Should consider making the values payloads if users should be allowed to encrypt this information

Makes sense. changed it to Payload, now it is consistent with workflow and schedules memo.

drewhoskins-temporal · 2024-11-19T18:20:34Z

temporal/api/enums/v1/deployment.proto

+//
+// (-- api-linter: core::0216::synonyms=disabled
+//     aip.dev/not-precedent: This enum specifies the status of the deployment among all deployments
+//     in the same series, therefore `DeploymentStatus` is a better name that `DeploymentState`. --)


I don't understand the reasoning here.

getting rid of this enum for now. just putting an is_current bool in DeploymentInfo until we finish the rollout API design.

drewhoskins-temporal · 2024-11-19T18:49:55Z

temporal/api/workflowservice/v1/request_response.proto

+    string series_name = 4;
+}
+
+message ListDeploymentsResponse {


Can we clarify the ordering of the results in a comment here?

This is going to be roughly sorted based on deployment creation time, decending. Although if a deployment has a very high number of TQs (or very high number of events happening to it) it will CaN and the sort will be mixed up. In reality we are relying on Visibility sort which is based on WF start time.

I prefer to keep this unspecified until we have a guaranteed order in the future.

Something's not right here. I don't see a use case for listing all the deployments, intermixed, regardless of deployment name. The two scenarios I can think of are users asking:
a) what are the deployment series? (or perhaps the current deployment for each series)
b) what's the history of a given deployment series?

If users do want them all in one endpoint, or if we want that for our UI, it would be a list of lists. But that seems puntable for the unblock , here.

I also question the value of an undisclosed, flaky ordering. Users will naturally expect a time-reverse ordering regardless of what we say here.
I would say a total ordering should be a hard requirement.
Is the scenario you mentioned very likely? Is it sufficiently unlikely to ship for the pre-release and then task up the improvement?

I believe the shuffle is sufficiently unlikely and we can fix it after pre-release. It also won't be a total shuffle, one/few over active deployment will be at the top, the fact that they are over active might mean they are also new builds.
Regarding list of lists, I agree such a UI will make sense and likely preferable. This is something I actually tried to bring up and get attention to but was not successful. Hopefully, it will be discussed during UI work and I'll be OK changing the list APIs then.

drewhoskins-temporal · 2024-11-19T18:55:54Z

temporal/api/deployment/v1/message.proto

+// Holds information about a worker deployment.
+message DeploymentInfo {
+    Deployment deployment = 1;
+    temporal.api.enums.v1.DeploymentStatus status = 2;


Yeah, I buy the user scenario, and I think this design may well be valid, but it just needs a design pass.
I think users would like to see a created timestamp on this object so they know when the command to create the deploy got run (and ideally, by whom, for auditing purposes). I presume that would be the sort order and it's a fairly straightforward field that doesn't have any design considerations, and would give people an intuition for these rollouts until we figure out what we want to do with state tracking.

Up to you if you want to completely remove the status/changed status fields for now. I'm okay adding the bool for pre-release.

temporal/api/workflowservice/v1/request_response.proto

Add Deployment APIs

aad202a

ShahabT requested review from carlydf, Shivs11 and antlai-temporal November 17, 2024 21:22

ShahabT requested review from a team as code owners November 17, 2024 21:22

Move UpdateDeploymentMemo to deployment dir

12ec5b2

cretz reviewed Nov 18, 2024

View reviewed changes

Shivs11 approved these changes Nov 18, 2024

View reviewed changes

Shivs11 reviewed Nov 18, 2024

View reviewed changes

temporal/api/deployment/v1/message.proto Outdated Show resolved Hide resolved

Address feedback & rename deployment_name

bf6ec80

Shivs11 reviewed Nov 18, 2024

View reviewed changes

temporal/api/deployment/v1/message.proto Show resolved Hide resolved

Shivs11 approved these changes Nov 18, 2024

View reviewed changes

antlai-temporal reviewed Nov 18, 2024

View reviewed changes

temporal/api/deployment/v1/message.proto Outdated Show resolved Hide resolved

carlydf reviewed Nov 18, 2024

View reviewed changes

antlai-temporal reviewed Nov 18, 2024

View reviewed changes

temporal/api/workflow/v1/message.proto Outdated Show resolved Hide resolved

antlai-temporal reviewed Nov 18, 2024

View reviewed changes

temporal/api/workflowservice/v1/request_response.proto Outdated Show resolved Hide resolved

antlai-temporal reviewed Nov 18, 2024

View reviewed changes

temporal/api/workflowservice/v1/service.proto Show resolved Hide resolved

antlai-temporal approved these changes Nov 18, 2024

View reviewed changes

mfateev reviewed Nov 18, 2024

View reviewed changes

carlydf approved these changes Nov 18, 2024

View reviewed changes

Addressed comments

a432c29

drewhoskins-temporal reviewed Nov 19, 2024

View reviewed changes

cretz approved these changes Nov 19, 2024

View reviewed changes

drewhoskins-temporal reviewed Nov 19, 2024

View reviewed changes

antlai-temporal mentioned this pull request Nov 19, 2024

Support the Deployment API temporalio/sdk-go#1713

Closed

Shivs11 reviewed Nov 19, 2024

View reviewed changes

temporal/api/workflowservice/v1/request_response.proto Show resolved Hide resolved

Addressed feedback

d3bd873

ShahabT changed the title ~~Add Deployment APIs~~ Add Deployment Pre-Release APIs Nov 19, 2024

improve VersioningOverride comments

6421777

ShahabT merged commit d4826db into versioning-3 Nov 19, 2024
2 checks passed

ShahabT deleted the shahab/deployment-apis branch November 19, 2024 23:55

		// The deployment does not have a special status.
		DEPLOYMENT_STATUS_NO_STATUS = 1;

Add Deployment Pre-Release APIs #479

Add Deployment Pre-Release APIs #479

Conversation

ShahabT commented Nov 17, 2024 • edited Loading

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

drewhoskins-temporal Nov 19, 2024 • edited Loading

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

Choose a reason for hiding this comment

drewhoskins-temporal Nov 19, 2024 • edited Loading

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

drewhoskins-temporal Nov 18, 2024 • edited Loading

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

drewhoskins-temporal Nov 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cretz left a comment • edited Loading

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

ShahabT Nov 19, 2024 • edited Loading

Choose a reason for hiding this comment

drewhoskins-temporal Nov 19, 2024 • edited Loading

Choose a reason for hiding this comment

drewhoskins-temporal Nov 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ShahabT commented Nov 17, 2024 •

edited

Loading

drewhoskins-temporal Nov 19, 2024 •

edited

Loading

drewhoskins-temporal Nov 19, 2024 •

edited

Loading

drewhoskins-temporal Nov 18, 2024 •

edited

Loading

drewhoskins-temporal Nov 19, 2024 •

edited

Loading

cretz left a comment •

edited

Loading

ShahabT Nov 19, 2024 •

edited

Loading

drewhoskins-temporal Nov 19, 2024 •

edited

Loading

drewhoskins-temporal Nov 19, 2024 •

edited

Loading