From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <t.lamprecht@proxmox.com>
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))
(No client certificate requested)
by lists.proxmox.com (Postfix) with ESMTPS id 3490D7D2DD
for <pve-devel@lists.proxmox.com>; Mon, 8 Nov 2021 16:53:37 +0100 (CET)
Received: from firstgate.proxmox.com (localhost [127.0.0.1])
by firstgate.proxmox.com (Proxmox) with ESMTP id D5C202889A
for <pve-devel@lists.proxmox.com>; Mon, 8 Nov 2021 16:53:36 +0100 (CET)
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))
(No client certificate requested)
by firstgate.proxmox.com (Proxmox) with ESMTPS id A02762888D
for <pve-devel@lists.proxmox.com>; Mon, 8 Nov 2021 16:53:35 +0100 (CET)
Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1])
by proxmox-new.maurer-it.com (Proxmox) with ESMTP id 7313D45E06
for <pve-devel@lists.proxmox.com>; Mon, 8 Nov 2021 16:53:35 +0100 (CET)
Message-ID: <96a51277-818a-43d6-0849-a311d4b3c9f3@proxmox.com>
Date: Mon, 8 Nov 2021 16:53:34 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:95.0) Gecko/20100101
Thunderbird/95.0
Content-Language: en-US
To: Proxmox VE development discussion <pve-devel@lists.proxmox.com>,
=?UTF-8?Q?Fabian_Gr=c3=bcnbichler?= <f.gruenbichler@proxmox.com>
References: <20211102150335.2371545-1-a.lauterer@proxmox.com>
<20211102150335.2371545-2-a.lauterer@proxmox.com>
<1636384211.m34yji3cck.astroid@nora.none>
From: Thomas Lamprecht <t.lamprecht@proxmox.com>
In-Reply-To: <1636384211.m34yji3cck.astroid@nora.none>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable
X-SPAM-LEVEL: Spam detection results: 0
AWL 1.176 Adjusted score from AWL reputation of From: address
BAYES_00 -1.9 Bayes spam probability is 0 to 1%
KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment
NICE_REPLY_A -2.039 Looks like a legit reply (A)
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 v4 storage 1/9] storage: add new
find_free_volname
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>
X-List-Received-Date: Mon, 08 Nov 2021 15:53:37 -0000
On 08.11.21 16:21, Fabian Gr=C3=BCnbichler wrote:
> On November 2, 2021 4:03 pm, Aaron Lauterer wrote:
>> This new method exposes the functionality to request a new, not yet
>> used, volname for a storage.
>>
>> The default implementation will return the result from
>> 'find_free_diskname' prefixed with "<VMID>/" if $scfg->{path} exists.
>> Otherwise it will only return the result from 'find_free_diskname'.
>>
>> If the format suffix is added also depends on the existence of
>> $scfg->{path}.
>>
>> $scfg->{path} will be present for directory based storage types.
>>
>> Should a storage need to return different volname, it needs to overrid=
e
>> the 'find_free_volname' method.
>>
>> Signed-off-by: Aaron Lauterer <a.lauterer@proxmox.com>
>=20
> as discussed off-list (I am sorry I didn't read through the full=20
> discussion on-list of all 10 versions of this and the previous series=20
> ;)) - IMHO this is not a backwards compatible addition since it's not=20
> guarded by any feature flag, so external plugins fall into two=20
> categories:
> - derived from Plugin.pm, so automatically implement it via=20
> find_free_diskname -> get_next_vm_diskname, which might or might not =
> be correct for that plugin
> - not derived from Plugin.pm, don't implement it, no safeguard to handl=
e=20
> that
>=20
> RIGHT NOW, the only callers are guarded by the rename feature so there =
> can't be any problems in practice - but there is no guarantee that this=
=20
> will be remembered down the line, and it also breaks the=20
> modularization/layering of the code to rely on this fact ;)
>=20
> while we could argue about whether we really want to be that strict,=20
> another point against exposing this method for me is that it's also=20
> prone to mis-use, since it encourages code patterns like in this=20
> series with:
>=20
> get_free_volname
> do_something_that_allocates_that_volname
>=20
> without holding a storage lock over both calls, since the storage lock =
> is only available on the plugin level and not exposed in PVE::Storage=20
> itself.
for the record, that's not a problem in the sense of consistency or the l=
ike,
the alloc is the important thing and that is locked, it can be a nuisance=
as
one can gets their rename rejected, but that's not uncommon in our API in=
general.
IIRC my idea was that the user (has to) get a free name proposed (i.e., t=
he
client can request a free name for $owner and submits that, so that they =
are
in the loop about the new name already, if the API call returns OK then a=
ll
worked out, else, well not -> check error.
That way we avoid that the user has no idea about what the new volid will=
be
and they won't have to parse some (task) logs or guess around..
>=20
> an interface like
> $plugin->rename_volume($scfg, $storeid, $source, $new_owner, $new_name)=
>=20
> with at least $new_owner or $new_name set would also cover both cases=20
> (reassign with just $new_owner, rename with just $new_name) and can be =
> feature-guarded while keeping the locking in PVE::Storage for the full =
> operation including finding a free slot if needed.
>=20
> alternatively, just implementing
>=20
> $plugin->reassign_volume($scfg, $storeid, $source, $new_owner)
>=20
> for now (guarded by a 'reassign' feature), and postponing the rename=20
> feature for a future series that handles 'custom suffix/volname'=20
> across the board of our stack would also be an option.
>=20