On 1/16/20 3:57 PM, Oguz Bektas wrote:
> Signed-off-by: Oguz Bektas <o.bek...@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);
>
_______________________________________________
pve-devel mailing list
pve-devel@pve.proxmox.com
https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
