[pve-devel] [PATCH storage] volume import: assume target API version is at least 9

[Fiona Ebner]

Am 04.07.24 um 13:51 schrieb Thomas Lamprecht:
> Am 04/07/2024 um 12:28 schrieb Fiona Ebner:
>> Am 04.07.24 um 11:52 schrieb Thomas Lamprecht:
>>> Am 10/06/2024 um 11:04 schrieb Fiona Ebner:
>>>> The storage API version has been bumped to at least 9 since
>>>> libpve-storage = 7.0-4. If the source node is on Proxmox VE 8, where
>>>> this change will come in, then the target node can be assumed to be
>>>> running either Proxmox VE 8 or, during upgrade, the latest version of
>>>> Proxmox VE 7.4, so it's safe to assume a storage API version of at
>>>> least 9 in all cases.
>>>
>>> it's fine by me that this was applied, but can we somehow assert this
>>> assumption with an early `die if $apiver < 7` ? (maybe don't die if the
>>> apiver could not be queried/parsed, i.e. is undef, if there are
>>> legitimate situations where this can happen).
>>>
>>
>> Why version 7? We'd need to distinguish between there not being an
> 
> I meant 9, just a brain fart from PVE version 7 vs API version 9.
> 
>> apiinfo API call and other errors. The previous code did just continue
>> if not being able to query (punting version to 1) and that lead to the
>> very issue reported by Maximiliano.
> 
> Rethinking this, your fix is mostly side-stepping the actual problem,
> and it can only afford to do so due to your assumption taken, but once
> one needs to bump the API version next they either just reintroduce the
> problem or are forced to actually fix it, both not nice.
> 

Next time we introduce a feature that depends on the target's api
version we need to add back code to query it. But it's not clear that
will be storage_migrate(), so I'd rather add it where it's required once
it's required.

> If I understand your commit message right, the actual problem was
> transient errors when querying the API version, I assume that because
> reading:
> 
>> [..] where an SSH connection could not always be established, because
>> the target's API version would fall back to 1.
> 
> As that sounds like the establishing fails because the API version falls
> back to 1, not that the API version falls back to 1 due to the former.
> 
> If my interpretation is correct then why not repeat those then a few
> times and do a hard-fail if it still cannot be queried – if that fails
> often there's probably a different underlying issue causing that.
> 
> I.e., I'd drop the fallback to 1, as that's safe to assume that all
> supported versions have that version call now, so the fallback is not
> required anymore, not the checks.
> 

Yes, next time we introduce an apiinfo call, we can just have it fail
hard upon errors.

>>> While just one major release difference might be seen as enough, we still
>>> have users that are doing some more funky stuff on upgrades of clusters,
>>> so if it's somewhat easy to assert the assumption, doing so can
>>> definitively only help to improve UX.
>>
>> Well, we'd have to put back in the apiinfo call for that. If using
>> version 9 for the check instead of 7, the only thing it would do is die
>> early when replicating back from an upgraded node to a node with
>> libpve-storage < 7.0-4
>>
>> For offline guest disk migration, the base_snapshot check doesn't matter
>> so the only thing the check would catch is migrating back to a node with
>> api version < 5 which means libpve-storage-perl < 6.1-6.
>>
>> I'd rather have the code cleaner than very slightly improve UX for users
>> running ancient versions.
> 
> How is your code cleaner?

There is no apiinfo call required anymore. No code is the cleanest kind
of code IMHO.

> Besides that: no, I definitively place UX over some abstract "code cleanliness"
> criteria – if, it can be fair to set the barrier such that one should achieve
> both, but putting UX below code tidiness is IMO just not acceptable.

I do put UX for users running ancient versions below code cleanliness,
but not UX for current versions of course.