Skip to content
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

postprocess: run systemctl preset-all #77

Merged
merged 1 commit into from
Apr 18, 2019

Conversation

cgwalters
Copy link
Member

This is a followup to #73

The problem we hit with RHCOS is that becomes a dangerous upgrade hazard.
Since we previously shipped with the unit files default enabled in /etc
(i.e. in ostree, the defaults are in /usr/etc), the problem comes
with the combination of the fact that during ConditionFirstBoot
we'll do systemctl preset-all which will write those files, but
they already existed.

Then if we try to upgrade to a tree without them, ostree will notice
the files were deleted in the new defaults and apparently not modified...
and then no NetworkManager.service anymore.

Really we want to have these defaults in /usr. We want e.g.
NetworkManager.service to be defined as defaulted to on by us,
not in /etc.

I couldn't see a way to do this directly via the systemd CLI tools but the two-line
shell implementation is pretty trivial, just need to do a mkdir
pass, then move the links.

I really wish rpm-ostree had done this from the very start, it seems
obvious in retrospect.

@cgwalters
Copy link
Member Author

Still doing some testing of this, having a weird issue with local cosa builds that is apparently the temp webserver dying during virt-install for some reason.

@cgwalters cgwalters marked this pull request as ready for review April 5, 2019 13:16
@cgwalters
Copy link
Member Author

