feat(registry): Allow blanking a Cloud Engine's replica_version_id. - #11172
feat(registry): Allow blanking a Cloud Engine's replica_version_id.#11172pietrodimarco-dfinity wants to merge 1 commit into
Conversation
`deploy_guestos_to_all_subnet_nodes` is the only endpoint that writes `SubnetRecord.replica_version_id`, and it required that version to be elected. A blank version never is, so a Cloud Engine that pins a version could not be put back on the standard engine version -- even though a blank `replica_version_id` is allowed (and typical) for Cloud Engines, and `create_subnet` creates them that way. Make the elected check conditional. A blank version is validated against the same conditions under which the SubnetRecord invariant tolerates it: the feature is enabled, the subnet is a CloudEngine, and Registry has a StandardEngineReplicaVersionRecord to determine the replica version from. Non-blank versions still have to be elected. The rule is caller-agnostic -- Governance and the engine controller canister get the same treatment. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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 5f13942. Security Overview
Detected Code Changes
|
Motivation
A blank
SubnetRecord.replica_version_idis the normal, documented state for a Cloud Engine: when blank, the engine's replica version comes fromStandardEngineReplicaVersionRecordinstead of being pinned (see the field docs insubnet.proto).create_subnetcreates engines that way.But an engine that does pin a version could not be put back on the standard version.
deploy_guestos_to_all_subnet_nodesis the only endpoint that writesreplica_version_id, and it required that version to be elected — a blank version never is, so the call traps withReplica version '' is NOT elected. That applies to every caller, Governance included, so there was no route at all (andupdate_subnetdeliberately does not carryreplica_version_id).Changes
Make the elected check conditional:
check_blank_replica_version_id_is_allowedasserts the same three conditions under which theSubnetRecordinvariant tolerates a blank id (invariants::replica_version), so the mutation cannot produce state the invariant would then reject:IS_BLANK_REPLICA_VERSION_ID_FOR_CLOUD_ENGINES_ENABLEDis on,CloudEngine,StandardEngineReplicaVersionRecordexists to determine the version from.Non-blank versions still have to be elected. The rule is caller-agnostic — Governance and the engine controller canister get the same treatment, and the existing engine-controller-only
CloudEngineguard is untouched.Incidental:
get_standard_engine_replica_version_recordbecomespub(crate)(it was private to its own module).Tests
Six unit tests: the engine controller and Governance can each blank a Cloud Engine; it panics when the feature is disabled, when there is no
StandardEngineReplicaVersionRecord, and for a non-CloudEnginesubnet; plus a non-blank unelected version still panics, so the new branch cannot silently weaken the existing check.Verification beyond
rustfmtis left to CI.🤖 Generated with Claude Code