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

Closed
wants to merge 18 commits into from
Closed

Conversation

ajs124
Copy link
Member

@ajs124 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 May 5, 2020 21:41
 - 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

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.

flokli and others added 8 commits May 24, 2020 02:36
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 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)
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

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

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

@flokli
Copy link
Contributor

flokli commented Jul 12, 2020

@7c6f434c I incorporated this back into #66856 (comment), feel free to take a look :-)

@ajs124 ajs124 closed this Jul 12, 2020
@ajs124 ajs124 deleted the lvm2 branch March 11, 2021 16:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants