Re: [pve-devel] [PATCH pve-container 1/1] Adding new mount point type named 'zfs' to let configure a ZFS dataset as mount point for LXC container

Re: [pve-devel] [PATCH pve-container 1/1] Adding new mount point type named 'zfs' to let configure a ZFS dataset as mount point for LXC container

[Fabian Grünbichler]

> As we know, ProxMox have only three possible "categories" of mount points: ProxMox storage provider supplied, block device and bind mount. I've prepared a little patch for pve-container package which adds a fourth "category" named "zfs"  - so with this patch it's possible to add such ZFS dataset into container config in a form "mpN: <ZFS pool name>/<dataset path>,<mount path>". This new type can be useful in some cases - for instance when we need to mount ZFS dataset in the container but need to keep this dataset not mounted on the host.

nit: for single patches, there is no need to add a coverletter. also, please include relevant information in the commit message!

introducing a new mountpoint type is definitely not the right approach. could you give a reason why you want to hide the container contents from the host?

this could be implemented in pve-container (e.g., by mounting the ZFS dataset corresponding to a PVE-managed volume like we mount block devices or raw images, instead of relying on the fact that they are already mounted and bind mounting them.. we already do the same for ZFS snapshots in __mountpoint_mount, for example) and in pve-storage (e.g., by having a flag there that controls mounting, or skipping mounting if mountpoint=none or legacy) without the need for any other special handling. careful checks to see whether we rely on ZFS-backed mountpoints already being mounted anywhere else would still be needed (move volume might be one place, for example).

> Konstantin Filippov via pve-devel <pve-devel@lists.proxmox.com> hat am 10.05.2023 02:08 CEST geschrieben:
> Signed-off-by: Konstantin Filippov <frank030366@hotmail.com>
> ---
>  src/PVE/LXC.pm        | 4 ++++
>  src/PVE/LXC/Config.pm | 3 ++-
>  2 files changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/src/PVE/LXC.pm b/src/PVE/LXC.pm
> index d138161..30cf48d 100644
> --- a/src/PVE/LXC.pm
> +++ b/src/PVE/LXC.pm
> @@ -1839,6 +1839,10 @@ sub __mountpoint_mount {
>  	my ($devpath) = (Cwd::realpath($volid) =~ /^(.*)$/s); # realpath() taints
>  	PVE::Tools::run_command(['mount', @extra_opts, $volid, $mount_path]) if $mount_path;
>  	return wantarray ? ($volid, 0, $devpath) : $volid;
> +    } elsif ($type eq 'zfs') {
> +	push @extra_opts, '-o', 'ro' if $readonly;
> +	PVE::Tools::run_command(['mount.zfs', @extra_opts, $volid, $mount_path]) if $mount_path;
> +	return wantarray ? ($volid, 0, undef) : $volid
>      } elsif ($type eq 'bind') {
>  	die "directory '$volid' does not exist\n" if ! -d $volid;
>  	bindmount($volid, $parentfd, $last_dir//$rootdir, $mount_path, $readonly, @extra_opts) if $mount_path;
> diff --git a/src/PVE/LXC/Config.pm b/src/PVE/LXC/Config.pm
> index ac9db94..056ec98 100644
> --- a/src/PVE/LXC/Config.pm
> +++ b/src/PVE/LXC/Config.pm
> @@ -1557,7 +1557,8 @@ sub classify_mountpoint {
>  	return 'device' if $vol =~ m!^/dev/!;
>  	return 'bind';
>      }
> -    return 'volume';
> +    return 'volume' if $vol =~ m!:.*(vm|subvol)-[0-9]*-disk-[0-9]*!;
> +    return 'zfs';
>  }
>  
>  my $__is_volume_in_use = sub {
> -- 
> 2.30.2

Re: [pve-devel] [PATCH pve-container 1/1] Adding new mount point type named 'zfs' to let configure a ZFS dataset as mount point for LXC container

[Fabian Grünbichler]

> Konstantin <frank030366@hotmail.com> hat am 11.05.2023 13:56 CEST geschrieben:
> 
> 
> Hello,
> > nit: for single patches, there is no need to add a coverletter. also, please include relevant information in the commit message!
> I'm new here, so sorry - will follow rules in future.

no worries! check out https://pve.proxmox.com/wiki/Developer_Documentation if you haven't already :)

> >could you give a reason why you want to hide the container contents from the host?
> I'll try to explain my points. I'm using Proxmox as a base for my home NAS in addition with possibility to setup some test environments to play around. So one LXC container is playing NAS role - it has all required software installed (samba/ftp/etc) and have a big volume mounted for data storage (8-10TB). If it will be created and configured as proxmox builtin storage volume (using ZFS storage provider) I have at least 3 points which I'm not comfortable with:
> - this big dataset will be mounted to PVE host and will be visible and accessible from host so every (for example) file search operation will be affected by this dataset. I would like to narrow any such file operation only to host related stuff, not to my NAS data;

most tools have ways to exclude certain paths ;)

> - in addition while operating on host I have a probability to accidentally affect or destroy my NAS data so I'd like to avoid this possibility anyway;

IMHO that's what backups are for, but I get the point.

> - simple "pct destroy" command will destroy all proxmox storage provided mount points as well. I'd like to avoid such possibilty anyway.

you could "protect" the guest:

$ pct set XXXX -protection 1
$ pct destroy XXXX
can't remove CT XXXX - protection mode enabled


another alternative would be to use a (protected) VM - no regular ZFS dataset, no visibility on the host.

> As I see in pve-container code - only bind mount and block device mount can be used as non-proxmox volume. But bind mount isn't acceptable for me according to points above. ZFS dataset isn't a block device - so it cannot be mounted using standard notation in LXC config. That's why I'm proposing this patch - it adds the capality to use ZFS filesystem as mount point for LXC container. With this functionality I can just add the following line (or configure with pct) to LXC container config:
> mp1: tank/nas-data,mp=/data
> And after that ZFS dataset "tank/nas-data" will be mounted inside container and will not be exposed to host (of course mountpoint=legacy should be set for this dataset). Maybe other more elegant ways possible to implement this but this the only way I've found.

the two existing special cases besides PVE-managed volumes have a rather big use case - passing through existing hard disks or partitions (shared with VMs, which have the same feature), and passing in host directories. this would be a very niche feature only applying to one specific storage type, so changing the syntax (and adding checks all over the place) would not be worth it.

but like I said, it can be implemented more properly as well - currently we say "a non-snapshot ZFS volume managed by PVE is always mounted on the host and can be bind-mounted", but we could just as well say "a non-snapshot ZFS volume is mounted directly via mount", either in general, or opt-in via flag in storage.cfg, or just for mountpoint=legacy/none mountpoints (e.g., where PVE::Storage::path returns a special value? or ..). nothing with regards to the container config syntax would change, just the mountpoint handling in pve-container (and the part in pve-storage that currently does the host-side mounting in activate_volume).

Re: [pve-devel] [PATCH pve-container 1/1] Adding new mount point type named 'zfs' to let configure a ZFS dataset as mount point for LXC container

[Fabian Grünbichler]

On May 16, 2023 3:07 pm, Konstantin wrote:
> Hello,
> 
>  > most tools have ways to exclude certain paths ;)
> 
> Yeah - and every time when this "need to be excluded datasets" 
> list/names changed we need to update exclude options for this tools as 
> well. It seems that just make this datasets not visible to host is 
> simpler, isn't it?

