soft-reboot: Many changes #3460
Conversation
There was a problem hiding this comment.
Code Review
The pull request introduces soft reboot functionality to ostree, allowing for staged updates and system resets. The changes include modifications to the documentation, symbol definitions, sysroot deployment logic, and command-line interface. The code introduces new functions for preparing and clearing soft reboot states, ensuring service finalization, and handling related file operations. The tests were also updated to verify the new functionality.
4f16be7 to
9372542
Compare
|
/test fcos-e2e |
There was a problem hiding this comment.
While the test passes, if I do this manually I see:
##########################################################################
Warning: '/sysroot' is not mounted read-only
To disable this warning, use:
sudo systemctl disable coreos-warn-invalid-mounts.service
##########################################################################
Another interesting thing is that with this I have to consistently call systemctl soft-reboot to be able to get an actual soft-reboot. Maybe because we are doing the /run/nextroot prep after calling reboot?
Yeah it's because the test suite explicitly remounts it just because it's convenient.
Ahhh good call! Yeah that has to be it. There's a bit of a clash then with staging and Or for now we could document this issue. I don't think it's critical; we're not setting up soft reboots by default, then tooling will need to opt-in/be aware and it's going to be pretty common I think for the tooling that initiates an upgrade to also be the thing doing a reboot. That said though, if we did decide to queue soft reboots by default, then we should definitely fix this because otherwise in use case like desktop environments and the like, the UX will just have a "reboot" option and probably not also a soft reboot option. |
I am not sure I follow, we expect '/sysroot' not mounted read-only` to be the correct behavior after a soft-reboot? |
Only in the test case because it calls |
|
OK I've been thinking/working on this more and I've come to the conclusion that when soft rebooting, we should not finalize the staged deployment if it's not the one targeted by soft reboots.
This one is going to be possible but messy. |
bdec06a to
c590811
Compare
926750d to
81b6f23
Compare
Well it wasn't, it was mostly undoing things... I think my original thought in moving the mount setup to shutdown is that we didn't have to deal with the possibility "skew" between the mount state of Anyways, we're now back to setting up the mounts when |
For posterity there was a bug here, we weren't including the right metadata and so |
f154dc8 to
d927ba7
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces significant and valuable changes to the soft-reboot functionality. The new command-line arguments, API renaming, and logic for handling staged deployments are well-implemented. The code is robust, and the tests are comprehensive. I've provided a few minor suggestions to improve code clarity and fix small issues in the test scripts. Overall, this is a great contribution.
- Add --reboot and --reset arguments - Don't compile on centos stream 9 (missing `open_tree` glibc wrapper) as the functionality isn't supported by systemd there; that said we should also do dynamic detection - Fix /sysroot writability - If we target as soft reboot a deployment *other* than the staged one, automatically clear the staged deployment as otherwise the semantics are too confusing. - Rename the APIs so they all say `soft_reboot` and not `next_root` Signed-off-by: Colin Walters <walters@verbum.org>
d927ba7 to
6bee4a0
Compare
other than the staged one, automatically clear the staged
deployment as otherwise the semantics are too confusing.
soft_rebootand notnext_rootSigned-off-by: Colin Walters walters@verbum.org