Re: [pve-devel] [PATCH pve-storage v2 5/5] vdisk_alloc: add optional assertion for volume's content type

[Fiona Ebner <f.ebner@proxmox.com>]

Am 20.02.25 um 11:40 schrieb Fabian Grünbichler:
> 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?

Sounds good to me. I'm in favor of making it required already, since we
already need versioned breaks for the series.

> 
> 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