feat(BA-5280): Consolidate deploying handlers and remove unused sub-steps#10276
Merged
Conversation
github-actions
Bot
added
size:L
Contributor
There was a problem hiding this comment.
Pull request overview
This PR refactors strategy application so that sub-step transitions (and rolled-back cleanup) are no longer written as side-effects by the strategy applier, aligning responsibilities with the coordinator/rollback handler in preparation for the rolling update strategy (BEP-1049).
Changes:
- Removed
assignments/rolled_back_ids-driven DB writes fromapply_strategy_mutations()across applier/repository/db_source. - Added a dedicated
clear_deploying_revision()API on applier/repository/db_source to cleardeploying_revision+sub_stepfor rolled-back deployments. - Updated unit tests to assert that sub-step-only summaries (and rolled-back-only summaries) skip
apply_strategy_mutations()DB calls.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
| tests/unit/manager/sokovan/deployment/strategy/test_applier.py | Updates tests to reflect that sub-step-only and rolled-back-only summaries should not trigger strategy DB mutations. |
| src/ai/backend/manager/sokovan/deployment/strategy/applier.py | Stops passing assignments/rolled-back IDs into DB mutations; adds clear_deploying_revision() delegation method. |
| src/ai/backend/manager/repositories/deployment/repository.py | Narrows apply_strategy_mutations() to route + revision swap; adds repository-level clear_deploying_revision(). |
| src/ai/backend/manager/repositories/deployment/db_source/db_source.py | Removes _update_sub_steps() / rolled-back cleanup from transaction; adds a standalone clear_deploying_revision() transaction. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
Comment on lines
+190
to
193
| await applier.apply(summary) | ||
|
|
||
| kwargs = mock_deployment_repo.apply_strategy_mutations.call_args.kwargs | ||
| assert kwargs["drain"] is None |
| ) -> None: | ||
| """Rolled-back IDs are tracked in result but not passed to DB mutations. | ||
|
|
||
| The RollingBackHandler is responsible for clearing deploying_revision. |
Comment on lines
47
to
50
| @@ -48,7 +48,10 @@ class StrategyResultApplier: | |||
| 1. Sub-step assignment updates | |||
| 2. Route rollout (create) and drain (terminate) | |||
| 3. Revision swap for COMPLETED deployments | |||
Comment on lines
52
to
55
| Note: Clearing ``deploying_revision`` for rolled-back deployments is NOT | ||
| done here — it is the responsibility of ``DeployingRollingBackHandler`` | ||
| to explicitly call ``clear_deploying_revision`` after rollback completes. | ||
| """ |
Comment on lines
1407
to
1411
| async def apply_strategy_mutations( | ||
| self, | ||
| assignments: Mapping[UUID, DeploymentSubStep], | ||
| rollout: BulkCreator[RoutingRow], | ||
| drain: BatchUpdater[RoutingRow] | None, | ||
| completed_ids: set[UUID], |
Comment on lines
+1428
to
+1432
| async def clear_deploying_revision(self, deployment_ids: set[UUID]) -> None: | ||
| """Clear deploying_revision and sub_step for rolled-back deployments. | ||
|
|
||
| Called explicitly by ``DeployingRollingBackHandler`` after rollback | ||
| completes — NOT automatically during strategy mutations. |
| if not rolled_back_ids: | ||
| """Clear deploying_revision and sub_step for rolled-back deployments. | ||
|
|
||
| This is called explicitly by the RollingBackHandler after rollback |
jopemachine
changed the title
jopemachine
changed the title
sub_step transitions to coordinator with rewind support
jopemachine
changed the title
sub_step transitions to coordinator with rewind supportsub_step transitions to coordinator with rewind support
jopemachine
changed the title
sub_step transitions to coordinator with rewind support
jopemachine
changed the title
sub_step transitions to coordinator with rewind support
jopemachine
changed the title
sub_step transitions to coordinator with rewind supportsub_step transitions to coordinator with rewind support
fregataa
reviewed
Mar 17, 2026
jopemachine
changed the title
sub_step transitions to coordinator with rewind support
Collaborator
|
please rebase this PR. @jopemachine |
HyeockJinKim
previously approved these changes
Mar 19, 2026
Collaborator
HyeockJinKim
left a comment
HyeockJinKim
left a comment
There was a problem hiding this comment.
I approved this PR. but you should rebase this and re-mention.
Remove assignments parameter and _update_sub_steps calls from apply_strategy_mutations so that sub_step transitions are handled exclusively by the coordinator's status transition mechanism. Extract clear_deploying_revision as a separate public method on both the repository and db_source layers. Update tests to reflect that assignments-only summaries no longer trigger DB mutations. This is a prerequisite refactoring for the rolling update deployment strategy (BEP-1049).
Add rewind as a new transition category in DeploymentStatusTransitions and DeploymentExecutionResult. Unlike need_retry, rewind does not increment phase_attempts — it represents normal forward progress (e.g. progressing → provisioning for the next batch in rolling update). Also add or_conditions/and_conditions utilities for combining QueryConditions, useful for building complex route filters. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Remove DeployingProgressingHandler — ProvisioningHandler handles the entire DEPLOYING lifecycle - Remove PROGRESSING and ROLLED_BACK from DeploymentSubStep enum - Simplify RollingBackHandler to transition directly to READY - Remove rolled_back_ids from StrategyApplyResult - Add allow_merge flag to DeploymentHistoryCreatorSpec so rewind history records are never merged (each route mutation cycle is visible as a separate record) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
jopemachine
force-pushed
the
refactor/applier-remove-substep-writes
branch
from
f539c10 to
3ae0da9
Compare
HyeockJinKim
approved these changes
Mar 19, 2026
This file contains hidden or 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
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.
Resolves BA-5280.
Summary
_update_sub_stepsremoved fromapply_strategy_mutations. Sub-step transitions are now handled exclusively by the coordinator's status transition mechanism.rewindwithneed_retry: Route mutations (create/drain) within the same sub-step (PROVISIONING → PROVISIONING) now use theneed_retrytransition. Unlike error-basedneed_retry, these are never escalated togive_upsince they represent normal progress.allow_mergelogic: History merging is now always attempted based onshould_merge_with()— no per-record opt-out needed.DeployingProgressingHandler—DeployingProvisioningHandlerhandles the entire DEPLOYING lifecycle. Remove unusedPROGRESSINGandROLLED_BACKsub-steps.has_mutations()method: Strategy apply skip-condition moved toStrategyApplyResult.has_mutations()for clarity.Test plan
🤖 Generated with Claude Code