feat(governance): Stop the target canister before taking or loading a snapshot... #8359
+559
−299
Conversation
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
…rs/nervous_system/root.
…th TakeCanisterSnapshot: moved request and response types from rs/handlers/root/interface to rs/nervous_system/root.
…mpl to rs/nervous_system/root.
…oot to rs/nervous_system/root, analogous to the previous commit.
daniel-wong-dfinity-org
force-pushed
the
stop-before-and-start-after-snapshot-ops-daniel-wong
branch
3 times, most recently
from
347aa21 to
0ef0b1d
Compare
…This is a private helper function that will be used in (near) future commits. Its main jobs are 1. Stop canister before main operation, and start it again after the main operation. 2. Acquire a lock on the canister so that different operations do not stomp on each other (and release after) This pattern is already used during canister upgrades. Therefore, one of the places where this will be used is canister upgrades. The other two places where this will be used in the very near future are taking and loading canister snapshots.
… locks the canister, and stops it before the main operation (i.e. calling the Management canister's `load_canister_snapshot` method), making it safer. (Ofc, the canister is also started after the main operation too, and the lock is released at the very end.) This is implemented with the new helper: `exclusively_stop_and_start_canister`. Next, we'll do the same thing for `take_canister_snapshot`.
…evious commit): acquire canister lock, and stop the canister (ofc, after the main operation, start the canister back up again, and release the lock).| (This is like the previous commit, which enhanced load_canister_snapshot.)
…lling (take|load)_canister_snapshot. Nobody ever used DfcRuntime in these cases; the ability to choose was just following an existing but depricated pattern.
…oot). Mainly, changed the implementation of change_canister to use the new exclusively_stop_and_start_canister helper function. Using the helper ensures that code upgrades and snapshottting do not interfere with one another. This also simplifies the implementation of change_canister by factoring out common implementation code. The other change to change_canister is that the Runtime generic parameter is removed. This is fine because all callers were using CdkRuntime anyway.
…ed the opportunity to delete MockRuntime.
daniel-wong-dfinity-org
force-pushed
the
stop-before-and-start-after-snapshot-ops-daniel-wong
branch
from
4bd5451 to
a058b35
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
daniel-wong-dfinity-org
dismissed
github-actions[bot]’s stale review
These changes are behind a flag.
More precisely, I have changed some Root canister methods that can only be called by the Governance canister, but Governance only calls these to execute proposal types that are currently flag disabled.
jasonz-dfinity
approved these changes
Jan 20, 2026
Contributor
jasonz-dfinity
left a comment
jasonz-dfinity
left a comment
There was a problem hiding this comment.
The title doesn't seem to be accurate, since the PR doesn't involve "governance" (at least not in a way we usually mean)
Comment on lines
+209
to
+217
| /* | ||
| let final_err = format!( | ||
| "Failed to stop canister {canister_id} while preparing to perform a {operation}: \ | ||
| {code} {message}. Tried re-starting the canister after that, and the result of \ | ||
| re-starting was {restart_status}." | ||
| ); | ||
|
|
||
| Err((code, final_err)) | ||
| */ |
Contributor
There was a problem hiding this comment.
Probably want to delete this?
| } | ||
| }; | ||
|
|
||
| let result = exclusively_stop_and_start_canister::<_, _, _>( |
Contributor
There was a problem hiding this comment.
Nit: I would assume ::<_, _, _> is not needed, but I could be wrong.
| @@ -235,7 +235,7 @@ async fn take_canister_snapshot( | |||
| take_canister_snapshot_request: TakeCanisterSnapshotRequest, | |||
| ) -> TakeCanisterSnapshotResponse { | |||
| check_caller_is_governance(); | |||
| canister_management::take_canister_snapshot( | |||
| ic_nervous_system_root::take_canister_snapshot::take_canister_snapshot( | |||
Contributor
There was a problem hiding this comment.
Nit: not sure why we don't stick to importing the symbol instead, especially since use ic_nervous_system_root::... already exists
|
|
||
| // Check the result of the install_code |
Contributor
There was a problem hiding this comment.
Usually I don't like comments just stating what the code does, but here, since the result above is a Result<Result<_, _>, _>, it's probably good to comment on what each error handling does.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
... and start it again afterwards, of course. Also, lock and unlock he canister.
How
At the core of this PR is a new helper function:
exclusively_stop_and_start_canister. It takes a bunch of arguments, but the main one is a function that does the "main" operation, such as taking a snapshot. Meanwhile, the helper takes care of locking and stopping the canister before the main operation, and afterwards, starting, and unlocking it again.Much code is moved from
rs/nnstors/nervous_system/root.References
Closes NNS1-4308.