[pve-devel] [RFC qemu-server] qemu: allow partial fastplugging of property string options

[Oguz Bektas]

hi,

thanks for the review!

On Thu, Jan 23, 2020 at 07:41:11AM +0100, Thomas Lamprecht wrote:
> On 1/22/20 5:37 PM, Oguz Bektas wrote:
> > this patch adds the partial_fast_plug function, which allows to partially
> > fastplug an option with a property string.
> > 
> > this is done by having a map $partial_fast_plug_option, the format is commented.
> > 
> > other helper functions:
> > 
> > * safe_boolean_ne (!$a != !$b)
> > * typesafe_ne (combines safe_string_ne, safe_num_ne and safe_boolean_ne by
> > taking $type as an argument)
> > 
> > the qemu-guest-agent is our first use case for this (although i am sure
> > there are more, this is more of a proof of concept. it should be trivial
> > to add other things via the map), specifically the fstrim_cloned_disks
> > option.
> > 
> > Co-Authored-by: Stefan Reiter <s.reiter at proxmox.com>
> > Signed-off-by: Oguz Bektas <o.bektas at proxmox.com>
> > ---
> > 
> > i added stefan as a co-author for this, to thank for his help debugging and 
> > testing it with me
> 
> no thanks for me providing the initial hunk of code doing (almost) all
> of this ? ;P
> 
> > 
> > also please note that i'm sending this as RFC for review only, and it
> > shouldn't be applied yet since i'm working on generalizing it a bit more
> > for code reuse via pve-guest-common
> 
> As you only call that method in the agent case this is also not ready to
> be taken in here. Some other issues below.
> 
> 
> Testing changing the agent from a deactivated to a "change every setting" leads
> to an error with your series:
> > agent: hotplug problem - format error enabled: property is missing and it is not optional
> 
> That happens because $old is empty in that case but you try to parse it as format
> nonetheless.
will fix that, thanks

