search

sailfishos-patches / patchmanager

Patchmanager for SailfishOS
https://openrepos.net/content/patchmanager/patchmanager
Other
21 stars 22 forks source link

Do not start pm in upgrade mode #39

Open CODeRUS opened 5 years ago

CODeRUS commented 5 years ago

Add os-update.target to conflicts of pm service

Olf0 commented 5 years ago

Thanks, this may resolve issue #33 as well.

Edit: No it won't, see https://github.com/sailfishos-patches/patchmanager/issues/33#issuecomment-926988815. Nevertheless, to implement this enhancement does make sense.

nephros commented 2 years ago

I'm sorry, I'd like to get some context on this.

Change proposed is to add a Conflicts=os-update.target entry to /usr/lib/systemd/system/dbus-org.SfietKonstantin.patchmanager.service, correct?

With the goal to not interfere with anything going on during a system update.

Qs:

  1. is this still relevant in the days of PM3 and SFOS 4.x?
  2. if yes, what creates os-update.target? Is it reliably* a thing that indicates a device is currently updating?
  3. if not, what else might be used to prevent PM to be active during updates?

[*] with definitions of reliable adapted to the reality of how SFOS/Jolla changes happen... ;)

Olf0 commented 2 years ago

As:

  1. Yes, I believe so: It does not make any sense for PM's background services to start, when os-update.target is active.
  2. a. SailfishOS! b. Well, it is reliably indicating that a SailfishOS installation has booted into the "single user upgrade phase" of an OS upgrade via GUI. You know (if not upgrading SFOS at the command line), one looks for an SFOS-upgrade in the Settings app, downloads its RPMs there; then the device reboots (or switches?) to os-update.target, only a progress bar is shown while the RPMs are installed and finally the device switches (or reboots, I do not remember precisely) to the regular multiuser-with-UI target. Mind that this method of upgrading SFOS cannot be used on community ports and "Sailfish Free / Trial" installations (i.e., SFOS installations without a paid license from Jolla) and there are many other ways of upgrading SFOS (the "zypper dance" / per zypper --dup, per version dup / sfos-upgrade, per pkcon upgrade-system), which all do not boot / switch to a special target. TL;DR: os-update.target is a kind of "single user mode for upgrading SFOS", used when triggering an OS upgrade per Settings app. c. Well, this mechanism exists at least since SFOS 1.1.x (when I joined the club).
  3. Good question. I guess the answer is "nothing", because the upgrading per command line (whichever way) cannot be detected, as it is just downloading and installing RPMs. Actually, what all "alternative" methods do have in common, is that one has to set the SSU utility into release mode per ssu re <version-to-upgrade-to> while at the regular multiuser-with-UI target (the GUI upgrade likely does also set SSU to release mode, but only while at the os-update.target). The Seamless Software Update (SSU) utility was originally developed by Nokia for the N900 and Jolla's documentation for it is extremely terse, but their MDM documentation shows that there is an "int deviceMode() - The repository configuration mode e.g. Release, RnD, Upgrade. See ssu docs for further info.", which seems to be query-able (but I have no idea, where the mentioned documentation may be and if there is a hook / callback to be triggered). But you can dig deeper into SSU, its local infrastructure and how it is embedded into SFOS: https://github.com/search?q=org%3Asailfishos+ssu

HTH If there are more questions, do ask, as the basics of this (local) low-level infrastructure is a field I was interested in for a long time (and hence have gathered some knowledge). But Andrey is likely an expert on this, because these things are part of his day job, AFAIK.

nephros commented 2 years ago

Thank you. Much clearer now.

@Olf0 Can you please test https://github.com/nephros/patchmanager/commit/d14a3ee9287fc48f48668d0978be92ee7edaef10 somehow? (Well, it's just the single line added :) ). I have confirmed adding the line doesn't affect restarting the service or rebooting, but I do not know how to test the actual "os-update.target exists" case.

Olf0 commented 2 years ago

Thank you. Much clearer now.

Well, then it is time for me to report the things I do not understand WRT @CODeRUS' original issue description, "Add os-update.target to conflicts of pm service": See below.

