[pve-devel] [RFC qemu-server 2/3] Integrate replica in the qemu migration.

[Thomas Lamprecht]

On 04/03/2017 04:53 PM, Wolfgang Link wrote:
 > Now it is possible to migrate a VM offline when replica is enabled.
 > It will reduce replication to an minimal amount.
 > ---
 >  PVE/QemuMigrate.pm | 34 +++++++++++++++++++++++++++++-----
 >  1 file changed, 29 insertions(+), 5 deletions(-)
 >
 > diff --git a/PVE/QemuMigrate.pm b/PVE/QemuMigrate.pm
 > index e6f147e..f4daaaf 100644
 > --- a/PVE/QemuMigrate.pm
 > +++ b/PVE/QemuMigrate.pm
 > @@ -10,6 +10,7 @@ use PVE::INotify;
 >  use PVE::Tools;
 >  use PVE::Cluster;
 >  use PVE::Storage;
 > +use PVE::ReplicaTools;
 >  use PVE::QemuServer;
 >  use Time::HiRes qw( usleep );
 >  use PVE::RPCEnvironment;
 > @@ -435,6 +436,15 @@ sub phase1 {
 >
 >      sync_disks($self, $vmid);
 >
 > +    # set new reptarget if necessary
 > +    if ($conf->{replica} =~ m/(?i:1|yes|on|true)/) {

check should not be necessary, $conf->{replica} should be bool here,
the CLI handler maps this active to 1 or 0 with the respective regex.

And PVE::JSONSchema::validate errors nowadays out if a boolean is something
else as 0 or 1, this validtaion gets made on every REST API handle entry
point, i.e. every API call.

 > +    if ($conf->{reptarget} eq $self->{node}) {
 > +        $conf->{reptarget} = $self->{opts}->{node};
 > +    }
 > +
 > +    PVE::ReplicaTools::job_remove($vmid);
 > +    PVE::QemuConfig->write_config($vmid, $conf);
 > +    }

I understand this, but silently & automatically writing in the config when
migrating to the current replica target and using the source node as
new replica seems a bit, magical. IMO, this needs at least an
documentation entry and maybe a hint for the user in the task log?


 >  };
 >
 >  sub phase1_cleanup {
 > @@ -842,13 +852,22 @@ sub phase3 {
 >      my $volids = $self->{volumes};
 >      return if $self->{phase2errors};
 >
 > +    my $rep = $self->{vmconf}->{replica} =~ m/(?i:1|yes|on|true)/;
 > +    my $sync_vol;
 > +    $sync_vol = 
PVE::ReplicaTools::get_syncable_guestdisks($self->{vmconf}, 'qemu')
 > +    if defined($rep);

Those three lines can be written as:

my $sync_vol = 
PVE::ReplicaTools::get_syncable_guestdisks($self->{vmconf}, 'qemu')
     if $self->{vmconf}->{replica};


 > +
 >      # destroy local copies
 >      foreach my $volid (@$volids) {
 > -    eval { PVE::Storage::vdisk_free($self->{storecfg}, $volid); };
 > -    if (my $err = $@) {
 > -        $self->log('err', "removing local copy of '$volid' failed - 
$err");
 > -        $self->{errors} = 1;
 > -        last if $err =~ /^interrupted by signal$/;
 > +
 > +    #do not destroy if new target is local_host

nitpick, you never leave a space between the comment start `#` and the
comment itself, but our general style does this and it increases
readabillity a bit, imo. as said nitpick, but if you touch this again it
would be nice to change :)

something like:
# don't delete volumes which would get synced to this node again after
# migration anyway

 > +    if (!defined( grep { /\Q$volid\E/ } @$sync_vol)) {

do you have the syncable volumes also available in a hash? then you could
use the following check:

if ($rep && !defined($synced_volumes->{$volid})) {
[...]

You could transform the array to a hash with:
my %synced_volumes = map { $_ => 1 } @$sync_vol;


 > +        eval { PVE::Storage::vdisk_free($self->{storecfg}, $volid); };
 > +        if (my $err = $@) {
 > +        $self->log('err', "removing local copy of '$volid' failed - 
$err");
 > +        $self->{errors} = 1;
 > +        last if $err =~ /^interrupted by signal$/;
 > +        }
 >      }
 >      }
 >  }
 > @@ -955,6 +974,11 @@ sub phase3_cleanup {
 >      # clear migrate lock
 >      my $cmd = [ @{$self->{rem_ssh}}, 'qm', 'unlock', $vmid ];
 >      $self->cmd_logerr($cmd, errmsg => "failed to clear migrate lock");
 > +
 > +    if ($self->{vmconf}->{replica} =~ m/(?i:1|yes|on|true)/) {

check could be minified here too, as mentioned above

 > +    my $cmd = [ @{$self->{rem_ssh}}, 'qm', 'set', $vmid, '--replica'];
 > +    $self->cmd_logerr($cmd, errmsg => "failed to activate replica");
 > +    }
 >  }
 >
 >  sub final_cleanup {