well the idea would be to exclude the whole dataset used by PVE, not
every single volume on it. but I understand this can be cumbersome.

>  > you could "protect" the guest:
> 
> I know about this option - but sometimes it isn't applicable. For 
> example I often use the following scenario when need to upgrade an OS on 
> container: save configs from container, destroy the container (dataset 
> with my data isn't destroyed because it's non PVE), deploy the new one 
> from updated template (dataset with my data just reattached back), 
> restore configs and it's ready to use. Maybe the following option will 
> be useful if you're insist on using Proxmox managed storage - introduce 
> the ability to protect a volume? If so - it probably will be acceptable 
> way for me.

you can just create a new container, then re-assign your "data" volume
that is managed by PVE to that new container (that feature is even on
the GUI nowadays ;)), then delete the old one. before that people used
"special" VMIDs to own such volumes, which also works, but is a bit more
brittle (e.g., migration will allocate a new volume owned by the guest,
and that would then be cleaned up, so extra care would need to be
applied).

>  > but like I said, it can be implemented more properly as well
> 
> In a couple with volume protection capability it could be an option - 
> make a possibility for PVE managed ZFS dataset to have a legacy 
> mountpoint instead of mandatory mount on host. But as I said - it's the 
> only (and working) method which I've found for me and I'm just proposing 
> it as starting point for possible improvement in such use cases like 
> mine. If you can propose a better solution for that - ok, let's discuss 
> in details how it can be done.

adding a protected flag that prevents certain operations is doable - the
question is then, what else except explicit detaching of the volume
should be forbidden? force restoring over that container? moving the
volume? reassigning it? migrating the container? changing some option of
the mountpoint? destruction of the container itself? the semantics are
not 100% clear to me, and should not be tailored to one specific use
case but match as broadly as sensible. but if you think this is
sensible, we can also discuss this enhancement further (but to me, it's
entirely orthogonal to the mountpoint issue at hand, other than you
happening to want both of them for your use case ;)).

my gut feeling is still that the root issue is that you have data
that is both too valuable to accidentally lose, but at the same time not
backed up? because usually when you have backups, you still try to
minimize the potential for accidents, but you accept the fact that you
cannot ever 100% prevent them. this is a time bomb waiting to explode,
no amount of features or workarounds will really help unless the root
problem is addressed. if I misunderstood something, I'd be glad to get
more information to help me understand the issue!

like I said, changing our ZFS mountpoint handling to either default to,
or optionally support working without the need to have the volume
dataset already mounted in a specific path by the storage layer sounds
okay to me. there is no need for this on the PVE side, so somebody that
wants this feature would need to write the patches and drive the change,
otherwise it will be a low-priority enhancement request.