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.

Sign up for GitHub

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

Jump to bottom

zpool: add option to mount zpool not at / #211

Merged
merged 1 commit into from
Apr 29, 2023
Merged

zpool: add option to mount zpool not at / #211

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 14 additions & 2 deletions types/zpool.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,17 @@
default = [ "defaults" ];
description = "Options to pass to mount";
};
mountRoot = lib.mkOption {
type = lib.types.str;
default = "/";
example = "/mnt";
description = ''
The root location where the zpool should be mounted.

Note:
Leaving this at the default "/" might break your live system.
'';
};
datasets = lib.mkOption {
type = lib.types.attrsOf (diskoLib.subType { inherit (subTypes) zfs_fs zfs_volume; });
# type = lib.types.attrsOf subTypes.zfs_fs;
Expand All @@ -55,7 +66,7 @@
default = _: ''
readarray -t zfs_devices < <(cat "$disko_devices_dir"/zfs_${config.name})
zpool create ${config.name} \
${config.mode} \
-R ${config.mountRoot} ${config.mode} \
Copy link
Member

Choose a reason for hiding this comment

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

man zpool-create suggests that implicitly sets cachefile=none and altroot properties on the pool, even if no alt-root is used. Do we want this or should we use lib.optionalString here?

Copy link
Member Author

@SuperSandro2000 SuperSandro2000 Apr 22, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I just set / to keep backwards compatibility but it doesn't make much sense as it will most likely overwrite the system that the command is run from.

We don't need to worry about altroot and since at this stage the system is not fully configured yet, the missing cachefile should not make a big difference.

Copy link
Member

Choose a reason for hiding this comment

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

Ah yes, did a bit more docs reading, cachefile=none seems harmless.

I'd still prefer to only set -R if actually used, but don't care too much :)

${lib.concatStringsSep " " (lib.mapAttrsToList (n: v: "-o ${n}=${v}") config.options)} \
${lib.concatStringsSep " " (lib.mapAttrsToList (n: v: "-O ${n}=${v}") config.rootFsOptions)} \
"''${zfs_devices[@]}"
Expand All @@ -70,7 +81,8 @@
in
{
dev = ''
zpool list '${config.name}' >/dev/null 2>/dev/null || zpool import '${config.name}'
zpool list '${config.name}' >/dev/null 2>/dev/null || \
zpool import -R ${config.mountRoot} '${config.name}'
Copy link
Member

Choose a reason for hiding this comment

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

Given that you set the altroot here, during import, is it even necessary to do so above, during creation, as well? The remark about cachefile=none still applies here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, because zpool create mounts the pool already.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe I am missing something, but why do we need to set altroot during creation? Shouldn't the import suffice to mount it in a live system? I'll try to do a practice test tomorrow

Copy link
Contributor

@oddlama oddlama Apr 29, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

It's important to have -R on create too, otherwise any datasets that have option.mountpoint set will be mounted on top of the live system when they are created. Not setting -R shadows existing mountpoints like /, which would be fatal.

${lib.concatMapStrings (x: x.dev or "") (lib.attrValues datasetMounts)}
'';
fs = (datasetMounts.fs or {}) // lib.optionalAttrs (config.mountpoint != null) {
Expand Down