-
Notifications
You must be signed in to change notification settings - Fork 61
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
OTA-2135: Aktualizr's config and recipe to auto reboot just after update #486
Conversation
Signed-off-by: Mike Sul <mykhaylo.sul@innoteka.com>
16052b7
to
d806431
Compare
It looks good I think but it will need to have an update of the aktualizr recipe's git commit before merging. So I will do a bit of manual testing first, then we can merge advancedtelematic/aktualizr#1098 when we'll clear all objections. And finally, this PR should be updated with the new (master) revision. |
…ot functionality Signed-off-by: Mike Sul <ext-mykhaylo.sul@here.com>
Not a big deal, but in general I prefer rebasing instead of merging. I'll run oe-selftest soon and give the feature a manual test, but it looks fine! |
yes, agree on rebasing, just screwed/forgot in this case. |
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.
Passes oe-selftest, but I still want to manually test it.
Manual test worked as expected. However, I'm seeing two issues in the log that are confusing:
And when we print these We can merge this PR as is if you'd like, but we should fix these logging issues before updating the other release branches of meta-updater. |
Those two are preexisting problems:
Agree that we should review these but I would merge this PR now as I don't think they are newly introduced. I also think (would need to check to be sure) they already show up in the release branches. |
Probably a bit late, but another elegant solution is to use a systemd path unit: The reboot is instant since systemd uses inotify... |
Oh cool, that's interesting! However we persist some state after creating this file, so wouldn't it potentially race with the reboot process? |
Uh, yes, that definitely races... I guess in practise there is a slight delay since systemd activates the service with the same name (in the above example But why is aktualizr persisting state after creating that file? That seems racy no matter what reboot mechanism is watching that file... |
It was originally not intended to be watched by an external process but only checked at aktualizr startup:
So, when we read the flag at launch: if the file is here, aktualizr was just restarted. If it's not here, the system has most likely been rebooted. Maybe we named this file poorly but I think the order is correct for our use case. However, we could use another file to trigger the reboot through systemd instead of having this platform-specific logic in the C++ app. |
Hm, I see... Yeah I guess ideally that needs to be documented, that it's internal state... I like the idea of having a file to signal that reboot is required. This allows e.g. a UI process to monitor for it too, and e.g. print a user visible hint that a reboot is required (at earliest convince...). With systemd path unit its actually quite easy to achieve automatic reboot, however I can imagine that other init systems that might be not that straight forward. I see the new reboot implementation directly calls the reboot syscall, which bypasses the init system completely. I am not sure if that is very safe to use in practise since data might get lost. I feel typically we want the init system to be notified. There are quite some standard commands to do that, e.g. executing Btw, what I also realized when using OSTree, after the update is applied, changes to |
Just looked a bit closer into aktualizr, and see now that the However, that said, from what I understand aktualizr tries to do updates such that in any point in time a power cut can happen, the system should recover from any such state...? |
Yes, if a power cut occurs it should either run the old or the new version. The danger here would not be bricking the device but wrongly reporting state changes to the backend, which should be an error that can be recovered from. |
+1
I personally prefer the reboot to be managed by the init system, as then the user can easily add other hooks that can be executed during the shutdown process. Having in systemd also allows the user to set up a systemd timer, and only reboot at an specific time (e.g. early morning).
This can probably be improved by having a systemd job that performs another /etc merge during the shutdown process. |
Relevant upstream issues: |
Thank you both for your feedback! I also agree it would fit better on the init system's side as it gives more control to the user. To give a bit of context, we've added this reboot-trigger feature for our testing needs without really focusing on production use at the moment (also why it's disabled by default). But since you've raised the issue, we can probably take another go and polish it a bit. |
Hi, I have created a pull request to introduce "/sbin/init 6" as the way to gracefully reboot, please kindly help me review: advancedtelematic/aktualizr#1148 |
No description provided.