Skip to content

ot-builtin-admin: Add admin prepare-soft-reboot #3420

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Jun 26, 2025
Merged

ot-builtin-admin: Add admin prepare-soft-reboot #3420

merged 3 commits into from
Jun 26, 2025

Conversation

jmarrero
Copy link
Member

@jmarrero jmarrero commented May 15, 2025 •
edited by cgwalters
Loading

Work on top of #3390 specifically working on implementing:
#3390 (comment)

TODO list

  • optional but nice: factor out prep commit(s): option processing, mkdir idempotence
  • default deny soft rebooting to a deployment with a different kernel
  • ostree admin status may need some tweaking
  • ostree admin prepare-soft-reboot is too long
  • Need to ensure handling of /boot (this is why bootc status etc are broken post soft-reboot)
  • Also need to force deployment finalization for staged deployments to ensure we get the updated /etc but also so that we have bootloader entries across reboot etc. TODO should happen via ostree-finalizes
  • tests
  • docs (man ostree-soft-reboot?)
  • TODO mechanism for tracking that we soft-rebooted (systemd is doing this a bit?); I saw a soft reboot counter but we should show it in "ostree admin status" or so...hmm how about adding a flag to /run/ostree-booted for this?
  • support for opting in to kernel skew; should bind mount old kernel dir but also make very clear in status that skew has occurred (may want to drill into kernel vs initramfs change)
  • handle detecting kernel argument changes (and this one gets into bootc kargs vs ostree)
  • determine behavior with staged deployments present; should soft rebooting clear the staged deployment? I think so...
  • (related to the above) possibly move soft reboot prep to finalization (shutdown) time; this would be cleaner conceptually

@openshift-ci OpenShift CI
Copy link

openshift-ci bot commented May 15, 2025

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@github-actions github-actions bot added the area/prepare-root Issue relates to ostree-prepare-root label May 15, 2025
@jmarrero jmarrero changed the title ostree-prepare-root: allow running on a booted system ot-builtin-admin: Add admin prepare-next-root May 15, 2025
@jmarrero jmarrero changed the title ot-builtin-admin: Add admin prepare-next-root ot-builtin-admin: Add admin prepare-soft-reboot May 15, 2025
cgwalters
cgwalters reviewed May 15, 2025
View reviewed changes
jmarrero
jmarrero commented May 16, 2025
View reviewed changes
@jmarrero
Copy link
Member Author

jmarrero commented May 16, 2025 •
edited
Loading

I feel I forgot how to properly pass the fd.

sudo strace -s 2048 -f -e execve ./ostree admin prepare-soft-reboot 0

[pid 58220] execve("/usr/lib/ostree/ostree-prepare-root", ["/usr/lib/ostree/ostree-prepare-root", "--mount", "/run/nextroot"], 0x10cad1b0 /* 26 vars */) = 0
ostree-prepare-root: realpath("--mount"): No such file or directory
[pid 58220] +++ exited with 1 +++
--- SIGCHLD {si_signo=SIGCHLD, si_code=CLD_EXITED, si_pid=58220, si_uid=0, si_status=1, si_utime=0, si_stime=0} ---
error: Preparing /run/nextroot for a soft-reboot: Child process exited with code 1
+++ exited with 1 +++

@cgwalters
Copy link
Member

We got this to run, but so far more work necessary:

  • optional but nice: factor out prep commit(s): option processing, mkdir idempotence
  • Need to ensure handling of /boot (this is why bootc status etc are broken post soft-reboot)
  • Also need to force deployment finalization for staged deployments to ensure we get the updated /etc but also so that we have bootloader entries across reboot etc. (note: in theory we could do this as part of the soft reboot target to ensure more of userspace is shut down)
  • mechanism for tracking that we soft-rebooted (systemd is doing this a bit?); maybe we inject that state into /run/ostree-booted
  • default deny soft rebooting to a deployment with a different kernel (opt in to bind mounting the old kernel dir)
  • tests
  • docs (man ostree-soft-reboot?)

@Mstrodl
Copy link
Contributor

Mstrodl commented May 29, 2025

Thanks for all the hard work everyone. Glad you guys were able to track down the weird behavior I was seeing. Sorry I didn't have time to give this the love it needed. Should I close my PR?

@jmarrero jmarrero mentioned this pull request May 29, 2025
Merged
@jmarrero
Copy link
Member Author

Thanks for all the hard work everyone. Glad you guys were able to track down the weird behavior I was seeing. Sorry I didn't have time to give this the love it needed. Should I close my PR?

we really appreciate all your work on this, no need to be sorry. Yeah, you can close your PR, we are splitting the changes into a couple of PRs and we have a more changes to add.

@Mstrodl Mstrodl mentioned this pull request May 30, 2025
Closed
@jmarrero jmarrero mentioned this pull request May 30, 2025
Merged
@ericcurtin ericcurtin requested a review from Copilot May 30, 2025 23:56
Copilot
Copilot AI reviewed May 30, 2025
View reviewed changes
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

