Ignition shouldn't use systemd presets to enable units

[coreosbot]

Issue by @bgilbert

---

Issue Report

Environment

Any

Desired Feature

Ignition should create the requisite unit symlinks itself, rather than using systemd presets.

Other Information

This would solve multiple problems: #583, #586, #587.

[coreosbot]

Comment by @lucab

---

I'd need to dig into this a bit more, but I think this would not work with services provided via torcx as the service units do not yet exist when ignition is run. Moreover I think the decision to using presets came after lessons learned from cloudinit issues, but I don't have the specific details at hand.

[coreosbot]

Comment by @euank

---

To duplicate some discussion with @bgilbert:

It seems plausible that we could split enable into two different forms of enabling things: things that exist and things that don't.

In the case of "things that exist", we can create symlinks. This includes aliases and templated units.

For things that don't, we could use a preset + log a warning. This would include torcx units and other units not written by Ignition directly.

Perhaps in the long term torcx should have an initrd component which handles unit enablement in its own way, and somehow coordinates with Ignition's intent to let user's override it?

[coreosbot]

Comment by @bgilbert

---

A torcx initrd component seems to add significant complexity. In principle any unit written but not enabled by a generator would need to be enabled via preset (i.e. it's not necessarily just a torcx issue) but I don't know if that's common.

Logging a warning would address #587, and as an additional notification mechanism we could write a unit whose sole job is to fail if any of the units enabled via preset weren't actually enabled.

[coreosbot]

Comment by @cgwalters

---

Just to be sure I understand so far:

diff --git a/internal/exec/util/unit.go b/internal/exec/util/unit.go
index a3e05af..ace06fc 100644
--- a/internal/exec/util/unit.go
+++ b/internal/exec/util/unit.go
@@ -91,11 +91,11 @@ func (u Util) MaskUnit(unit types.Unit) error {
 }
 
 func (u Util) EnableUnit(unit types.Unit) error {
-	return u.appendLineToPreset(fmt.Sprintf("enable %s", unit.Name))
+	return chroot(fmt.Sprintf("systemctl enable %s", unit.Name))
 }
 
 func (u Util) DisableUnit(unit types.Unit) error {
-	return u.appendLineToPreset(fmt.Sprintf("disable %s", unit.Name))
+	return chroot(fmt.Sprintf("systemctl mask %s", unit.Name))
 }
 
 func (u Util) appendLineToPreset(data string) error {

would break torcx?

[coreosbot]

Comment by @ajeddeloh

---

IIRC systemctl talks to systemd over dbus and does not create the links manually, right? systemctl also refuses to run in a chroot (at least in the CL SDK that's the case)

[coreosbot]

Comment by @cgwalters

---

See systemd/systemd#7631 for systemctl and dbus.

[coreosbot]

Comment by @cgwalters

---

In the end systemctl enable X is generally a really complicated way to invoke ln -s, so TL;DR: if we set SYSTEMCTL_OFFLINE=1 in the environment now you'll consistently have it just do that and not try to do DBus.

[coreosbot]

Comment by @cgwalters

---

I see a few paths here. One is to enhance systemd in some way to make this nicer. It could be enhancing the preset file. For #587, it could be as simple as:

enable-required foo.service

Which would be like enable but fail if foo.service doesn't exist. For #583...maybe just define enable-required to also work with aliases, it's not clear to me why it wouldn't. (I don't understand offhand why presets explicitly ban aliases) Lennart replied on #586 - probably best to continue that conversation.

That said, we could likely kill all of these issues at once if (as this issue suggests) we don't use presets, but instead write out a small systemd service that runs in early boot that does basically:

#!/bin/bash
set -euo pipefail
systemctl enable foo.service bar@blah.service
systemctl daemon-reload

[coreosbot]

Comment by @bgilbert

---

One of the major lessons from coreos-cloudinit is that services shouldn't be fiddling with the boot process. We could use a generator, though.

[coreosbot]

Comment by @cgwalters

---

We could use a generator, though.

The tricky thing there is generators are designed to be transient. So Ignition would have to write out something like /etc/ignition/default-units.json and install a generator from that. Which would feel weird versus just directly creating the multi-user.target.wants/ symlinks in /etc - we can do it at the same time as generators.

[coreosbot]

Comment by @ajeddeloh

---

It sounds like we need to figure out what failure in the real root looks like. We generally want failures to take the machine down entirely (e.g. Ignition failures prevent the switch-root from ever happening). Failing in the real root is less "bulletproof" as failing in the initramfs. Users can work around it and it doesn't set off all sorts of alarms like the initramfs failures.

[kdomanski]

@cgwalters @lucab Given the apparent deprecation of torcx, is the case of "service units that do not yet exist" still valid?

[travier]

Probably not relevant anymore. See: #588 (comment)

---

I'm +1 to move forward on this issue and to move away from the use of presets to enable symlinks on first boot.

We should however keep a way to enable transient / runtime units via Ignition and the presets might be a good way to do that.

This would be required for example for https://github.com/containers/quadlet, which generates service units at runtime from static definitions. Quadlet is not currently included in FCOS (coreos/fedora-coreos-tracker#998) but could be in the future.

There might also be other cases where this could be useful.

Example Ignition config:

systemd:
  units:
   - name:
     enabled:
     mask:
     preset: true (defaults to false to use the symbolic link logic)
     ...

Other suggestions:

     type or mode: symlink/preset or direct/indirect/preset/allow-non-existing-units

[Nemric]

+1
This would help to solve some issues like : #1296 (comment)

[travier]

Update: Quadlet enables the units it generates, thus this example is not relevant: https://github.com/containers/quadlet/blob/1e83798d144bfb659c9293d47ef26c282aabea3d/src/generator.c#L890-L891