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. _______________________________________________ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
