[RayService] Assert rollback reason in TestReconcileRollbackState - #5076
[RayService] Assert rollback reason in TestReconcileRollbackState#5076zacheryasc wants to merge 1 commit into
Conversation
|
I know it's a small change, and debated opening a PR for this alone, but I checked the git history and there were similar small changes. Therefore, decided to have it standalone as opposed to riding some unrelated feature. |
machichima
left a comment
machichima
left a comment
There was a problem hiding this comment.
That let the #5022 rename (TargetClusterChanged -> DesiredClusterSpecChanged) drift at one of two callsites in rayservice_controller.go for ~9 days until the hotfix in #5063.
The reason for CI failure after #5022 is because they forgot to merge master, so that the PR's CI does not catch it. But this fix is still good to have, thank you for catching this!
|
@zacheryasc Please run pre-commit locally and fix the lint error, and fix the e2e test failure if it's related to this PR. Thank! |
bed347f to
3372df4
Compare
Embarrassing! Lint has been fixed. It seems like the other failing CI is unrelated. Current |
|
@zacheryasc the error is fixed on master |
reconcileRollbackState sets the RollbackInProgress condition with reason DesiredClusterSpecChanged, but TestReconcileRollbackState asserted only the condition's boolean, never its reason. That let the ray-project#5022 rename (TargetClusterChanged -> DesiredClusterSpecChanged) drift at one of two callsites for 9 days until the hotfix in ray-project#5063. Assert the reason, but only on the cases where the controller itself emits the condition (expectRollbackStatus && !isRollbackInProgress), excluding the pre-seeded "continues rolling back" path where the controller returns early without re-setting the condition.
3372df4 to
d6b2b30
Compare
|
@zacheryasc Please ping me again when all CI are green. Thanks! |
|
@machichima green to go |
machichima
left a comment
machichima
left a comment
There was a problem hiding this comment.
LGTM! Thank you. cc @rueian or @andrewsykim to merge
2 participants
Why are these changes needed?
reconcileRollbackStatesets theRollbackInProgresscondition with reasonDesiredClusterSpecChanged, butTestReconcileRollbackStateasserted only the condition's boolean, never its reason. That let the #5022 rename (TargetClusterChanged->DesiredClusterSpecChanged) drift at one of two callsites inrayservice_controller.gofor ~9 days until the hotfix in #5063.The new assertion checks the emitted reason, but only on the cases where the controller itself emits the condition (
expectRollbackStatus && !isRollbackInProgress), excluding the pre-seeded "continues rolling back" path where the controller returns early without re-setting the condition.Related issue number
No issue — test-only regression-prevention gap found while reviewing #5063 (the hotfix for the #5022 rename drift).
Labels
doc-updates-requiredlabel.breaking-changelabel.Checks