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

40ignition-ostree: add service for deleting /etc enablements #363

Closed
wants to merge 1 commit into from

Conversation

jlebon
Copy link
Member

@jlebon jlebon commented 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: #77

Closes: coreos/fedora-coreos-tracker#392

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
@jlebon
Copy link
Member Author

jlebon commented Apr 21, 2020

This is a short-term solution to coreos/fedora-coreos-tracker#392 while we figure out what the long-term strategy should be (discussions ongoing in systemd/systemd#15205).

(To re-iterate, as mentioned in systemd/systemd#15205 (comment), yes, this is really awkward to basically undo what the OSTree commit is baking in.)

@jlebon
Copy link
Member Author

jlebon commented Apr 21, 2020

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).

This might actually not be as hard as I initially imagined. I didn't want to write systemd services for this that would muck around with analyzing a previous deployment's /etc and the ambiguity there, but I think we could avoid that and do it in two steps:

  1. an update barrier with a systemd service which runs right before ostree-finalize-staged.service on shutdown and uses ostree admin config-diff to find all added and modified files in /etc/systemd/system and copies those to e.g. /etc/systemd/system.new.
  2. another update barrier where we switch to presets as canonical and we have a systemd service which runs in the initrd that looks for /etc/systemd/system.new, restores it, and deletes it.

Still a sensitive operation of course, given that failure potentially means a broken boot. Though it'd be a nice legacy cleanup.

@jlebon
Copy link
Member Author

jlebon commented Apr 27, 2020

Thoughts on just merging this until systemd/systemd#15205 is decided (and possibly backported to Fedora/RHEL8)?

@cgwalters
Copy link
Member

cgwalters commented Apr 27, 2020

To be honest I am scared of this 😄
I believe you it works but we should be sure multiple people review it and think through it carefully and we've tested it locally.

Not to mention that the systemd PR seemed to come to a conclusion and looks likely to merge. (Once it does, we can just e.g. detect the systemd version in postprocess and not do the "copy units to /etc" right?)

Plus we have a workaround for the zincati thing today (it's not a difficult workaround, but it's also admittedly not an obvious workaround - people are going to keep hitting that issue, but hopefully web search takes them to it quickly)

@dustymabe
Copy link
Member

Yeah I'm a bit wary. Since we have an easy workaround and hopefully some momentum upstream maybe let's just see if we can get that in sooner than later. Obviously if upstream lingers we can revisit.

@jlebon
Copy link
Member Author

jlebon commented Apr 27, 2020

Sure, we can wait some more. I think there was a misconception on my part on how bad this actually is.

@jlebon
Copy link
Member Author

jlebon commented Mar 16, 2022

Looks like upstream is lingering. And the service disablement issue continues to be UX wart. So I think we should consider doing something here.

I actually don't think this PR is that scary. The main thing to note is that it's purely a first boot thing which means (1) lots of CI coverage, and (2) it cannot affect upgrading nodes.

@jlebon
Copy link
Member Author

jlebon commented Mar 16, 2022

Actually, I think another approach here is to tweak Ignition so that enabled: false will also make sure that no enablement symlink exists in the real root on top of emitting a disable in the presets.

@travier
Copy link
Member

travier commented Mar 16, 2022

Actually, I think another approach here is to tweak Ignition so that enabled: false will also make sure that no enablement symlink exists in the real root on top of emitting a disable in the presets.

While I agree that this PR is closer to what we actually want in the end, doing the above seems to be safer as a first step.

@jlebon
Copy link
Member Author

jlebon commented Mar 23, 2022

Actually, I think another approach here is to tweak Ignition so that enabled: false will also make sure that no enablement symlink exists in the real root.

This is coreos/ignition#588.

@travier
Copy link
Member

travier commented May 12, 2022

Closing as per coreos/fedora-coreos-tracker#392 (comment)

@travier travier closed this May 12, 2022
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.

Unable to disable zincati.service using Ignition
4 participants