@Olf0 Can you please test nephros@d14a3ee somehow? (Well, it's just the single line added :) ). I have confirmed adding the line doesn't affect restarting the service or rebooting, but I do not know how to test the actual "os-update.target exists" case.

Just adding Conflicts=os-update.target (to bin/patchmanager-daemon/systemd/dbus-org.SfietKonstantin.patchmanager.service) is definitely too simple:

  1. As with all dependency statements in systemd units, Conflicts= does not include a (temporal) order and does not make much sense (or results in unexpected behaviour) without one. See https://www.freedesktop.org/software/systemd/man/systemd.unit.html#Conflicts= Hence an After=os-update.target or Before=os-update.target should be added, but which? → See below.
  2. I tried research how this is supposed to work in practice (aside of the theoretical considerations I described above).
    • Searching the WWW for "os-update.target" reveals ... nothing.
    • But I remembered that systemd has some mechanisms Jolla might utilise:
    • So I tried to look at this in practice on an XperiaX@SFOS3.2.1 But executing a systemctl -l -t target --all (or an equivalent systemctl -l -t target --all list-units) does not show an os-update.target! Neither does a systemctl -l -t target --all list-unit-files show anything indicating such a target!

But at least re-reading systemd's man-page for "offline updates", specifically "Recommendation 4" convinces me that Before=os-update.target is the better ordering statement. Unfortunately system-update.target is completely missing on systemd's man-page "bootup".

@CODeRUS, can you please provide some background on os-update.target, how it works, where it can be found and analysed, and if simply adding Conflicts=os-update.target and Before=os-update.target to bin/patchmanager-daemon/systemd/dbus-org.SfietKonstantin.patchmanager.service is what you had in mind.

nephros commented 2 years ago

Good hints on where to look for some more! I conclude that os-upgrade.target was never meant literally, and it has always been about that systemd special.

I found that the packagekit package does contain some things related to OS-update:

For one there's a snippet of documentation on how to use the built-in feature: https://github.com/sailfishos/PackageKit/blob/master/docs/offline-updates.txt

And looking at the installed package on my device:

$ rpm -ql  PackageKit | grep systemd
/usr/lib/systemd/system/packagekit-offline-update.service
/usr/lib/systemd/system/packagekit.service
/usr/lib/systemd/system/system-update.target.wants/packagekit-offline-update.service

So this: https://github.com/sailfishos/PackageKit/blob/master/data/packagekit-offline-update.service.in

Aaand there's also sailfish-upgrade-ui which contains:

/usr/lib/systemd/system/osupdate-logging.service
/usr/lib/systemd/system/sailfish-upgrade-ui.service
/usr/lib/systemd/system/system-update-cleanup.service.d
/usr/lib/systemd/system/system-update-cleanup.service.d/os-update-running.conf
/usr/lib/systemd/system/system-update-pre.target.wants
/usr/lib/systemd/system/system-update-pre.target.wants/sailfish-upgrade-ui.service
/usr/libexec/sailfish-upgrade-ui

of which /usr/lib/systemd/system/system-update-cleanup.service.d/os-update-running.conf contains the very interesting:

[Unit]
ConditionPathExists=!/tmp/os-update-running

with /usr/lib/systemd/system/sailfish-upgrade-ui.service doing

ExecStartPre=-/bin/touch /tmp/os-update-running
ExecStopPost=-/bin/rm -f /tmp/os-update-running

So assuming that mechanism is used during OS updates, we should either

It looks to me like just having that Condition is sufficient for the purpose of not starting if update is running.
And if I read the documentation you linked to correctly, we shouldn't have system-update.target anywhere in the Unit definition, or else we're actually pulled as a dep in that target which is the opposite of what we want. In fact, we shouldn't even be started for system-update.target so maybe this whole issue is actually moot -- unless our After=dbus.service somehow pulls us into the dep graph during updates.

nephros commented 2 years ago

One more thing:

Our RPM installs or changes /etc/ld.so.preload, which will of course be in effect always, even in upgrade mode. We could introduce a special service which Requires/Before=system-update-pre.target which moves that file out of the way during updates, so failure to load the library doesn't kill the update process.

Or we could set NO_PM_PRELOAD as an environment variable, but I don't know how to set that globally for everything and only in system-update mode.

Olf0 commented 2 years ago

I conclude that os-upgrade.target was never meant literally, and it has always been about that systemd special.

… system-update.target. Yes, probably.

[...] of which /usr/lib/systemd/system/system-update-cleanup.service.d/os-update-running.conf contains the very interesting:

[Unit]
ConditionPathExists=!/tmp/os-update-running

Yes, now I remember to have seen this pattern a couple of times in systemd units created by Jolla.

with /usr/lib/systemd/system/sailfish-upgrade-ui.service doing

ExecStartPre=-/bin/touch /tmp/os-update-running
ExecStopPost=-/bin/rm -f /tmp/os-update-running

Thanks, while I have become a little bit of a systemd expert due to analysing SailfishOS, I never took the effort to track that down.

So assuming that mechanism is used during OS updates, we should either

* `Conflicts=` (and `Before=/After=`) one of those services

IMO, rather not, but ...

* `ConditionPathExists=!/tmp/os-update-running`

exactly that.

It looks to me like just having that Condition is sufficient for the purpose of not starting if update is running.

Well, sort of: But a ConditionPathExists=!/tmp/os-update-running will not stop our unit, when the condition becomes untrue, when it is already started. While dependencies define when units are queued for starting, the Condition and Assert statements are evaluated, when a unit is started; but after that … it would take some research for me to see if anything can be done then.

I think the solution is not to have it enqueued too early.

And if I read the documentation you linked to correctly, we shouldn't have system-update.target anywhere in the Unit definition, or else we're actually pulled as a dep in that target which is the opposite of what we want.

No, I do not believe so. But it is not necessary this way (with ConditionPathExists=!/tmp/os-update-running), any way.

In fact, we shouldn't even be started for system-update.target so maybe this whole issue is actually moot -- unless our After=dbus.service somehow pulls us into the dep graph during updates.

Wrong conclusion: It is systemd and its automatisms make it complicated to comprehend! :stuck_out_tongue_winking_eye: After=dbus.service alone does nothing, it is just a directive for temporal ordering, if some dependency is defined. Requires=dbus.service is such a dependency. But both are unnecessary, because Type=dbus automatically sets both (Requires=dbus.service and After=dbus.service) any way, unless one sets DefaultDependencies=No. OTOH, it does no harm to duplicate statements, i.e. to express explicitly, what systemd already does implicitly.

And AFAIR (look at the boot chart) the dbus.target is reached before the units for the system-update.target are started. Mind that targets are just defined steps in the boot process to group units.

The systemd man-pages are basically well written (except for the never properly defined, but often used phrase "to pull in"), but one has to read the most important ones thoroughly to gather an overall understanding. The issue is to find "the most important ones", because that is nowhere documented and depends a bit on the use case.

TL;DR: Solely use ConditionPathExists=!/tmp/os-update-running instead of the currently inserted Conflicts=os-update.target.

But please also take a look at the other units, they shall not be activated when /tmp/os-update-running exists, too.

P.S.: I will try to take a closer look at all these units some time, because at first sight a couple of things can and likely should be cleaned up. One issue with systemd is, that it often accepts superfluous and even contradicting statements silently, while trying to do the right thing (and even often succeeds with that), but sometimes such statements cause weird side effects. In general systemd is a topic for which I should be able to assist well with guidance, know-how (just having studied all man-pages at least 3 times, some 10+) and practical experience. It is just very tedious to read and reread these man-pages often, but they seem to be exhausting, i.e. nothing is not defined in them. Trying things out is even more tedious with systemd, because everything is somewhat complicated (command syntax etc.), but feasible.

Olf0 commented 2 years ago

One more thing:

Our RPM installs or changes /etc/ld.so.preload, which will of course be in effect always, even in upgrade mode.

But that behaves transparently (i.e., like its original version), when it loads things not on a whitelist, correct? Or does nothing call it, when the PM-unit is not active?

Sorry, I never looked those things up and have not yet thought about them.

"installs or changes" (i.e., which of both) is the first thing we should know for sure!

We could introduce a special service which Requires/Before=system-update-pre.target which moves that file out of the way during updates, so failure to load the library doesn't kill the update process.

Which "the library"? (Will ponder about that, some time.)

Or we could set NO_PM_PRELOAD as an environment variable, but I don't know how to set that globally for everything and only in system-update mode.

I do not think that is viable.