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

Unit documentation and build-system tweaks #14378

Merged
merged 3 commits into from Dec 19, 2019
Merged

Conversation

keszybz
Copy link
Member

@keszybz keszybz commented Dec 18, 2019

No description provided.

@keszybz keszybz changed the title Unit docs Unit documentation and build-system tweaks Dec 18, 2019
man/bootup.xml Outdated Show resolved Hide resolved
@@ -10,7 +10,7 @@
[Unit]
Description=Remote Encrypted Volumes
Documentation=man:systemd.special(7)
After=remote-fs-pre.target cryptsetup-pre.target
After=cryptsetup-pre.target
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm, so this was borked in 362c378 (cc @fbuihuu)

i am tempted to just revert 362c378. It has the major drawback anyway that it globally orderers all cryptsetup volumes before all mounts. people have stacked stuff though, such a global ordering is just sloppy and problematic in those cases.

I think we should handle the cryptsetup ordering problem that 362c378 attempted to fix differently. let's stick to local ordering. i.e. introduce an instantiated target unit that is ordered before any mount unit that is backed by a device matching /dev/mapper/*, maybe called mount-prepare@.target. The target would be pulled in by cryptsetup@.service, which would order itself before that.

let's say we have an encrypted volume /dev/mapper/mysql which is ultimately mounted to /var/lib/mysql. For that fstab-generator would automatically synthesize based on some entry /etc/fstab a unit essentially like this:

# var-lib-mysql.mount
[Unit]
After=mount-prepare@mysql.target
…

[Mount]
What=/dev/mapper/mysql
Where=/var/lib/mysql
…

And cryptsetup-generator would automatically synthesize based on some entry in /etc/crypttab a unit like this:

# systemd-cryptsetup@mysql.service
[Unit]
Wants=mount-prepare@%i.target
Before=mount-prepare@%i.target

[Service]
…

And we'd ship a static mount-prepare@.target template:

# mount-prepare@.target
[Unit]
Description=Passive Mount Preparation Hook

That way we'd always get the following ordering:

systemd-cryptsetup@mysql.service → mount-prepare@mysql.target
                                                              🡖 
                                                                  var-lib-mysql.mount
                                                              🡕
                                     dev-mapper-mysql.device

With that we make sure the mount is for the first time properly ordered after cryptsetup and the device popping up, which is already pretty nice, but more importantly means at shutdown we'd first unmount the thing and only then destroy the luks volume.

Does that make sense?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The units already have a "connection" through the .device unit. If these were just normal units, we could simply order them systemd-cryptsetup@mysql.service Before dev-mapper-mysql.device Before var-lib-mysql.mount, and we would get correct job ordering on shutdown. I know .device units cannot be delayed, but maybe we could still add an ordering? I'm confused why some .device units have After=/Before= deps, and others don't and when it is allowed to have them.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

with your ordering at shutdown we'd first umount the mount point then wait for the device to disappear and then stop the cryptsetup service. But that's not desired, we want to stop the service first and that results in the device to disappear, i.e. the device coming/going is a side effect of cryptsetup service and not an event that can be ordered before or after it.

if you so will it's effect of systemd's design model that shutdown order is the reverse of the startup order. because here during both startup and shutdown we want the invoke cryptsetup first, and that then has effect on the device unit.

Hence I think the right approach to fit this into our model is to consider the .device and .service units as independently ordered but then insert an additional unit in between to order both together against the stuff we care about.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i am tempted to just revert 362c378. It has the major drawback anyway that it globally orderers all cryptsetup volumes before all mounts. people have stacked stuff though, such a global ordering is just sloppy and problematic in those cases.

Could you please be more descriptive here ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am prepping a patch implementing what I propose right now.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I posted PR #14398 now (wip) which does this how I think this should be done.

meson.build Outdated Show resolved Hide resolved
…ng shutdown"

This reverts commit 362c378.

This commit introduced an ordering loop: remote-cryptsetup.target was both
before and after remote-fs-pre.target. It also globally ordered all cryptsetup
volumes before all mounts. Such global ordering is problematic if people have
stacked storage. Let's look for a different solution.

See systemd#14378 (comment).
@keszybz
Copy link
Member Author

keszybz commented Dec 19, 2019

Updated. There are now three commits:

  1. revert of 362c378
  2. adding remote-*.targets to bootup
  3. adding user manager to bootup

We'll need to figure out a replacement for the reverted commit, but this can happen independently of the doc changes.

@keszybz
Copy link
Member Author

keszybz commented Dec 19, 2019

#8472 will need to be reopened if this is merged.

man/bootup.xml Outdated
(various mounts and | | | | | (various low-level
fsck services...) | v remote-fs.target | | services: udevd,
| | | | | tmpfiles, random
v | v | | seed, sysctl, ...)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in this graph i'd move these two to the side and order it before multi-user.target. When enabled these two targets are ordered before that after all, see their [Install] sections.

I think this makes it easier to see the difference between local and remote
mounts.

Make the graph a bit narrower while at it.
@keszybz
Copy link
Member Author

keszybz commented Dec 19, 2019

Updated. It's a work of art now.

@fbuihuu
Copy link
Contributor

fbuihuu commented Dec 19, 2019

#8472 will need to be reopened if this is merged.

Well that would be pretty bad... I mean if you revert the fix it would be nice to provide something else as a replacement.

@poettering poettering merged commit 10ad50d into systemd:master Dec 19, 2019
@keszybz keszybz deleted the unit-docs branch December 19, 2019 20:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

3 participants