[pve-devel] [RFC qemu-server 1/3] Insert new options in the Qemu config for the PVE Replica.

[Thomas Lamprecht]

I did not verified the behavior or have the full picture yet,
but some more general comments from taking a view at the patch inline.

On 04/03/2017 04:53 PM, Wolfgang Link wrote:
> This patch will include all necessary config for the replication.
> Also will it enable and disable a replication job
> when appointed flags are set or deleted.
> ---
>   PVE/API2/Qemu.pm  | 41 +++++++++++++++++++++++++++++++++++++++++
>   PVE/QemuServer.pm | 31 +++++++++++++++++++++++++++++++
>   2 files changed, 72 insertions(+)
>
> diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
> index 616dc48..20491c9 100644
> --- a/PVE/API2/Qemu.pm
> +++ b/PVE/API2/Qemu.pm
> @@ -24,6 +24,7 @@ use PVE::INotify;
>   use PVE::Network;
>   use PVE::Firewall;
>   use PVE::API2::Firewall::VM;
> +use PVE::ReplicaTools;
>   
>   BEGIN {
>       if (!$ENV{PVE_GENERATING_DOCS}) {
> @@ -1028,6 +1029,20 @@ my $update_vm_api  = sub {
>   			if defined($conf->{pending}->{$opt});
>   		    PVE::QemuServer::vmconfig_delete_pending_option($conf, $opt, $force);
>   		    PVE::QemuConfig->write_config($vmid, $conf);
> +		} elsif ($opt eq "replica" || $opt eq "reptarget") {
> +		    delete $conf->{$opt};
> +		    delete $conf->{replica} if $opt eq "reptarget";

does:

delete $conf->{reptarget} if $opt eq "replica";

miss here to be complete?
or in general just delete both options if we take the if branch?

> +
> +		    #delete replica in snap to permit uncontrolled behavior.
> +		   foreach my $snap (keys %{$conf->{snapshots}}) {
> +		       delete $conf->{snapshots}->{$snap}->{replica};
> +		   }
> +
> +		    PVE::QemuConfig->write_config($vmid, $conf);
> +		    PVE::ReplicaTools::job_remove($vmid);
> +		} elsif ($opt eq "repinterval" || $opt eq "replimit") {
> +		    delete $conf->{$opt};
> +		    PVE::QemuConfig->write_config($vmid, $conf);
>   		} else {
>   		    PVE::QemuServer::vmconfig_delete_pending_option($conf, $opt, $force);
>   		    PVE::QemuConfig->write_config($vmid, $conf);
> @@ -1050,6 +1065,15 @@ my $update_vm_api  = sub {
>   			if defined($conf->{pending}->{$opt});
>   
>   		    &$create_disks($rpcenv, $authuser, $conf->{pending}, $storecfg, $vmid, undef, {$opt => $param->{$opt}});
> +		} elsif ($opt eq "replica") {
> +		    PVE::ReplicaTools::get_syncable_guestdisks($conf, 'qemu', 1);
> +		    $conf->{$opt} = $param->{$opt};
> +		} elsif ($opt eq "repinterval" || $opt eq "replimit") {
> +		     $conf->{$opt} = $param->{$opt};
> +		} elsif ($opt eq "reptarget" ) {
> +		    die "Node: $param->{$opt} does not exists in Cluster.\n"
> +			if !PVE::Cluster::check_node_exists($param->{$opt});
> +		    $conf->{$opt} = $param->{$opt};
>   		} else {
>   		    $conf->{pending}->{$opt} = $param->{$opt};
>   		}
> @@ -1057,6 +1081,23 @@ my $update_vm_api  = sub {
>   		PVE::QemuConfig->write_config($vmid, $conf);
>   	    }
>   
> +	    if ($param->{replica}) {
this should be
if (defined($param->{replica}))

else you never take this if branch if replica is , e.g., 0


> +
> +		eval {
> +		    if ($param->{replica} =~ m/^(?i:0|no|off|false)$/) {
AFAIK, we restrict booleans since a few versions to 0/1, at least in 
configs, so a

if ($param->{replicas})

should be enough here

> +			PVE::ReplicaTools::job_disable($vmid);
> +		    } else {
> +			PVE::ReplicaTools::job_enable($vmid);
> +		    }
> +		};
> +		if (my $err = $@) {
> +		    $conf->{replica} = '0';
> +		    PVE::QemuConfig->write_config($vmid, $conf);
> +		    die $err;
> +		}
> +
> +	    }
> +
>   	    # remove pending changes when nothing changed
>   	    $conf = PVE::QemuConfig->load_config($vmid); # update/reload
>   	    my $changes = PVE::QemuServer::vmconfig_cleanup_pending($conf);
> diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
> index 6865d60..dbdaf35 100644
> --- a/PVE/QemuServer.pm
> +++ b/PVE/QemuServer.pm
> @@ -491,6 +491,31 @@ EODESCR
>   	maxLength => 256,
>   	optional => 1,
>       },
> +    replica => {
> +       optional => 1,
> +       description => "Enable asyncron replica for local storage.",
> +       type => 'boolean',
> +       default => 0,
> +    },
> +    replimit => {
> +       optional => 1,
> +       description => "Asysncron replica speed limit.",
> +       type => 'integer',
> +       minimum => 0,
> +    },
> +    repinterval => {
> +       optional => 1,
> +       description => "Asyncron replica interval [1-1440] in minutes default 15",
AFAIK, you do not need to state the interval and default in the 
description if it's set below,
our api->doc export should handle this.
> +       type => 'integer',
> +       minimum => 1,
> +       maximum => 1440,
> +       default => 15,
> +    },
> +    reptarget => {
> +	optional => 1,
> +	description => "Asysncron replica taget node.",
> +	type => 'string',
> +    },

`replica_rate_limit`, `replica_interval` and `replica_target` would be 
more descriptive, imo.

>       protection => {
>   	optional => 1,
>   	type => 'boolean',
> @@ -747,6 +772,12 @@ my %drivedesc_base = (
>   	description => "Whether the drive should be included when making backups.",
>   	optional => 1,
>       },
> +    rep => {
> +	type => 'boolean',
> +	description => 'Will include this drive to an asyncron replical job when it run.',
> +	optional => 1,
> +	default => 1,
> +    },
IMHO, `rep` is a bit confusing here - as above replicate is used, the 
full name `replicate` would be better.
also typo in description s/asyncron/asynchronous/ s/replica/replicate/

In general the description is a bit confusing, maybe:

"Include this drive when running asynchronous replication jobs"

>       werror => {
>   	type => 'string',
>   	enum => [qw(enospc ignore report stop)],