[pve-devel] [PATCH storage manager docs] Allow mounting of CIFS subdirectories

[pve-devel] [PATCH storage manager docs] Allow mounting of CIFS subdirectories

[Leo Nunner]

CIFS supports mounting subdirectories inside a share, so it makes sense
to also have the 'subdir' parameter for the CIFS backend. I'm also
looking into allowing overrides for all the fixed directories, but 
even then, I think it makes sense to support this parameter (for either
providing a prefix to all the other directories, or just changing the
base directory).

storage:

Leo Nunner (1):
  fix #2641: allow mounting of CIFS subdirectories

 PVE/Storage/CIFSPlugin.pm   | 39 ++++++++++++++++++++-----------------
 PVE/Storage/CephFSPlugin.pm |  4 ----
 PVE/Storage/Plugin.pm       |  5 +++++
 3 files changed, 26 insertions(+), 22 deletions(-)

manager:

Leo Nunner (1):
  fix #2641: expose CIFS subdir parameter through GUI

 www/manager6/storage/CIFSEdit.js | 11 +++++++++++
 1 file changed, 11 insertions(+)

docs:

Leo Nunner (1):
  fix #2641: document subdir parameter for CIFS backend

 pve-storage-cifs.adoc | 5 +++++
 1 file changed, 5 insertions(+)

-- 
2.30.2

[pve-devel] [PATCH storage 1/1] fix #2641: allow mounting of CIFS subdirectories

[Leo Nunner]

CIFS/SMB supports directly mounting subdirectories, so it makes sense to
also allow the --subdir parameter for these storages. The subdir
parameter was moved from CephFSPlugin.pm to Plugin.pm, because it isn't
specific to CephFS anymore.

Signed-off-by: Leo Nunner <l.nunner@proxmox.com>
---
 PVE/Storage/CIFSPlugin.pm   | 39 ++++++++++++++++++++-----------------
 PVE/Storage/CephFSPlugin.pm |  4 ----
 PVE/Storage/Plugin.pm       |  5 +++++
 3 files changed, 26 insertions(+), 22 deletions(-)

diff --git a/PVE/Storage/CIFSPlugin.pm b/PVE/Storage/CIFSPlugin.pm
index 982040a..dc41e0b 100644
--- a/PVE/Storage/CIFSPlugin.pm
+++ b/PVE/Storage/CIFSPlugin.pm
@@ -13,11 +13,16 @@ use base qw(PVE::Storage::Plugin);
 
 # CIFS helper functions
 
