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)],
_______________________________________________
pve-devel mailing list
[email protected]
http://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