Adds support for preparing a deployment for a systemd “soft reboot” by introducing a new prepare-soft-reboot admin command, extending the ostree-prepare-root helper, exposing a new libostree API, and covering it with an integration test.

  • Register and implement the prepare-soft-reboot subcommand in ot-builtin-admin and ot-admin-functions
  • Extend ostree-prepare-root to handle --soft-reboot and move mounts into /run/nextroot
  • Expose ostree_sysroot_deployment_prepare_next_root() in libostree with symbol versioning and update build files
  • Add an end-to-end test script in tests/kolainst/destructive/soft-reboot.sh

Reviewed Changes

Copilot reviewed 14 out of 14 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
tests/kolainst/destructive/soft-reboot.sh Add integration test for soft-reboot workflow
src/switchroot/ostree-prepare-root.c Handle --soft-reboot, new mount targets and flags
src/ostree/ot-admin-builtin-prepare-soft-reboot.c Implement the prepare-soft-reboot admin builtin
src/libostree/ostree-sysroot-deploy.c Introduce ostree_sysroot_deployment_prepare_next_root
src/libostree/ostree-sysroot.h Declare new prepare-next-root API
src/libostree/libostree-devel.sym Update symbol version script for new API
Makefile-ostree.am / Makefile-libostree.am Add new source and include devel symbol file
apidoc/ostree-sections.txt Document the new API section

@cgwalters cgwalters mentioned this pull request Jun 3, 2025
Closed
9 tasks
@jmarrero jmarrero force-pushed the builds-soft branch 2 times, most recently from 459405e to 705b834 Compare June 15, 2025 09:13
@github-actions github-actions bot added the area/rust-bindings Relates to the Rust bindings for the C library label Jun 18, 2025
@github-actions github-actions bot removed area/rust-bindings Relates to the Rust bindings for the C library area/prepare-root Issue relates to ostree-prepare-root labels Jun 25, 2025
@cgwalters cgwalters force-pushed the builds-soft branch 4 times, most recently from f277879 to 22e5e33 Compare June 25, 2025 22:36
@cgwalters
Copy link
Member

@jmarrero can you review the latest two commits?

Also with this, the soft reboot test is passing!

@cgwalters cgwalters force-pushed the builds-soft branch 2 times, most recently from 54318cb to c9ea464 Compare June 26, 2025 00:52
@jmarrero
Copy link
Member Author

They both look good to me @cgwalters. Also I tested it manually and it works too.

cgwalters added 2 commits June 26, 2025 10:41
@cgwalters
prepare-root: Use tempdir for transient etc backing
416f2c3 
Prep for soft reboots.

Signed-off-by: Colin Walters <walters@verbum.org>
@cgwalters
admin: Expand column for subcommands
381288d 
Prep for soft reboot.

Signed-off-by: Colin Walters <walters@verbum.org>
@cgwalters
Copy link
Member

Cool. OK I

  • Squashed the various commits into one, and added Co-authored-by for all three of us (including @Mstrodl )
  • Tweaked the man page a bit
  • Edited the TODO list at the top to add the one item we'd chatted about

Anything else? I think we could consider lifting draft on this and merging, and doing other things as followups?

@cgwalters cgwalters marked this pull request as ready for review June 26, 2025 15:14
@jmarrero
Copy link
Member Author

That works for me, do you want to remove the WIP: from the commit?

@cgwalters @jmarrero @Mstrodl
Add ostree admin prepare-soft-reboot
fafca2f 
This adds support for systemd soft reboots.

Closes: ostreedev#3242

Signed-off-by: Colin Walters <walters@verbum.org>
Co-authored-by: Joseph Marrero Corchado <jmarrero@redhat.com>
Co-authored-by: Mary Strodl <ipadlover8322@gmail.com>
Signed-off-by: Colin Walters <walters@verbum.org>
@cgwalters
Copy link
Member

That works for me, do you want to remove the WIP: from the commit?

Done!

[2025-06-26T16:43:17.369Z] error: Installing packages: Packages not found: bootupd

I think this is a usual transient race condition with the FCOS koji pool, will wait and retry in a bit...

@jmarrero jmarrero enabled auto-merge June 26, 2025 18:27
@cgwalters
Copy link
Member

/override ci/prow/fcos-e2e

@openshift-ci OpenShift CI
Copy link

openshift-ci bot commented Jun 26, 2025

@cgwalters: Overrode contexts on behalf of cgwalters: ci/prow/fcos-e2e

In response to this:

/override ci/prow/fcos-e2e

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@jmarrero jmarrero merged commit e04dc5b into ostreedev:main Jun 26, 2025
25 checks passed
@cgwalters cgwalters mentioned this pull request Jun 27, 2025
Closed
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

Copilot code review Copilot Copilot left review comments

@cgwalters cgwalters cgwalters approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jmarrero @cgwalters @Mstrodl @openshift-merge-robot