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

lvm: cleanups #93024

Merged
merged 17 commits into from Jul 14, 2020
Merged

lvm: cleanups #93024

merged 17 commits into from Jul 14, 2020

Conversation

flokli
Copy link
Contributor

@flokli flokli commented Jul 12, 2020

This includes quite some fixes to lvm2 accumulated while working on enabling cryptsetup support in systemd (#66856). I intend to rebase #66856 on top of the changes here.

lvm2 is now a multi-output derivation, which helps for closure size reduction, but also becomes necessary as soon as you try to build systemd with cryptsetup support (that requires the libdevmapper component of lvm2) - due to lvm itself also containing references to libsystemd (via their shipped systemd generators).

Properly splitting this avoids getting rid of hacks like systemd_with_lvm2.

This also contains quite some cleanups in the unit itself, making it more maintainable in the longer run.

nixos/tests/installer: lvm: test lvm2-pvscan@ units
nixos/test/installer: add postBootCommands
systemd_with_lvm2: remove
nixos/tasks/lvm: add dmeventd and lvmthin support
lvm2: only split bin and lib out from out if cmdlib isn't enabled
lvm2: fix location to systemd-run invocation
lvm2: fix paths to use /run instead of /var/run.
lvm2: add multiple output support
lvm2: don't embed ./configure line in lvm2 binary
lvm2: make --enable-cmdlib optional
lvm2: 2.03.01 -> 2.03.09
lvm2: fetch sources from http instead of git
lvm2: add myself as maintainer
lvm2: cleanups, make udev optional
lvm2: remove unused default.upstream file
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.

flokli and others added 11 commits July 12, 2020 23:04
 - 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
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
@@ -552,16 +555,26 @@ in {
+ " mkpart primary 2048M -1s" # PV2
+ " set 2 lvm on",
"udevadm settle",
"sleep 1",
Copy link
Member

Choose a reason for hiding this comment

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

Maybe add a comment why the sleep is necessary here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's already in the commit message.

Also, it's not really "needed".

There's some racyness causing some waiting every now and then, and I at least felt like this will make it less likely to trigger. The test still succeeds, it's just less likely to take longer.

ajs124 and others added 5 commits July 14, 2020 12:00
Introduce a pkgs.lvm2_dmeventd that contains dmeventd support, and
enable if services.lvm.dmeventd.enable is true.
This is already handled by the lvm module, which now properly adds the
lvm systemd generators.

Initially introduced in c0fd887.
Also, add some sleep statements in between, which seems to at least feel
like it causes

> WARNING: Device /dev/vda* not initialized in udev database even after waiting 10000000 microseconds.

To occur less frequently.

This eventually still succeeds after some amount of waiting, I suspect
some racyness in the way lvm's udev-triggered scripts trigger other
units.
Make this more consistent with how these flags look like in the rest of
nixpkgs.
@flokli
Copy link
Contributor Author

flokli commented Jul 14, 2020

Adressed @Mic92's comments.

@arianvp
Copy link
Member

arianvp commented Jul 14, 2020

LGTM once the tests finish

@flokli
Copy link
Contributor Author

flokli commented Jul 14, 2020

Well, tests weren't in passthru. I pushed a commit adding them, good idea!

@flokli
Copy link
Contributor Author

flokli commented Jul 14, 2020

Tests succeeded on ofborg x86_64-linux, and failed on aarch64 due to another unrelated error/flakyness:

 --- Error --- nix-build
error: --- SysError --- nix-daemon

@flokli flokli merged commit a224b6e into NixOS:staging Jul 14, 2020
systemd automation moved this from In Progress to Done Jul 14, 2020
@flokli flokli deleted the lvm-fixes branch July 14, 2020 11:53
@ajs124 ajs124 mentioned this pull request Jul 25, 2020
10 tasks
andersk added a commit to andersk/nixpkgs that referenced this pull request Jul 27, 2020
It was broken by commit d3a991d
(NixOS#93024).

Signed-off-by: Anders Kaseorg <andersk@mit.edu>
@andersk
Copy link
Contributor

andersk commented Jul 27, 2020

This broke multipath-tools; the fix in #93949 is needed.

jerith666 pushed a commit to jerith666/nixpkgs that referenced this pull request Jul 29, 2020
It was broken by commit d3a991d
(NixOS#93024).

Signed-off-by: Anders Kaseorg <andersk@mit.edu>
(cherry picked from commit a3a1e27)
Yarny0 added a commit to Yarny0/nixpkgs that referenced this pull request Jan 17, 2022
lvm2 support was broken when lvm2 got
converted to a multiple-output derivation:

NixOS#93024
NixOS@d3a991d

The `runtimeDependencies` attribute doesn't specifically
look for a `lib` output, so it uses the main `out` output
which no longer contains the library object files.

Since TSM loads the `libdevmapper.so` library
dynamically (likely with `dlfcn.h` functions),
the breakage couldn't be detected at build time.

The commit at hand simply uses
`getLib` to pick the correct output.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
systemd
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

6 participants