-
Notifications
You must be signed in to change notification settings - Fork 88
Add Create and Update for Worker Deployment Version, update scaler config and enable per-task-type compute providers #731
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.
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
base: serverless
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
Large diffs are not rendered by default.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -11,15 +11,34 @@ option csharp_namespace = "Temporalio.Api.Compute.V1"; | |
|
|
||
| import "temporal/api/compute/v1/provider.proto"; | ||
| import "temporal/api/compute/v1/scaler.proto"; | ||
| import "temporal/api/enums/v1/task_queue.proto"; | ||
|
|
||
| // ComputeConfig stores configuration that helps a worker control plane | ||
| // controller understand *when* and *how* to respond to worker lifecycle | ||
| // events. | ||
| message ComputeConfig { | ||
| // Stores instructions for a worker control plane controller how to respond | ||
| // to worker lifeycle events. | ||
| temporal.api.compute.v1.ComputeProvider provider = 1; | ||
| // Informs a worker lifecycle controller *when* and *how often* to perform | ||
| // certain worker lifecycle actions like starting a serverless worker. | ||
| temporal.api.compute.v1.ComputeScaler scaler = 2; | ||
| message ScalingGroup { | ||
| // Optional. The set of task queue types this scaling group serves. | ||
| // If not provided, this scaling group serves all not otherwise defined | ||
| // task types. | ||
| repeated temporal.api.enums.v1.TaskQueueType task_queue_types = 1; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would empty list be supported? if so, would it mean a catch-all?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes empty would be supported and catch-all - updated the text |
||
|
|
||
| // Stores instructions for a worker control plane controller how to respond | ||
| // to worker lifeycle events. | ||
| temporal.api.compute.v1.ComputeProvider provider = 3; | ||
|
|
||
| // Informs a worker lifecycle controller *when* and *how often* to perform | ||
| // certain worker lifecycle actions like starting a serverless worker. | ||
| temporal.api.compute.v1.ComputeScaler scaler = 4; | ||
| } | ||
|
|
||
| // Each scaling group describes a compute config for a specific subset of the worker | ||
| // deployment version: covering a specific set of task types and/or regions. | ||
| // Having different configurations for different task types, allows independent | ||
| // tuning of activity and workflow task processing (for example). | ||
| // | ||
| // The key of the map is the ID of the scaling group used to reference it in subsequent | ||
| // update calls. | ||
| map<string, ScalingGroup> scaling_groups = 1; | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -23,15 +23,13 @@ message ComputeProvider { | |
| string type = 1; | ||
| // Contains provider-specific instructions and configuration. | ||
| oneof detail { | ||
| // will be an unencrypted, JSON-encoded object of provider-specific | ||
| // information | ||
| // Unencrypted, JSON-encoded object of provider-specific information | ||
| // (-- api-linter: core::0146::any=disabled | ||
| // aip.dev/not-precedent: This needs to be extensible to | ||
| // externally-written compute providers --) | ||
| string detail_json = 2; | ||
| // will be an encrypted, encoded bytestring containing | ||
| // provider-specific information. The implementation must understand | ||
| // how to decrypt the payload. | ||
| // Encrypted, encoded bytestring containing provider-specific | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This would be optionally encrypted. |
||
| // information. The implementation must understand how to decrypt the payload. | ||
| temporal.api.common.v1.Payload detail_payload = 3; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Use only payload please, there's a straightforward way of expressing plain JSON in a payload.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @bergundy got a pointer by any chance? |
||
| } | ||
| // Optional. If the compute provider is a Nexus service, this should point | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -9,14 +9,26 @@ option java_outer_classname = "ScalerProto"; | |
| option ruby_package = "Temporalio::Api::Compute::V1"; | ||
| option csharp_namespace = "Temporalio.Api.Compute.V1"; | ||
|
|
||
| import "temporal/api/common/v1/message.proto"; | ||
|
|
||
| // ComputeScaler instructs the Temporal Service when to scale up or down the number of | ||
| // Workers that comprise a WorkerDeployment. | ||
| message ComputeScaler { | ||
| // The lower limit for the number of Workers (in the WorkerDeployment) to | ||
| // which the ComputeScaler can scale down. | ||
| int32 min_instances = 1; | ||
| // The upper limit for the number of Workers (in the WorkerDeployment) to | ||
| // which the ComputeScaler can scale up. Must be greater than or equal to | ||
| // min_instances. | ||
| int32 max_instances = 2; | ||
| // Type of the compute scaler. this string is implementation-specific and | ||
| // can be used by implementations to understand how to interpret the | ||
| // contents of the scaler_details field. | ||
| string type = 1; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: use upper case letters when starting sentences to be consistent with the rest of the API.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done |
||
|
|
||
| // Contains scaler-specific instructions and configuration. | ||
| oneof detail { | ||
| // Unencrypted, JSON-encoded object of scaler-specific information | ||
| // (-- api-linter: core::0146::any=disabled | ||
| // aip.dev/not-precedent: This needs to be extensible to | ||
| // externally-written compute scalers --) | ||
| string detail_json = 2; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why not use payload always? Payload with
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The If even with that context, you believe this should always be a payload, happy to change it (this was specific feedback we had received on the first API PR). |
||
|
|
||
| // Encrypted, encoded bytestring containing scaler-specific | ||
| // information. The implementation must understand how to decrypt the payload. | ||
| temporal.api.common.v1.Payload detail_payload = 3; | ||
|
Comment on lines
+28
to
+32
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same here, no need for both of these fields. |
||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1268,10 +1268,10 @@ message GetSystemInfoResponse { | |
| // This flag is dependent both on server version and for Nexus to be enabled via server configuration. | ||
| bool nexus = 11; | ||
|
|
||
| // True if the server supports serverless deployments. | ||
| // This flag is dependent both on server version and for serverless deployments | ||
| // True if the server supports server-scaled deployments. | ||
| // This flag is dependent both on server version and for server-scaled deployments | ||
| // to be enabled via server configuration. | ||
| bool serverless_deployments = 12; | ||
| bool server_scaled_deployments = 12; | ||
|
|
||
| } | ||
| } | ||
|
|
@@ -2353,10 +2353,6 @@ message CreateWorkerDeploymentRequest { | |
| // this name already exists, an error will be returned. | ||
| string deployment_name = 2; | ||
|
|
||
| // Optional. Contains the new worker compute configuration for the Worker | ||
| // Deployment. Used for worker scale management. | ||
| temporal.api.compute.v1.ComputeConfig compute_config = 3; | ||
|
|
||
| // Optional. The identity of the client who initiated this request. | ||
| string identity = 4; | ||
| // A unique identifier for this create request for idempotence. Typically UUIDv4. | ||
|
|
@@ -2396,6 +2392,32 @@ message ListWorkerDeploymentsResponse { | |
| } | ||
| } | ||
|
|
||
| // Creates a new WorkerDeploymentVersion. | ||
| message CreateWorkerDeploymentVersionRequest { | ||
| string namespace = 1; | ||
| // Required. | ||
| temporal.api.deployment.v1.WorkerDeploymentVersion deployment_version = 2; | ||
|
|
||
| // Optional. Contains the new worker compute configuration for the Worker | ||
| // Deployment. Used for worker scale management. | ||
| temporal.api.compute.v1.ComputeConfig compute_config = 4; | ||
|
|
||
| // Optional. The identity of the client who initiated this request. | ||
| string identity = 3; | ||
|
|
||
| // A unique identifier for this create request for idempotence. Typically UUIDv4. | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Worth noting how this is used, does the server dedupe requests with the same idempotency key?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @ShahabT can you confirm the details so I am not mis-representing here?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. here is the behavior, we can document:
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Updated the comment with Shahab's notes |
||
| // If a second request with the same ID is recieved, it is considered a successful no-op. | ||
| // Retrying with a different request ID for the same deployment name + build ID is an error. | ||
| string request_id = 5; | ||
| } | ||
|
|
||
| message CreateWorkerDeploymentVersionResponse { | ||
| // This value is returned so that it can be optionally passed to APIs that | ||
| // write to the WorkerDeployment state to ensure that the state did not | ||
| // change between this API call and a future write. | ||
| bytes conflict_token = 1; | ||
| } | ||
|
|
||
| // Used for manual deletion of Versions. User can delete a Version only when all the | ||
| // following conditions are met: | ||
| // - It is not the Current or Ramping Version of its Deployment. | ||
|
|
@@ -2430,6 +2452,58 @@ message DeleteWorkerDeploymentRequest { | |
| message DeleteWorkerDeploymentResponse { | ||
| } | ||
|
|
||
| // Used to update the compute config of a Worker Deployment Version. | ||
| message UpdateWorkerDeploymentVersionComputeConfigRequest { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Shouldn't this take a conflict token and a ComputeConfig object?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What about a request ID?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done - both |
||
| string namespace = 1; | ||
|
|
||
| // Required. | ||
| temporal.api.deployment.v1.WorkerDeploymentVersion deployment_version = 2; | ||
|
|
||
| // Optional. Contains the compute config scaling groups to add or updated for the Worker | ||
| // Deployment. | ||
| map<string, temporal.api.compute.v1.ComputeConfig.ScalingGroup> compute_config_scaling_groups = 6; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would recommend using field masks for update APIs. We've done that in a few places (see UpdateActivityOptions). This way if you add fields that the client is not aware of they will not accidentally be deleted on update.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @ShahabT ? This was what other APIs around here already used. |
||
|
|
||
| // Optional. Contains the compute config scaling groups to remove from the Worker Deployment. | ||
| repeated string remove_compute_config_scaling_groups = 7; | ||
|
Comment on lines
+2466
to
+2467
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not a huge fan of separating out a "remove" list from an "add/update" list, but if this is a pattern used elsewhere in the API, ok :) I prefer a Replace-style pattern where the user passes the exact desired state, including any existing list or map fields and the server simply replaces those fields with the supplied values. Additionally, having a But again, if this is not a pattern used in the existing API, so be it :)
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, this was changed from replace-style to add/update based on feedback what is used elsewhere in the API |
||
|
|
||
| // Optional. The identity of the client who initiated this request. | ||
| string identity = 3; | ||
|
|
||
| // A unique identifier for this create request for idempotence. Typically UUIDv4. | ||
| // If a second request with the same ID is recieved, it is considered a successful no-op. | ||
| // Retrying with a different request ID for the same deployment name + build ID is an error. | ||
| string request_id = 4; | ||
|
|
||
| // This value is returned so that it can be optionally passed to APIs | ||
| // that write to the Worker Deployment state to ensure that the state | ||
| // did not change between this API call and a future write. | ||
| bytes conflict_token = 5; | ||
| } | ||
|
|
||
| message UpdateWorkerDeploymentVersionComputeConfigResponse { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we need to return a conflict token here?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done |
||
| // This value is returned so that it can be optionally passed to APIs that | ||
| // write to the WorkerDeployment state to ensure that the state did not | ||
| // change between this API call and a future write. | ||
| bytes conflict_token = 1; | ||
| } | ||
|
|
||
| // Used to validate the compute config without attaching it to a Worker Deployment Version. | ||
| message ValidateWorkerDeploymentVersionComputeConfigRequest { | ||
| string namespace = 1; | ||
|
|
||
| // Required. | ||
| temporal.api.deployment.v1.WorkerDeploymentVersion deployment_version = 2; | ||
|
|
||
| // Required. Contains the new worker compute configuration for the Worker Deployment. | ||
| temporal.api.compute.v1.ComputeConfig compute_config = 4; | ||
|
|
||
| // Optional. The identity of the client who initiated this request. | ||
| string identity = 3; | ||
| } | ||
|
|
||
| message ValidateWorkerDeploymentVersionComputeConfigResponse { | ||
| } | ||
|
|
||
| // Used to update the user-defined metadata of a Worker Deployment Version. | ||
| message UpdateWorkerDeploymentVersionMetadataRequest { | ||
| string namespace = 1; | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I would flatten this out to a top level message if there's any chance that this message will be reused in the future.