-
Notifications
You must be signed in to change notification settings - Fork 129
lib: add support for soft-reboots #1392
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
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 adds support for soft-reboots. The implementation has code duplication in lib/src/cli.rs
and lib/src/status.rs
. An unnecessary backup file (lib/src/status.rs.orig
) has been included and must be removed.
f063416
to
825778d
Compare
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.
Looking good in general!
lib/src/cli.rs
Outdated
let deployments_list = sysroot.deployments(); | ||
let staged_deployment = deployments_list | ||
.iter() | ||
.find(|d| d.is_staged()) |
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.
I think this is what we need to do at the moment, the code is fine.
However just looking at things overall, we should probably try to refactor in the future so that we have a clear staged state that has both the status object and the deployment and we shouldn't then need the unwrap_or
and error checking here.
408dbd9
to
b5ec31d
Compare
Moving to draft since this is blocked until we either:
|
That said one thing that would be cool would be us keeping cached container images for PRs on In this PR (actually the dependency one) we could explicitly bump the ostree version even before it's shipped $everywhere. |
This will require ostreedev/ostree#3487 |
On.. it's https://gitlab.com/redhat/centos-stream/containers/bootc/-/merge_requests/641#b1795a50b9b140a9b6dcd2d5c10d39bc56d39805 that has not merged because ppc build does not pass with:
which I think is the issue coreos/rpm-ostree#5436 @cgwalters was debugging this week right? Should I just force merge that MR since the new rpm-ostree is on the way? |
Sorry yes I edited that TODO item to link to the issue.
Which MR? |
I was talking about https://gitlab.com/redhat/centos-stream/containers/bootc/-/merge_requests/641which is a bad idea either way since it will break ppc until we get rpm-ostree. |
0019619
to
fa6bde3
Compare
885e55e
to
7814631
Compare
4d22a43
to
d82acda
Compare
man... this is so sad:
But at least https://gitlab.com/redhat/centos-stream/containers/bootc/-/merge_requests/685/diffs is merged and should resolve this hopefully today. |
/gemini review |
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 support for soft reboots in bootc
for upgrade
, switch
, and rollback
commands. The changes include adding a --soft-reboot
CLI option, implementing the logic to check for soft reboot capability, and preparing the system for a soft reboot when applicable. The implementation correctly handles different modes (required
and auto
) and integrates with the existing ostree deployment mechanisms. My review focuses on improving code maintainability by reducing duplication and refactoring complex closures. Overall, the changes are well-implemented and include corresponding tests.
This will enable soft reboots. Signed-off-by: Colin Walters <[email protected]>
This commit adds --soft-reboot=required|auto to the cli which uses the ostree api's to setup soft-reboots during switch, update and rollback operations. Co-authored-by: Colin Walters <[email protected]> Signed-off-by: Joseph Marrero Corchado <[email protected]> Signed-off-by: Colin Walters <[email protected]>
/gemini review |
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 adds support for soft reboots, a feature that allows for faster, userspace-only restarts when possible. The changes include new command-line options, logic to detect soft-reboot capability, and integration into the upgrade, switch, and rollback flows. My review focuses on improving code structure for better maintainability by reducing duplication and simplifying control flow.
let can_soft_reboot = has_soft_reboot_capability(entry); | ||
match mode { | ||
SoftRebootMode::Required => { | ||
if can_soft_reboot { | ||
execute_soft_reboot()?; | ||
} else { | ||
anyhow::bail!( | ||
"Soft reboot was required but {} deployment is not soft-reboot capable", | ||
deployment_type | ||
); | ||
} | ||
} | ||
SoftRebootMode::Auto => { | ||
if can_soft_reboot { | ||
execute_soft_reboot()?; | ||
} | ||
} | ||
} |
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 logic in this match
statement can be simplified to be more concise and arguably easier to read by handling the can_soft_reboot
check first.
let can_soft_reboot = has_soft_reboot_capability(entry);
if can_soft_reboot {
execute_soft_reboot()?;
} else if matches!(mode, SoftRebootMode::Required) {
anyhow::bail!(
"Soft reboot was required but {} deployment is not soft-reboot capable",
deployment_type
);
}
/// Configure soft reboot behavior. | ||
/// | ||
/// 'required' will fail if soft reboot is not available. | ||
/// 'auto' will use soft reboot if available, otherwise fall back to regular reboot. | ||
#[clap(long = "soft-reboot", conflicts_with = "check")] | ||
pub(crate) soft_reboot: Option<SoftRebootMode>, |
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 soft_reboot
option, including its documentation, is duplicated across UpgradeOpts
, SwitchOpts
, and RollbackOpts
. This could become a maintainability issue.
Consider factoring out this common option to avoid repetition. A #[clap(flatten)]
struct could be an option if the conflicts_with
attribute can be handled. Another approach could be a macro to generate the field.
Closes: #1350
depends on