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

[Thomas Lamprecht <t.lamprecht@proxmox.com>]

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.

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.

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


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