[pve-devel] [PATCH v2 qemu-server 5/5] hotplug_pending: allow partial fast plugging

[Oguz Bektas]

hi,


On Tue, Mar 10, 2020 at 11:25:23AM +0100, Thomas Lamprecht wrote:
> On 2/19/20 5:07 PM, Oguz Bektas wrote:
> > adds a loop after the main fastplug loop, to check if any of the options
> > are partially fast pluggable.
> > 
> > these are defined in $partial_fast_plug_option
> > 
> > Signed-off-by: Oguz Bektas <o.bektas at proxmox.com>
> > ---
> > 
> > v1->v2:
> > * set $changes according to partial_fast_plug result as well as fast_plug result
> > * do cleanup_pending before writing fastplug changes
> > 
> > 
> > 
> >  PVE/QemuConfig.pm |  7 +++++++
> >  PVE/QemuServer.pm | 19 +++++++++++++++++++
> >  2 files changed, 26 insertions(+)
> > 
> > diff --git a/PVE/QemuConfig.pm b/PVE/QemuConfig.pm
> > index 1ba728a..d1727b2 100644
> > --- a/PVE/QemuConfig.pm
> > +++ b/PVE/QemuConfig.pm
> > @@ -399,6 +399,13 @@ sub __snapshot_foreach_volume {
> >  
> >      PVE::QemuServer::foreach_drive($conf, $func);
> >  }
> > +
> > +sub get_partial_fast_plug_option {
> > +    my ($class) = @_;
> > +
> > +    return $PVE::QemuServer::partial_fast_plug_option;
> > +}
> > +
> >  # END implemented abstract methods from PVE::AbstractConfig
> >  
> >  1;
> > diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
> > index 44d0dee..8a689a0 100644
> > --- a/PVE/QemuServer.pm
> > +++ b/PVE/QemuServer.pm
> > @@ -4732,6 +4732,18 @@ my $fast_plug_option = {
> >      'tags' => 1,
> >  };
> >  
> > +# name of opt
> > +# -> fmt -> format variable
> > +# -> properties -> fastpluggable options hash
> > +our $partial_fast_plug_option = {
> > +    agent => {
> > +	fmt => $agent_fmt,
> > +	properties => {
> > +	    fstrim_cloned_disks => 1
> > +	},
> > +    },
> > +};
> 
> this belongs solely in the get_partial_fast_plug_option method, I do not want
> to tighten the cyclic use of both modules and it's not required here.

when we move the hash to QemuConfig it complains about $agent_fmt which
resides in QemuServer (as well as other formats which could be used
later on for partial fastplugging). that was the initial reason i added
it there because i didn't want to move the formats. should we change the
definitions from 'my' to 'our' to use them in QemuConfig? or how to
handle this better?

> 
> > +
> >  # hotplug changes in [PENDING]
> >  # $selection hash can be used to only apply specified options, for
> >  # example: { cores => 1 } (only apply changed 'cores')
> > @@ -4761,7 +4773,14 @@ sub vmconfig_hotplug_pending {
> >  	}
> >      }
> >  
> > +    foreach my $opt (keys %{$conf->{pending}}) {
> > +	if ($partial_fast_plug_option->{$opt}) {
> > +	    $changes ||= PVE::QemuConfig->partial_fast_plug($conf, $opt);
> > +	}
> > +    }
> 
> with my followup for the guest-common series where I early return with 0 (no
> change) if no partial_fast_plug_option is set for $opt you could just do:
> 
> for my $opt (keys %{$conf->{pending}}) {
>     $changes ||= PVE::QemuConfig->partial_fast_plug($conf, $opt);
> }
> 
> This avoids a split of logic between here and the guest common method, i.e.,
> untangles spaghetti code a bit :)

okay i'll do that
> 
> > +
> >      if ($changes) {
> > +	PVE::QemuConfig->cleanup_pending($conf);
> >  	PVE::QemuConfig->write_config($vmid, $conf);
> >      }
> >  
> > 
>