From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id BB5EE76445 for ; Fri, 23 Apr 2021 09:08:23 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id A9B9B244A8 for ; Fri, 23 Apr 2021 09:08:23 +0200 (CEST) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [94.136.29.106]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS id 87E9A2449B for ; Fri, 23 Apr 2021 09:08:21 +0200 (CEST) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id 38EF342808 for ; Fri, 23 Apr 2021 09:08:21 +0200 (CEST) Date: Fri, 23 Apr 2021 09:08:13 +0200 From: Fabian =?iso-8859-1?q?Gr=FCnbichler?= To: Proxmox VE development discussion References: <20210104153051.27815-1-a.lauterer@proxmox.com> In-Reply-To: <20210104153051.27815-1-a.lauterer@proxmox.com> MIME-Version: 1.0 User-Agent: astroid/0.15.0 (https://github.com/astroidmail/astroid) Message-Id: <1619161215.nw2u4dqxb6.astroid@nora.none> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-SPAM-LEVEL: Spam detection results: 0 AWL 0.044 Adjusted score from AWL reputation of From: address KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Subject: Re: [pve-devel] [PATCH storage] deprecate mkdir option for create_path and populate_directory X-BeenThere: pve-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox VE development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 23 Apr 2021 07:08:23 -0000 On January 4, 2021 4:30 pm, Aaron Lauterer wrote: > The `mkdir` option has two meanings[0][1] which are split up in `create_p= ath` > and `populate_directory`. >=20 > The `create_path` option decides if the path to the storage is > automatically created or not. > The `populate_directory` options decides if the default directory > structure (dump, images, ...) at the storage location is created. >=20 > The `mkdir` option is still working but will trigger a warning in the > logs. >=20 > As a side effect, this also fixes #3214 because the `create_path` option > is now run after the `is_mountpoint` check in the `activate_storage` > method in DirPlugin.pm. >=20 > Signed-off-by: Aaron Lauterer Reviewed-by: Fabian Gr=C3=BCnbichler with two nits that could be addressed as follow-up or folded in, see=20 below >=20 > [0] https://lists.proxmox.com/pipermail/pve-devel/2020-December/046575.ht= ml > [1] https://lists.proxmox.com/pipermail/pve-devel/2020-December/046576.ht= ml > --- >=20 > To not be too spammy I only created a warning for mkdir in > Plugin::activate_storage. >=20 > I wasn't sure if this warrants a 'fix #3214' in the commit message as the > fix is only a side effect. >=20 > PVE/Storage/CIFSPlugin.pm | 4 +++- > PVE/Storage/CephFSPlugin.pm | 4 +++- > PVE/Storage/DirPlugin.pm | 19 ++++++++++++++++--- > PVE/Storage/GlusterfsPlugin.pm | 4 +++- > PVE/Storage/NFSPlugin.pm | 4 +++- > PVE/Storage/Plugin.pm | 22 ++++++++++++++++++++++ > 6 files changed, 50 insertions(+), 7 deletions(-) >=20 > diff --git a/PVE/Storage/CIFSPlugin.pm b/PVE/Storage/CIFSPlugin.pm > index be06cc7..5ee3190 100644 > --- a/PVE/Storage/CIFSPlugin.pm > +++ b/PVE/Storage/CIFSPlugin.pm > @@ -142,6 +142,8 @@ sub options { > domain =3D> { optional =3D> 1}, > smbversion =3D> { optional =3D> 1}, > mkdir =3D> { optional =3D> 1 }, > + create_path =3D> { optional =3D> 1 }, > + populate_directory =3D> { optional =3D> 1 }, > bwlimit =3D> { optional =3D> 1 }, > }; > } > @@ -225,7 +227,7 @@ sub activate_storage { > =20 > if (!cifs_is_mounted($server, $share, $path, $cache->{mountdata})) { > =20 > - mkpath $path if !(defined($scfg->{mkdir}) && !$scfg->{mkdir}); > + mkpath $path if $class->should_create_path($scfg); > =20 > die "unable to activate storage '$storeid' - " . > "directory '$path' does not exist\n" if ! -d $path; > diff --git a/PVE/Storage/CephFSPlugin.pm b/PVE/Storage/CephFSPlugin.pm > index 8eb7c70..3d6309d 100644 > --- a/PVE/Storage/CephFSPlugin.pm > +++ b/PVE/Storage/CephFSPlugin.pm > @@ -147,6 +147,8 @@ sub options { > content =3D> { optional =3D> 1 }, > format =3D> { optional =3D> 1 }, > mkdir =3D> { optional =3D> 1 }, > + create_path =3D> { optional =3D> 1 }, > + populate_directory =3D> { optional =3D> 1 }, we have a preference for new parameters/properties to not use=20 underscores, and the storage.cfg already has both (is_mountpoint and=20 prune-backups), but we might want to fix this up before adding more=20 underscores ;) I am not 100% sure about the naming either, since both options do very=20 similar stuff (one creates the top level dir, the other the sub dirs).=20 maybe create-path create-sub-dirs is more straightforward? (and a bit shorter for the latter as well ;)) > fuse =3D> { optional =3D> 1 }, > bwlimit =3D> { optional =3D> 1 }, > maxfiles =3D> { optional =3D> 1 }, > @@ -203,7 +205,7 @@ sub activate_storage { > if (!cephfs_is_mounted($scfg, $storeid, $cache->{mountdata})) { > my $path =3D $scfg->{path}; > =20 > - mkpath $path if !(defined($scfg->{mkdir}) && !$scfg->{mkdir}); > + mkpath $path if $class->should_create_path($scfg); > =20 > die "unable to activate storage '$storeid' - " . > "directory '$path' does not exist\n" if ! -d $path; > diff --git a/PVE/Storage/DirPlugin.pm b/PVE/Storage/DirPlugin.pm > index 2267f11..22bdeff 100644 > --- a/PVE/Storage/DirPlugin.pm > +++ b/PVE/Storage/DirPlugin.pm > @@ -30,10 +30,22 @@ sub properties { > type =3D> 'string', format =3D> 'pve-storage-path', > }, > mkdir =3D> { > + description =3D> > + "Deprecated, use the 'create_path' and " . > + "'populate_directory' options instead.", > + type =3D> 'boolean', > + default =3D> 'yes', > + }, > + create_path =3D> { > description =3D> "Create the directory if it doesn't exist.", > type =3D> 'boolean', > default =3D> 'yes', > }, > + populate_directory =3D> { > + description =3D> "Populate the directory with the default structure= .", > + type =3D> 'boolean', > + default =3D> 'yes', > + }, > is_mountpoint =3D> { > description =3D> > "Assume the given path is an externally managed mountpoint " . > @@ -57,6 +69,8 @@ sub options { > content =3D> { optional =3D> 1 }, > format =3D> { optional =3D> 1 }, > mkdir =3D> { optional =3D> 1 }, > + create_path =3D> { optional =3D> 1 }, > + populate_directory =3D> { optional =3D> 1 }, > is_mountpoint =3D> { optional =3D> 1 }, > bwlimit =3D> { optional =3D> 1 }, > }; > @@ -133,9 +147,6 @@ sub activate_storage { > my ($class, $storeid, $scfg, $cache) =3D @_; > =20 > my $path =3D $scfg->{path}; > - if (!defined($scfg->{mkdir}) || $scfg->{mkdir}) { > - mkpath $path; > - } > =20 > my $mp =3D parse_is_mountpoint($scfg); > if (defined($mp) && !path_is_mounted($mp, $cache->{mountdata})) { > @@ -143,6 +154,8 @@ sub activate_storage { > "directory is expected to be a mount point but is not mounted: '$mp= '\n"; > } > =20 > + mkpath $path if $class->should_create_path($scfg); > + > $class->SUPER::activate_storage($storeid, $scfg, $cache); > } > =20 > diff --git a/PVE/Storage/GlusterfsPlugin.pm b/PVE/Storage/GlusterfsPlugin= .pm > index 2dd414d..4ad632f 100644 > --- a/PVE/Storage/GlusterfsPlugin.pm > +++ b/PVE/Storage/GlusterfsPlugin.pm > @@ -136,6 +136,8 @@ sub options { > content =3D> { optional =3D> 1 }, > format =3D> { optional =3D> 1 }, > mkdir =3D> { optional =3D> 1 }, > + create_path =3D> { optional =3D> 1 }, > + populate_directory =3D> { optional =3D> 1 }, > bwlimit =3D> { optional =3D> 1 }, > }; > } > @@ -300,7 +302,7 @@ sub activate_storage { > =20 > if (!glusterfs_is_mounted($volume, $path, $cache->{mountdata})) { > =09 > - mkpath $path if !(defined($scfg->{mkdir}) && !$scfg->{mkdir}); > + mkpath $path if $class->should_create_path($scfg); > =20 > die "unable to activate storage '$storeid' - " . > "directory '$path' does not exist\n" if ! -d $path; > diff --git a/PVE/Storage/NFSPlugin.pm b/PVE/Storage/NFSPlugin.pm > index e8e27c0..a32b707 100644 > --- a/PVE/Storage/NFSPlugin.pm > +++ b/PVE/Storage/NFSPlugin.pm > @@ -87,6 +87,8 @@ sub options { > content =3D> { optional =3D> 1 }, > format =3D> { optional =3D> 1 }, > mkdir =3D> { optional =3D> 1 }, > + create_path =3D> { optional =3D> 1 }, > + populate_directory =3D> { optional =3D> 1 }, > bwlimit =3D> { optional =3D> 1 }, > }; > } > @@ -129,7 +131,7 @@ sub activate_storage { > =20 > if (!nfs_is_mounted($server, $export, $path, $cache->{mountdata})) { > # NOTE: only call mkpath when not mounted (avoid hang when NFS server i= s offline > - mkpath $path if !(defined($scfg->{mkdir}) && !$scfg->{mkdir}); > + mkpath $path if $class->should_create_path($scfg); > =20 > die "unable to activate storage '$storeid' - " . > "directory '$path' does not exist\n" if ! -d $path; > diff --git a/PVE/Storage/Plugin.pm b/PVE/Storage/Plugin.pm > index 57c58a9..c77acb4 100644 > --- a/PVE/Storage/Plugin.pm > +++ b/PVE/Storage/Plugin.pm > @@ -1158,7 +1158,13 @@ sub activate_storage { > "directory '$path' does not exist or is unreachable\n"; > } > =20 > + warn "'mkdir' option is deprecated. Use 'create_path' or 'populate_d= irectory' instead.\n" > + if defined($scfg->{mkdir}); this warning here would benefit from having '$storeid: ' as prefix. > =20 > + return if defined($scfg->{populate_directory}) > + && !$scfg->{populate_directory}; > + > + # FIXME The mkdir option is deprecated and populate_directory should= be used > return if defined($scfg->{mkdir}) && !$scfg->{mkdir}; > =20 > if (defined($scfg->{content})) { > @@ -1454,4 +1460,20 @@ sub volume_import_formats { > return (); > } > =20 > +sub should_create_path { > + my ($class, $scfg) =3D @_; > + > + # FIXME the mkdir parameter is deprecated and create_path should be = used > + my $mkpath =3D 0; > + if (!defined($scfg->{create_path}) && !defined($scfg->{mkdir})) { > + $mkpath =3D 1; > + } elsif (defined($scfg->{create_path})) { > + $mkpath =3D 1 if $scfg->{create_path}; > + } else { > + $mkpath =3D 1 if $scfg->{mkdir}; > + } > + > + return $mkpath; > +} > + > 1; > --=20 > 2.20.1 >=20 >=20 >=20 > _______________________________________________ > pve-devel mailing list > pve-devel@lists.proxmox.com > https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel >=20 >=20 >=20 =