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

lvm2: systemd fixes and update #88565

Draft
wants to merge 18 commits into
base: master
from
Draft

lvm2: systemd fixes and update #88565

wants to merge 18 commits into from

Conversation

@ajs124
Copy link
Member

ajs124 commented May 22, 2020

Motivation for this change

We're deploying lvmthin and they recommend running dmeventd, and while there is an option on the lvm2 package, it sort of didn't work.

The update is in there, because our current version is from October of 2018.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

This probably needs a changelog entry and some documentation updates, because:

  1. it's probably worth noting that we support lvm2 services now. You can run dmeventd simply by setting systemd.packages = with pkgs; [ lvm2 ]; and systemd.sockets."dm-event".wantedBy = [ "sockets.target" ];. And mabye you also need a lvm.conf. (And also lvm2.override { enable_dmeventd = true; }).
  2. blk_availability_systemd_red_hat.service and lvm2_activation_generator_systemd_red_hat are currently manually installed with these names. Using the upstream install they're simply called blk-availability.service and lvm2-activation-generator.

I haven't looked through the commit history that much, but I'm wondering why dmeventd is optional in the first place. Is the difference in closure size really that big?

There's also at least two other lvm daemons (lvmetad and lvmpoold), which we're not shipping right now.

Point 1. above might also warrant the introduction of a small module to configure dmeventd and the other daemons, if anyone is interested in those.

