-
Notifications
You must be signed in to change notification settings - Fork 150
Make update and switch operations idempotent #1797
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Parse toml files in usr/lib/bootc/kargs.d and append them to kernel cmdline on install and upgrade/switch. Also, copy over current deployment's cmdline args on upgrade/switch to another deployment Signed-off-by: Pragyan Poudyal <pragyanpoudyal41999@gmail.com>
We have a lot of places where we mount the ESP temporarily and a lot of switch cases for Grub's vs SystemdBoot's 'boot' directory. We add a `boot_dir` field in Storage which points to `/sysroot/boot` for systems with Grub as the bootloader and points to the ESP for systems with SystemdBoot as the bootloader. Also we mount the ESP temporarily while creating the storage struct, which cleans up the code quite a bit. Signed-off-by: Pragyan Poudyal <pragyanpoudyal41999@gmail.com>
Signed-off-by: Pragyan Poudyal <pragyanpoudyal41999@gmail.com>
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>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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"), |
Similar to how we handle bootc update Signed-off-by: Pragyan Poudyal <pragyanpoudyal41999@gmail.com>
36509c1 to
b9ecaac
Compare
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