[pve-devel] [PATCH qemu-server] hotplug_pending: allow hotplugging of 'fstrim_cloned_disks'

[Thomas Lamprecht]

On 1/16/20 3:57 PM, Oguz Bektas wrote:
> Signed-off-by: Oguz Bektas <o.bektas at proxmox.com>
> ---
> 
> some thoughts about this, based on our offline discussion with dominik.
> 
> this patch currently allows us to hotplug the fstrim option for agent,
> when it's the only change.
> however if the user changes both the type and this option for example,
> it will not hotplug the option. this is the same behavior/semantic we
> have with the other options like for disks (where you can hotplug some
> options like backup etc. but they won't hotplug if you change
> non-hotpluggable things at the same time).

that's a bit of a bummer

> 
> one way of handling this in a generic way (for reuse) would be to have a
> schema for each property string, to define the hotpluggability (or other
> traits) of each substring/option.

funnily I did not read this, straight went out and tried it and though roughly
about the same.

> 
> so for now i decided to leave it like this since it behaves similar to
> our other options, but we can discuss some solutions

we do? ^^ Let us rather try to do it the right™ wait now :)

This would've been fixed with:
----8<----
diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index 1d01a68..1197be1 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -5032,10 +5032,15 @@ sub vmconfig_hotplug_pending {
                vmconfig_update_disk($storecfg, $conf, $hotplug_features->{disk},
                                     $vmid, $opt, $value, 1, $arch, $machine_type);
            } elsif ($opt eq 'agent') {
-               my $old_agent = parse_guest_agent($conf);
-               my $new_agent = parse_guest_agent($conf->{pending});
-               if ($old_agent->{enabled} ne $new_agent->{enabled} ||
-                   $old_agent->{type} ne $new_agent->{type}) {
+               my $old = parse_guest_agent($conf);
+               my $new = parse_guest_agent($conf->{pending});
+               if ($old->{enabled} ne $new->{enabled} ||
+                   $old->{type} ne $new->{type}) {
+
+                   if ($old->{fstrim_cloned_disks} != $new->{fstrim_cloned_disks}) {
+                       $old->{fstrim_cloned_disks} = $new->{fstrim_cloned_disks};
+                       $conf->{$opt} = PVE::JSONSchema::print_property_string($old, $agent_fmt);
+                   }
                    die "skip\n"; # can't hotplug-enable agent or change type
                }
            } elsif ($opt =~ m/^memory$/) { #dimms
---

You may've guessed that the variable renaming was to make it more genreral.
I'd create a $partial_fast_plug_option map with 
{
    opt1 => {
        fmt => $fmt_ref,
        properties => [ 'hotpluggable_fmtstr_subkey1', 'hotpluggable_fmtstr_subkey2', ... ]
    },
    opt2 => {
        ....
    },
   ...
};

A small helper method then can do the actual parse old/new format string, check if
* only hotpluggable keys changed -> fully apply (just early return, let the common
  code path handle it)
* a mix of those falue changed, apply hotpluggable one to the old has, print_property_string
  it to $conf->{opt}, die "skip"

Shouldn't be to hard, may want to see if you want to pass a comparator for old new (or have
it in the map? then a array won't cut it) or have a general one which compares by using the
schema info, i.e., checks if string, boolean, integer and does the respective comparission, 
i.e., a safe_ne on steroids - latter would probably be nicer to may be a bit complexer if
complex edge cases need to be handled, you'll see once you try it.

> 
>  PVE/QemuServer.pm | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
> index 9ef3b71..c2547d6 100644
> --- a/PVE/QemuServer.pm
> +++ b/PVE/QemuServer.pm
> @@ -5031,6 +5031,13 @@ sub vmconfig_hotplug_pending {
>  		}
>  		vmconfig_update_disk($storecfg, $conf, $hotplug_features->{disk},
>  				     $vmid, $opt, $value, 1, $arch, $machine_type);
> +	    } elsif ($opt eq 'agent') {
> +		my $old_agent = parse_guest_agent($conf);
> +		my $new_agent = parse_guest_agent($conf->{pending});
> +		if ($old_agent->{enabled} ne $new_agent->{enabled} ||
> +		    $old_agent->{type} ne $new_agent->{type}) {
> +		    die "skip\n"; # can't hotplug-enable agent or change type
> +		}
>  	    } elsif ($opt =~ m/^memory$/) { #dimms
>  		die "skip\n" if !$hotplug_features->{memory};
>  		$value = PVE::QemuServer::Memory::qemu_memory_hotplug($vmid, $conf, $defaults, $opt, $value);
>