try to delete version if we are at limit #7271

carlydf · 2025-02-06T06:11:18Z

What changed?

Make updates and signals use args
Try to delete oldest to newest version if we hit limit

Why?

How did you test it?

New functional test

Potential risks

Documentation

Is hotfix candidate?

## What changed?  - Updated deployment entity wf's API's (naming) for versioning-3.1. Removed those which will not be required. - Added functionality for RegisterTaskQueue + DescribeVersion - Added functional tests for verifying the above two work. - UserData stuff has been removed right now since there are pending discussions ## Why?  - Versioning-3.1 ## How did you test it?  - Functional tests have been added. Note, the tests added are exact replicas (in terms of their core functionality) with the existing tests under `deployment_test.go` ## Potential risks  - None, feature branch. ## Documentation  ## Is hotfix candidate?  - No

## What changed?  Using new Deployment Options fields sent by SDK in Versioning 3 functionality. Old fields are still used when new fields are absent. Implementation did not change, both new and old fields sent in polls and task responses are still converted to old `Deployment` object and used as before. Later, code will be refactored to change the `Deployment` usages to `DeploymentVersion`. Also added new fields to replace `Deployment` with `DeploymentVersion` fields in internal protos where needed. Matching<->History communication happens via these new fields, only new internal fields are written but both new and old fields are read. ## Why?  Incorporating latest renames in Versioning APIs. ## How did you test it?  Existing tests changed to use new fields (or both old and new depending on the test). ## Potential risks  None. ## Documentation  None yet. ## Is hotfix candidate?  No.

## What changed?  - DescribeWorkerDeployment API for versioning-3.1 For this API to work, I realized that the worker deployment workflow has to have information about the various deployment versions it encapsulates. Now that version deployment workflows won't have a `deploymentName` present in their workflowID (for now), partial matching workflow ID's with the received deployment name to target version workflows was not feasible. Another option was to store the mapping from "versions" to "the local state of a version workflow". However, this would have made the deployment workflow arguments large in size since in theory, we could have many deployment versions each with large sized local states. The option I ended up going with was storing the version in a deployment workflow during task-queue registration. During a `DescribeWorkerDeployment` call, we would call `Describe` on each stored version and return the aggregated info to the user. A version will be removed from a worker deployment when it becomes "scavenged" (to be implemented) Potential future improvements include adding a caching mechanism. ## Why?  - Versioning-3.1 ## How did you test it?  - Added a functional test + more tests to follow ## Potential risks  - None, going inside a feature. ## Documentation  ## Is hotfix candidate?  - No

## What changed?  Matching to Worker Deployment Ramping Version and its percentage when routing tasks. User-facing APIs are not implemented yet. The implementation also supports ramping from a Deployment Version to unversioned workers. ## Why?  Part of Worker Versioning features. ## How did you test it?  Added unit and functional tests. ## Potential risks  None. ## Documentation  None. ## Is hotfix candidate?  No --------- Co-authored-by: Shivam Saraf <shivam.saraf@temporal.io>

## What changed?  - Fix flaky `DescribeWorkerDeployment`. The flake was caused because I forgot to pass in the right parameters when creating version entity workflows. Moreover, recent proto changes meant that my assertions also had to be updated and have done so :) - Other breaking unit tests in the versioning-3.1 repo ## Why?  - don't like seeing red markers after CI is done running ## How did you test it?  - Ran unit and functional tests ## Potential risks  - None, going to a feature. ## Documentation  ## Is hotfix candidate?

## What changed?  - title ## Why?  - versioning-3.1 ## How did you test it?  - existing suite of tests + added functional tests ## Potential risks  - None, feature branch ## Documentation  ## Is hotfix candidate?  - No

## What changed?  - ListWorkerDeployments API - Also removed presence of locks when adding a version to a worker-deployment workflow. Causes deadlocks and we don't need them. ## Why?  - Versioning-3.1 ## How did you test it?  - Added functional tests (happy-path) - Existing suite ## Potential risks  - None, feature ## Documentation  ## Is hotfix candidate?  - No

