From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <pve-devel-bounces@lists.proxmox.com>
Received: from firstgate.proxmox.com (firstgate.proxmox.com [IPv6:2a01:7e0:0:424::9])
	by lore.proxmox.com (Postfix) with ESMTPS id E6CF31FF16B
	for <inbox@lore.proxmox.com>; Thu, 20 Feb 2025 11:41:26 +0100 (CET)
Received: from firstgate.proxmox.com (localhost [127.0.0.1])
	by firstgate.proxmox.com (Proxmox) with ESMTP id E44C6E98D;
	Thu, 20 Feb 2025 11:41:19 +0100 (CET)
MIME-Version: 1.0
In-Reply-To: <8d062f4c-cd2f-48ae-9aa0-c81cedfd5c84@proxmox.com>
References: <20250211160825.254167-1-d.kral@proxmox.com>
 <20250211160825.254167-6-d.kral@proxmox.com>
 <8d062f4c-cd2f-48ae-9aa0-c81cedfd5c84@proxmox.com>
From: Fabian =?utf-8?q?Gr=C3=BCnbichler?= <f.gruenbichler@proxmox.com>
To: Daniel Kral <d.kral@proxmox.com>, Fiona Ebner <f.ebner@proxmox.com>,
 Proxmox VE development discussion <pve-devel@lists.proxmox.com>
Date: Thu, 20 Feb 2025 11:40:41 +0100
Message-ID: <174004804172.338734.4519880313579913500@yuna.proxmox.com>
User-Agent: alot/0.10
X-SPAM-LEVEL: Spam detection results:  0
 AWL 0.045 Adjusted score from AWL reputation of From: address
 BAYES_00                 -1.9 Bayes spam probability is 0 to 1%
 DMARC_MISSING             0.1 Missing DMARC policy
 KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment
 RCVD_IN_VALIDITY_CERTIFIED_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to
 Validity was blocked. See
 https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more
 information.
 RCVD_IN_VALIDITY_RPBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to
 Validity was blocked. See
 https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more
 information.
 RCVD_IN_VALIDITY_SAFE_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to
 Validity was blocked. See
 https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more
 information.
 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 pve-storage v2 5/5] vdisk_alloc: add
 optional assertion for volume's content type
X-BeenThere: pve-devel@lists.proxmox.com
X-Mailman-Version: 2.1.29
Precedence: list
List-Id: Proxmox VE development discussion <pve-devel.lists.proxmox.com>
List-Unsubscribe: <https://lists.proxmox.com/cgi-bin/mailman/options/pve-devel>, 
 <mailto:pve-devel-request@lists.proxmox.com?subject=unsubscribe>
List-Archive: <http://lists.proxmox.com/pipermail/pve-devel/>
List-Post: <mailto:pve-devel@lists.proxmox.com>
List-Help: <mailto:pve-devel-request@lists.proxmox.com?subject=help>
List-Subscribe: <https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel>, 
 <mailto:pve-devel-request@lists.proxmox.com?subject=subscribe>
Reply-To: Proxmox VE development discussion <pve-devel@lists.proxmox.com>
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Errors-To: pve-devel-bounces@lists.proxmox.com
Sender: "pve-devel" <pve-devel-bounces@lists.proxmox.com>

Quoting Fiona Ebner (2025-02-20 10:03:09)
> Am 11.02.25 um 17:07 schrieb Daniel Kral:
> > Allow a caller to specify the volume's intended content type and assert
> > whether the specified content type may be stored on the specified
> > storage before allocating any volume.
> > 
> > Signed-off-by: Daniel Kral <d.kral@proxmox.com>
> > ---
> > changes since v1:
> > - add assertion at `vdisk_alloc` instead of wrapper `alloc_volume_disk`
> > 
> >  src/PVE/Storage.pm | 10 ++++++++++
> >  1 file changed, 10 insertions(+)
> > 
> > diff --git a/src/PVE/Storage.pm b/src/PVE/Storage.pm
> > index 3776565..96d4e41 100755
> > --- a/src/PVE/Storage.pm
> > +++ b/src/PVE/Storage.pm
> > @@ -1059,6 +1059,13 @@ Specifies the name of the new volume.
> >  
> >  If undefined, the name will be generated with C<PVE::Storage::Plugin::find_free_diskname>.
> >  
> > +=item * C<< $vtype => $string >>
> > +
> > +Specifies the content type of the new volume, which can be one of C<'images'>, C<'rootdir'>,
> > +C<'vztmpl'>, C<'iso'>, C<'backup'>, C<'snippets'> or C<'import'>.
> 
> Hmm, vdisk_alloc() is only for allocating guest and import images. So I
> think the other content types should be prohibited here (can still be
> extended later if it ever comes up).
> 
> We plan to better distinguish between 'rootdir' and 'images' in the
> future, so I think we should aim to make the $vtype parameter even
> required here. Not quite possible yet, because that would require
> breaking 'pvesm alloc', but we can postpone that part for PVE 9 and have
> all other callers already use it.
> 
> So my question is if we should rather make it a required parameter
> already rather than putting it into $opts? I mean while supporting
> passing in undef too, until we are ready to require it for 'pvesm alloc'
> too.
> 
> @Fabian sounds good to you too?

we could also make it required for real in vdisk_alloc, and make `pvesm alloc`
auto-select one of the allowed ones based on the storage config and error out
if the storage doesn't support either rootdir or images? or use a different
"magic" placeholder value like "any" - using undef is a bit opaque and could
happen by accident, although it is not very likely for this hash ;) then when
we introduce properly scoped volume names, we can replace that fallback logic
in `pvesm alloc` with properly setting *either* rootdir or images, depending on
what gets allocated?

OTOH, vdisk_alloc doesn't have too call sites anyway, so doing $opts now, and
then updating those when it becomes a regular argument would also work fine I
think.. the only downside to that approach is that we might miss setting the
option for newly introduce callers in the meantime..


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel