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

nixos/lib/utils: Add `fileSystems.<name>.depends` option and generalise fsBefore (fixes #86955) #86967

Open
wants to merge 2 commits into
base: master
from

Conversation

@jakobrs
Copy link
Contributor

jakobrs commented May 5, 2020

Motivation for this change

See #86955 for an example configuration where this matters.

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.
@jakobrs jakobrs changed the title nixos/lib/utils: Generalise fsBefore nixos/lib/utils: Generalise fsBefore (in response to #86955) May 5, 2020
@jakobrs
Copy link
Contributor Author

jakobrs commented May 5, 2020

Also, could we add a fileSystem.<name>.depends option that simply lists all mounts a mount depends on explicitly (in case there's more edge cases)? Something like

fsBefore = a: b: a.mountPoint == b.device
              || hasPrefix "${a.mountPoint}${optionalString (!(hasSuffix "/" a.mountPoint)) "/"}" b.device
              || hasPrefix "${a.mountPoint}${optionalString (!(hasSuffix "/" a.mountPoint)) "/"}" b.mountPoint
              || lib.elem a.mountPoint b.depends;

Edit: See jakobrs:fsbefore-custom-depends for an implementation of the fileSystems.<name>.depends option.

@jakobrs jakobrs force-pushed the jakobrs:more-general-fsbefore branch from 481d8d9 to aba704a May 6, 2020
@jakobrs jakobrs changed the title nixos/lib/utils: Generalise fsBefore (in response to #86955) nixos/lib/utils: Generalise fsBefore (fixes #86955) May 10, 2020
@jakobrs
Copy link
Contributor Author

jakobrs commented May 21, 2020

Note that this feels somewhat fragile. I'd rather the paths were normalised, but I'm not sure how that would be done. The following seems less likely to break:

# Check whenever `b` depends on `a` as a fileSystem
fsBefore = a: b:
  let
    # normalizePath should also remove duplicate slashes, etc
    # Note that we're assuming that the path is a directory (because only
    # directories should have a trailing slash). This isn't always the case,
    # but it shouldn't cause any issues.
    normalizePath = path: "${path}${optionalString (!(hasSuffix "/" path)) "/"}";
    normalize = mount: mount // { device = normalizePath mount.device;
                                  mountPoint = normalizePath mount.mountPoint;
                                  depends = map normalizePath mount.depends;
                                };

    a' = normalize a;
    b' = normalize b;

  in hasPrefix a'.mountPoint b'.device
  || hasPrefix a'.mountPoint b'.mountPoint
  || any (dependency: hasPrefix a'.mountPoint dependency) b'.depends;

but I'm not sure if this it the right place to be normalising the paths.

@jakobrs
Copy link
Contributor Author

jakobrs commented May 21, 2020

I've attempted to find more cases similar to what @griff mentioned, and the results are in my jakobrs:alternate-fsbefore-implementation branch. Note that nothing needs to be done for nixos/modules/virtualisation/qemu-vm.nix, because the /nix/store file system is mounted manually.

From 10fcd6e2d3f832bdf5882b2d8937850637ae8734 Mon Sep 17 00:00:00 2001
From: jakobrs <jakobrs100@gmail.com>
Date: Thu, 21 May 2020 09:10:47 +0200
Subject: [PATCH 1/2] nixos/lib, nixos/filesystems: Make fsBefore more stable,
 and add `depends` option

---
 nixos/lib/utils.nix                 | 17 +++++++++++++++--
 nixos/modules/tasks/filesystems.nix | 10 ++++++++++
 2 files changed, 25 insertions(+), 2 deletions(-)

diff --git a/nixos/lib/utils.nix b/nixos/lib/utils.nix
index 21f4c7c6988..8b1da882013 100644
--- a/nixos/lib/utils.nix
+++ b/nixos/lib/utils.nix
@@ -7,8 +7,21 @@ rec {
                      || elem fs.mountPoint [ "/" "/nix" "/nix/store" "/var" "/var/log" "/var/lib" "/etc" ];
 
   # Check whenever `b` depends on `a` as a fileSystem
-  fsBefore = a: b: a.mountPoint == b.device
-                || hasPrefix "${a.mountPoint}${optionalString (!(hasSuffix "/" a.mountPoint)) "/"}" b.mountPoint;
+  fsBefore = a: b:
+    let
+      # normalisePath should also remove duplicate slashes, etc
+      normalisePath = path: "${path}${optionalString (!(hasSuffix "/" path)) "/"}";
+      normalise = mount: mount // { device = normalisePath mount.device;
+                                    mountPoint = normalisePath mount.mountPoint;
+                                    depends = map normalisePath mount.depends;
+                                  };
+
+      a' = normalise a;
+      b' = normalise b;
+
+    in hasPrefix a'.mountPoint b'.device
+    || hasPrefix a'.mountPoint b'.mountPoint
+    || any (dependency: hasPrefix a'.mountPoint dependency) b'.depends;
 
   # Escape a path according to the systemd rules, e.g. /dev/xyzzy
   # becomes dev-xyzzy.  FIXME: slow.
diff --git a/nixos/modules/tasks/filesystems.nix b/nixos/modules/tasks/filesystems.nix
index 0ade74b957a..0c4443669a3 100644
--- a/nixos/modules/tasks/filesystems.nix
+++ b/nixos/modules/tasks/filesystems.nix
@@ -56,6 +56,16 @@ let
         type = types.listOf nonEmptyStr;
       };
 
+      depends = mkOption {
+        default = [ ];
+        example = [ "/persist" ];
+        type = types.listOf nonEmptyStr;
+        description = ''
+          List of paths that must be mounted before this one. Note that you do
+          not need to add the value of .device or .mountPoint to this list.
+        '';
+      };
+
     };
 
     config = {
-- 
2.23.0


From 5509117cace58b2c1c5346b40f6abe354bb71d91 Mon Sep 17 00:00:00 2001
From: jakobrs <jakobrs100@gmail.com>
Date: Thu, 21 May 2020 09:32:06 +0200
Subject: [PATCH 2/2] treewide: Use `fileSystems.<name>.depends` option where
 possible/necessary

---
 nixos/modules/installer/cd-dvd/iso-image.nix | 6 ++++++
 nixos/modules/installer/netboot/netboot.nix  | 6 ++++++
 2 files changed, 12 insertions(+)

diff --git a/nixos/modules/installer/cd-dvd/iso-image.nix b/nixos/modules/installer/cd-dvd/iso-image.nix
index cce7cc235ec..4d0a7e0f9e1 100644
--- a/nixos/modules/installer/cd-dvd/iso-image.nix
+++ b/nixos/modules/installer/cd-dvd/iso-image.nix
@@ -598,6 +598,12 @@ in
           "upperdir=/nix/.rw-store/store"
           "workdir=/nix/.rw-store/work"
         ];
+
+        depends = [
+          "/nix/.ro-store"
+          "/nix/.rw-store/store"
+          "/nix/.rw-store/work"
+        ];
       };
 
     boot.initrd.availableKernelModules = [ "squashfs" "iso9660" "uas" "overlay" ];
diff --git a/nixos/modules/installer/netboot/netboot.nix b/nixos/modules/installer/netboot/netboot.nix
index 95eba86bcb6..9d3f9fe311b 100644
--- a/nixos/modules/installer/netboot/netboot.nix
+++ b/nixos/modules/installer/netboot/netboot.nix
@@ -57,6 +57,12 @@ with lib;
           "upperdir=/nix/.rw-store/store"
           "workdir=/nix/.rw-store/work"
         ];
+
+        depends = [
+          "/nix/.ro-store"
+          "/nix/.rw-store/store"
+          "/nix/.rw-store/work"
+        ];
       };
 
     boot.initrd.availableKernelModules = [ "squashfs" "overlay" ];
-- 
2.23.0
@jakobrs jakobrs force-pushed the jakobrs:more-general-fsbefore branch from aba704a to 6cbb64e May 23, 2020
@jakobrs jakobrs force-pushed the jakobrs:more-general-fsbefore branch from 6cbb64e to 8ccc574 May 23, 2020
@jakobrs jakobrs changed the title nixos/lib/utils: Generalise fsBefore (fixes #86955) nixos/lib/utils: Add `fileSystems.<name>.depends` option and generalise fsBefore (fixes #86955) Jun 13, 2020
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

1 participant
You can’t perform that action at this time.