bootloader: gracefully reboot after update completes #1148

liuming50 · 2019-03-21T10:18:43Z

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 liu.ming50@gmail.com

agners · 2019-03-21T13:15:29Z

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 · 2019-03-21T13:18:42Z

Good suggestion, will send a V2 soon. //Ming Liu Stefan Agner <notifications@github.com> 於 2019年3月21日週四下午2:15寫道：

…

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... — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#1148 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AHsYsvVcY1vQxNA9HzKVjb2Ur9xKuPcYks5vY4XxgaJpZM4cBGlg> .

agners · 2019-03-21T13:25:10Z

src/libaktualizr/bootloader/bootloader.cc

+    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";
+      }


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)...

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)

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).

@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?

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 · 2019-03-21T14:12:25Z

@mike-sul, thanks for the explanation of how systemd handle reboot system call, did not know that before. But shall we also consider supporting sysvinit systems in which case "/sbin/init 6" or "/sbin/reboot" probably could be a more graceful way rather than "reboot" system call? //Ming Liu mike-sul <notifications@github.com> 於 2019年3月21日週四下午3:01寫道：

…

***@***.**** commented on this pull request. ------------------------------ In src/libaktualizr/bootloader/bootloader.cc <#1148 (comment)> : > if (setuid(0) != 0) { LOG_ERROR << "Failed to set/verify a root user so cannot reboot system programmatically"; + return; + } + + 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"; + } 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). — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#1148 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AHsYstFTq6beKQCx-mqirN3Zv_PNuyasks5vY5C_gaJpZM4cBGlg> .

doanac · 2019-03-21T14:15:51Z

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 · 2019-03-22T09:22:55Z

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

liuming50 · 2019-03-22T10:55:58Z

@doanac will remake a V3 as you suggested.

pattivacek · 2019-04-23T15:07:47Z

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

liuming50 · 2019-04-23T19:37:56Z

Hi, Patrick: 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. the best, thank you Patrick Vacek <notifications@github.com> 於 2019年4月23日週二下午5:07寫道：

…

@liuming50 <https://github.com/liuming50> Is this still something you are interested in? I think your plan sounded fine. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1148 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AB5RRMX6RUMUWHJ62INZZE3PR4Q4JANCNFSM4HAENFQA> .

pattivacek · 2019-04-24T07:17:49Z

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!

codecov-io · 2019-04-30T13:31:45Z

Codecov Report

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

@@            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 · 2019-04-30T13:33:52Z

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 · 2019-05-02T12:08:34Z

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 · 2019-05-02T15:33:51Z

@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 · 2019-05-09T08:55:46Z

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"

lbonn · 2019-05-09T10:24:18Z

src/libaktualizr/bootloader/bootloader.h

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

  INvStorage& storage_;
  boost::filesystem::path reboot_sentinel_;
+  std::string reboot_command_;


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.

lbonn · 2019-05-09T10:26:30Z

docs/configuration.adoc

@@ -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.


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).

lbonn · 2019-05-09T10:36:47Z

src/libaktualizr/bootloader/bootloader.cc

+      return;
+    }
+
+    reboot_command_.copy(full_cmd, len);


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

mike-sul · 2019-05-09T11:42:46Z

src/libaktualizr/bootloader/bootloader.cc

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


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.

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.

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.

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

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

mike-sul · 2019-07-30T08:58:50Z

@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 · 2019-07-30T09:15:39Z

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> .

mike-sul · 2019-07-30T15:28:25Z

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.

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 <liu.ming50@gmail.com> Signed-off-by: Mykhaylo Sul <myk.sul@gmail.com>

mike-sul · 2019-07-31T10:33:10Z

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

/sbin/reboot, systemd reboot, system(reboot) - these three will have the same effect in our case
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)
don't have any default

lbonn · 2019-07-31T11:46:44Z

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 · 2019-07-31T16:02:17Z

Replaced by #1274.

