feat(registry): Allow advancing standard engine replica version after a roll back. - #11403
Conversation
2e0ba0d to
3f76f71
Compare
|
✅ No security or compliance issues detected. Reviewed everything up to 692b5aa. Security Overview
Detected Code Changes
|
There was a problem hiding this comment.
Pull request overview
Allows standard engine replica deployment to advance to a different version after a complete rollback.
Changes:
- Treats deployment progress
0.0and1.0as settled states. - Validates the next deployment against the currently active version.
- Adds rollback-transition and partial-rollback tests.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
- Done.
- Existing calls still work. This RELAXES the rules.
- Existing Registry data is fine.
- Seems low risk.
bjoernek
left a comment
There was a problem hiding this comment.
The reasoning here looks right to me, and a nice side effect is that it makes the registry self-consistent: do_split_subnet.rs:295-296 already treats 0.0 < deployment_progress < 1.0 as "the fleet is split", so before this change the two mutations disagreed about what "settled" means. I also checked that nothing else encoded the old "must be 1.0" rule: UpdateStandardEngineReplicaVersion::validate() on the Governance side only does static checks, and the invariant checker doesn't look at transitions, so there is nothing stale left behind. Changelog entry is there and accurate.
Two small comments inline (one missing test, one panic message). Neither is blocking.
Non-blocking follow-up idea: the "is the fleet split / what is the unique version" predicate now exists twice, computed two different ways (here as a match, and inline in do_split_subnet.rs:295-296). Since this file already has StandardEngineReplicaVersionRecordExt, a shared fn unique_replica_version_id(&self) -> Option<&String> there (or in ic_protobuf::registry::standard_engine_replica_version) would let do_split_subnet express fleet_is_split as .is_none() and keep the two from drifting apart again. Out of scope for this PR, just flagging it.
|
I might do the split/unique deduping later. Claude already wrote it for me, but now that I look at it, it hardly seems like spinning out "pulls its own weight", at least not in my estimation. It's just way too easy to look at deployment_progress and realize that it's either 0.0 or 1.0. I'll consider doing it when I come back, but I haven't made up my mind, which I guess you would be ok with since, you do not seem to be insisting on it. |
Previously, if you rolled back by reducing deployment_progress to 0.0, the only change you would be allowed to do was increase deployment_progress, which is really not what you after you have just rolled back!
What this does is relax the rules about what you are allowed to do from there.
In particular, now, when deployment_progress is 0.0, you are allowed to change new_replica_version_id (and deployment_progress). old_replica_version_id must remain fixed.
This is very much like how before, after you reach 100%, you can then do a new new_replica_version_id. This is very much like that, because at 0%, the fleet is not split. That's the real constraint, which we didn't correctly recognize before.