Make update and switch operations idempotent #1797
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces idempotency for update and switch operations by checking if the target image is already present in some form. The logic is well-encapsulated in update.rs and shared between both commands. A significant and welcome part of this change is the refactoring of the Storage struct to manage the boot directory and ESP mount, which greatly cleans up the code and removes duplication across several files. Additionally, kernel argument handling is improved to preserve user changes during updates. The changes are well-structured and effectively achieve the PR's goal. I have left a couple of minor suggestions for improvement.
| Cmdline::from(options) | ||
| } | ||
|
|
||
| _ => anyhow::bail!("Found NonEFI config"), |
There was a problem hiding this comment.
The error message here is confusing. It says "Found NonEFI config", but it's in the _ arm of a match where the only other arm is BLSConfigType::NonEFI. This means it triggers for anything that is not NonEFI. A better error message would clarify that a NonEFI config was expected.
| _ => anyhow::bail!("Found NonEFI config"), | |
| _ => anyhow::bail!("Unsupported BLS config type: expected a NonEFI config"), |
Johan-Liebert1
force-pushed
the
idempotency
branch
from
36509c1 to
b9ecaac
Compare
|
This will need a rebase after #1783 lands |
Handle the following cases we can encounter on `bootc udpate`
1. The verity is the same as that of the currently booted deployment
- Nothing to do here in case of update as we're currently booted. But if we're switching
then we update the target imageref in the .origin file for the deployment
2. The verity is the same as that of the staged deployment
- Nothing to do, as we only get a "staged" deployment if we have
/run/composefs/staged-deployment which is the last thing we create
while upgrading
3. The verity is the same as that of the rollback deployment or any
other deployment we have already deployed
- Nothing to do since this is a rollback deployment which means
this was unstaged at some point
4. The verity is not found
- The update/switch might've been canceled before
/run/composefs/staged-deployment was created, or at any other point
in time, or it's a new one.
Any which way, we can overwrite everything. In this case we remove
all the staged bootloader entries, if any, and remove the entire
state directory, as it would most probably be in an inconsistent
state.
Signed-off-by: Pragyan Poudyal <pragyanpoudyal41999@gmail.com>
Similar to how we handle bootc update Signed-off-by: Pragyan Poudyal <pragyanpoudyal41999@gmail.com>
Johan-Liebert1
force-pushed
the
idempotency
branch
from
b9ecaac to
f5e2c4f
Compare
|
Thought the TMT failure was a flake and reran the test, but it seems like reboot seems to be failing on centos-10? |
|
|
||
| match boot_type { | ||
| BootType::Bls => { | ||
| boot_digest = Some(setup_composefs_bls_boot( |
There was a problem hiding this comment.
Only tangential to this but I think we want to more carefully tease apart setup_composefs_bls_boot so that we don't need the enum anymore.
| // remote_image is the exact same image as the one currently booted, but | ||
| // they are wanting to change the target | ||
| // | ||
| // We could simply update the image origin file here |
There was a problem hiding this comment.
We are doing that right?
There was a problem hiding this comment.
Oh yeah, we are doing that. This comment was for implementation reference
2 participants
Pulls in #1783
Handle the following cases we can encounter on
bootc udpateandbootc switchThere is a lot here, and I managed to test the main code paths and the above four cases
Related: #1735