feat(registry): ensure no version deployment is in progress when splitting a cloud engine + that the record was not changed - #11242
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
This pull request changes code owned by the Governance team. Therefore, make sure that
you have considered the following (for Governance-owned code):
-
Update
unreleased_changelog.md(if there are behavior changes, even if they are
non-breaking). -
Are there BREAKING changes?
-
Is a data migration needed?
-
Security review?
How to Satisfy This Automatic Review
-
Go to the bottom of the pull request page.
-
Look for where it says this bot is requesting changes.
-
Click the three dots to the right.
-
Select "Dismiss review".
-
In the text entry box, respond to each of the numbered items in the previous
section, declare one of the following:
-
Done.
-
$REASON_WHY_NO_NEED. E.g. for
unreleased_changelog.md, "No
canister behavior changes.", or for item 2, "Existing APIs
behave as before.".
Brief Guide to "Externally Visible" Changes
"Externally visible behavior change" is very often due to some NEW canister API.
Changes to EXISTING APIs are more likely to be "breaking".
If these changes are breaking, make sure that clients know how to migrate, how to
maintain their continuity of operations.
If your changes are behind a feature flag, then, do NOT add entrie(s) to
unreleased_changelog.md in this PR! But rather, add entrie(s) later, in the PR
that enables these changes in production.
Reference(s)
For a more comprehensive checklist, see here.
GOVERNANCE_CHECKLIST_REMINDER_DEDUP
|
✅ No security or compliance issues detected. Reviewed everything up to 531cc43. Security Overview
Detected Code Changes
|
- Done.
- Nothing breaks.
- No migration needed.
- No security review needed.
There was a problem hiding this comment.
Pull request overview
Adds concurrency checks around subnet splitting and expands guard tests.
Changes:
- Guards the standard engine replica-version record during DKG generation.
- Adds parameterized tests for all guarded records.
- Documents the behavior change.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
rs/registry/canister/unreleased_changelog.md |
Records the new split failure condition. |
rs/registry/canister/src/mutations/do_split_subnet.rs |
Adds the guard and related tests. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| // The replica version that the newly created subnet is going to run may be derived from | ||
| // this record, so it must not change under our feet either. | ||
| if record_changed_across_versions(make_standard_engine_replica_version_record_key()) { |
There was a problem hiding this comment.
Cloud engines are indeed not supported for the moment, but they could be in the future. This PR preemptively adds this check in case we forget to do so when enabling the feature for cloud engines.
It is true that it could abort a subnet split that was intended for regular subnets even though this record only affects cloud engines, but I would keep it as is for simplicity since this has quite a low probability of happening anyways (the async call should take about 10 seconds).
Something else came to mind though: 531cc43. Cloud engines shouldn't be split if the deployment of a new replica version is in progress as the new subnet could end up in a new version.
pierugo-dfinity
changed the title
| if 0.0 < standard_engine_replica_version_record.deployment_progress | ||
| && standard_engine_replica_version_record.deployment_progress < 1.0 | ||
| { |
There was a problem hiding this comment.
The probability of hitting a priority precisely equal to 0.0 is negligible (1/2^64), and I could imagine that a priority 0.0 could genuinely happen in practice to tell all cloud engines to stay on old_replica_version_id. So I think this is fine, but I leave the final decision to the code owners.
| if 0.0 < standard_engine_replica_version_record.deployment_progress | ||
| && standard_engine_replica_version_record.deployment_progress < 1.0 |
There was a problem hiding this comment.
I would say that 0.0 indicates that a deployment is in progress. Only when it reaches 1.0 would I say it's complete. But even then, changing from 1.0 to < 1.0 is (currently) allowed... But maybe, we can disallow roll back once it reaches 1.0? That would be fine, because you can still roll back by doing
{
new: previous
old: current
progress: ...
}
if current is already at 100%.
Anyway, this is moot if we go with my other suggestion.
There was a problem hiding this comment.
Changing from 1.0 to < 1.0 is not a problem because these would end up in different registry versions. We ensure in Consensus that while the subnet is getting split, it only reads at the registry version at which the registry is applying those mutations, not before, not later.
The problem is that even for a fixed registry version, two engines reading the same record can derive a different replica version. Only 0.0 or 1.0 ensure that both engines will derive the same version, though I have to say that since this is a continuous spectrum, only 1.0 is strictly valid since one of the engines could have a priority of exactly 0.0. I just wanted to relax the check 0.0 is expected to be used somewhat frequently, but I'm happy to change it to 1.0 only.
| // We want to reject a split if the source subnet is a cloud engine and a deployment of | ||
| // a new replica version is in progress. This is because the new destination subnet will | ||
| // have a new subnet ID which we do not know yet and could end up upgrading to a | ||
| // different version. We would like to avoid weird situations where both a split and an | ||
| // upgrade are scheduled at the same time, so we enforce that the cloud engine | ||
| // deployment is complete before allowing a split. In that case, it is guaranteed that | ||
| // both subnets will be on the same replica version after the split. |
There was a problem hiding this comment.
This seems verbose.
Basically, we want upgrading a subnet and splitting it to be mutually exclusive, right?
In that case, what I'd suggest is that we just disallow splitting engines that have blank replica_version_id. Seems simpler, right?
What I like about my suggestion is that splitting one engine does NOT block upgrading all the others. That seems like a feature we really want to preserve.
Furthermore, what we can do here (to more conveniently fulfill "engines with blank replica_version_id cannot be split") is fill in the replica_version_id field, fix the field to whatever StandardEngineReplicaVersion is currently assigning the engine to. But this is just a convenience; you could instead force people to do a deploy_guestos to fix the replica_version_id manually.
There was a problem hiding this comment.
Seems simpler, right?
Simpler in code but definitely not simpler operationally, since you'd first need to set replica_version_id to the engine, then unset the field on both created engines -> 3 additional proposals + potential human error in the loop.
What I like about my suggestion is that splitting one engine does NOT block upgrading all the others.
The current implementation satisfies that too. It's the other way around: upgrading the others will block splitting an engine (which is acceptable).
fix the field to whatever StandardEngineReplicaVersion is currently assigning the engine to
Hmm, yes that could work, but you'd still need 2 proposals to unset them. That would make it asymmetric + it isn't natural to make an operation responsible for splitting a subnet also mutate that subnet's replica version.
| } | ||
|
|
||
| #[cfg(test)] | ||
| mod tests { |
There was a problem hiding this comment.
I have not reviewed changes past this line. Let me resolve this once I've done that.
| * A subnet-split request will now fail if a concurrent call modified the `StandardEngineReplicaVersionRecord` | ||
| while the fresh key material was being generated for the splitting subnet. |
There was a problem hiding this comment.
Does this only apply to engines that have blank replica_version_id?
There was a problem hiding this comment.
No this applies to all subnets including engines setting their replica_version_id and regular subnets. This is suboptimal, but I kept it as is for simplicity.
See discussion here and feel free to suggest relaxing that check if you think it is worth the added complexity.
Even though splitting cloud engines are not supported for now, this PR makes the registry additionally check that the
StandardEngineReplicaVersionRecordhasn't changed during the time taken to generate fresh key material for the two new subnets during a split.This is to avoid weird interactions where an upgrade would trigger at the same time as a subnet split.
It also adds unit tests for the other records that were previously checked.
Also, ensure that no version is in deployment when splitting a cloud engine, as the newly created engine could otherwise end up in a different replica version as the original engine.