> 
> > 
> >  PVE/QemuServer.pm | 78 +++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 78 insertions(+)
> > 
> > diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
> > index bcdadca..74cbba0 100644
> > --- a/PVE/QemuServer.pm
> > +++ b/PVE/QemuServer.pm
> > @@ -5031,6 +5031,10 @@ sub vmconfig_hotplug_pending {
> >  		}
> >  		vmconfig_update_disk($storecfg, $conf, $hotplug_features->{disk},
> >  				     $vmid, $opt, $value, 1, $arch, $machine_type);
> > +	    } elsif ($opt eq 'agent') {
> > +		# partially fastpluggable
> 
> useless comment
okay
> 
> > +		# skip if all options were fastpluggable
> 
> No! You do the contrary, you skip if *not* all options were fastpluggable!
right. i'll change the comment
> 
> You could *always* skip if you deleted $cond->{pending}->{$opt} in partial_fast_plug
> if $no_changes there is false. If you want to do it like this and let the deletion
> from pending happen here, OK, but get the comments right - else this is confusing
> as hell.
i'd like to let the hotplug_pending path handle the deletion from
pending. i'll fix the comments in the new version

also based on our offline discussion this belongs in the else branch at
the end, i'll also take a look at that

> 
> > +		die "skip\n" if partial_fast_plug($conf, $opt);
> >  	    } elsif ($opt =~ m/^memory$/) { #dimms
> >  		die "skip\n" if !$hotplug_features->{memory};
> >  		$value = PVE::QemuServer::Memory::qemu_memory_hotplug($vmid, $conf, $defaults, $opt, $value);
> > @@ -5165,6 +5169,80 @@ my $safe_string_ne = sub {
> >      return $a ne $b;
> >  };
> >  
> > +my $safe_boolean_ne = sub {
> > +    my ($a, $b) = @_;
> > +
> > +    # we don't check if value is defined, since undefined
> > +    # is false (so it's a valid boolean)
> 
> The comment below is enough, above adds just noise - it's a common enough
> pattern, IMO.
okay
> 
> > +    # negate both values to normalize and compare
> > +    return !$a != !$b;
> > +};
> > +
> > +my $typesafe_ne = sub {
> > +    my ($a, $b, $type) = @_;
> > +
> > +    return 0 if !defined($a) && !defined($b);
> > +    return 1 if !defined($a);
> > +    return 1 if !defined($b);
> > +
> > +    if ($type eq 'string') {
> > +	$safe_string_ne->($a, $b);
> 
> 1. The returns are still missing, as told off-list. missing those works by chance only
>    and is very prone for subtle breakage.
right, i forgot about those, i'll change that as well
> 
> 2. if you move this to guest-common do the check directly here, no point in calling (and
>    then also moving that) method for a single line. I.e., just do here:
i thought i'd move all of it to guest-common to have both guest types
reuse (to avoid possible future inconsistencies between
implementations).
right now we have them duplicated in both
> 
> return $a ne $b;
> 
> > +    } elsif ($type eq 'number') {
> > +	$safe_num_ne->($a, $b);
> 
> return $a != $b;
> 
> > +    } elsif ($type eq 'boolean') {
> > +	$safe_boolean_ne->($a, $b);
> 
> return !$a != !$b;
> 
> directly.
> 
> > +    }
> > +};
> > +
> > +my $partial_fast_plug_option =
> > +# name
> > +# -> fmt -> format variable
> > +# -> properties -> fastpluggable options hash
> 
> move above variable def.
> 
> > +{
> 
> this belongs at the line of variable def
okay
> 
> > +    agent => {
> > +	fmt => $agent_fmt,
> 
> Maybe we could/should really get the format from the $confdesc and simplify this
> to
> my $partial_fast_plug_option = {
>     agent => {
>         fstrim_cloned_disks => 1
>     },
> };

yeah that also makes sense, as long as there's no $format outside of
$confdesc. i'll check that and change it accordingly

> > +	properties => {
> > +	    fstrim_cloned_disks => 1
> > +	},
> > +    },
> > +};
> > +
> > +sub partial_fast_plug {
> > +    my ($conf, $opt) = @_;
> > +
> > +    my $format = $partial_fast_plug_option->{$opt}->{fmt};
> > +    my $properties = $partial_fast_plug_option->{$opt}->{properties};
> 
> if you would call this $fast_pluggable instead of this over general name
> you could avoid that extra $is_fastpluggable variable below.
okay
> 
> > +
> > +    my $old = PVE::JSONSchema::parse_property_string($format, $conf->{$opt});
> 
> my $old = {};
> if (exists($conf->{$opt})) {
>     $old = PVE::JSONSchema::parse_property_string($format, $conf->{$opt});
> }
> 
> Else you throw up if $conf->{$opt} was not set previously.
indeed.
> 
> > +    my $new = PVE::JSONSchema::parse_property_string($format, $conf->{pending}->{$opt});
> 
> above should be OK as the pending $opt has to be always set, else this mustn't
> be called.
> 
> > +
> > +    my $changes_left = 0;
> > +
> > +    # merge old and new opts to iterate
> > +    my $all_opts = dclone($old);
> > +    foreach my $opt1 (keys %$new) {
> > +	$all_opts->{$opt1} = $new->{$opt1} if !defined($all_opts->{$opt1});
> > +    }
> 
> Instead of above 5 lines you could just do:
> 
> my @all_keys = keys %{{ %$new, %$old }};
> 
> (you only need the keys, not the data)
nice, i didn't know about that syntax
> 
> > +
> > +    foreach my $opt2 (keys %$all_opts) {
> 
> you can reuse variable names from for(each) in different scopes, no need
> to use $opt1, $opt2, ...
having $opt in all the loops made it look a bit confusing since $opt is also
in the function parameters, that's why i didn't reuse it. seemed easier
to read this way
if you insist i can change it though
> 
> > +	my $is_fastpluggable = $properties->{$opt2};
> > +	my $type = $format->{$opt2}->{type};
> > +	if ($typesafe_ne->($old->{$opt2}, $new->{$opt2}, $type)) {
> > +	    if (defined($is_fastpluggable)) {
> 
> why defined check? We control that definition, so normal "boolean" check is enough,
> with above suggested changes together just write:
> 
> if ($fast_pluggable->{$opt}) {
>     ...
> 

will take a look at that

> > +		$old->{$opt2} = $new->{$opt2};
> > +	    } else {
> > +		$changes_left = 1;
> > +	    }
> > +	}
> > +    }
> > +
> > +    $conf->{$opt} = PVE::JSONSchema::print_property_string($old, $format);
> > +
> > +    return $changes_left;
> > +}
> > +
> > +
> >  sub vmconfig_update_net {
> >      my ($storecfg, $conf, $hotplug, $vmid, $opt, $value, $arch, $machine_type) = @_;
> >  
> > 
>