flokli added 3 commits Aug 19, 2019
 - use installTargets again ("install", and
   "install_systemd_{generators,units,configuration}" when udev is not
   null)
 - The call to the blkid binary in lvm2's 13-dm-disk.rules file
   disappeared (so we don't need to patch in blkid anymore).
   lvm seems to rely on udev's internal blkid functionality.
 - Call /run/current-system/systemd/bin/udevadm instead
   of ${systemd}/bin/udevadm in the lvm activation generator.
   This is not necessary to break the dependency cycle (as we don't use
   that file when building without udev), but a good idea anyways -
   We want to trigger the udevadm of the current system, not the one
   that lvm was built with.
Otherwise, we end up in a dependency cycle:

systemd -> cryptsetup -> lvm -> fetchgit -> git -> openssh -> libfido2 -> hidapi -> libusb -> udev=systemd
@7c6f434c
Copy link
Member

7c6f434c commented May 23, 2020

I would guess that nobody wanted to check if dmeventd is fully ignorable if enabled (although the man page seems to suggest so). The dependency set does not seem to differ, so I guess making it on by default should be safe. Same with the other daemons, hopefully.

As for modules — well, I guess this is a necessary step to eventually having a NixOS test for all that…

No idea whether addition of more things should be mentioned in release notes (but I am not a user of stable), renaming installed files probably is a good thing to mention. (I wonder if it breaks some rev-deps…)

@ajs124
Copy link
Member Author

ajs124 commented May 23, 2020

I looked a bit more into this. Turns out lvmetad isn't part of lvm2 anymore. lvmpolld still exists, though.

For turning them off there is global/use_lvmpolld and activation/monitoring.

The other daemons include some python d-bus stuff and cluster stuff (lvmlockd, cmirrord), but I'd not add them right now, as we don't use them and I don't really know how to test them.

As for tests, there is nixos.tests.installer.lvm, but it's pretty basic.

The main hold-up for extending nixos/modules/tasks/lvm.nix with thin support is that #46541 never got merged, for valid reasons, but with #72401 still in progress, not much has changed on that front.

arianvp and others added 8 commits May 13, 2019
This package previously did override the systemd package, and instructed
ninja, systemd's previous build system, to only build the
cryptsetup-specific systemd generators (plus some rpath massaging, as we
didn't use ninja install).

Afterwards, users were expected to add this package to their
`systemd.generator-packages` (or since
https://github.com/NixOS/nixpkgs/pull/65376/files `systemd.packages`)
NixOS module options, so systemd will use these generators.

As the previous commit added cryptsetup support directly to the systemd
package (and pkgs.systemd now already ships the cryptsetup generators),
we don't need another package shipping the same generators.
Instead, prompt user to add lvm to systemd.packages, which is a
cleaner way, and we don't need to rebuild the world by changing systemd
globally.

Initially introduced in c0fd887.
We might want to do this conditionally on whether lvm is enabled (or add
the lvm system-generators unconditionally, too)
There's a circular dependency to systemd via cryptsetup and lvm2
(systemd -> cryptsetup -> lvm2 -> udev=systemd).

However, cryptsetup only really needs the devmapper component shipped
with lvm2. So build `pkgs.cryptsetup` with a lvm2 that doesn't come with
udev.
This seems to be mostly used to simplify LV management tasks from a web
interface
(https://www.redhat.com/archives/linux-lvm/2008-September/msg00029.html),
and is as fat as the `lvm` binary itself
@7c6f434c
Copy link
Member

7c6f434c commented May 25, 2020

Well, maybe it is enough for the first step just not to break the basic operation…

So, for this PR what is needed is a release note entry that we rename installed files back to upstream names?

@ajs124 ajs124 marked this pull request as draft May 31, 2020
@ajs124 ajs124 force-pushed the helsinki-systems:lvm2 branch from 5bf657f to 80501a1 May 31, 2020
@ajs124
Copy link
Member Author

ajs124 commented May 31, 2020

Enabling dmeventd doesn't actually work right now, because of

cycle detected in the references of '/nix/store/9x3ny30dvb8i2lrc5pybgjvrjci4hb3x-lvm2-2.03.09-lib' from '/nix/store/nhrji14hh4nq3grg1jzgkrzbp04qymg1-lvm2-2.03.09-bin'

Not sure how to fix that while keeping the split outputs.

@ofborg ofborg bot added the 6.topic: nixos label May 31, 2020
@flokli
Copy link
Contributor

flokli commented Jun 1, 2020

@ajs124 I propose checking the outputs, and /why/ there are these circular references.

@ajs124
Copy link
Member Author

ajs124 commented Jun 1, 2020

It's not that much of a "why", but:

find /nix/store/9x3ny30dvb8i2lrc5pybgjvrjci4hb3x-lvm2-2.03.09-lib -type f -exec strings -f {} + | grep /nix/store/nhrji14hh4nq3grg1jzgkrzbp04qymg1-lvm2-2.03.09-bin

/nix/store/9x3ny30dvb8i2lrc5pybgjvrjci4hb3x-lvm2-2.03.09-lib/lib/libdevmapper-event.so.1.02: /nix/store/nhrji14hh4nq3grg1jzgkrzbp04qymg1-lvm2-2.03.09-bin/bin
/nix/store/9x3ny30dvb8i2lrc5pybgjvrjci4hb3x-lvm2-2.03.09-lib/lib/liblvm2cmd.so.2.03: /nix/store/nhrji14hh4nq3grg1jzgkrzbp04qymg1-lvm2-2.03.09-bin/bin/fsadm
/nix/store/9x3ny30dvb8i2lrc5pybgjvrjci4hb3x-lvm2-2.03.09-lib/lib/liblvm2cmd.so.2.03: /nix/store/nhrji14hh4nq3grg1jzgkrzbp04qymg1-lvm2-2.03.09-bin/bin/dmeventd
/nix/store/9x3ny30dvb8i2lrc5pybgjvrjci4hb3x-lvm2-2.03.09-lib/lib/liblvm2cmd.so.2.03: /nix/store/nhrji14hh4nq3grg1jzgkrzbp04qymg1-lvm2-2.03.09-bin/bin/lvm

find /nix/store/nhrji14hh4nq3grg1jzgkrzbp04qymg1-lvm2-2.03.09-bin -type f -exec strings -f {} + | grep /nix/store/9x3ny30dvb8i2lrc5pybgjvrjci4hb3x-lvm2-2.03.09-lib
/nix/store/nhrji14hh4nq3grg1jzgkrzbp04qymg1-lvm2-2.03.09-bin/bin/dmeventd: /nix/store/9x3ny30dvb8i2lrc5pybgjvrjci4hb3x-lvm2-2.03.09-lib/lib:/nix/store/xrxbwxsvdsj07cgfpxmxk2qzvwmrnv3j-systemd-245.5-lib/lib:/nix/store/9s2w6ahf8zxdip7nhybs8gkzfbln6cxv-util-linux-2.35.1/lib:/nix/store/bwzra330vib0ik4d3l8rq6gp6y2ah1fr-glibc-2.30/lib
/nix/store/nhrji14hh4nq3grg1jzgkrzbp04qymg1-lvm2-2.03.09-bin/bin/dmsetup: /nix/store/9x3ny30dvb8i2lrc5pybgjvrjci4hb3x-lvm2-2.03.09-lib/lib:/nix/store/bwzra330vib0ik4d3l8rq6gp6y2ah1fr-glibc-2.30/lib
/nix/store/nhrji14hh4nq3grg1jzgkrzbp04qymg1-lvm2-2.03.09-bin/bin/lvm: /nix/store/9x3ny30dvb8i2lrc5pybgjvrjci4hb3x-lvm2-2.03.09-lib/lib:/nix/store/xrxbwxsvdsj07cgfpxmxk2qzvwmrnv3j-systemd-245.5-lib/lib:/nix/store/9s2w6ahf8zxdip7nhybs8gkzfbln6cxv-util-linux-2.35.1/lib:/nix/store/bh2lqv8646c12hfrb4ympygv7qz9x2ck-libaio-0.3.110/lib:/nix/store/bwzra330vib0ik4d3l8rq6gp6y2ah1fr-glibc-2.30/lib
@flokli
Copy link
Contributor

flokli commented Jun 2, 2020

Yeah, I dropped liblvm2cmdfrom the expresssion - it's "a library aiming to simplify typical LV management tasks from a web interface" - see 3b7d76b.

If dmeventd now depends on it as well, that'll make things more complicated. How do other distros handle this? It seems it's not installed in Gentoo as well.

@ajs124
Copy link
Member Author

ajs124 commented Jun 2, 2020

The problem isn't only liblvm2cmd though. The problem is also that libdevmapper-event.so depends on bin when dmeventd is enabled and dmeventd, lvm and dmsetup depend on libdevmapper-event.so.

@flokli
Copy link
Contributor

flokli commented Jun 14, 2020

I'm not entirely sure which of the components are needed for lvm operation these days - I mostly introduced the multiple outputs to get the closure size down, as systemd only uses the devmapper components shipped inside lvm.

@ajs124
Copy link
Member Author

ajs124 commented Jun 15, 2020

I'm not entirely sure which of the components are needed for lvm operation these days

I'm not sure what either, to be honest.

I mostly introduced the multiple outputs to get the closure size down, as systemd only uses the devmapper components shipped inside lvm.

That's what I assumed. Reducing closure size for something like systemd is always a good idea.

The question is where we go from here. I can spend some time to figure out which parts of LVM are relevant for what, which is probably relevant to us in any case.
However, that might lead to the conclusion that features like dmeventd or lockd are incompatible with multiple outputs. It's probably technically possible to disable the multiple outputs in case such a feature is activated, but that's sounds ugly.

@flokli
Copy link
Contributor

flokli commented Jun 15, 2020

I'm all fine with reducing some of the multiple-output stuff, if it's necessary to get LVM to work - if closure sizes are a problem, we can still re-evaluate.

Does your PR currently build, and does the new test succeed?

In that case, I could cherry-pick this into #66856, possibly squashing some of the changes for readability.

Did you check the closure size of the minimal iso before and after this change?

@ajs124
Copy link
Member Author

ajs124 commented Jun 18, 2020

It doesn't build as is, but probably builds and runs fine without multiple outputs.

I haven't checked the closure size yet. Not sure when I'll get around to put some time into this, might not happen before next week, sorry.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.