Re: [pve-devel] [PATCH v3 qemu-server] vzdump: move include logic for volumes to method
Thx for the info :)
On 3/24/20 10:16 AM, Fabian Ebner wrote:
On 17.03.20 15:34, Fabian Grünbichler wrote:
On March 16, 2020 4:44 pm, Aaron Lauterer wrote:
Move the logic which volumes are included in the backup job to its own
method and adapt the VZDump code accordingly. This makes it possible to
develop other features around backup jobs.
Signed-off-by: Aaron Lauterer
---
v2 -> v3: rebased
v1 -> v2:
* implemented the suggestions from Fabian [0]
* incorporated changes that happened in the meantime, most notably the
check for efidisk without omvf bios set
I decided to keep the check for IOThreaded VMs where it is because it
will only trigger if the backup is run with an older qemu version. By
the time this patch series gets applied and shipped I think it unlikely
that this will actually be triggered anymore.
There also seems to be a problem with the way the IFs are nested in that
section. I sent in separate small patch to fix it [1]. I wasn't sure if
I should have implemented that patch here as well. Depending on which
patch gets applied first, the other will need some rebasing.
[0] https://pve.proxmox.com/pipermail/pve-devel/2020-January/041360.html
[1]
https://pve.proxmox.com/pipermail/pve-devel/2020-February/041968.html
PVE/QemuConfig.pm | 31 +++
PVE/VZDump/QemuServer.pm | 38 +++---
2 files changed, 50 insertions(+), 19 deletions(-)
diff --git a/PVE/QemuConfig.pm b/PVE/QemuConfig.pm
index 1ba728a..f5b737c 100644
--- a/PVE/QemuConfig.pm
+++ b/PVE/QemuConfig.pm
@@ -130,6 +130,37 @@ sub get_replicatable_volumes {
return $volhash;
}
+sub get_backup_volumes {
+ my ($class, $conf) = @_;
+
+ my $ret_volumes = [];
+
+ my $test_volume = sub {
+ my ($key, $drive) = @_;
+
+ return if PVE::QemuServer::drive_is_cdrom($drive);
+
+ my $volume = {};
+ my $included = $drive->{backup} // 1;;
+ my $reason = defined($drive->{backup}) ? 'disabled' : 'enabled';
+
+ if ($key =~ m/^efidisk/ && (!defined($conf->{bios}) ||
$conf->{bios} ne 'ovmf')) {
+ $included = 0;
+ $reason = "efidisk with non omvf bios";
+ }
+ $volume->{key} = $key;
+ $volume->{included} = $included;
+ $volume->{reason} = $reason;
+ $volume->{data} = $drive;
+
+ push @$ret_volumes, $volume;
+ };
+
+ PVE::QemuServer::foreach_drive($conf, $test_volume);
nit: this is in PVE::QemuServer::Drive now, and we don't want to add new
dependencies from QemuConfig to QemuServer.
Once it's available, the new abstract foreach_volume (will be part of
QemuConfig.pm via AbstractConfig.pm) can also be used here. I'll send
the next version of the series[0] later this week and hopefully it's in
good enough shape to be applied.
[0]: https://pve.proxmox.com/pipermail/pve-devel/2020-March/042324.html
+
+ return $ret_volumes;
+}
+
sub __snapshot_save_vmstate {
my ($class, $vmid, $conf, $snapname, $storecfg, $statestorage,
$suspend) = @_;
diff --git a/PVE/VZDump/QemuServer.pm b/PVE/VZDump/QemuServer.pm
index 7695ad6..38471de 100644
--- a/PVE/VZDump/QemuServer.pm
+++ b/PVE/VZDump/QemuServer.pm
@@ -69,37 +69,37 @@ sub prepare {
my $vollist = [];
my $drivehash = {};
- PVE::QemuServer::foreach_drive($conf, sub {
- my ($ds, $drive) = @_;
+ my $backup_included = PVE::QemuConfig->get_backup_volumes($conf);
- return if PVE::QemuServer::drive_is_cdrom($drive);
+ foreach my $volume(@{$backup_included}) {
+ my $volid = $volume->{data}->{file};
+ my $name = $volume->{key};
- my $volid = $drive->{file};
-
- if (defined($drive->{backup}) && !$drive->{backup}) {
- $self->loginfo("exclude disk '$ds' '$volid' (backup=no)");
- return;
- } elsif ($self->{vm_was_running} && $drive->{iothread}) {
+ if (!$volume->{included}) {
+ if ($volume->{reason} eq 'efidisk with non omvf bios') {
+ $self->loginfo("excluding '$name' (efidisks can only be
backed up when BIOS is set to 'ovmf')");
+ next;
+ }
+ $self->loginfo("exclude disk '$name' '$volid' (backup=no)");
+ next;
+ } elsif ($self->{vm_was_running} && $volume->{data}->{iothread}) {
if
(!PVE::QemuServer::Machine::runs_at_least_qemu_version($vmid, 4, 0,
1)) {
- die "disk '$ds' '$volid' (iothread=on) can't use backup
feature with running QEMU " .
+ die "disk '$name' '$volid' (iothread=on) can't use backup
feature with running QEMU " .
"version < 4.0.1! Either set backup=no for this drive
or upgrade QEMU and restart VM\n";
}
- } elsif ($ds =~ m/^efidisk/ && (!defined($conf->{bios}) ||
$conf->{bios} ne 'ovmf')) {
- $self->loginfo("excluding '$ds' (efidisks can only be backed
up when BIOS is set to 'ovmf')");
- return;
} else {
- my $log = "include disk '$ds' '$volid'";
- if (defined $drive->{size}) {
- my $readable_size =
PVE::JSONSchema::format_size($drive->{size});
+
Re: [pve-devel] [PATCH v3 qemu-server] vzdump: move include logic for volumes to method
On 17.03.20 15:34, Fabian Grünbichler wrote:
On March 16, 2020 4:44 pm, Aaron Lauterer wrote:
Move the logic which volumes are included in the backup job to its own
method and adapt the VZDump code accordingly. This makes it possible to
develop other features around backup jobs.
Signed-off-by: Aaron Lauterer
---
v2 -> v3: rebased
v1 -> v2:
* implemented the suggestions from Fabian [0]
* incorporated changes that happened in the meantime, most notably the
check for efidisk without omvf bios set
I decided to keep the check for IOThreaded VMs where it is because it
will only trigger if the backup is run with an older qemu version. By
the time this patch series gets applied and shipped I think it unlikely
that this will actually be triggered anymore.
There also seems to be a problem with the way the IFs are nested in that
section. I sent in separate small patch to fix it [1]. I wasn't sure if
I should have implemented that patch here as well. Depending on which
patch gets applied first, the other will need some rebasing.
[0] https://pve.proxmox.com/pipermail/pve-devel/2020-January/041360.html
[1] https://pve.proxmox.com/pipermail/pve-devel/2020-February/041968.html
PVE/QemuConfig.pm| 31 +++
PVE/VZDump/QemuServer.pm | 38 +++---
2 files changed, 50 insertions(+), 19 deletions(-)
diff --git a/PVE/QemuConfig.pm b/PVE/QemuConfig.pm
index 1ba728a..f5b737c 100644
--- a/PVE/QemuConfig.pm
+++ b/PVE/QemuConfig.pm
@@ -130,6 +130,37 @@ sub get_replicatable_volumes {
return $volhash;
}
+sub get_backup_volumes {
+my ($class, $conf) = @_;
+
+my $ret_volumes = [];
+
+my $test_volume = sub {
+ my ($key, $drive) = @_;
+
+ return if PVE::QemuServer::drive_is_cdrom($drive);
+
+ my $volume = {};
+ my $included = $drive->{backup} // 1;;
+ my $reason = defined($drive->{backup}) ? 'disabled' : 'enabled';
+
+ if ($key =~ m/^efidisk/ && (!defined($conf->{bios}) || $conf->{bios} ne
'ovmf')) {
+ $included = 0;
+ $reason = "efidisk with non omvf bios";
+ }
+ $volume->{key} = $key;
+ $volume->{included} = $included;
+ $volume->{reason} = $reason;
+ $volume->{data} = $drive;
+
+ push @$ret_volumes, $volume;
+};
+
+PVE::QemuServer::foreach_drive($conf, $test_volume);
nit: this is in PVE::QemuServer::Drive now, and we don't want to add new
dependencies from QemuConfig to QemuServer.
Once it's available, the new abstract foreach_volume (will be part of
QemuConfig.pm via AbstractConfig.pm) can also be used here. I'll send
the next version of the series[0] later this week and hopefully it's in
good enough shape to be applied.
[0]: https://pve.proxmox.com/pipermail/pve-devel/2020-March/042324.html
+
+return $ret_volumes;
+}
+
sub __snapshot_save_vmstate {
my ($class, $vmid, $conf, $snapname, $storecfg, $statestorage, $suspend)
= @_;
diff --git a/PVE/VZDump/QemuServer.pm b/PVE/VZDump/QemuServer.pm
index 7695ad6..38471de 100644
--- a/PVE/VZDump/QemuServer.pm
+++ b/PVE/VZDump/QemuServer.pm
@@ -69,37 +69,37 @@ sub prepare {
my $vollist = [];
my $drivehash = {};
-PVE::QemuServer::foreach_drive($conf, sub {
- my ($ds, $drive) = @_;
+my $backup_included = PVE::QemuConfig->get_backup_volumes($conf);
- return if PVE::QemuServer::drive_is_cdrom($drive);
+foreach my $volume(@{$backup_included}) {
+ my $volid = $volume->{data}->{file};
+ my $name = $volume->{key};
- my $volid = $drive->{file};
-
- if (defined($drive->{backup}) && !$drive->{backup}) {
- $self->loginfo("exclude disk '$ds' '$volid' (backup=no)");
- return;
- } elsif ($self->{vm_was_running} && $drive->{iothread}) {
+ if (!$volume->{included}) {
+ if ($volume->{reason} eq 'efidisk with non omvf bios') {
+ $self->loginfo("excluding '$name' (efidisks can only be backed up
when BIOS is set to 'ovmf')");
+ next;
+ }
+ $self->loginfo("exclude disk '$name' '$volid' (backup=no)");
+ next;
+ } elsif ($self->{vm_was_running} && $volume->{data}->{iothread}) {
if (!PVE::QemuServer::Machine::runs_at_least_qemu_version($vmid, 4,
0, 1)) {
- die "disk '$ds' '$volid' (iothread=on) can't use backup feature with
running QEMU " .
+ die "disk '$name' '$volid' (iothread=on) can't use backup feature
with running QEMU " .
"version < 4.0.1! Either set backup=no for this drive or upgrade
QEMU and restart VM\n";
}
- } elsif ($ds =~ m/^efidisk/ && (!defined($conf->{bios}) ||
$conf->{bios} ne 'ovmf')) {
- $self->loginfo("excluding '$ds' (efidisks can only be backed up when
BIOS is set to 'ovmf')");
- return;
} else {
- my $log = "include disk '$ds' '$volid'";
- if (defined $drive->{size}) {
Re: [pve-devel] [PATCH v3 qemu-server] vzdump: move include logic for volumes to method
On March 16, 2020 4:44 pm, Aaron Lauterer wrote:
> Move the logic which volumes are included in the backup job to its own
> method and adapt the VZDump code accordingly. This makes it possible to
> develop other features around backup jobs.
>
> Signed-off-by: Aaron Lauterer
> ---
>
> v2 -> v3: rebased
>
> v1 -> v2:
> * implemented the suggestions from Fabian [0]
> * incorporated changes that happened in the meantime, most notably the
> check for efidisk without omvf bios set
>
> I decided to keep the check for IOThreaded VMs where it is because it
> will only trigger if the backup is run with an older qemu version. By
> the time this patch series gets applied and shipped I think it unlikely
> that this will actually be triggered anymore.
>
> There also seems to be a problem with the way the IFs are nested in that
> section. I sent in separate small patch to fix it [1]. I wasn't sure if
> I should have implemented that patch here as well. Depending on which
> patch gets applied first, the other will need some rebasing.
>
> [0] https://pve.proxmox.com/pipermail/pve-devel/2020-January/041360.html
> [1] https://pve.proxmox.com/pipermail/pve-devel/2020-February/041968.html
>
> PVE/QemuConfig.pm| 31 +++
> PVE/VZDump/QemuServer.pm | 38 +++---
> 2 files changed, 50 insertions(+), 19 deletions(-)
>
> diff --git a/PVE/QemuConfig.pm b/PVE/QemuConfig.pm
> index 1ba728a..f5b737c 100644
> --- a/PVE/QemuConfig.pm
> +++ b/PVE/QemuConfig.pm
> @@ -130,6 +130,37 @@ sub get_replicatable_volumes {
> return $volhash;
> }
>
> +sub get_backup_volumes {
> +my ($class, $conf) = @_;
> +
> +my $ret_volumes = [];
> +
> +my $test_volume = sub {
> + my ($key, $drive) = @_;
> +
> + return if PVE::QemuServer::drive_is_cdrom($drive);
> +
> + my $volume = {};
> + my $included = $drive->{backup} // 1;;
> + my $reason = defined($drive->{backup}) ? 'disabled' : 'enabled';
> +
> + if ($key =~ m/^efidisk/ && (!defined($conf->{bios}) || $conf->{bios} ne
> 'ovmf')) {
> + $included = 0;
> + $reason = "efidisk with non omvf bios";
> + }
> + $volume->{key} = $key;
> + $volume->{included} = $included;
> + $volume->{reason} = $reason;
> + $volume->{data} = $drive;
> +
> + push @$ret_volumes, $volume;
> +};
> +
> +PVE::QemuServer::foreach_drive($conf, $test_volume);
nit: this is in PVE::QemuServer::Drive now, and we don't want to add new
dependencies from QemuConfig to QemuServer.
> +
> +return $ret_volumes;
> +}
> +
> sub __snapshot_save_vmstate {
> my ($class, $vmid, $conf, $snapname, $storecfg, $statestorage, $suspend)
> = @_;
>
> diff --git a/PVE/VZDump/QemuServer.pm b/PVE/VZDump/QemuServer.pm
> index 7695ad6..38471de 100644
> --- a/PVE/VZDump/QemuServer.pm
> +++ b/PVE/VZDump/QemuServer.pm
> @@ -69,37 +69,37 @@ sub prepare {
>
> my $vollist = [];
> my $drivehash = {};
> -PVE::QemuServer::foreach_drive($conf, sub {
> - my ($ds, $drive) = @_;
> +my $backup_included = PVE::QemuConfig->get_backup_volumes($conf);
>
> - return if PVE::QemuServer::drive_is_cdrom($drive);
> +foreach my $volume(@{$backup_included}) {
> + my $volid = $volume->{data}->{file};
> + my $name = $volume->{key};
>
> - my $volid = $drive->{file};
> -
> - if (defined($drive->{backup}) && !$drive->{backup}) {
> - $self->loginfo("exclude disk '$ds' '$volid' (backup=no)");
> - return;
> - } elsif ($self->{vm_was_running} && $drive->{iothread}) {
> + if (!$volume->{included}) {
> + if ($volume->{reason} eq 'efidisk with non omvf bios') {
> + $self->loginfo("excluding '$name' (efidisks can only be backed
> up when BIOS is set to 'ovmf')");
> + next;
> + }
> + $self->loginfo("exclude disk '$name' '$volid' (backup=no)");
> + next;
> + } elsif ($self->{vm_was_running} && $volume->{data}->{iothread}) {
> if (!PVE::QemuServer::Machine::runs_at_least_qemu_version($vmid, 4,
> 0, 1)) {
> - die "disk '$ds' '$volid' (iothread=on) can't use backup feature
> with running QEMU " .
> + die "disk '$name' '$volid' (iothread=on) can't use backup
> feature with running QEMU " .
> "version < 4.0.1! Either set backup=no for this drive or
> upgrade QEMU and restart VM\n";
> }
> - } elsif ($ds =~ m/^efidisk/ && (!defined($conf->{bios}) ||
> $conf->{bios} ne 'ovmf')) {
> - $self->loginfo("excluding '$ds' (efidisks can only be backed up
> when BIOS is set to 'ovmf')");
> - return;
> } else {
> - my $log = "include disk '$ds' '$volid'";
> -if (defined $drive->{size}) {
> - my $readable_size =
> PVE::JSONSchema::format_size($drive->{size});
> + my $log = "include disk '$name' '$volid'";
> + if (defined $volume->{data}->{size}) {
> + my $readable
