Add Conflict Tokens to Deployment APIs #7203
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
## What changed? <!-- Describe what has changed in this PR --> - 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? <!-- Tell your future self why have you made these changes --> - Versioning-3.1 ## How did you test it? <!-- How have you verified this change? Tested locally? Added a unit test? Checked in staging env? --> - 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 <!-- Assuming the worst case, what can be broken when deploying this change to production? --> - None, feature branch. ## Documentation <!-- Have you made sure this change doesn't falsify anything currently stated in `docs/`? If significant new behavior is added, have you described that in `docs/`? --> ## Is hotfix candidate? <!-- Is this PR a hotfix candidate or does it require a notification to be sent to the broader community? (Yes/No) --> - No
## What changed? <!-- Describe what has changed in this PR --> 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? <!-- Tell your future self why have you made these changes --> Incorporating latest renames in Versioning APIs. ## How did you test it? <!-- How have you verified this change? Tested locally? Added a unit test? Checked in staging env? --> Existing tests changed to use new fields (or both old and new depending on the test). ## Potential risks <!-- Assuming the worst case, what can be broken when deploying this change to production? --> None. ## Documentation <!-- Have you made sure this change doesn't falsify anything currently stated in `docs/`? If significant new behavior is added, have you described that in `docs/`? --> None yet. ## Is hotfix candidate? <!-- Is this PR a hotfix candidate or does it require a notification to be sent to the broader community? (Yes/No) --> No.
## What changed? <!-- Describe what has changed in this PR --> - 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? <!-- Tell your future self why have you made these changes --> - Versioning-3.1 ## How did you test it? <!-- How have you verified this change? Tested locally? Added a unit test? Checked in staging env? --> - Added a functional test + more tests to follow ## Potential risks <!-- Assuming the worst case, what can be broken when deploying this change to production? --> - None, going inside a feature. ## Documentation <!-- Have you made sure this change doesn't falsify anything currently stated in `docs/`? If significant new behavior is added, have you described that in `docs/`? --> ## Is hotfix candidate? <!-- Is this PR a hotfix candidate or does it require a notification to be sent to the broader community? (Yes/No) --> - No
## What changed? <!-- Describe what has changed in this PR --> 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? <!-- Tell your future self why have you made these changes --> Part of Worker Versioning features. ## How did you test it? <!-- How have you verified this change? Tested locally? Added a unit test? Checked in staging env? --> Added unit and functional tests. ## Potential risks <!-- Assuming the worst case, what can be broken when deploying this change to production? --> None. ## Documentation <!-- Have you made sure this change doesn't falsify anything currently stated in `docs/`? If significant new behavior is added, have you described that in `docs/`? --> None. ## Is hotfix candidate? <!-- Is this PR a hotfix candidate or does it require a notification to be sent to the broader community? (Yes/No) --> No --------- Co-authored-by: Shivam Saraf <[email protected]>
## What changed? <!-- Describe what has changed in this PR --> - 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? <!-- Tell your future self why have you made these changes --> - don't like seeing red markers after CI is done running ## How did you test it? <!-- How have you verified this change? Tested locally? Added a unit test? Checked in staging env? --> - Ran unit and functional tests ## Potential risks <!-- Assuming the worst case, what can be broken when deploying this change to production? --> - None, going to a feature. ## Documentation <!-- Have you made sure this change doesn't falsify anything currently stated in `docs/`? If significant new behavior is added, have you described that in `docs/`? --> ## Is hotfix candidate? <!-- Is this PR a hotfix candidate or does it require a notification to be sent to the broader community? (Yes/No) -->
## What changed? <!-- Describe what has changed in this PR --> - title ## Why? <!-- Tell your future self why have you made these changes --> - versioning-3.1 ## How did you test it? <!-- How have you verified this change? Tested locally? Added a unit test? Checked in staging env? --> - existing suite of tests + added functional tests ## Potential risks <!-- Assuming the worst case, what can be broken when deploying this change to production? --> - None, feature branch ## Documentation <!-- Have you made sure this change doesn't falsify anything currently stated in `docs/`? If significant new behavior is added, have you described that in `docs/`? --> ## Is hotfix candidate? <!-- Is this PR a hotfix candidate or does it require a notification to be sent to the broader community? (Yes/No) --> - No
## What changed? <!-- Describe what has changed in this PR --> - 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? <!-- Tell your future self why have you made these changes --> - Versioning-3.1 ## How did you test it? <!-- How have you verified this change? Tested locally? Added a unit test? Checked in staging env? --> - Added functional tests (happy-path) - Existing suite ## Potential risks <!-- Assuming the worst case, what can be broken when deploying this change to production? --> - None, feature ## Documentation <!-- Have you made sure this change doesn't falsify anything currently stated in `docs/`? If significant new behavior is added, have you described that in `docs/`? --> ## Is hotfix candidate? <!-- Is this PR a hotfix candidate or does it require a notification to be sent to the broader community? (Yes/No) --> - 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 <!-- Assuming the worst case, what can be broken when deploying this change to production? --> ## Documentation <!-- Have you made sure this change doesn't falsify anything currently stated in `docs/`? If significant new behavior is added, have you described that in `docs/`? --> ## Is hotfix candidate? <!-- Is this PR a hotfix candidate or does it require a notification to be sent to the broader community? (Yes/No) --> --------- Co-authored-by: Shivam Saraf <[email protected]>
…, fix comments (#7178) ## What changed? <!-- Describe what has changed in this PR --> ## Why? <!-- Tell your future self why have you made these changes --> ## How did you test it? <!-- How have you verified this change? Tested locally? Added a unit test? Checked in staging env? --> ## Potential risks <!-- Assuming the worst case, what can be broken when deploying this change to production? --> ## Documentation <!-- Have you made sure this change doesn't falsify anything currently stated in `docs/`? If significant new behavior is added, have you described that in `docs/`? --> ## Is hotfix candidate? <!-- Is this PR a hotfix candidate or does it require a notification to be sent to the broader community? (Yes/No) -->
## What changed? <!-- Describe what has changed in this PR --> - Ramping PR for versioning-3.1 - now, if a ramping version becomes current, the ramp will be unset. ## Why? <!-- Tell your future self why have you made these changes --> - Versioning-3.1 ## How did you test it? <!-- How have you verified this change? Tested locally? Added a unit test? Checked in staging env? --> - Added more functional tests - Existing suite ## Potential risks <!-- Assuming the worst case, what can be broken when deploying this change to production? --> - None, going to feature. ## Documentation <!-- Have you made sure this change doesn't falsify anything currently stated in `docs/`? If significant new behavior is added, have you described that in `docs/`? --> ## Is hotfix candidate? <!-- Is this PR a hotfix candidate or does it require a notification to be sent to the broader community? (Yes/No) --> No
## What changed? <!-- Describe what has changed in this PR --> ## Why? <!-- Tell your future self why have you made these changes --> ## How did you test it? <!-- How have you verified this change? Tested locally? Added a unit test? Checked in staging env? --> ## Potential risks <!-- Assuming the worst case, what can be broken when deploying this change to production? --> ## Documentation <!-- Have you made sure this change doesn't falsify anything currently stated in `docs/`? If significant new behavior is added, have you described that in `docs/`? --> ## Is hotfix candidate? <!-- Is this PR a hotfix candidate or does it require a notification to be sent to the broader community? (Yes/No) -->
## What changed? <!-- Describe what has changed in this PR --> - 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? <!-- Tell your future self why have you made these changes --> - Versioning-3.1 ## How did you test it? <!-- How have you verified this change? Tested locally? Added a unit test? Checked in staging env? --> - Existing suite of tests ## Potential risks <!-- Assuming the worst case, what can be broken when deploying this change to production? --> ## Documentation <!-- Have you made sure this change doesn't falsify anything currently stated in `docs/`? If significant new behavior is added, have you described that in `docs/`? --> ## Is hotfix candidate? <!-- Is this PR a hotfix candidate or does it require a notification to be sent to the broader community? (Yes/No) --> --------- Co-authored-by: Carly de Frondeville <[email protected]>
## What changed? <!-- Describe what has changed in this PR --> ## Why? <!-- Tell your future self why have you made these changes --> ## How did you test it? <!-- How have you verified this change? Tested locally? Added a unit test? Checked in staging env? --> ## Potential risks <!-- Assuming the worst case, what can be broken when deploying this change to production? --> ## Documentation <!-- Have you made sure this change doesn't falsify anything currently stated in `docs/`? If significant new behavior is added, have you described that in `docs/`? --> ## Is hotfix candidate? <!-- Is this PR a hotfix candidate or does it require a notification to be sent to the broader community? (Yes/No) --> --------- Co-authored-by: Shivam <[email protected]>
## What changed? <!-- Describe what has changed in this PR --> 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? <!-- Tell your future self why have you made these changes --> See temporalio/api#547. ## How did you test it? <!-- How have you verified this change? Tested locally? Added a unit test? Checked in staging env? --> ## Potential risks <!-- Assuming the worst case, what can be broken when deploying this change to production? --> ## Documentation <!-- Have you made sure this change doesn't falsify anything currently stated in `docs/`? If significant new behavior is added, have you described that in `docs/`? --> ## Is hotfix candidate? <!-- Is this PR a hotfix candidate or does it require a notification to be sent to the broader community? (Yes/No) --> --------- Co-authored-by: Carly de Frondeville <[email protected]>
Shivs11
reviewed
Feb 4, 2025
|
we should also probably test this out before getting into the branch - can be two simple happy-path tests and don't think they would be super tedious too |
carlydf
commented
Feb 4, 2025
carlydf
changed the title
Shivs11
approved these changes
Feb 4, 2025
| firstVersion := tv.WithBuildIDNumber(1) | ||
| secondVersion := tv.WithBuildIDNumber(2) | ||
|
|
||
| // Start deployment version workflow + worker-deployment workflow. Only one version is stared manually |
There was a problem hiding this comment.
nit: started
Also, struggling to understand the comment actually
There was a problem hiding this comment.
nice test!
maybe we can also have a test that errors out because an invalid conflict token has been passed? Not sure if this is tedious but might confirm if things are fully working or not
not a blocker if you think things are working though
ShahabT
reviewed
Feb 4, 2025
ShahabT
approved these changes
Feb 4, 2025
ShahabT
added a commit
that referenced
this pull request
Feb 5, 2025
## What changed? <!-- Describe what has changed in this PR --> ## Why? <!-- Tell your future self why have you made these changes --> ## How did you test it? <!-- How have you verified this change? Tested locally? Added a unit test? Checked in staging env? --> ## Potential risks <!-- Assuming the worst case, what can be broken when deploying this change to production? --> ## Documentation <!-- Have you made sure this change doesn't falsify anything currently stated in `docs/`? If significant new behavior is added, have you described that in `docs/`? --> ## Is hotfix candidate? <!-- Is this PR a hotfix candidate or does it require a notification to be sent to the broader community? (Yes/No) --> --------- Co-authored-by: Shivam <[email protected]> Co-authored-by: Shahab Tajik <[email protected]> Co-authored-by: Shivam Saraf <[email protected]> Co-authored-by: ShahabT <[email protected]>
ShahabT
added a commit
that referenced
this pull request
Feb 5, 2025
## What changed? <!-- Describe what has changed in this PR --> ## Why? <!-- Tell your future self why have you made these changes --> ## How did you test it? <!-- How have you verified this change? Tested locally? Added a unit test? Checked in staging env? --> ## Potential risks <!-- Assuming the worst case, what can be broken when deploying this change to production? --> ## Documentation <!-- Have you made sure this change doesn't falsify anything currently stated in `docs/`? If significant new behavior is added, have you described that in `docs/`? --> ## Is hotfix candidate? <!-- Is this PR a hotfix candidate or does it require a notification to be sent to the broader community? (Yes/No) --> --------- Co-authored-by: Shivam <[email protected]> Co-authored-by: Shahab Tajik <[email protected]> Co-authored-by: Shivam Saraf <[email protected]> Co-authored-by: ShahabT <[email protected]>
ShahabT
added a commit
that referenced
this pull request
Feb 6, 2025
## What changed? <!-- Describe what has changed in this PR --> ## Why? <!-- Tell your future self why have you made these changes --> ## How did you test it? <!-- How have you verified this change? Tested locally? Added a unit test? Checked in staging env? --> ## Potential risks <!-- Assuming the worst case, what can be broken when deploying this change to production? --> ## Documentation <!-- Have you made sure this change doesn't falsify anything currently stated in `docs/`? If significant new behavior is added, have you described that in `docs/`? --> ## Is hotfix candidate? <!-- Is this PR a hotfix candidate or does it require a notification to be sent to the broader community? (Yes/No) --> --------- Co-authored-by: Shivam <[email protected]> Co-authored-by: Shahab Tajik <[email protected]> Co-authored-by: Shivam Saraf <[email protected]> Co-authored-by: ShahabT <[email protected]>
ShahabT
added a commit
that referenced
this pull request
Feb 6, 2025
## What changed? <!-- Describe what has changed in this PR --> ## Why? <!-- Tell your future self why have you made these changes --> ## How did you test it? <!-- How have you verified this change? Tested locally? Added a unit test? Checked in staging env? --> ## Potential risks <!-- Assuming the worst case, what can be broken when deploying this change to production? --> ## Documentation <!-- Have you made sure this change doesn't falsify anything currently stated in `docs/`? If significant new behavior is added, have you described that in `docs/`? --> ## Is hotfix candidate? <!-- Is this PR a hotfix candidate or does it require a notification to be sent to the broader community? (Yes/No) --> --------- Co-authored-by: Shivam <[email protected]> Co-authored-by: Shahab Tajik <[email protected]> Co-authored-by: Shivam Saraf <[email protected]> Co-authored-by: ShahabT <[email protected]>
ShahabT
added a commit
that referenced
this pull request
Feb 6, 2025
## What changed? <!-- Describe what has changed in this PR --> ## Why? <!-- Tell your future self why have you made these changes --> ## How did you test it? <!-- How have you verified this change? Tested locally? Added a unit test? Checked in staging env? --> ## Potential risks <!-- Assuming the worst case, what can be broken when deploying this change to production? --> ## Documentation <!-- Have you made sure this change doesn't falsify anything currently stated in `docs/`? If significant new behavior is added, have you described that in `docs/`? --> ## Is hotfix candidate? <!-- Is this PR a hotfix candidate or does it require a notification to be sent to the broader community? (Yes/No) --> --------- Co-authored-by: Shivam <[email protected]> Co-authored-by: Shahab Tajik <[email protected]> Co-authored-by: Shivam Saraf <[email protected]> Co-authored-by: ShahabT <[email protected]>
ShahabT
added a commit
that referenced
this pull request
Feb 6, 2025
## What changed? <!-- Describe what has changed in this PR --> ## Why? <!-- Tell your future self why have you made these changes --> ## How did you test it? <!-- How have you verified this change? Tested locally? Added a unit test? Checked in staging env? --> ## Potential risks <!-- Assuming the worst case, what can be broken when deploying this change to production? --> ## Documentation <!-- Have you made sure this change doesn't falsify anything currently stated in `docs/`? If significant new behavior is added, have you described that in `docs/`? --> ## Is hotfix candidate? <!-- Is this PR a hotfix candidate or does it require a notification to be sent to the broader community? (Yes/No) --> --------- Co-authored-by: Shivam <[email protected]> Co-authored-by: Shahab Tajik <[email protected]> Co-authored-by: Shivam Saraf <[email protected]> Co-authored-by: ShahabT <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What changed?
Why?
How did you test it?
Potential risks
Documentation
Is hotfix candidate?