liuming50 mentioned this pull request Mar 21, 2019

OTA-2135: Aktualizr's config and recipe to auto reboot just after update advancedtelematic/meta-updater#486

Merged

liuming50 force-pushed the bootloader-gracefully-reboot branch from 409cd87 to 4cc4bc7 Compare March 21, 2019 13:24

agners reviewed Mar 21, 2019

View reviewed changes

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

liuming50 force-pushed the bootloader-gracefully-reboot branch from b101006 to c4a11c8 Compare May 9, 2019 08:47

liuming50 changed the title ~~bootloader: gracefully reboot using '/sbin/init 6'~~ bootloader: gracefully reboot after update completes May 9, 2019

lbonn reviewed May 9, 2019

View reviewed changes

mike-sul reviewed May 9, 2019

View reviewed changes

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

mike-sul force-pushed the bootloader-gracefully-reboot branch from 69c351d to 9c0d102 Compare July 31, 2019 10:19

mike-sul force-pushed the bootloader-gracefully-reboot branch from 9c0d102 to 1f6171d Compare July 31, 2019 10:26

lbonn approved these changes Jul 31, 2019

View reviewed changes

mike-sul mentioned this pull request Jul 31, 2019

Configurable reboot command #1274

Merged

pattivacek closed this Jul 31, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bootloader: gracefully reboot after update completes #1148

bootloader: gracefully reboot after update completes #1148

liuming50 commented Mar 21, 2019

agners commented Mar 21, 2019

liuming50 commented Mar 21, 2019 via email

agners Mar 21, 2019

liuming50 Mar 21, 2019

mike-sul Mar 21, 2019

agners Mar 21, 2019

mike-sul Mar 21, 2019

liuming50 commented Mar 21, 2019 via email

doanac commented Mar 21, 2019

lbonn commented Mar 22, 2019

liuming50 commented Mar 22, 2019

pattivacek commented Apr 23, 2019

liuming50 commented Apr 23, 2019 via email

pattivacek commented Apr 24, 2019

codecov-io commented Apr 30, 2019 •

edited

Loading

liuming50 commented Apr 30, 2019

lbonn commented May 2, 2019

liuming50 commented May 2, 2019

liuming50 commented May 9, 2019

lbonn May 9, 2019

lbonn May 9, 2019

lbonn May 9, 2019

mike-sul May 9, 2019 •

edited

Loading

lbonn May 9, 2019

zabbal Jun 26, 2019

lbonn Jun 26, 2019

zabbal Jun 28, 2019

mike-sul commented Jul 30, 2019

liuming50 commented Jul 30, 2019 via email

mike-sul commented Jul 30, 2019

mike-sul commented Jul 31, 2019

lbonn commented Jul 31, 2019

pattivacek commented Jul 31, 2019

bootloader: gracefully reboot after update completes #1148

bootloader: gracefully reboot after update completes #1148

Conversation

liuming50 commented Mar 21, 2019

agners commented Mar 21, 2019

liuming50 commented Mar 21, 2019 via email

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liuming50 commented Mar 21, 2019 via email

doanac commented Mar 21, 2019

lbonn commented Mar 22, 2019

liuming50 commented Mar 22, 2019

pattivacek commented Apr 23, 2019

liuming50 commented Apr 23, 2019 via email

pattivacek commented Apr 24, 2019

codecov-io commented Apr 30, 2019 • edited Loading

Codecov Report

liuming50 commented Apr 30, 2019

lbonn commented May 2, 2019

liuming50 commented May 2, 2019

liuming50 commented May 9, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mike-sul May 9, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mike-sul commented Jul 30, 2019

liuming50 commented Jul 30, 2019 via email

mike-sul commented Jul 30, 2019

mike-sul commented Jul 31, 2019

lbonn commented Jul 31, 2019

pattivacek commented Jul 31, 2019

codecov-io commented Apr 30, 2019 •

edited

Loading

mike-sul May 9, 2019 •

edited

Loading