## What changed? Add DrainageStatus child workflow to worker deployment system ## Why? To periodically update the version workflow with results from visibility. ## How did you test it? Functional tests. Currently broken (see todo comment in code) ## Potential risks  ## Documentation  ## Is hotfix candidate?  --------- Co-authored-by: Shivam Saraf <shivam.saraf@temporal.io>

…, fix comments (#7178) ## What changed?  ## Why?  ## How did you test it?  ## Potential risks  ## Documentation  ## Is hotfix candidate?

## What changed?  - Ramping PR for versioning-3.1 - now, if a ramping version becomes current, the ramp will be unset. ## Why?  - Versioning-3.1 ## How did you test it?  - Added more functional tests - Existing suite ## Potential risks  - None, going to feature. ## Documentation  ## Is hotfix candidate?  No

## What changed?  - Deployment BuildId's are no longer unique across a namespace. The combination of <DeploymentName, BuildID> will be. - Constraints for not allowing "/" and "__" in deploymentName and buildID respectively - We want the APIs to leave open the possibility of using a custom version id instead of deployment_name/build_id to set current or ramping. Also we want to accept the "__unversioned__" string without having to accep an "unversioned version" . To support this, refactored APIs to accept version strings. Refactored internal code to use build id string for build id only, and version string for fully-qualified version string only. ## Why?  - Versioning-3.1 ## How did you test it?  - Existing suite of tests ## Potential risks  ## Documentation  ## Is hotfix candidate?  --------- Co-authored-by: Carly de Frondeville <carly.defrondeville@temporal.io>

## What changed?  ## Why?  ## How did you test it?  ## Potential risks  ## Documentation  ## Is hotfix candidate?  --------- Co-authored-by: Shivam <57200924+Shivs11@users.noreply.github.com>

## What changed?  Updated code to use latest API changes in which Deployment Versions are represented by string fields rather than structs. `WorkerDeploymentVersion` proto message still exists but only for the internal APIs. ## Why?  See temporalio/api#547. ## How did you test it?  ## Potential risks  ## Documentation  ## Is hotfix candidate?  --------- Co-authored-by: Carly de Frondeville <carly.defrondeville@temporal.io>

## What changed?  ## Why?  ## How did you test it?  ## Potential risks  ## Documentation  ## Is hotfix candidate?  --------- Co-authored-by: Shivam <57200924+Shivs11@users.noreply.github.com> Co-authored-by: Shahab Tajik <shahab@temporal.io> Co-authored-by: Shivam Saraf <shivam.saraf@temporal.io> Co-authored-by: ShahabT <shahab.tajik@temporal.io>

## What changed?  ## Why?  ## How did you test it?  ## Potential risks  ## Documentation  ## Is hotfix candidate?

## What changed?  - added poller presence (called MissingTaskQueue in code) checks when a version starts to ramp or wants to be set as current - happy path tests for delete version have been added - happy path tests for poller presence checks have been added ## Why?  - versioning-3.1 ## How did you test it?  - NO TESTS have been added - just an initial prototype for review. ## Potential risks  ## Documentation  ## Is hotfix candidate?  --------- Co-authored-by: Carly de Frondeville <carly.defrondeville@temporal.io>

… wf (#7240) ## What changed?  ## Why?  ## How did you test it?  ## Potential risks  ## Documentation  ## Is hotfix candidate?

…kQueueFamilyData (#7234) ## What changed? 1. Pass Task Queue Types to `CheckIfTaskQueuesHavePollers` activity so that we check pollers for only the task queue types that are registered in that version. 2. Remove `DeploymentVersionData` from the per-type map in `TaskQueueFamilyData`. `DeploymentVersionData` stores the same info as `VersionLocalState`, so repeating it num_task_queue times inside the VersionLocalState was a waste of space, and kind of confusing. I spoke with Shahab yesterday about how `TaskQueueFamilyData` should contain information that is specific to each task queue type within that version. So far, the only data that is specific to the version + task queue tuple is `first_poller_time`, so I put that in a new `TaskQueueVersionData` struct and put it in `TaskQueueFamilyData`. ## Why? See explanation above. ## How did you test it? Tested that the existing Versioning 3.1 functional tests still work, including DeleteVersion with poller presence. ## Potential risks  ## Documentation  ## Is hotfix candidate?

… wf (#7240) ## What changed?  ## Why?  ## How did you test it?  ## Potential risks  ## Documentation  ## Is hotfix candidate?

## What changed? - The initial current version of a worker deployment is now `"__unversioned__"` instead of `""` - When the ramping version of a worker deployment becomes unversioned, the deployment tells all the task queues in the current version that they now have an unversioned ramp. - If the unversioned ramp is promoted to current version, or if the unversioned ramping version is unset, the deployment tells all the task queues of the previous current version that they no longer have an unversioned ramp. ## Why?  ## How did you test it? - Made sure all existing WorkerDeploymentSuite and DeploymentVersionSuite tests pass - Wrote new tests in WorkerDeploymentSuite for the specific edge cases that this code handles: - `TestSetCurrentVersion_Unversioned_NoRamp`: Test that when current version changes from versioned -> unversioned, the task queues become unversioned - `TestSetCurrentVersion_Unversioned_PromoteUnversionedRamp`: Test that when the current version changes from versioned -> unversioned and unversioned was previously ramping, the task queues become unversioned with no more unversioned ramp - `TestSetRampingVersion_Unversioned_UnversionedCurrent`: Test that this fails with "Ramping version __unversioned__ is already current" error - `TestSetRampingVersion_Unversioned_VersionedCurrent`: Test that the ramping version of the current version's task queues becomes unversioned ## Potential risks  ## Documentation  ## Is hotfix candidate?  --------- Co-authored-by: Shivam <57200924+shivs11@users.noreply.github.com> Co-authored-by: Shahab Tajik <shahab@temporal.io> Co-authored-by: Shivam Saraf <shivam.saraf@temporal.io> Co-authored-by: ShahabT <shahab.tajik@temporal.io>

## What changed?  ## Why?  ## How did you test it?  ## Potential risks  ## Documentation  ## Is hotfix candidate?

## What changed?  - Changed areas of code in the deployment client whereby we were using update-with-start instead of update ## Why?  - We don't want users to create workflows on operations like setCurrent/setRamping - we advice users to first create them by registering your workers ## How did you test it?  - Bunch of tests have been modified and the suites now pass ## Potential risks  ## Documentation  ## Is hotfix candidate?

## What changed?  Child workflows start in parent's pinned Worker Deployment Version. ## Why?  So user do not have to worry about interface compatibility between pinned parents and children. ## How did you test it?  Added new tests. ## Potential risks  ## Documentation  ## Is hotfix candidate?

## What changed?  Syncing TQ User Data for all types of a single TQ name at once to reduce the SetCurrent and SetRamping latency and flakiness of tests. ## Why?  ## How did you test it?  ## Potential risks  ## Documentation  ## Is hotfix candidate?  --------- Co-authored-by: Carly de Frondeville <carly.defrondeville@temporal.io>

## What changed?  - make deletes idempotent ## Why?  ## How did you test it?  - added functional tests - also changes to functional test to make them have a stricter consistency check ## Potential risks  ## Documentation  ## Is hotfix candidate?

carlydf · 2025-02-06T18:50:11Z

proto/internal/temporal/server/api/deployment/v1/message.proto

+// Sent from Drainage child workflow to Version parent
+message SyncDrainageInfoSignalArgs {
+    temporal.api.deployment.v1.VersionDrainageInfo drainage_info = 1;
+}
+
+// Sent from Version workflow to Worker Deployment workflow
+message SyncDrainageStatusSignalArgs {
+    string version = 1;
+    temporal.api.enums.v1.VersionDrainageStatus drainage_status = 2;
+}


the SyncDrainage...Args are not currently used, I wanted to keep this PR short, but I will have another PR using them so that we can keep the VersionSummaries up to date in the deployment WF

carlydf · 2025-02-06T18:50:38Z

tests/worker_deployment_version_test.go

+	fmt.Println("CARLY: AFTER DRAINAGE")
+
+	// try to add a version again, and it succeeds, after deleting the second version but not the third (both are eligible)
+	// TODO: This fails if I try to add tvMax again...


not sure if this is a problem

or, I think it is a problem but I'm not sure how to fix it. Hopefully in most cases when this is happening, at least one out of 100 versions will be eligible deletion so the first add would not fail.

I think it's not worth fixing this until everything else we need to do is done

ShahabT · 2025-02-06T19:00:00Z

service/worker/workerdeployment/workflow.go

+		}
+		return 0
+	})
+	for _, v := range d.State.Versions {


Do you want to check for version not being drained here, or that info is not sent to the deployment wf yet?

it's not sent to the deployment wf yet, but when I do that I will

## What changed? Make updates and signals use args Try to delete oldest to newest version if we hit limit ## Why?  ## How did you test it? New functional test ## Potential risks  ## Documentation  ## Is hotfix candidate?  --------- Co-authored-by: Shivam <57200924+shivs11@users.noreply.github.com> Co-authored-by: Shahab Tajik <shahab@temporal.io> Co-authored-by: Shivam Saraf <shivam.saraf@temporal.io> Co-authored-by: ShahabT <shahab.tajik@temporal.io>

Shivs11 and others added 30 commits February 5, 2025 16:57

Upgrade to latest api/versioning-3.1 with new build id -> version naming

f3f211b

Rename UpdateWorkerDeploymentVersionMetadata

b826136

UpdateWorkerDeploymentVersionMetadata

054dcb3

Cleanup Versioning internal protos

51fad8f

DeleteWorkerDeployment

22ebf9f

try to delete version if we are at limit

385fb60

carlydf requested a review from a team as a code owner February 6, 2025 06:11

carlydf marked this pull request as draft February 6, 2025 06:11

Fix versioning flaky tests

616671c

ShahabT force-pushed the versioning-3.1-merge branch from f356ee3 to 616671c Compare February 6, 2025 07:58

try to delete version if we are at limit

04cdaf1

ShahabT force-pushed the cdf/scavenger branch from 385fb60 to 04cdaf1 Compare February 6, 2025 08:16

ShahabT force-pushed the versioning-3.1-merge branch 2 times, most recently from 122d338 to 9140a68 Compare February 6, 2025 17:17

carlydf added 5 commits February 6, 2025 10:20

add protos and tests

df398ab

rebase everything except matching enginge

627443c

reset matching engine

1c883fb

restore defaults

d3544ef

rebase

7243d5f

carlydf marked this pull request as ready for review February 6, 2025 18:47

carlydf commented Feb 6, 2025

View reviewed changes

remove print

7ce4af6

ShahabT reviewed Feb 6, 2025

View reviewed changes

Shivs11 approved these changes Feb 6, 2025

View reviewed changes

carlydf merged commit 44afdad into versioning-3.1-merge Feb 6, 2025
47 of 49 checks passed

carlydf deleted the cdf/scavenger branch February 6, 2025 19:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

try to delete version if we are at limit #7271

try to delete version if we are at limit #7271

Uh oh!

carlydf commented Feb 6, 2025 •

edited

Loading

Uh oh!

carlydf Feb 6, 2025 •

edited

Loading

Uh oh!

carlydf Feb 6, 2025

Uh oh!

carlydf Feb 6, 2025

Uh oh!

carlydf Feb 6, 2025

Uh oh!

ShahabT Feb 6, 2025

Uh oh!

carlydf Feb 6, 2025

Uh oh!

Uh oh!

Uh oh!

try to delete version if we are at limit #7271

try to delete version if we are at limit #7271

Uh oh!

Conversation

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

Uh oh!

What changed?

Why?

How did you test it?

Potential risks

Documentation

Is hotfix candidate?

Uh oh!

carlydf Feb 6, 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!

carlydf Feb 6, 2025

Choose a reason for hiding this comment

Uh oh!

carlydf Feb 6, 2025

Choose a reason for hiding this comment

Uh oh!

carlydf Feb 6, 2025

Choose a reason for hiding this comment

Uh oh!

ShahabT Feb 6, 2025

Choose a reason for hiding this comment

Uh oh!

carlydf Feb 6, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

carlydf commented Feb 6, 2025 •

edited

Loading

carlydf Feb 6, 2025 •

edited

Loading