[systemd-devel] offline updates

[Zbigniew Jędrzejewski-Szmek]

On Tue, Jul 21, 2015 at 03:00:39PM -0400, Will Woods wrote:
> On Tue, 2015-07-21 at 03:27 +0000, Zbigniew Jędrzejewski-Szmek wrote:
> 
> > fedup-system-upgrade.service uses an additional flag file which is
> > checked with ConditionPathExists so it will not run if 'dnf fedup 
> > reboot'
> > did not create the flag, even if we go into system-upgrade.target.
> > 
> > packagekit-offline-update.service does not have anything like this, 
> > and
> > is always run in system-upgrade.target.

> Perhaps your system was also trying to install new updates
> via PackageKit at the time?
This seems to be a likely explanation. This VM had a graphical
environment installed at some point, so maybe there was a pending
update scheduled. Next time I'll try to test in a pristine environment ;)

> Strange - from what I can tell from the source, pk-offline-update is
> supposed to bail out unless the update was prepared by PackageKit
> itself[1]?
You're right, it the code seems to do that. But there's still a problem:
if the update is started, action is set to e.g. PK_OFFLINE_ACTION_REBOOT,
and then an error occurs, pk_offline_update_reboot() will be called
in line 466. This should be easy enough to fix:
https://github.com/hughsie/PackageKit/pull/72

> Maybe packagekit-offline-update.service also needs a
> ConditionPathExists..
Let's see... If dnf fedup scheduled an offline update,
packagekit-offline-update.service would run too. It would look
for it's state files, not find them, and return with an error.
systemd would treat this as a signal to start reboot.target.
Yikes, indeed, this cannot work in current version.
(Based on this, a new proposal below.)

> > Running two upgrade mechanisms in parallel does not seem like a good
> > idea. (Even if they use a lock file to prevent concurrent access to
> > the rpm database, they are bound to interfere with one another: the
> > first finishes and decides to reboot, or the first updates some
> > packages and messes up the state for the second one...) It seems that
> > *some* mechanism to run only one upgrade mechanism is wanted. The 
> > approach
> > that dnf-plugin-fedup uses seems reasonable: it creates the file only 
> > when
> > a reboot with 'dnf fedup reboot' is requested.
> > As an alternative we could allow only one upgrade mechanism to be 
> > enabled.
> > Dunno.
> 
> How would that be enforced, though? Special handling for system
> -updates.target?
> 
> Or: does systemd need some convention/support for handling the general
> problem of choosing one of multiple (mutually-exclusive) services that
> provide the same functionality?
One can play with Alias=, but I don't think this is applicable here,
because we want something dynamic: even if I have packagekit-offline-update.service
enabled to run updates from gnome, it should not preclude
me from running 'dnf fedup' to update the system.

So a solution might be to use /system-update as a flag:
updater service should run, look for /system-update, and
if it is missing or does not point to it's own cache, do nothing and return 0.
If it points at its own cache, try to run an update, and either
schedule a reboot/shutdown at the end, or return an error.

> > Also, which is a minor thing, but related: OnFailure=reboot.target
> > seems inferior to FailureAction=reboot. IIRC, the second one uses
> > irreversible transaction and should be more robust. It also is a
> > higher level setting in some sense.  OnFailure=reboot.target is taken
> > directly from the spec, so should be changed there first.
> 
> I'll add a note about this to the fedup-system-upgrade.service, I
> guess, and if the spec changes I'll change it there too.
I prepared a patch locally, and I'll send a pull request if the
spec is updated.

> > So maybe tmpfiles snippet should be used to remove the link. Such a
> > change would mean that the update services should not depend on the
> > symlink being present, and should instead look for their
> > installation data in their own state directory.
> 
> Right - this is what dnf-plugin-fedup does, since pk-offline-update
> might run first and remove the symlink before we get there.
I think that running two system updates concurrently or
sequentially is risky. I think only one update should be allowed
to be scheduled and to run.

> > To summarize, following changes to the spec are proposed:
> > - use Condition* or similar to conditionalize whether a specific
> >   upgrade mechanism should run
> > - use Action=reboot
> 
> FailureAction=reboot, I guess?
Right.

> > - use Type=oneshot
> 
> This is the default, correct? Should it be explicitly listed in the
> unit file, or should I rely on the default behavior?
No, the default it Type=simple.

> > - check that logind.Reboot() is not called on failure by the service
> > - services should not look for /systemd-update symlink,
> >   and the symlink should be removed by tmpfiles before we even get to
> >   the upgrade.
> 
> These all seem reasonable to me. I'll happily make changes to dnf
> -plugin-fedup to follow any changes to the spec, once consensus is
> reached and the spec is changed.
Unfortunately this is not good enough to work. Version two proposal:

- when an offline update mechanism wants to schedule a reboot, it 
  should look for /system-update. If the file is present and does
  not point at its own cache, refuse.
  (The presence of that file means that another update was scheduled
   already. In that case it is not wanted anymore, it should be somehow
   cancelled first.)

- when the offline update service is run, it should look for /system-update.
  If it is missing or does not point to it's own cache, do nothing and return 0.
  If it points at its own cache (even if it is dangling), remove it,
  and try to run an update. Then schedule a reboot/shutdown on
  success, or return an error.

- Condition* should not be used, and the update service should always
  run to get a chance to remove /system-update symlink.

- FailureAction=reboot
- Type=oneshot

This should make the update mechanisms exclusive, but still allow
multiple mechanisms to co-exist.

Zbyszek