-
Notifications
You must be signed in to change notification settings - Fork 146
Create /run/reboot-required by default #1378
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
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 introduces a straightforward and effective change to create the /run/reboot-required file after a new deployment is staged. This aligns with standard practices for signaling a required reboot and facilitates integration with tools like kured.
The implementation in lib/src/deploy.rs is clear, concise, and uses appropriate error handling with anyhow and context. The use of cap_std for file operations is suitable, and atomic_write combined with a try_exists check provides a robust way to create the signal file idempotently.
After a thorough review, I found no issues of medium, high, or critical severity. The code is correct for its intended purpose, handles potential error conditions appropriately, and is maintainable.
cgwalters
left a comment
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.
Thanks! One nit, and can you also add an integration test in say test-image-pushpull-upgrade.nu around like 68?
lib/src/deploy.rs
Outdated
| if !run_dir.try_exists("reboot-required")? { | ||
| run_dir.atomic_write("reboot-required", b"").context("Creating /run/reboot-required")?; | ||
| } |
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 I'd say we should drop the conditional and always do the write - there's no harm to doing it, and updating the timestamp on the file also acts a signal that something changed again.
| let rr_meta = ls /run/reboot-required | ||
| assert equal $rr_meta.name "/run/reboot-required" |
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 failing because ls returns a table. It should work if you do ls /run/reboot-required | first instead.
When staging a new deployment, create /run/reboot-required to signal that a reboot is needed. This file is monitored by kured (Kubernetes Reboot Daemon) and other tools to detect when a system needs to be rebooted. This makes it easier to integrate bootc with kured and similar tools without requiring manual configuration or bridging. Signed-off-by: Colin Walters <[email protected]>
|
Yeah confusingly nushell has a dedicated "size" type that's not an integer, I fixed the test |
When staging a new deployment, create /run/reboot-required to signal that a reboot is needed. This file is monitored by kured (Kubernetes Reboot Daemon) and other tools to detect when a system needs to be rebooted.
This makes it easier to integrate bootc with kured and similar tools without requiring manual configuration or bridging.
Fixes: #1353