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

cryptsetup, lvm2, systemd: Break cyclic dependency at a different point #97008

Merged
merged 1 commit into from Sep 4, 2020

Conversation

@andersk
Copy link
Contributor

@andersk andersk commented Sep 3, 2020

Motivation for this change

The cyclic dependency of systemd → cryptsetup → lvm2 → udev=systemd needs to be broken somewhere. The previous strategy of building cryptsetup with an lvm2 built without udev (#66856) caused the installer.luksroot test to fail. Instead, build lvm2 with a udev built without cryptsetup.

Fixes #96479.

Simplified variant of #97002.

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.
@arianvp
Copy link
Member

@arianvp arianvp commented Sep 3, 2020

I'm building this PR locally to test it out; but it's taking a while. This looks better than #97002 to me though. So feel free to close that one already

@FRidh FRidh added this to the 20.09 milestone Sep 3, 2020
@FRidh FRidh added this to To Do in 20.09 Blockers via automation Sep 3, 2020
@FRidh
Copy link
Member

@FRidh FRidh commented Sep 3, 2020

This probably means that a change like in f6286de needs to be made again as well.

@arianvp
Copy link
Member

@arianvp arianvp commented Sep 3, 2020

I dont fully understand what was done there and why; but I think @flokli has more context.

The test suite passed by the way

@arianvp
arianvp approved these changes Sep 3, 2020
@arianvp
Copy link
Member

@arianvp arianvp commented Sep 3, 2020

@FRidh as you use a separate lvm2 override there; I don't see how this PR messes with what you did in your PR. Do you agree? In that case I'd prefer to merge this now.

@FRidh
Copy link
Member

@FRidh FRidh commented Sep 3, 2020

For the installer tests all packages being installed need to be made available in the test, because the installer cannot download them.

Instead of building without udev we now build without cryptsetup. Thus, the installer also needs to have offline lvm2 without cryptsetup instead of without udev.

@arianvp
Copy link
Member

@arianvp arianvp commented Sep 3, 2020

@andersk do you want to make the suggested change by @FRidh ?

@FRidh
Copy link
Member

@FRidh FRidh commented Sep 3, 2020

$ nix-build nixos/release-combined.nix -A nixos.tests.simple.x86_64-linux builds successfully so I don't think it is needed after all.

$ nix-build nixos/release-combined.nix -A nixos.tests.installer.simple.x86_64-linux builds, so I think I was wrong.

@vcunat
Copy link
Member

@vcunat vcunat commented Sep 3, 2020

This PR's approach seems to put two different instances of systemd.out into closures, each around 28 MiB. I can't say I like that, at least as a long-term solution.

@arianvp
Copy link
Member

@arianvp arianvp commented Sep 3, 2020

I agree it is not ideal; but we can merge it now to unbreak staging and nixpkgs-unstable and then iterate on a better approach in a follow-up PR. Lets keep the issue open even after mergintg this. Does that sound good @vcunat ?

@FRidh
Copy link
Member

@FRidh FRidh commented Sep 3, 2020

I think we should go ahead with this. That way we can have a branch-off. The fix for this we require for release and can be cherry-picked then to the release branch. cc @jonringer

edit:
in retrospect I think we can have a branch-off even with this issue unresolved. Nonetheless, if this can unblock a channel its probably good.

@vcunat
Copy link
Member

@vcunat vcunat commented Sep 3, 2020

Well, I haven't looked into correctness of this PR, except for some doubts I have but those should be fixable without a significant rebuild. EDIT: meaning... feel free to use this PR for now, if you think it's good enough (overall the situation is unpleasant for 20.09).

I'm rather thinking of a better solution; perhaps a separate libudev derivation.

@flokli
Copy link
Contributor

@flokli flokli commented Sep 3, 2020

@jonringer
Copy link
Contributor

@jonringer jonringer commented Sep 3, 2020

@FRidh does this break the luks scenario, or just the tests?

If the luks scenario still works, I think we should go forward with the branchoff, I would rather not delay ZHF work for any one particular scenario. We could do a staging-20.09 cycle to integrate the changes after the branchoff.

EDIT:
Also, a change from last year is that the "beta" period will be a week after ZHF starts (or more accurately, ZHF was pushed forward a week). So there will still be some time to iterate on this.

@flokli
Copy link
Contributor

@flokli flokli commented Sep 3, 2020

@andersk
Copy link
Contributor Author

@andersk andersk commented Sep 3, 2020

This probably means that a change like in f6286de needs to be made again as well.

If I understand correctly, f6286de should simply be reverted now.

Before my change, the problem that it solved was that there are two incompatible copies of libdevmapper from the two lvm2s in the stage-1 closure, and dmsetup might be re-linked against the wrong one.

After my change, there are instead two copies of libudev in the closure—but this time I believe they are compatible; whether systemd is built with cryptsetup should not make a difference to libudev. (There are also two incompatible copies of libsystemd-shared in the closure, but only the right one is ever recursively referenced by the binaries we scan with find-libs, so this isn’t a problem.)

@andersk
Copy link
Contributor Author

@andersk andersk commented Sep 3, 2020

Updated with that revert, and successfully tested luksformat again locally.

@FRidh
Copy link
Member

@FRidh FRidh commented Sep 3, 2020

Also e02793d then. Please do the reverts as separate commits for clarity.

@andersk andersk force-pushed the andersk:cryptception-1 branch from d21cb57 to a7676d7 Sep 3, 2020
@andersk
Copy link
Contributor Author

@andersk andersk commented Sep 3, 2020

@FRidh I had squashed the commits because it doesn’t make sense to do one without the other, and I don’t like leaving behind bad intermediate states for future git bisecters to trip on. Separated now, but are you sure that’s what we want?

@FRidh
Copy link
Member

@FRidh FRidh commented Sep 3, 2020

Good remark, I had not thought of the bisecting case. Often these type of PR's are merged instead of rebased so they're grouped together, but that still does not help you (at least not yet). Personally I like git log here clearly stating the commits that have been reverted, but given the bisecting case I have no strong opinion on it. I don't think we have guidelines on this part aside from the "commit per logical unit".

@Atemu
Copy link
Member

@Atemu Atemu commented Sep 3, 2020

I'd say two commits that only work together qualifies as a "logical unit".

The cyclic dependency of systemd → cryptsetup → lvm2 → udev=systemd
needs to be broken somewhere.  The previous strategy of building
cryptsetup with an lvm2 built without udev (#66856) caused the
installer.luksroot test to fail.  Instead, build lvm2 with a udev built
without cryptsetup.

Fixes #96479.

Signed-off-by: Anders Kaseorg <andersk@mit.edu>
@andersk andersk force-pushed the andersk:cryptception-1 branch from a7676d7 to f4b2c9d Sep 3, 2020
@andersk
Copy link
Contributor Author

@andersk andersk commented Sep 3, 2020

Yeah, that’s my feeling—a “logical unit” is a minimal change that makes sense independently given its before and after states, not a mechanical step along the historical chain of development between more distant states. Squashed again.

@Atemu
Copy link
Member

@Atemu Atemu commented Sep 3, 2020

test script finished in 335.86s
cleaning up
(0.00 seconds)
/nix/store/7h4m56523m97b1qqcn59v0qfxsf7wc9y-vm-test-run-installer-luksroot-format1
@vcunat
Copy link
Member

@vcunat vcunat commented Sep 3, 2020

@flokli: libudev split is harder than I hoped; more on #97051

@arianvp
Copy link
Member

@arianvp arianvp commented Sep 4, 2020

I'm ok to merge.

@vcunat I personally don't see how splitting up systemd in libudev and not libudev solves the core problem. We'd still have to have two udevs (one with and one without cryptsetup). It's just that the closure size is potentially a bit smaller, but it doesn't solve the actual cycle as far as I can see?

@jonringer
Copy link
Contributor

@jonringer jonringer commented Sep 4, 2020

@jonringer
Copy link
Contributor

@jonringer jonringer commented Sep 4, 2020

do you mind rebasing on the lastest staging. The evaluation error was fixed

EDIT: nevermind, seems to be fine

@flokli
flokli approved these changes Sep 4, 2020
@flokli flokli merged commit 176d5e0 into NixOS:staging Sep 4, 2020
19 checks passed
19 checks passed
tests tests
Details
action
Details
cryptsetup, cryptsetup.passthru.tests, lvm2, lvm2.passthru.tests, systemd, systemd.passthru.tests on x86_64-linux Failure
Details
Evaluation Performance Report Evaluator Performance Report
Details
Wait for ofborg
Details
cryptsetup, cryptsetup.passthru.tests, lvm2, lvm2.passthru.tests, systemd, systemd.passthru.tests on aarch64-linux Success
Details
grahamcofborg-eval ^.^!
Details
grahamcofborg-eval-check-maintainers matching changed paths to changed attrs...
Details
grahamcofborg-eval-check-meta config.nix: checkMeta = true
Details
grahamcofborg-eval-darwin nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="f4b2c9d"; rev="f4b2c9dfe7cb63edc8059c6caeaa9de015068628"; } ./pkgs/t
Details
grahamcofborg-eval-lib-tests nix-build --arg pkgs import ./. {} ./lib/tests/release.nix
Details
grahamcofborg-eval-nixos nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="f4b2c9d"; rev="f4b2c9dfe7cb63edc8059c6caeaa9de015068628"; } ./nixos/
Details
grahamcofborg-eval-nixos-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="f4b2c9d"; rev="f4b2c9dfe7cb63edc8059c6caeaa9de015068628"; } ./nixos/
Details
grahamcofborg-eval-nixos-options nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="f4b2c9d"; rev="f4b2c9dfe7cb63edc8059c6caeaa9de015068628"; } ./nixos/
Details
grahamcofborg-eval-nixpkgs-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="f4b2c9d"; rev="f4b2c9dfe7cb63edc8059c6caeaa9de015068628"; } ./pkgs/t
Details
grahamcofborg-eval-nixpkgs-tarball nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="f4b2c9d"; rev="f4b2c9dfe7cb63edc8059c6caeaa9de015068628"; } ./pkgs/t
Details
grahamcofborg-eval-nixpkgs-unstable-jobset nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="f4b2c9d"; rev="f4b2c9dfe7cb63edc8059c6caeaa9de015068628"; } ./pkgs/t
Details
grahamcofborg-eval-package-list nix-env -qa --json --file .
Details
grahamcofborg-eval-package-list-no-aliases nix-env -qa --json --file . --arg config { allowAliases = false; }
Details
20.09 Blockers automation moved this from To Do to Done Sep 4, 2020
@flokli flokli mentioned this pull request Sep 5, 2020
2 of 10 tasks complete
@doronbehar doronbehar mentioned this pull request Sep 18, 2020
0 of 10 tasks complete
@worldofpeace worldofpeace added this to In progress in 20.09 Blockers via automation Oct 5, 2020
@worldofpeace worldofpeace removed this from Done in 20.09 Blockers Oct 5, 2020
@worldofpeace worldofpeace moved this from In progress to Done in 20.09 Blockers Oct 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.