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

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

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

As reported by Maximiliano, the fact that the 'apiinfo' call was
guarded with a quiet eval could lead to strange errors for replication
on a customer system where an SSH connection could not always be
established, because the target's API version would fall back to 1.
Because of that, the '-base' argument would be missing for the import
call on the target which would in turn lead to an error about the
target ZFS volume already existing (rather than doing an incremental
sync).

Reported-by: Maximiliano Sandoval <m.sandoval@proxmox.com>
Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
 src/PVE/Storage.pm | 18 ++++++------------
 1 file changed, 6 insertions(+), 12 deletions(-)

diff --git a/src/PVE/Storage.pm b/src/PVE/Storage.pm
index f19a115..bee2361 100755
--- a/src/PVE/Storage.pm
+++ b/src/PVE/Storage.pm
@@ -709,7 +709,7 @@ sub storage_migrate_snapshot {
 }
 
 my $volume_import_prepare = sub {
-    my ($volid, $format, $path, $apiver, $opts) = @_;
+    my ($volid, $format, $path, $opts) = @_;
 
     my $base_snapshot = $opts->{base_snapshot};
     my $snapshot = $opts->{snapshot};
@@ -724,11 +724,11 @@ my $volume_import_prepare = sub {
     if ($migration_snapshot) {
 	push @$recv, '-delete-snapshot', $snapshot;
     }
-    push @$recv, '-allow-rename', $allow_rename if $apiver >= 5;
+    push @$recv, '-allow-rename', $allow_rename;
 
     if (defined($base_snapshot)) {
 	# Check if the snapshot exists on the remote side:
-	push @$recv, '-base', $base_snapshot if $apiver >= 9;
+	push @$recv, '-base', $base_snapshot;
     }
 
     return $recv;
@@ -814,12 +814,7 @@ sub storage_migrate {
 	$import_fn = "tcp://$net";
     }
 
-    my $target_apiver = 1; # if there is no apiinfo call, assume 1
-    my $get_api_version = [@$ssh, 'pvesm', 'apiinfo'];
-    my $match_api_version = sub { $target_apiver = $1 if $_[0] =~ m!^APIVER (\d+)$!; };
-    eval { run_command($get_api_version, logfunc => $match_api_version); };
-
-    my $recv = [ @$ssh, '--', $volume_import_prepare->($target_volid, $format, $import_fn, $target_apiver, $opts)->@* ];
+    my $recv = [ @$ssh, '--', $volume_import_prepare->($target_volid, $format, $import_fn, $opts)->@* ];
 
     my $new_volid;
     my $pattern = volume_imported_message(undef, 1);
@@ -911,8 +906,7 @@ sub storage_migrate {
 	    run_command($cmds, logfunc => $match_volid_and_log);
 	}
 
-	die "unable to get ID of the migrated volume\n"
-	    if !defined($new_volid) && $target_apiver >= 5;
+	die "unable to get ID of the migrated volume\n" if !defined($new_volid);
     };
     my $err = $@;
     if ($opts->{migration_snapshot}) {
@@ -1961,7 +1955,7 @@ sub volume_import_start {
     my $info = IO::File->new();
 
     my $unix = $opts->{unix} // "/run/pve/storage-migrate-$vmid.$$.unix";
-    my $import = $volume_import_prepare->($volid, $format, "unix://$unix", APIVER, $opts);
+    my $import = $volume_import_prepare->($volid, $format, "unix://$unix", $opts);
 
     unlink $unix;
     my $cpid = open3($input, $info, $info, @$import)
-- 
2.39.2



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

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

[Max Carrara]

On Mon Jun 10, 2024 at 11:04 AM CEST, Fiona Ebner wrote:
> 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.
>
> As reported by Maximiliano, the fact that the 'apiinfo' call was
> guarded with a quiet eval could lead to strange errors for replication
> on a customer system where an SSH connection could not always be
> established, because the target's API version would fall back to 1.
> Because of that, the '-base' argument would be missing for the import
> call on the target which would in turn lead to an error about the
> target ZFS volume already existing (rather than doing an incremental
> sync).
>
> Reported-by: Maximiliano Sandoval <m.sandoval@proxmox.com>
> Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>

Wasn't able to test this at the moment yet (please holler at me if you
need me to), but these changes are pretty straightforward anyway.

I completely agree with all of these changes; LGTM.

Reviewed-by: Max Carrara <m.carrara@proxmox.com>

> ---
>  src/PVE/Storage.pm | 18 ++++++------------
>  1 file changed, 6 insertions(+), 12 deletions(-)
>
> diff --git a/src/PVE/Storage.pm b/src/PVE/Storage.pm
> index f19a115..bee2361 100755
> --- a/src/PVE/Storage.pm
> +++ b/src/PVE/Storage.pm
> @@ -709,7 +709,7 @@ sub storage_migrate_snapshot {
>  }
>  
>  my $volume_import_prepare = sub {
> -    my ($volid, $format, $path, $apiver, $opts) = @_;
> +    my ($volid, $format, $path, $opts) = @_;
>  
>      my $base_snapshot = $opts->{base_snapshot};
>      my $snapshot = $opts->{snapshot};
> @@ -724,11 +724,11 @@ my $volume_import_prepare = sub {
>      if ($migration_snapshot) {
>  	push @$recv, '-delete-snapshot', $snapshot;
>      }
> -    push @$recv, '-allow-rename', $allow_rename if $apiver >= 5;
> +    push @$recv, '-allow-rename', $allow_rename;
>  
>      if (defined($base_snapshot)) {
>  	# Check if the snapshot exists on the remote side:
> -	push @$recv, '-base', $base_snapshot if $apiver >= 9;
> +	push @$recv, '-base', $base_snapshot;
>      }
>  
>      return $recv;
> @@ -814,12 +814,7 @@ sub storage_migrate {
>  	$import_fn = "tcp://$net";
>      }
>  
> -    my $target_apiver = 1; # if there is no apiinfo call, assume 1
> -    my $get_api_version = [@$ssh, 'pvesm', 'apiinfo'];
> -    my $match_api_version = sub { $target_apiver = $1 if $_[0] =~ m!^APIVER (\d+)$!; };
> -    eval { run_command($get_api_version, logfunc => $match_api_version); };
> -
> -    my $recv = [ @$ssh, '--', $volume_import_prepare->($target_volid, $format, $import_fn, $target_apiver, $opts)->@* ];
> +    my $recv = [ @$ssh, '--', $volume_import_prepare->($target_volid, $format, $import_fn, $opts)->@* ];
>  
>      my $new_volid;
>      my $pattern = volume_imported_message(undef, 1);
> @@ -911,8 +906,7 @@ sub storage_migrate {
>  	    run_command($cmds, logfunc => $match_volid_and_log);
>  	}
>  
> -	die "unable to get ID of the migrated volume\n"
> -	    if !defined($new_volid) && $target_apiver >= 5;
> +	die "unable to get ID of the migrated volume\n" if !defined($new_volid);
>      };
>      my $err = $@;
>      if ($opts->{migration_snapshot}) {
> @@ -1961,7 +1955,7 @@ sub volume_import_start {
>      my $info = IO::File->new();
>  
>      my $unix = $opts->{unix} // "/run/pve/storage-migrate-$vmid.$$.unix";
> -    my $import = $volume_import_prepare->($volid, $format, "unix://$unix", APIVER, $opts);
> +    my $import = $volume_import_prepare->($volid, $format, "unix://$unix", $opts);
>  
>      unlink $unix;
>      my $cpid = open3($input, $info, $info, @$import)



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

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

[Fiona Ebner]

Am 10.06.24 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.
> 
> As reported by Maximiliano, the fact that the 'apiinfo' call was
> guarded with a quiet eval could lead to strange errors for replication
> on a customer system where an SSH connection could not always be
> established, because the target's API version would fall back to 1.
> Because of that, the '-base' argument would be missing for the import
> call on the target which would in turn lead to an error about the
> target ZFS volume already existing (rather than doing an incremental
> sync).
> 
> Reported-by: Maximiliano Sandoval <m.sandoval@proxmox.com>
> Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>

applied, with Max's R-b, thanks for that!


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

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

[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).

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.


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

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

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

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


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

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

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

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

Re: [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.


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

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

[Thomas Lamprecht]

Am 04/07/2024 um 14:11 schrieb Fiona Ebner:
> There is no apiinfo call required anymore. No code is the cleanest kind

Yeah, by the assumption you self choose to use and that I question, so
not really a useful argument.

In practice, users can upgrade a from one major release to the next one,
nothing is really blocking them w.r.t apt, that cannot be said for jumping
releases, so one should definitively have different levels of standard for
"any X to any X + 1" compared to "X to X + >= 2".

What we recommend users (run latest 7 before upgrade) is not necessarily
the exact same boundary of what we should hedge against to reduce negative
feelings of users and resulting work/soothing our support has to do as
a result; I'd think of it more like the minimum and recommended system
requirements.

> of code IMHO.

Less code can be nicer as long as all features and edge cases are still
handled properly and also still easy to read, not code golf minimalism.

Or, to exaggerate for the points' sake, should I just delete all our
(user) error code handling and tell any users complaining that they just
need to do it right the next time? While that would result in quite
a few lines less, it definitively won't help our popularity.

>> 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 7 is still supported, so definitively not ancient! We normally put
in quite a bit of work to ensure that systems talking with other
incompatible versions get a good error, why bother adding versioning
else in the first place? And while there's definitively areas that can
still be improved, that's no argument for going backwards again.

I mean this specific case here might be relatively harmless in effect,
but if you really apply this philosophy to all development here then I
wish you good luck into explaining angry users and our support agents
that had to answer them, why it was good for them to remove some simple
check for code cleanliness, because that should not be relevant if
the users did everything 100% correct. We already need to justify
us too much about not being hacky as a FLOSS solution, after having
to use the shell occasionally any missing/incomplete error handling is
the next big reason for people to complain.
And sure, most alternative (proprietary) solutions are far from being
better, and while it's annoying that some people are hypocrites here,
I do not have anything against being held to a high(er) standard,
as one can really stick out with good UX, which always consists of tons
of little things.

Anyhow, as mentioned, this might not fall back on us here, but I hope
I could convince to not use that philosophy unconditionally, and I
wished for some more background explanation on this in the commit message.

I just cannot think that there could have any negative effect, be it in
using nor maintaining that code for neither users nor developers, if we
just fixed the actual (error handling) behavior of querying the API
version instead, and possibly dropped the real old version checks, i.e.
those not just a single major version apart, in a separate patch.


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

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

[Fiona Ebner]

Am 04.07.24 um 19:45 schrieb Thomas Lamprecht:
> Am 04/07/2024 um 14:11 schrieb Fiona Ebner:
>> There is no apiinfo call required anymore. No code is the cleanest kind
> 
> Yeah, by the assumption you self choose to use and that I question, so
> not really a useful argument.
> 
> In practice, users can upgrade a from one major release to the next one,
> nothing is really blocking them w.r.t apt, that cannot be said for jumping
> releases, so one should definitively have different levels of standard for
> "any X to any X + 1" compared to "X to X + >= 2".
> 
> What we recommend users (run latest 7 before upgrade) is not necessarily
> the exact same boundary of what we should hedge against to reduce negative
> feelings of users and resulting work/soothing our support has to do as
> a result; I'd think of it more like the minimum and recommended system
> requirements.
> 

Okay, I understand.

>> of code IMHO.
> 
> Less code can be nicer as long as all features and edge cases are still
> handled properly and also still easy to read, not code golf minimalism.
> 
> Or, to exaggerate for the points' sake, should I just delete all our
> (user) error code handling and tell any users complaining that they just
> need to do it right the next time? While that would result in quite
> a few lines less, it definitively won't help our popularity.
> 

Of course I'm not arguing for such a thing. Being accused of arguing for
that hurts :(, especially when coming from somebody I highly respect. I
honestly did not know that we consider having a node with PVE 7.0 while
upgrading to PVE 8 supported.

>>> 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 7 is still supported, so definitively not ancient! We normally put
> in quite a bit of work to ensure that systems talking with other
> incompatible versions get a good error, why bother adding versioning
> else in the first place? And while there's definitively areas that can
> still be improved, that's no argument for going backwards again.
> 

Again, it's not all of PVE 7, just an early subset of it that's about
three years without updates. If it weren't that, I wouldn't have it
considered safe to remove the call. But okay, I'm fine with being more
strict.

> I mean this specific case here might be relatively harmless in effect,
> but if you really apply this philosophy to all development here then I
> wish you good luck into explaining angry users and our support agents
> that had to answer them, why it was good for them to remove some simple
> check for code cleanliness, because that should not be relevant if
> the users did everything 100% correct. We already need to justify
> us too much about not being hacky as a FLOSS solution, after having
> to use the shell occasionally any missing/incomplete error handling is
> the next big reason for people to complain.
> And sure, most alternative (proprietary) solutions are far from being
> better, and while it's annoying that some people are hypocrites here,
> I do not have anything against being held to a high(er) standard,
> as one can really stick out with good UX, which always consists of tons
> of little things.
> 
> Anyhow, as mentioned, this might not fall back on us here, but I hope
> I could convince to not use that philosophy unconditionally, and I
> wished for some more background explanation on this in the commit message.
> 
> I just cannot think that there could have any negative effect, be it in
> using nor maintaining that code for neither users nor developers, if we
> just fixed the actual (error handling) behavior of querying the API
> version instead, and possibly dropped the real old version checks, i.e.
> those not just a single major version apart, in a separate patch.

Let's just revert it then and drop the eval and the fallback. Fine by
me. Or actually, a fresh install of PVE 7.0 comes with
libpve-storage-perl = 7.0-7 (respectively 7.0-10 for the second release
of the ISO). The API version was bumped to 9 in 7.0-4, so actually the
subset of PVE 7 that's affected is empty.


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

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

[Thomas Lamprecht]

For the record, we talked about this in person for a bit with the following
outcome:

- there was a bit of a misunderstanding w.r.t. my heavy exaggeration for the
  point's sake, I really did not mean that as accusation at all, but that's
  now talked out

- it's a good point that the package that was included in the first 7.0 ISO
  release was already new enough API version wise, and we also checked the
  package archives, and there we also got only new enough libpve-storage-perl
  package versions, so we can keep the change as is.
  We also agreed that it would be great to mention such analysis in the commit
  message the next time, and I know Fiona is very thorough with that stuff, and
  that this time it was just not mentioned due to the difference in how upgrade
  requirements and recommendations got interpreted by her and me, so I mention
  this mostly for other readers, as this applies to all of us.

- we might want to document this expectation w.r.t API backward compat more
  definitively in a more approachable way, but should ensure that its clarified
  that this is for developers, not users, to avoid users thinking its always
  fine to upgrade from an outdated point release to a newer major release.

tl;dr: got sorted out and change can be kept as is


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

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

[Thomas Lamprecht]

Am 04/07/2024 um 14:11 schrieb Fiona Ebner:
> Yes, next time we introduce an apiinfo call, we can just have it fail
> hard upon errors.

Oh, and just to avoid potential future error potential here:
For a new topic-specific API version call that might not work, as the fallback
and (lacking) error handling here was added explicitly for backward
<compatibility.

Also, for a future topic-API version info call a new central API endpoint
would be preferable. That we could more easily expand for various different
internal API versioning and have central tooling to query it correctly,
which would be simpler too as one can nicely differentiate auth/network/..
errors from an actual application level one, like 404 not found.


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