-sub cifs_is_mounted {
-    my ($server, $share, $mountpoint, $mountdata) = @_;
+sub cifs_is_mounted : prototype($$) {
+    my ($scfg, $mountdata) = @_;
+
+    my $mountpoint = $scfg->{path};
+    my $server = $scfg->{server};
+    my $share = $scfg->{share};
+    my $subdir = $scfg->{subdir} // "/";
 
     $server = "[$server]" if Net::IP::ip_is_ipv6($server);
-    my $source = "//${server}/$share";
+    my $source = "//${server}/$share$subdir";
     $mountdata = PVE::ProcFSTools::parse_proc_mounts() if !$mountdata;
 
     return $mountpoint if grep {
@@ -63,11 +68,16 @@ sub get_cred_file {
     return undef;
 }
 
-sub cifs_mount {
-    my ($server, $share, $mountpoint, $storeid, $smbver, $user, $domain) = @_;
+sub cifs_mount : prototype($$$$$) {
+    my ($scfg, $storeid, $smbver, $user, $domain) = @_;
+
+    my $mountpoint = $scfg->{path};
+    my $server = $scfg->{server};
+    my $share = $scfg->{share};
+    my $subdir = $scfg->{subdir} // "/";
 
     $server = "[$server]" if Net::IP::ip_is_ipv6($server);
-    my $source = "//${server}/$share";
+    my $source = "//${server}/$share$subdir";
 
     my $cmd = ['/bin/mount', '-t', 'cifs', $source, $mountpoint, '-o', 'soft', '-o'];
 
@@ -130,6 +140,7 @@ sub options {
 	path => { fixed => 1 },
 	server => { fixed => 1 },
 	share => { fixed => 1 },
+	subdir => { optional => 1 },
 	nodes => { optional => 1 },
 	disable => { optional => 1 },
 	maxfiles => { optional => 1 },
@@ -204,12 +215,8 @@ sub status {
     $cache->{mountdata} = PVE::ProcFSTools::parse_proc_mounts()
 	if !$cache->{mountdata};
 
-    my $path = $scfg->{path};
-    my $server = $scfg->{server};
-    my $share = $scfg->{share};
-
     return undef
-	if !cifs_is_mounted($server, $share, $path, $cache->{mountdata});
+	if !cifs_is_mounted($scfg, $cache->{mountdata});
 
     return $class->SUPER::status($storeid, $scfg, $cache);
 }
@@ -221,17 +228,15 @@ sub activate_storage {
 	if !$cache->{mountdata};
 
     my $path = $scfg->{path};
-    my $server = $scfg->{server};
-    my $share = $scfg->{share};
 
-    if (!cifs_is_mounted($server, $share, $path, $cache->{mountdata})) {
+    if (!cifs_is_mounted($scfg, $cache->{mountdata})) {
 
 	mkpath $path if !(defined($scfg->{mkdir}) && !$scfg->{mkdir});
 
 	die "unable to activate storage '$storeid' - " .
 	    "directory '$path' does not exist\n" if ! -d $path;
 
-	cifs_mount($server, $share, $path, $storeid, $scfg->{smbversion},
+	cifs_mount($scfg, $storeid, $scfg->{smbversion},
 	    $scfg->{username}, $scfg->{domain});
     }
 
@@ -245,10 +250,8 @@ sub deactivate_storage {
 	if !$cache->{mountdata};
 
     my $path = $scfg->{path};
-    my $server = $scfg->{server};
-    my $share = $scfg->{share};
 
-    if (cifs_is_mounted($server, $share, $path, $cache->{mountdata})) {
+    if (cifs_is_mounted($scfg, $cache->{mountdata})) {
 	my $cmd = ['/bin/umount', $path];
 	run_command($cmd, errmsg => 'umount error');
     }
diff --git a/PVE/Storage/CephFSPlugin.pm b/PVE/Storage/CephFSPlugin.pm
index 4976747..944f0b8 100644
--- a/PVE/Storage/CephFSPlugin.pm
+++ b/PVE/Storage/CephFSPlugin.pm
@@ -127,10 +127,6 @@ sub properties {
 	    description => "Mount CephFS through FUSE.",
 	    type => 'boolean',
 	},
-	subdir => {
-	    description => "Subdir to mount.",
-	    type => 'string', format => 'pve-storage-path',
-	},
 	'fs-name' => {
 	    description => "The Ceph filesystem name.",
 	    type => 'string', format => 'pve-configid',
diff --git a/PVE/Storage/Plugin.pm b/PVE/Storage/Plugin.pm
index 8a41df1..af53a99 100644
--- a/PVE/Storage/Plugin.pm
+++ b/PVE/Storage/Plugin.pm
@@ -169,6 +169,11 @@ my $defaultData = {
 	    type => 'boolean',
 	    optional => 1,
 	},
+	subdir => {
+	    description => "Subdir to mount.",
+	    type => 'string', format => 'pve-storage-path',
+	    optional => 1,
+	},
 	'format' => {
 	    description => "Default image format.",
 	    type => 'string', format => 'pve-storage-format',
-- 
2.30.2

[pve-devel] applied: [PATCH storage 1/1] fix #2641: allow mounting of CIFS subdirectories

[Thomas Lamprecht]

Am 01/12/2022 um 12:32 schrieb Leo Nunner:
> CIFS/SMB supports directly mounting subdirectories, so it makes sense to
> also allow the --subdir parameter for these storages. The subdir
> parameter was moved from CephFSPlugin.pm to Plugin.pm, because it isn't
> specific to CephFS anymore.
> 
> Signed-off-by: Leo Nunner <l.nunner@proxmox.com>
> ---
>  PVE/Storage/CIFSPlugin.pm   | 39 ++++++++++++++++++++-----------------
>  PVE/Storage/CephFSPlugin.pm |  4 ----
>  PVE/Storage/Plugin.pm       |  5 +++++
>  3 files changed, 26 insertions(+), 22 deletions(-)
> 
>

the patch does more at once than ideal, I'd recommend doing such things as the
signature cleanups, which should not have any change functional effect after all,
first - that simplify review and also working with the history (e.g., bisect or just
wanting to see how a specific feature was done if adding a similar one).

But this was relatively minor crowding here, and I did not wanted to delay this
further... so applied, with some cleanups as follow up, thanks!

ps. the safer fallback for $subdir would have been a empty string '', as then it'd
be 1:1 the exact same value as before if one doesn't have any subdir configured -
but lets hope no cifs client/server throws up on a newly added trailing slash.

[pve-devel] [PATCH manager 1/1] fix #2641: expose CIFS subdir parameter through GUI

[Leo Nunner]

makes it possible to optionally set the 'subdir' parameter when adding a
new CIFS storage.

Signed-off-by: Leo Nunner <l.nunner@proxmox.com>
---
RFC: I'm not sure whether this should be exposed, since it's not
available for CephFS either, but it might be a good idea to make this
more obvious to the user.

 www/manager6/storage/CIFSEdit.js | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/www/manager6/storage/CIFSEdit.js b/www/manager6/storage/CIFSEdit.js
index 71415401..4c06de5e 100644
--- a/www/manager6/storage/CIFSEdit.js
+++ b/www/manager6/storage/CIFSEdit.js
@@ -129,6 +129,9 @@ Ext.define('PVE.storage.CIFSInputPanel', {
 	if (values.username?.length === 0) {
 	    delete values.username;
 	}
+	if (values.subdir?.length === 0) {
+	    delete values.subdir;
+	}
 
 	return me.callParent([values]);
     },
@@ -216,6 +219,14 @@ Ext.define('PVE.storage.CIFSInputPanel', {
 		    },
 		},
 	    },
+	    {
+		xtype: 'pmxDisplayEditField',
+		editable: me.isCreate,
+		name: 'subdir',
+		fieldLabel: 'Subdirectory',
+		allowBlank: true,
+		emptyText: gettext('/some/path'),
+	    },
 	];
 
 	me.callParent();
-- 
2.30.2

[pve-devel] [PATCH docs 1/1] fix #2641: document subdir parameter for CIFS backend

[Leo Nunner]

Signed-off-by: Leo Nunner <l.nunner@proxmox.com>
---
 pve-storage-cifs.adoc | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/pve-storage-cifs.adoc b/pve-storage-cifs.adoc
index bb4b902..15f8034 100644
--- a/pve-storage-cifs.adoc
+++ b/pve-storage-cifs.adoc
@@ -57,6 +57,10 @@ path::
 
 The local mount point. Optional, defaults to `/mnt/pve/<STORAGE_ID>/`.
 
+subdir::
+
+The subdirectory of the share to mount. Optional, defaults to the root directory of the share.
+
 .Configuration Example (`/etc/pve/storage.cfg`)
 ----
 cifs: backup
@@ -66,6 +70,7 @@ cifs: backup
 	content backup
 	username anna
 	smbversion 3
+	subdir /data
 
 ----
 
-- 
2.30.2

Re: [pve-devel] [PATCH storage manager docs] Allow mounting of CIFS subdirectories

[Leo Nunner]

On 2022-12-01 12:32, Leo Nunner wrote:
> CIFS supports mounting subdirectories inside a share, so it makes sense
> to also have the 'subdir' parameter for the CIFS backend. I'm also
> looking into allowing overrides for all the fixed directories, but 
> even then, I think it makes sense to support this parameter (for either
> providing a prefix to all the other directories, or just changing the
> base directory).
>
> storage:
>
> Leo Nunner (1):
>   fix #2641: allow mounting of CIFS subdirectories
>
>  PVE/Storage/CIFSPlugin.pm   | 39 ++++++++++++++++++++-----------------
>  PVE/Storage/CephFSPlugin.pm |  4 ----
>  PVE/Storage/Plugin.pm       |  5 +++++
>  3 files changed, 26 insertions(+), 22 deletions(-)
>
> manager:
>
> Leo Nunner (1):
>   fix #2641: expose CIFS subdir parameter through GUI
>
>  www/manager6/storage/CIFSEdit.js | 11 +++++++++++
>  1 file changed, 11 insertions(+)
>
> docs:
>
> Leo Nunner (1):
>   fix #2641: document subdir parameter for CIFS backend
>
>  pve-storage-cifs.adoc | 5 +++++
>  1 file changed, 5 insertions(+)
Bump due to continued user demand. storage and manager seem to still
apply, while docs produces a merge conflict because it clashes with the
"content-dirs" documentation. Does this warrant a v2?

[pve-devel] applied: [PATCH storage manager docs] Allow mounting of CIFS subdirectories

[Thomas Lamprecht]

Am 02/02/2023 um 15:20 schrieb Leo Nunner:
> On 2022-12-01 12:32, Leo Nunner wrote:
>> CIFS supports mounting subdirectories inside a share, so it makes sense
>> to also have the 'subdir' parameter for the CIFS backend. I'm also
>> looking into allowing overrides for all the fixed directories, but 
>> even then, I think it makes sense to support this parameter (for either
>> providing a prefix to all the other directories, or just changing the
>> base directory).
>>
>> storage:
>>
>> Leo Nunner (1):
>>   fix #2641: allow mounting of CIFS subdirectories
>>
>>  PVE/Storage/CIFSPlugin.pm   | 39 ++++++++++++++++++++-----------------
>>  PVE/Storage/CephFSPlugin.pm |  4 ----
>>  PVE/Storage/Plugin.pm       |  5 +++++
>>  3 files changed, 26 insertions(+), 22 deletions(-)
>>
>> manager:
>>
>> Leo Nunner (1):
>>   fix #2641: expose CIFS subdir parameter through GUI
>>
>>  www/manager6/storage/CIFSEdit.js | 11 +++++++++++
>>  1 file changed, 11 insertions(+)
>>
>> docs:
>>
>> Leo Nunner (1):
>>   fix #2641: document subdir parameter for CIFS backend
>>
>>  pve-storage-cifs.adoc | 5 +++++
>>  1 file changed, 5 insertions(+)
>
> Bump due to continued user demand. storage and manager seem to still

storage produces a merge conflict, that git could auto-merge.
Would be great if you could send a v2 for manager and docs, as those I'd apply
then once storage is bumped.