I verified that the system looks fine after this (haven't yet verified that it fixes our upgrade issue though). However, I also notice that systemctl preset-all still writes out enablement links to /etc even when they exist in /usr which is surprising.

@cgwalters
Copy link
Member Author

@jlebon and I had a quick chat about this, I'm going to try updating it to run systemctl preset-all so we are totally consistent on having enabled units derive from the presets - explicitly breaking anything like a RPM %post or a human doing systemctl enable foo without also updating the presets.

@cgwalters
Copy link
Member Author

OK updated, and here's the diff from the result:

--- /tmp/nopreset-all.txt	2019-04-05 15:28:48.540432966 +0000
+++ /tmp/preset-all.txt	2019-04-05 15:28:00.403493260 +0000
@@ -19,7 +19,7 @@
 -00664 0 0    238 /usr/lib/systemd/system/coreos-growpart.service
 -00644 0 0    465 /usr/lib/systemd/system/cryptsetup-pre.target
 -00644 0 0    412 /usr/lib/systemd/system/cryptsetup.target
-l00777 0 0      0 /usr/lib/systemd/system/ctrl-alt-del.target -> reboot.target
+l00777 0 0      0 /usr/lib/systemd/system/ctrl-alt-del.target -> /usr/lib/systemd/system/reboot.target
 -00644 0 0    560 /usr/lib/systemd/system/dbus-daemon.service
 l00777 0 0      0 /usr/lib/systemd/system/dbus-org.freedesktop.NetworkManager.service -> /usr/lib/systemd/system/NetworkManager.service
 l00777 0 0      0 /usr/lib/systemd/system/dbus-org.freedesktop.hostname1.service -> systemd-hostnamed.service
@@ -303,10 +303,14 @@
 l00777 0 0      0 /usr/lib/systemd/system/multi-user.target.wants/chronyd.service -> /usr/lib/systemd/system/chronyd.service
 l00777 0 0      0 /usr/lib/systemd/system/multi-user.target.wants/console-login-helper-messages-issuegen.service -> /usr/lib/systemd/system/console-login-helper-messages-issuegen.service
 l00777 0 0      0 /usr/lib/systemd/system/multi-user.target.wants/console-login-helper-messages-motdgen.service -> /usr/lib/systemd/system/console-login-helper-messages-motdgen.service
+l00777 0 0      0 /usr/lib/systemd/system/multi-user.target.wants/coreos-growpart.service -> /usr/lib/systemd/system/coreos-growpart.service
 l00777 0 0      0 /usr/lib/systemd/system/multi-user.target.wants/dbus-daemon.service -> /usr/lib/systemd/system/dbus-daemon.service
 l00777 0 0      0 /usr/lib/systemd/system/multi-user.target.wants/dbxtool.service -> /usr/lib/systemd/system/dbxtool.service
 l00777 0 0      0 /usr/lib/systemd/system/multi-user.target.wants/getty.target -> ../getty.target
+l00777 0 0      0 /usr/lib/systemd/system/multi-user.target.wants/ignition-firstboot-complete.service -> /usr/lib/systemd/system/ignition-firstboot-complete.service
 l00777 0 0      0 /usr/lib/systemd/system/multi-user.target.wants/mdmonitor.service -> /usr/lib/systemd/system/mdmonitor.service
+l00777 0 0      0 /usr/lib/systemd/system/multi-user.target.wants/ostree-finalize-staged.path -> /usr/lib/systemd/system/ostree-finalize-staged.path
+l00777 0 0      0 /usr/lib/systemd/system/multi-user.target.wants/remote-cryptsetup.target -> /usr/lib/systemd/system/remote-cryptsetup.target
 l00777 0 0      0 /usr/lib/systemd/system/multi-user.target.wants/remote-fs.target -> /usr/lib/systemd/system/remote-fs.target
 l00777 0 0      0 /usr/lib/systemd/system/multi-user.target.wants/sshd.service -> /usr/lib/systemd/system/sshd.service
 l00777 0 0      0 /usr/lib/systemd/system/multi-user.target.wants/sssd.service -> /usr/lib/systemd/system/sssd.service

And yep, doing the preset-all helps ensure we're fully consistent.

@bgilbert
Copy link
Contributor

bgilbert commented Apr 5, 2019

CL works this way now: default-enabled units are enabled via symlinks in /usr. The problem with that is that those units can't be disabled. On a reasonably current systemd it's possible to override the /usr symlinks by putting a symlink to /dev/null in /etc/systemd/system/*.{wants,requires}, but systemctl disable doesn't know how to do that; it'll silently pretend to disable the unit but will actually do nothing. systemctl status is also confused by units that are enabled this way; they're reported as static, which is unhelpful.

So, on CL, the only way users can disable a default-enabled unit is to mask it. That works, but is an abuse of masks and tends to confuse users. The decision to ship enablement links in /usr is also difficult to change without breaking existing machines, with the result that it's been a piece of tech debt that CL was never able to address.

So let's please not do this. Presets are 100% the way to go. Can rpm-ostree be taught not to mess with enablement links in /etc during upgrades?

@bgilbert
Copy link
Contributor

bgilbert commented Apr 5, 2019

The CL issue is coreos/bugs#2178 and the upstream issue is systemd/systemd#4830.

@cgwalters
Copy link
Member Author

cgwalters commented Apr 5, 2019

On a reasonably current systemd it's possible to override the /usr symlinks by putting a symlink to /dev/null in /etc/systemd/system/*.{wants,requires}, but systemctl disable doesn't know how to do that; it'll silently pretend to disable the unit but will actually do nothing. systemctl status is also confused by units that are enabled this way; they're reported as static, which is unhelpful.

I think that's a systemd bug.

So, on CL, the only way users can disable a default-enabled unit is to mask it. That works, but is an abuse of masks and tends to confuse users.

How is it an abuse of masks? EDIT: Read the linked issues; I agree that is a problem but a lot of the conversation seems to agree this is just a systemd bug.

he decision to ship enablement links in /usr is also difficult to change without breaking existing machines, with the result that it's been a piece of tech debt that CL was never able to address.

What's difficult to change?

Presets are 100% the way to go.

We are using presets I'd say.

Can rpm-ostree be taught not to mess with enablement links in /etc during upgrades?

Ugh...that'd be a huge special case in the currently generic /etc merge code.

Moreover, dropping all this stuff in /etc has two problems:

  1. It doesn't align with the "/etc/ only contains things I changed" model
  2. It creates hysteresis - if we ship a unit default enabled in /usr at version A, user boots, ConditionFirstBoot=true triggers a preset-all, an enablement link is written into `/etc. If we later stop enabling that unit, on old systems it will still be enabled.

@jlebon
Copy link
Member

jlebon commented Apr 5, 2019

It creates hysteresis - if we ship a unit default enabled in /usr at version A, user boots, ConditionFirstBoot=true triggers a preset-all, an enablement link is written into `/etc. If we later stop enabling that unit, on old systems it will still be enabled.

Hmm, though if we do preset-all at compose time, then it's part of the OSTree commit, right? I.e. we drop everything from this postprocess script, except systemctl preset-all. Then:

  • the first boot preset-all can still be leveraged through Ignition to change defaults
  • new updates that e.g. disable a previously enabled service will just remove the symlink through the 3-way merge

Right?

This also lets us transition later on from defaults in /etc to /usr if systemd fixes preset-all (so that it doesn't write to /etc) and disable (so that it can disable units enabled through /usr).

The downside though is that there's no way for admins to say "definitely always have this unit enabled, even if a later update disables it", though that's still better UX-wise than systemctl disable silently not doing what it should.

@bgilbert
Copy link
Contributor

bgilbert commented Apr 5, 2019

I think that's a systemd bug.

More of a missing feature. The systemd maintainers had not originally considered allowing /usr enablement symlinks to be overridden; see the upstream bug. @csssuf did some work on this, but it's not a trivial change; there are a number of cases that need to be handled. We could pick up that work, but it wouldn't solve the upgrade problem; see below.

How is it an abuse of masks?

Masks are for saying that you don't want a service to be started, even if another service pulls it in as a dependency. They're stronger than ordinary disablement, and aren't really for casual use.

What's difficult to change?

IIRC the main issues were:

  1. Path dependence on upgrades. Now that I think of it, though, there's a straightforward workaround for that, even if update barriers are not available. (That is: shipping the list of compatibility symlinks in the new image rather than generating it from the old one.)
  2. Maintaining correct behavior across OS rollbacks.

There's also the pain of testing the upgrade path, since a botched conversion will badly brick a system.

We are using presets I'd say.

If we're precalculating the preset and packaging the resulting symlinks (in either /etc or /usr), we're not using them the way they're designed to be used. Except for units that are mandatorily-enabled, your-system-won't-boot-if-you-disable-this-unless-you-know-what-you're-doing (which is what is meant by static), AIUI we should only create symlinks on first boot and shouldn't be shipping them in either /etc or /usr. Then the special case for /etc merging wouldn't be necessary, since rpm-ostree would think all the links are locally created.

It creates hysteresis - if we ship a unit default enabled in /usr at version A, user boots, ConditionFirstBoot=true triggers a preset-all, an enablement link is written into `/etc. If we later stop enabling that unit, on old systems it will still be enabled.

Yep, that's a feature. For example, on CL we changed the default from ntpd to systemd-timesyncd. Users who configured a custom ntp.conf would be broken by an upgrade that disabled the former and enabled the latter. Since the user didn't tell us that they depended on a default, we wouldn't know not to change it on upgrade.

(Changing the default is still a breaking change for new launches, of course. We'd need to provide lots of advance notice. But breaking the installed base is worse.)

Fixing systemd/systemd#4830 wouldn't actually address this problem. It would allow the system to encode the distinction between default-{en,dis}abled and manually-{en,dis}abled services, but we can't reasonably ask users to use that knob. How would we document it? "When adding custom configuration to a service, always enable that service using Ignition, even if it's enabled by default, so updates don't break your machine in the future."

@cgwalters
Copy link
Member Author

Yep, that's a feature. For example, on CL we changed the default from ntpd to systemd-timesyncd. [...] (Changing the default is still a breaking change for new launches, of course. We'd need to provide lots of advance notice. But breaking the installed base is worse.)

I think you're really arguing against changing services that could have reasonably been configured by the user at all. Not totally buying into the "we're just breaking new installs".

(This discussion gets into release cycles, epochs etc.)

"When adding custom configuration to a service, always enable that service using Ignition, even if it's enabled by default, so updates don't break your machine in the future."

One side note, rpm-ostree supports this today, if you rpm-ostree install logrotate e.g. it will always be installed, even if a future update would drop it, it would switch to package layering then.

Hmm, though if we do preset-all at compose time, then it's part of the OSTree commit, right? I.e. we drop everything from this postprocess script, except systemctl preset-all

We could do that...it would be basically reverting #73 and going the other way?

@bgilbert
Copy link
Contributor

bgilbert commented Apr 5, 2019

I think you're really arguing against changing services that could have reasonably been configured by the user at all.

It's not just configuration, of course; it's behavior the user could be relying on. If a service is really only an implementation detail... then it should be pulled in as a dependency of something else, and there's no explicit enablement to worry about here. But if the service is explicitly enabled, I'm assuming it's probably relevant to the user.

One side note, rpm-ostree supports this today, if you rpm-ostree install logrotate e.g. it will always be installed, even if a future update would drop it, it would switch to package layering then.

Do you expect users to peform that operation?

@jlebon
Copy link
Member

jlebon commented Apr 5, 2019

We could do that...it would be basically reverting #73 and going the other way?

Hmm, OK how about:

rm -vrf /etc/systemd/system/*
systemctl preset-all

?

Then, /etc/systemd/system exactly matches what the presets say it should be. That should still address the issues that motivated #73, right?

Yep, that's a feature. For example, on CL we changed the default from ntpd to systemd-timesyncd.

OK, so here's a hypothetical: let's imagine this was FCOS, at some point eventually we want to be able to drop ntpd from the host, right? (After of course a generous and well-communicated deprecation window). At which point, we'll want the default to become systemd-timesyncd on upgrade. Relying on OSTree's 3-way merge allows us to do this.

I agree that we should be very careful with any change in the default set of enabled services. But I'm not sure if that implies we shouldn't have that ability at all as maintainers.

@cgwalters
Copy link
Member Author

Maintaining correct behavior across OS rollbacks.

BTW remember ostree has a separate /etc for each deployment.

Then, /etc/systemd/system exactly matches what the presets say it should be. That should still address the issues that motivated #73, right?

I think so.

@bgilbert
Copy link
Contributor

bgilbert commented Apr 5, 2019

OK, so here's a hypothetical: let's imagine this was FCOS, at some point eventually we want to be able to drop ntpd from the host, right? (After of course a generous and well-communicated deprecation window). At which point, we'll want the default to become systemd-timesyncd on upgrade. Relying on OSTree's 3-way merge allows us to do this.

Sure. But we'd very likely want to change the default for new installs before switching existing nodes. The latter might not happen until we stop shipping the old daemon.

I'm not thrilled with the 3-way merge approach here, but it'd probably be okay as a starting point, and the infrastructure is already set up to work in those terms. I guess we can work around it with custom upgrade logic if needed.

I agree that we should be very careful with any change in the default set of enabled services. But I'm not sure if that implies we shouldn't have that ability at all as maintainers.

Sure. At the least, let's add a kola test that checks default unit enablement against a hardcoded list.

BTW remember ostree has a separate /etc for each deployment.

Right, fair enough.

@ajeddeloh
Copy link
Contributor

So let's please not do this. Presets are 100% the way to go.

In some ways I wonder if presets really are the right way for immutable OSs to ship defaults. What advantage do we have over shipping the enablement links in /etc? Why not do what @jlebon sugguested and calculate out the enablement links using the presets (since that's our source of truth for the defaults) then delete the presets files. Ignition and other tools will still write out presets files and we can still use those, but what do we gain by not pre-computing the enablement?

@bgilbert
Copy link
Contributor

In some ways I wonder if presets really are the right way for immutable OSs to ship defaults.

Yeah, I agree they're not intended for that.

What advantage do we have over shipping the enablement links in /etc?

On CL, my answer would have been that they're a more natural way to express unit enablement than systemd-tmpfiles. On FCOS, where /etc is managed and we can pre-render the presets, I'd say the advantage is that the links don't change once the OS is upgraded. But I've already been talked out of the latter. 🤷‍♂️

Why not do what @jlebon sugguested and calculate out the enablement links using the presets (since that's our source of truth for the defaults) then delete the presets files.

Yeah, that seems to be the logical conclusion.

@jlebon
Copy link
Member

jlebon commented Apr 16, 2019

@cgwalters Want to update this patch to just add a systemctl preset-all after the rm -rf?

@cgwalters
Copy link
Member Author

Sure, done, though haven't tested it yet.

One issue is this is going to be a point of divergence between RHCOS and FCOS.

@jlebon
Copy link
Member

jlebon commented Apr 16, 2019

Hmm, offhand it seems we should be able to switch to this in RHCOS too, right? Enablements shouldn't change on update going from the previous version of this patch to this latest version.

@cgwalters
Copy link
Member Author

Hmm, offhand it seems we should be able to switch to this in RHCOS too, right? Enablements shouldn't change on update going from the previous version of this patch to this latest version.

Yes, though we need to think about upgrades from the case of dropping the units from /usr/lib and re-adding them to /usr/etc. I think that will work though.

@jlebon
Copy link
Member

jlebon commented Apr 16, 2019

Yup, that's what I meant by "previous version of this patch" :)

The only thing I can think of is if any preset changed from enabled to disabled since the switch to /usr/lib was done, the symlink in /etc will linger on. But anyway that's an issue also with the status quo of keeping things in /usr/lib and part of why we're changing this.

This is a followup to coreos#73

This way we're sure that the enabled units are just things that have
been preset.
@cgwalters
Copy link
Member Author

Here's a diff of the build with this patch; I notice this also fixes the dbus issues mentioned here. Basically we have the usual "things that change every build"; the unit file changes look expected to me:

ostree --repo=repo diff fedora/30/x86_64/coreos
M    /usr/etc/iscsi/initiatorname.iscsi
M    /usr/etc/pki/ca-trust/extracted/java/cacerts
M    /usr/lib/os-release
M    /usr/lib/modules/5.0.7-300.fc30.x86_64/initramfs.img
M    /usr/lib/sysimage/rpm-ostree-base-db/Basenames
M    /usr/lib/sysimage/rpm-ostree-base-db/Conflictname
M    /usr/lib/sysimage/rpm-ostree-base-db/Dirnames
M    /usr/lib/sysimage/rpm-ostree-base-db/Enhancename
M    /usr/lib/sysimage/rpm-ostree-base-db/Filetriggername
M    /usr/lib/sysimage/rpm-ostree-base-db/Group
M    /usr/lib/sysimage/rpm-ostree-base-db/Installtid
M    /usr/lib/sysimage/rpm-ostree-base-db/Name
M    /usr/lib/sysimage/rpm-ostree-base-db/Obsoletename
M    /usr/lib/sysimage/rpm-ostree-base-db/Packages
M    /usr/lib/sysimage/rpm-ostree-base-db/Providename
M    /usr/lib/sysimage/rpm-ostree-base-db/Recommendname
M    /usr/lib/sysimage/rpm-ostree-base-db/Requirename
M    /usr/lib/sysimage/rpm-ostree-base-db/Sha1header
M    /usr/lib/sysimage/rpm-ostree-base-db/Sigmd5
M    /usr/lib/sysimage/rpm-ostree-base-db/Suggestname
M    /usr/lib/sysimage/rpm-ostree-base-db/Supplementname
M    /usr/lib/sysimage/rpm-ostree-base-db/Transfiletriggername
M    /usr/lib/sysimage/rpm-ostree-base-db/Triggername
M    /usr/share/rpm/Basenames
M    /usr/share/rpm/Conflictname
M    /usr/share/rpm/Dirnames
M    /usr/share/rpm/Enhancename
M    /usr/share/rpm/Filetriggername
M    /usr/share/rpm/Group
M    /usr/share/rpm/Installtid
M    /usr/share/rpm/Name
M    /usr/share/rpm/Obsoletename
M    /usr/share/rpm/Packages
M    /usr/share/rpm/Providename
M    /usr/share/rpm/Recommendname
M    /usr/share/rpm/Requirename
M    /usr/share/rpm/Sha1header
M    /usr/share/rpm/Sigmd5
M    /usr/share/rpm/Suggestname
M    /usr/share/rpm/Supplementname
M    /usr/share/rpm/Transfiletriggername
M    /usr/share/rpm/Triggername
M    /usr/share/rpm-ostree/treefile.json
D    /usr/lib/ostree-boot/initramfs-5.0.7-300.fc30.x86_64.img-689a12dbde0320213290f09b6205a56fe77d9d307df339e6ca7ada18a002c5c2
D    /usr/lib/ostree-boot/vmlinuz-5.0.7-300.fc30.x86_64-689a12dbde0320213290f09b6205a56fe77d9d307df339e6ca7ada18a002c5c2
A    /usr/etc/systemd/system/ctrl-alt-del.target
A    /usr/etc/systemd/system/dbus-org.freedesktop.nm-dispatcher.service
A    /usr/etc/systemd/system/dbus-org.freedesktop.timedate1.service
A    /usr/etc/systemd/system/dbus.service
A    /usr/etc/systemd/system/messagebus.service
A    /usr/etc/systemd/system/getty.target.wants
A    /usr/etc/systemd/system/getty.target.wants/getty@tty1.service
A    /usr/etc/systemd/system/local-fs.target.wants
A    /usr/etc/systemd/system/local-fs.target.wants/ostree-remount.service
A    /usr/etc/systemd/system/multi-user.target.wants
A    /usr/etc/systemd/system/multi-user.target.wants/NetworkManager.service
A    /usr/etc/systemd/system/multi-user.target.wants/chronyd.service
A    /usr/etc/systemd/system/multi-user.target.wants/console-login-helper-messages-issuegen.service
A    /usr/etc/systemd/system/multi-user.target.wants/console-login-helper-messages-motdgen.service
A    /usr/etc/systemd/system/multi-user.target.wants/coreos-growpart.service
A    /usr/etc/systemd/system/multi-user.target.wants/dbxtool.service
A    /usr/etc/systemd/system/multi-user.target.wants/ignition-firstboot-complete.service
A    /usr/etc/systemd/system/multi-user.target.wants/mdmonitor.service
A    /usr/etc/systemd/system/multi-user.target.wants/ostree-finalize-staged.path
A    /usr/etc/systemd/system/multi-user.target.wants/remote-cryptsetup.target
A    /usr/etc/systemd/system/multi-user.target.wants/remote-fs.target
A    /usr/etc/systemd/system/multi-user.target.wants/sshd.service
A    /usr/etc/systemd/system/multi-user.target.wants/sssd.service
A    /usr/etc/systemd/system/network-online.target.wants
A    /usr/etc/systemd/system/network-online.target.wants/NetworkManager-wait-online.service
A    /usr/etc/systemd/system/sockets.target.wants
A    /usr/etc/systemd/system/sockets.target.wants/dbus.socket
A    /usr/etc/systemd/system/sockets.target.wants/dm-event.socket
A    /usr/etc/systemd/system/sockets.target.wants/docker.socket
A    /usr/etc/systemd/system/sockets.target.wants/multipathd.socket
A    /usr/etc/systemd/system/sysinit.target.wants
A    /usr/etc/systemd/system/sysinit.target.wants/lvm2-lvmetad.socket
A    /usr/etc/systemd/system/sysinit.target.wants/lvm2-lvmpolld.socket
A    /usr/etc/systemd/system/sysinit.target.wants/lvm2-monitor.service
A    /usr/etc/systemd/system/sysinit.target.wants/multipathd.service
A    /usr/etc/systemd/system/sysinit.target.wants/selinux-autorelabel-mark.service
A    /usr/lib/ostree-boot/initramfs-5.0.7-300.fc30.x86_64.img-084474cc7b2b0447e64af19a1386c5b6fa27489e1c58debc08d71ba3425761a0
A    /usr/lib/ostree-boot/vmlinuz-5.0.7-300.fc30.x86_64-084474cc7b2b0447e64af19a1386c5b6fa27489e1c58debc08d71ba3425761a0

@cgwalters
Copy link
Member Author

then delete the presets files.

Um...like including the versions in /usr/lib/systemd/system-preset? I am having trouble picturing the full ramifications. One thing offhand is we'd lose e.g.:

$ cat /usr/lib/systemd/system-preset/99-default-disable.preset 
disable *

I see what you're arguing though that the /usr/lib presets files duplicate data that is also reflected in /usr/etc.

But I still keep circling back to: it seems cleanest if systemd supported more fully having things enabled statically in /usr/lib and not writing to /etc unnecessarily.

Oh actually, a reason not to delete the presets files is that it would break package layering anything that had preset file config.

@jlebon
Copy link
Member

jlebon commented Apr 17, 2019

OK, so going over the final diff between before #73 and after this patch with a fine tooth comb, I see there are only three files which we lose:

/etc/systemd/system/dbus-org.freedesktop.network1.service
/etc/systemd/system/dbus-org.freedesktop.resolve1.service
/etc/systemd/system/systemd-timedated.service

The first one is a symlink to systemd-networkd. Seems like the Alias in there is still causing symlink creation even if the unit itself is not enabled? The second is a broken symlink to systemd-resolved. The third is interesting. It's from timedatex and was due to an apparent limitation in systemd to have two units with the same BusName= even if only one of them is enabled.

This was four years ago though. And testing now, I don't actually see systemd complaining about this anymore (and chronyd and timedatex are working fine). So it looks like that's no longer an issue.

Anyway, this LGTM. Will give others some time to do a final review before merging.

@jlebon jlebon merged commit dc1c4a9 into coreos:master Apr 18, 2019
@dustymabe
Copy link
Member

The third is interesting. It's from timedatex and was due to an apparent limitation in systemd to have two units with the same BusName= even if only one of them is enabled.

This was four years ago though. And testing now, I don't actually see systemd complaining about this anymore (and chronyd and timedatex are working fine). So it looks like that's no longer an issue.

I wonder if we should try to get them to drop it from the rpm?

@dustymabe
Copy link
Member

I wonder if we should try to get them to drop it from the rpm?

Here goes nothing: https://src.fedoraproject.org/rpms/timedatex/pull-request/1

@jlebon jlebon changed the title Move enabled units to /usr postprocess: run systemctl preset-all Dec 20, 2019
@jlebon
Copy link
Member

jlebon commented Dec 20, 2019

(I renamed this PR for posterity to reflect the final approach we went with.)

jlebon added a commit to jlebon/fedora-coreos-config that referenced this pull request Apr 21, 2020
A lot of backstory on this, but essentially right now, we always bake a
run of `systemctl preset-all` into the OSTree because upgrading hosts
rely on these links for service enablement.

In hindsight, we should've just stuck with pure systemd preset only as
canonicaly from the get go, though it's a bit difficult now to
transition from one to the other without breaking things. (Though I'll
note not impossible, since we do have update barriers which could allow
us to e.g. run a script to restore lost symlinks).

For now though, let's at least fix the ability to disable services,
which is a pretty big gap in our Ignition configuration story right now.

Related: systemd/systemd#15205
Related: coreos#77

Closes: coreos/fedora-coreos-tracker#392
kahenteikou pushed a commit to SereneLinux/serenelinux-release that referenced this pull request Apr 9, 2022
Commit 059e64f generalized the rpm-ostree count me unit activation using
the systemd preset configuration file.

This previous change incorrectly assumed that all rpm-ostree based
variants where using the presets as source of truth work enabled
services, but this is right now only true for Fedora CoreOS.

Work is in progress to update Silverblue/Kinoite and IoT to preset-all
by default. However this change will most likely be F36+ only.

Thus this change statically enables the timer unit for IoT, Silverblue &
Kinoite. Users can still opt out of counting as before by masking the
unit and this will not re-enable it for those that opted-out earlier.

See:
- https://pagure.io/workstation-ostree-config/pull-request/246
- https://github.com/coreos/fedora-coreos-config/blob/testing-devel/manifests/ignition-and-ostree.yaml#L39..L48
- coreos/fedora-coreos-config#73
- coreos/fedora-coreos-config#77
- coreos/fedora-coreos-config#122
- coreos/rpm-ostree#1803

Fixes: 059e64f 90-default.preset: Enable rpm-ostree count me by default
keszybz added a commit to jlebon/systemd that referenced this pull request May 4, 2022
A compile time option is added to select behaviour: by default
UNIT_FILE_PRESET_ENABLE_ONLY is still used, but the intent is to change to
UNIT_FILE_PRESET_FULL at some point in the future. Distros that want to
opt-in can use the config option to change the behaviour.

(The option is just a boolean: it would be possible to make it multi-valued,
and allow full, enable-only, disable-only, none. But so far nobody has asked
for this, and it's better not to complicate things needlessly.)

With the configuration option flipped, instead of only doing enablements,
perform a full preset on first boot. The reason is that although
`/etc/machine-id` might be missing, there may be other files provisioned in
`/etc` (in fact, this use case is mentioned in `log_execution_mode`). Some of
those possible files include enablement symlinks even if presets dictate it
should be disabled.

Such a seemingly contradictory situation occurs in {RHEL,Fedora} CoreOS,
where we ship `/etc` as if `preset-all` were called. However, we want to
allow users to disable default-enabled services via Ignition, which does
this by creating preset dropins before switchroot. (For why we do
`preset-all` at compose time, see:
coreos/fedora-coreos-config#77).

For example, the composed FCOS image has a `enable zincati.service`
preset and an enablement for that in `/etc`, while at boot time when we
switch root, there may be a `disable zincati.service` preset with higher
precedence. In that case, we want systemd to disable the service.

This is essentially a revert of 304b307. It seems like systemd
*used* to do this, but it was changed to try to make the container
workflow a bit faster.

Resolves: coreos/fedora-coreos-tracker#392

Co-authored-by: Zbigniew Jędrzejewski-Szmek <zbyszek@in.waw.pl>
AndreKalb pushed a commit to AndreKalb/systemd that referenced this pull request Jul 6, 2022
A compile time option is added to select behaviour: by default
UNIT_FILE_PRESET_ENABLE_ONLY is still used, but the intent is to change to
UNIT_FILE_PRESET_FULL at some point in the future. Distros that want to
opt-in can use the config option to change the behaviour.

(The option is just a boolean: it would be possible to make it multi-valued,
and allow full, enable-only, disable-only, none. But so far nobody has asked
for this, and it's better not to complicate things needlessly.)

With the configuration option flipped, instead of only doing enablements,
perform a full preset on first boot. The reason is that although
`/etc/machine-id` might be missing, there may be other files provisioned in
`/etc` (in fact, this use case is mentioned in `log_execution_mode`). Some of
those possible files include enablement symlinks even if presets dictate it
should be disabled.

Such a seemingly contradictory situation occurs in {RHEL,Fedora} CoreOS,
where we ship `/etc` as if `preset-all` were called. However, we want to
allow users to disable default-enabled services via Ignition, which does
this by creating preset dropins before switchroot. (For why we do
`preset-all` at compose time, see:
coreos/fedora-coreos-config#77).

For example, the composed FCOS image has a `enable zincati.service`
preset and an enablement for that in `/etc`, while at boot time when we
switch root, there may be a `disable zincati.service` preset with higher
precedence. In that case, we want systemd to disable the service.

This is essentially a revert of 304b307. It seems like systemd
*used* to do this, but it was changed to try to make the container
workflow a bit faster.

Resolves: coreos/fedora-coreos-tracker#392

Co-authored-by: Zbigniew Jędrzejewski-Szmek <zbyszek@in.waw.pl>
keszybz pushed a commit to systemd/systemd-stable that referenced this pull request Oct 1, 2022
A compile time option is added to select behaviour: by default
UNIT_FILE_PRESET_ENABLE_ONLY is still used, but the intent is to change to
UNIT_FILE_PRESET_FULL at some point in the future. Distros that want to
opt-in can use the config option to change the behaviour.

(The option is just a boolean: it would be possible to make it multi-valued,
and allow full, enable-only, disable-only, none. But so far nobody has asked
for this, and it's better not to complicate things needlessly.)

With the configuration option flipped, instead of only doing enablements,
perform a full preset on first boot. The reason is that although
`/etc/machine-id` might be missing, there may be other files provisioned in
`/etc` (in fact, this use case is mentioned in `log_execution_mode`). Some of
those possible files include enablement symlinks even if presets dictate it
should be disabled.

Such a seemingly contradictory situation occurs in {RHEL,Fedora} CoreOS,
where we ship `/etc` as if `preset-all` were called. However, we want to
allow users to disable default-enabled services via Ignition, which does
this by creating preset dropins before switchroot. (For why we do
`preset-all` at compose time, see:
coreos/fedora-coreos-config#77).

For example, the composed FCOS image has a `enable zincati.service`
preset and an enablement for that in `/etc`, while at boot time when we
switch root, there may be a `disable zincati.service` preset with higher
precedence. In that case, we want systemd to disable the service.

This is essentially a revert of 304b307. It seems like systemd
*used* to do this, but it was changed to try to make the container
workflow a bit faster.

Resolves: coreos/fedora-coreos-tracker#392

Co-authored-by: Zbigniew Jędrzejewski-Szmek <zbyszek@in.waw.pl>
(cherry picked from commit 9365158)
Yamakuzure pushed a commit to elogind/elogind that referenced this pull request Dec 23, 2022
A compile time option is added to select behaviour: by default
UNIT_FILE_PRESET_ENABLE_ONLY is still used, but the intent is to change to
UNIT_FILE_PRESET_FULL at some point in the future. Distros that want to
opt-in can use the config option to change the behaviour.

(The option is just a boolean: it would be possible to make it multi-valued,
and allow full, enable-only, disable-only, none. But so far nobody has asked
for this, and it's better not to complicate things needlessly.)

With the configuration option flipped, instead of only doing enablements,
perform a full preset on first boot. The reason is that although
`/etc/machine-id` might be missing, there may be other files provisioned in
`/etc` (in fact, this use case is mentioned in `log_execution_mode`). Some of
those possible files include enablement symlinks even if presets dictate it
should be disabled.

Such a seemingly contradictory situation occurs in {RHEL,Fedora} CoreOS,
where we ship `/etc` as if `preset-all` were called. However, we want to
allow users to disable default-enabled services via Ignition, which does
this by creating preset dropins before switchroot. (For why we do
`preset-all` at compose time, see:
coreos/fedora-coreos-config#77).

For example, the composed FCOS image has a `enable zincati.service`
preset and an enablement for that in `/etc`, while at boot time when we
switch root, there may be a `disable zincati.service` preset with higher
precedence. In that case, we want systemd to disable the service.

This is essentially a revert of 304b307. It seems like systemd
*used* to do this, but it was changed to try to make the container
workflow a bit faster.

Resolves: coreos/fedora-coreos-tracker#392

Co-authored-by: Zbigniew Jędrzejewski-Szmek <zbyszek@in.waw.pl>
mightysai1997 pushed a commit to mightysai1997/systemd that referenced this pull request Apr 19, 2024
A compile time option is added to select behaviour: by default
UNIT_FILE_PRESET_ENABLE_ONLY is still used, but the intent is to change to
UNIT_FILE_PRESET_FULL at some point in the future. Distros that want to
opt-in can use the config option to change the behaviour.

(The option is just a boolean: it would be possible to make it multi-valued,
and allow full, enable-only, disable-only, none. But so far nobody has asked
for this, and it's better not to complicate things needlessly.)

With the configuration option flipped, instead of only doing enablements,
perform a full preset on first boot. The reason is that although
`/etc/machine-id` might be missing, there may be other files provisioned in
`/etc` (in fact, this use case is mentioned in `log_execution_mode`). Some of
those possible files include enablement symlinks even if presets dictate it
should be disabled.

Such a seemingly contradictory situation occurs in {RHEL,Fedora} CoreOS,
where we ship `/etc` as if `preset-all` were called. However, we want to
allow users to disable default-enabled services via Ignition, which does
this by creating preset dropins before switchroot. (For why we do
`preset-all` at compose time, see:
coreos/fedora-coreos-config#77).

For example, the composed FCOS image has a `enable zincati.service`
preset and an enablement for that in `/etc`, while at boot time when we
switch root, there may be a `disable zincati.service` preset with higher
precedence. In that case, we want systemd to disable the service.

This is essentially a revert of 304b307. It seems like systemd
*used* to do this, but it was changed to try to make the container
workflow a bit faster.

Resolves: coreos/fedora-coreos-tracker#392

Co-authored-by: Zbigniew Jędrzejewski-Szmek <zbyszek@in.waw.pl>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants