Skip to content
This repository has been archived by the owner on May 21, 2024. It is now read-only.

bootloader: gracefully reboot after update completes #1148

Conversation

liuming50
Copy link

This patch mainly aims to add a graceful reboot by executing
'/sbin/init 6' rather than calling reboot system call.

Signed-off-by: Ming Liu [email protected]

@agners
Copy link
Contributor

agners commented Mar 21, 2019

The CI finding is probably relevant:

/aktualizr/src/libaktualizr/bootloader/bootloader.cc: In member function 'void Bootloader::reboot(bool)':
/aktualizr/src/libaktualizr/bootloader/bootloader.cc:184:13: error: ignoring return value of 'int system(const char*)', declared with attribute warn_unused_result [-Werror=unused-result]
       system("/sbin/init 6");
       ~~~~~~^~~~~~~~~~~~~~~~

Probably a good idea to check the return value and write a log message or similar in case something is wrong with the command...

@liuming50
Copy link
Author

liuming50 commented Mar 21, 2019 via email

@liuming50 liuming50 force-pushed the bootloader-gracefully-reboot branch from 409cd87 to 4cc4bc7 Compare March 21, 2019 13:24
if ((stat ("/sbin/init", &st) >= 0) && (st.st_mode & (S_IXUSR | S_IXGRP | S_IXOTH))) {
if (system("/sbin/init 6") != 0) {
LOG_ERROR << "Failed to execute /sbin/init 6";
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Tried to figure out whether there is a POSIX way to reboot a machine, but it seems reboot or similar is not part of the POSIX spec. This StackOverflow thread suggests the same: https://unix.stackexchange.com/questions/507259/is-there-a-posix-way-to-shutdown-a-unix-machine

Is there a reason you opted for /sbin/init 6? Runlevel seem to be a System V init system concept, and not really standardized either,... I feel that /sbin/reboot is probably better suited those days, as it clearly says what it's doing, and is supported by all init systems I know of (sysvinit, systemd, upstart)...

Copy link
Author

Choose a reason for hiding this comment

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

The reason for choosing '/sbin/init 6' instead of '/sbin/reboot' was that: I thought it's possible that '/sbin/reboot' does not exist on some sysvinit based systems, for instance, when busybox is being used, the option 'CONFIG_INIT' is optional, which means there could be no '/sbin/reboot' compiled.

And I verified the '/sbin/init 6' worked also for systemd. (But I did not verify it for upstart)

Copy link
Collaborator

Choose a reason for hiding this comment

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

IMHO,
'reboot' should be part of Platform Abstraction Layer to enable platform specific implementations one of which can be specified/chosen during an image bitbaking and/or aktualizr building.
+
Default/Reference implementation targeting systemd-based system that would reboot by either calling system("systemctl reboot") or ::reboot() which effectively leads to the same effect (if you you look at the logs you can see that actually systemd takes control in case of ::reboot call and does exactly the same stuff as in case of calling systemctl reboot).

Copy link
Contributor

Choose a reason for hiding this comment

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

@mike-sul

Default/Reference implementation targeting systemd-based system that would reboot by either calling system("systemctl reboot") or ::reboot() which effectively leads to the same effect (if you you look at the logs you can see that actually systemd takes control in case of ::reboot call and does exactly the same stuff as in case of calling systemctl reboot).

Really? How? Isn't ::reboot calling the reboot syscall? man 2 reboot says that RB_AUTOBOOT is initiating a restart immediately, and data are lost unless synced before. How is systemd intercepting that syscall?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, your're right. I confused it with system("reboot") which leads to the same effect as system("systemctl reboot") since the 'reboot' file/cmd is a link to systemctl

root@qemux86-64:/sysroot/home/root# ls -l /usr/sbin/reboot lrwxrwxrwx 11 root root 18 Mar 21 15:11 /usr/sbin/reboot -> /usr/bin/systemctl

I apologize for making the incorrect statement.

@liuming50
Copy link
Author

liuming50 commented Mar 21, 2019 via email

@doanac
Copy link
Collaborator

doanac commented Mar 21, 2019

I think you should make the reboot command be a configuration option and the default value can be /sbin/reboot. Then all platforms will have a way to reboot. This might be nice for test automation also. ie - you could create a fake-reboot script to assert it gets called properly.

@lbonn
Copy link
Contributor

lbonn commented Mar 22, 2019

@doanac I agree, it looks like it's the way to go at this point

@liuming50
Copy link
Author

@doanac will remake a V3 as you suggested.

@pattivacek
Copy link
Collaborator

@liuming50 Is this still something you are interested in? I think your plan sounded fine.

@liuming50
Copy link
Author

liuming50 commented Apr 23, 2019 via email

@pattivacek
Copy link
Collaborator

Sorry for the delayed response, I was busy on some other tasks, yes, I would like to fix this, I actually have made a patch for that, just have not managed time to test/verify it, will do that in this week.

No problem, just wanted to check in. Thanks!

@liuming50 liuming50 force-pushed the bootloader-gracefully-reboot branch from 4cc4bc7 to b101006 Compare April 30, 2019 13:31
@codecov-io
Copy link

codecov-io commented Apr 30, 2019

Codecov Report

Merging #1148 into master will increase coverage by 0.06%.
The diff coverage is 33.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1148      +/-   ##
==========================================
+ Coverage   79.61%   79.68%   +0.06%     
==========================================
  Files         173      173              
  Lines       10315    10320       +5     
==========================================
+ Hits         8212     8223      +11     
+ Misses       2103     2097       -6
Impacted Files Coverage Δ
src/libaktualizr/bootloader/bootloader.h 100% <ø> (ø) ⬆️
src/libaktualizr/bootloader/bootloader_config.h 100% <ø> (ø) ⬆️
src/libaktualizr/bootloader/bootloader_config.cc 90% <100%> (+0.71%) ⬆️
src/libaktualizr/bootloader/bootloader.cc 49.23% <14.28%> (-0.77%) ⬇️
src/libaktualizr-posix/ipuptanesecondary.cc 82.66% <0%> (-4%) ⬇️
src/aktualizr_primary/secondary.cc 82.89% <0%> (-1.32%) ⬇️
src/libaktualizr/storage/sqlstorage.cc 76.02% <0%> (ø) ⬆️
src/libaktualizr/storage/sql_utils.h 85.91% <0%> (+1.4%) ⬆️
src/libaktualizr/storage/sqlstorage_base.cc 80.4% <0%> (+6.08%) ⬆️
src/libaktualizr/storage/sqlstorage_base.h 100% <0%> (+40%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3d3b131...1f6171d. Read the comment docs.

@liuming50
Copy link
Author

As suggested by @doanac, I have made a V3 patch, please kindly help me review it.

Veified with the following setting in OE:

EXTRA_OECMAKE_append = " -DREBOOT_COMMAND='/sbin/reboot'"
EXTRA_OECMAKE_append = " -DREBOOT_COMMAND='/sbin/init 6'"
EXTRA_OECMAKE_append = " -DREBOOT_COMMAND='/path/to/non-exist'"

@lbonn
Copy link
Contributor

lbonn commented May 2, 2019

Thanks, it looks like it would work!
However, having it fixed at compile-time seems a bit rigid: it's harder to test and configure.

An alternative would be to do it via regular configuration, for example reboot_command in bootloader's configuration. On oe side, it would need creating a small configuration fragment, but we already have some examples of that.

If you are still keen on doing the work, let us know if you have questions about that.

@liuming50
Copy link
Author

@lbonn Thanks for the review.

Yes, getting a preferred reboot command from configuration sounds like a better solution, will make a V4 if nobody has objection. @agners @doanac @patrickvacek please comment if you have some other suggestions, or I will work on the V4.

@liuming50 liuming50 force-pushed the bootloader-gracefully-reboot branch from b101006 to c4a11c8 Compare May 9, 2019 08:47
@liuming50 liuming50 changed the title bootloader: gracefully reboot using '/sbin/init 6' bootloader: gracefully reboot after update completes May 9, 2019
@liuming50
Copy link
Author

Please kindly help me review the V4 patch:

I have verified it with the following steps:

$ aktualizr --config /path/to/20-sota_autoprov.toml

Scenario 1: 20-sota_autoprov.toml

[bootloader]
reboot_command = "/sbin/reboot"

Scenario 2: 20-sota_autoprov.toml

[bootloader]
reboot_command = "/sbin/init 6"

Scenario 3: 20-sota_autoprov.toml

[bootloader]
reboot_command = "/path/to/non-exist"

@@ -29,6 +29,7 @@ class Bootloader {

INvStorage& storage_;
boost::filesystem::path reboot_sentinel_;
std::string reboot_command_;
Copy link
Contributor

Choose a reason for hiding this comment

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

It is only used in one place, so it probably doesn't need to be an attribute, simply a variable in the concerned method if needed.

@@ -163,4 +163,5 @@ Options for configuring boot-specific behavior
| `rollback_mode` | `"none"` | Controls rollback on supported platforms, see link:{aktualizr-github-url}/docs/rollback.adoc[]. Options: `"none"`, `"uboot_generic"`, `"uboot_masked"`
| `reboot_sentinel_dir` | `"/var/run/aktualizr-session"` | Base directory for reboot detection sentinel. Must reside in a temporary file system.
| `reboot_sentinel_name` | `"need_reboot"` | Name of the reboot detection sentinel.
| `reboot_command` | `"/sbin/reboot"` | Command to reboot the system after update completes.
Copy link
Contributor

Choose a reason for hiding this comment

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

Great that you've updated the docs!
Would be extra helpful with a link to uptane.force_install_completion (reboot_command would only be used if it's on).

return;
}

reboot_command_.copy(full_cmd, len);
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems odd (and unsafe) to copy in a fixed buffer with the length of the original string as a parameter. Doing .copy(full_cmd, sizeof(full_cmd)-1) would be more logical.

However, even then, some points still bother me, as I think this part could be much simpler:

  • handling c strings when the c++ standard library should be sufficient
  • do we really need to parse the command and stat the file? Just system() and checking the return code + logging errors should be plenty
  • we'd probably want to run sync() in any case, even with a custom command
  • maybe not defaulting to ::reboot if the default command fails. It's really not the nominal case (probably misconfiguration) and rebooting anyway could make debugging more difficult

} else {
LOG_WARNING << "The preferred reboot command: '" << reboot_command_ << "' does not exist or is not executable";
sync();
::reboot(RB_AUTOBOOT);
Copy link
Collaborator

@mike-sul mike-sul May 9, 2019

Choose a reason for hiding this comment

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

IMHO, system("reboot") (it will call 'systemd reboot' in our default case) would work in most of cases so I would use it as a default value in case if 'reboot_command' is not specified in the config. Not sure if it makes to fallback to it if the 'reboot_command' fails - agree with lbonn here.
Also, it looks a bit insecure/dangerous to allow an user to execute whatever command they specify in the config, it could be 'rm -rf /', 'cat /etc/passwd', etc :), perhaps the 'freedom' to choose the command can be narrowed downed to some pre-defined list.

Copy link
Contributor

Choose a reason for hiding this comment

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

aktualizr runs as root and should only read configurations writable by root + it does dangerous operations by nature. Using a hard-coded list sounds like a band-aid that can be bypassed.

A possible security check would be that the executable indeed belongs to root and is not writeable by group and others.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we should add another way for user to shoot themselves in a foot. How many ways to reboot a sytem do we really have to support? Also, those commands are very rarely change so we can simply use "reboot_command = systemv" and use /sbin/reboot explicitly in this case instead of default "systemctl reboot". If we really have to we can always expand this by adding "reboot_command = systemwhatever" which will map to another command to add support for some exotic system. Using arbitrary command is more flexible of course but I don't think it's worth it in this case.

Copy link
Contributor

Choose a reason for hiding this comment

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

In the end it doesn't make any difference, if we make room for a calling an arbitrary script with a fixed name, we can as well allow an arbitrary command. It doesn't add nor subtract any foot-gun.

My wish was primarily to allow this use case: https://github.com/toradex/meta-toradex-torizon/blob/a8b50aa8b9f7f14eb60ebc8fc68919ba87413804/recipes-sota/ostree/files/ostree-pending-reboot.path. Touching a file as a trigger to a systemd service seems really useful as it allows a variety of use cases which need close interaction with the init system.
See here for background: advancedtelematic/meta-updater#486.

In this case, the reboot command would just be touch /run/aktualizr-session/trigger_reboot

Copy link
Contributor

Choose a reason for hiding this comment

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

That make sense, nice. I wonder if we could always use reboot via this external path trigger.

@mike-sul
Copy link
Collaborator

@liuming50 Would you like to finalize this PR according to the latest comments ? As an option, I could take over this PR if you don't mind ?

@liuming50
Copy link
Author

liuming50 commented Jul 30, 2019 via email

@mike-sul mike-sul force-pushed the bootloader-gracefully-reboot branch from c4a11c8 to 69c351d Compare July 30, 2019 15:24
@mike-sul
Copy link
Collaborator

Hi, I am glad if you could take over it, I’ve been quite busy recently and have not managed time to redo it. Thanks. //Ming Liu @liuming50 https://github.com/liuming50 Would you like to finalize this

PR according to the latest comments ? As an option, I could take over this PR if you don't mind ? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1148?email_source=notifications&email_token=AB5RRMT5SA6MFI64Y6TC3TLQB77EZA5CNFSM4HAENFQKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD3DJNII#issuecomment-516331169>, or mute the thread https://github.com/notifications/unsubscribe-auth/AB5RRMULLXU5IGCFS4ZEMHLQB77EZANCNFSM4HAENFQA .

OK, so I am taking over it. Just rebase it with the current master and going to start from here.

@mike-sul mike-sul force-pushed the bootloader-gracefully-reboot branch from 69c351d to 9c0d102 Compare July 31, 2019 10:19
This patch mainly aims to add a graceful way to reboot the system by
executing a user preferred reboot command rather than calling reboot
system call. The reboot command could be set in 'bootloader' section of
a config file, it defaults to be '/sbin/reboot'.

Signed-off-by: Ming Liu <[email protected]>
Signed-off-by: Mykhaylo Sul <[email protected]>
@mike-sul mike-sul force-pushed the bootloader-gracefully-reboot branch from 9c0d102 to 1f6171d Compare July 31, 2019 10:26
@mike-sul
Copy link
Collaborator

@lbonn Please, review the updated version. Let's agree on the default reboot command, I see a few options here

  1. /sbin/reboot, systemd reboot, system(reboot) - these three will have the same effect in our case

  2. touch /run/aktualizr-session/trigger_reboot which will require adding corresponding functionality/recipe into meta-updater (I have no objection here and can do it)

  3. don't have any default

@lbonn
Copy link
Contributor

lbonn commented Jul 31, 2019

Great! Right now, the easiest would be to just use /sbin/reboot by default which is probably enough for our default integration layer in meta-updater.
At least now, integrators have a bit more control.

@pattivacek
Copy link
Collaborator

Replaced by #1274.

@pattivacek pattivacek closed this Jul 31, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants