* [pve-devel] [RFC pve-container] backup: do not delete not backed-up mps on restore
@ 2023-10-17 12:38 Christian Ebner
2023-10-23 11:21 ` Christian Ebner
0 siblings, 1 reply; 2+ messages in thread
From: Christian Ebner @ 2023-10-17 12:38 UTC (permalink / raw)
To: pve-devel
The current behaviour of the restore is to recreate all backed up
mountpoints and remove all previous ones, causing potential data loss on
restore when the mountpoint was not included in the backup and the user
not aware of this behaviour.
By checking the mountpoint configuration from the backup, only recreate
the disks which are included in the backup and add them as mountpoints.
Leave all other mountpoints untouched and attach them as unused disks
as final step of the restore.
To facilitate selective restore from PBS backups, split the currently
single root pxar archive into a pxar archive for each individual
mountpoint, while remaining backwards compatible.
Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
---
I'm sending this as RFC since backwards compatibility for restore still
suffers from the fact, that mountpoints are now expected to have their
corresponding pxar archive for PBS backups, all previous backups however
do not follow this scheme.
For now, this is handled by treating restore errors of these archives as
soft errors, meaning restore will continue. This is not ideal and I am
very much open for suggestions on how this might be handled better.
src/PVE/API2/LXC.pm | 26 ++++++++++++----
src/PVE/LXC/Create.pm | 72 +++++++++++++++++++++++++++++++++++--------
src/PVE/VZDump/LXC.pm | 10 +++---
3 files changed, 85 insertions(+), 23 deletions(-)
diff --git a/src/PVE/API2/LXC.pm b/src/PVE/API2/LXC.pm
index 28d14de..3c7b22d 100644
--- a/src/PVE/API2/LXC.pm
+++ b/src/PVE/API2/LXC.pm
@@ -381,8 +381,10 @@ __PACKAGE__->register_method({
my $vollist = [];
eval {
my $orig_mp_param; # only used if $restore
+ my $clear_mps;
if ($restore) {
die "can't overwrite running container\n" if PVE::LXC::check_running($vmid);
+
if ($archive ne '-') {
my $orig_conf;
print "recovering backed-up configuration from '$archive'\n";
@@ -424,6 +426,14 @@ __PACKAGE__->register_method({
if !defined($mp_param->{rootfs});
PVE::LXC::Config->foreach_volume($mp_param, sub {
my ($ms, $mountpoint) = @_;
+ if ($ms eq 'rootfs' || $mountpoint->{backup}) {
+ # backup conf contains the mp, clear for retsore
+ $clear_mps->{$ms} = $mountpoint;
+ } else {
+ # do not add as mp, will be attach as unused at the end
+ delete $mp_param->{$ms};
+ return;
+ }
my $type = $mountpoint->{type};
if ($type eq 'volume') {
die "unable to detect disk size - please specify $ms (size)\n"
@@ -459,18 +469,19 @@ __PACKAGE__->register_method({
$vollist = PVE::LXC::create_disks($storage_cfg, $vmid, $mp_param, $conf);
- # we always have the 'create' lock so check for more than 1 entry
- if (scalar(keys %$old_conf) > 1) {
- # destroy old container volumes
- PVE::LXC::destroy_lxc_container($storage_cfg, $vmid, $old_conf, { lock => 'create' });
- }
+ # Delete old mountpoints which are restored from backup.
+ PVE::LXC::Config->foreach_volume($old_conf, sub {
+ my ($name, $mountpoint, undef) = @_;
+ return if !defined($clear_mps->{$name});
+ PVE::LXC::delete_mountpoint_volume($storage_cfg, $vmid, $mountpoint->{volume});
+ });
eval {
my $rootdir = PVE::LXC::mount_all($vmid, $storage_cfg, $conf, 1);
$bwlimit = PVE::Storage::get_bandwidth_limit('restore', [keys %used_storages], $bwlimit);
print "restoring '$archive' now..\n"
if $restore && $archive ne '-';
- PVE::LXC::Create::restore_archive($storage_cfg, $archive, $rootdir, $conf, $ignore_unpack_errors, $bwlimit);
+ PVE::LXC::Create::restore_archive($storage_cfg, $archive, $rootdir, $conf, $ignore_unpack_errors, $bwlimit, $orig_mp_param);
if ($restore) {
print "merging backed-up and given configuration..\n";
@@ -501,6 +512,9 @@ __PACKAGE__->register_method({
$conf->{template} = 1;
}
PVE::LXC::Config->write_config($vmid, $conf);
+
+ # Attach all additionally found mountpoints as unused disks.
+ PVE::LXC::rescan($vmid, 1, 0);
};
if (my $err = $@) {
PVE::LXC::destroy_disks($storage_cfg, $vollist);
diff --git a/src/PVE/LXC/Create.pm b/src/PVE/LXC/Create.pm
index f4c3220..74d7954 100644
--- a/src/PVE/LXC/Create.pm
+++ b/src/PVE/LXC/Create.pm
@@ -83,27 +83,33 @@ sub detect_architecture {
}
sub restore_archive {
- my ($storage_cfg, $archive, $rootdir, $conf, $no_unpack_error, $bwlimit) = @_;
+ my ($storage_cfg, $archive, $rootdir, $conf, $no_unpack_error, $bwlimit, $orig_mp_param) = @_;
+ my $orig_mps;
+ PVE::LXC::Config->foreach_volume($orig_mp_param, sub {
+ my ($name, $vol, undef) = @_;
+ $orig_mps->{$name} = $vol;
+ });
my ($storeid, $volname) = PVE::Storage::parse_volume_id($archive, 1);
if (defined($storeid)) {
my $scfg = PVE::Storage::storage_check_enabled($storage_cfg, $storeid);
if ($scfg->{type} eq 'pbs') {
- return restore_proxmox_backup_archive($storage_cfg, $archive, $rootdir, $conf, $no_unpack_error, $bwlimit);
+ return restore_proxmox_backup_archive(
+ $storage_cfg, $archive, $rootdir, $conf, $no_unpack_error, $bwlimit, $orig_mps);
}
}
$archive = PVE::Storage::abs_filesystem_path($storage_cfg, $archive) if $archive ne '-';
- restore_tar_archive($archive, $rootdir, $conf, $no_unpack_error, $bwlimit);
+ restore_tar_archive($archive, $rootdir, $conf, $no_unpack_error, $bwlimit, $orig_mps);
}
sub restore_proxmox_backup_archive {
- my ($storage_cfg, $archive, $rootdir, $conf, $no_unpack_error, $bwlimit) = @_;
+ my ($storage_cfg, $archive, $rootdir, $conf, $no_unpack_error, $bwlimit, $orig_mps) = @_;
my ($storeid, $volname) = PVE::Storage::parse_volume_id($archive);
my $scfg = PVE::Storage::storage_config($storage_cfg, $storeid);
- my ($vtype, $name, undef, undef, undef, undef, $format) =
+ my ($vtype, $snapshot, undef, undef, undef, undef, $format) =
PVE::Storage::parse_volname($storage_cfg, $archive);
die "got unexpected vtype '$vtype'\n" if $vtype ne 'backup';
@@ -114,14 +120,47 @@ sub restore_proxmox_backup_archive {
my $userns_cmd = PVE::LXC::userns_command($id_map);
my $cmd = "restore";
- my $param = [$name, "root.pxar", $rootdir, '--allow-existing-dirs'];
+ PVE::LXC::Config->foreach_volume($conf, sub {
+ my ($name, $vol, undef) = @_;
- if ($no_unpack_error) {
- push(@$param, '--ignore-extract-device-errors');
- }
+ my $orig_mp = $orig_mps->{$name};
+ if (!defined($orig_mp)) {
+ print "'$name': mp not present in original backup.\n";
+ return;
+ }
- PVE::Storage::PBSPlugin::run_raw_client_cmd(
- $scfg, $storeid, $cmd, $param, userns_cmd => $userns_cmd);
+ if ($name ne 'rootfs' && (!defined($orig_mp->{backup}) || !$orig_mp->{backup})) {
+ print "'$name': mp not included in original backup.\n";
+ return;
+ }
+
+ $name = 'root' if $name eq 'rootfs';
+ my $target = "$rootdir/.$vol->{mp}";
+ if ($orig_mp->{mp} ne $vol->{mp}) {
+ print "'$name': path differs from backed-up path, restore to backed-up path.\n";
+ print " If this is not intended, change the path after restore.\n";
+ $target = "$rootdir/.$orig_mp->{mp}";
+ }
+
+ my $param = [$snapshot, "$name.pxar", $target, '--allow-existing-dirs'];
+
+ if ($no_unpack_error) {
+ push(@$param, '--ignore-extract-device-errors');
+ }
+
+ eval {
+ # This will fail for backups created without mp archive splitting
+ PVE::Storage::PBSPlugin::run_raw_client_cmd(
+ $scfg, $storeid, $cmd, $param, userns_cmd => $userns_cmd);
+ };
+ my $err = $@;
+ if ($err) {
+ # Only handle root restore failure as hard error
+ die $err if $name eq 'root';
+ print "extracting moutpoint '$name' failed:\n$err";
+ print "backup created with older version?\n";
+ }
+ });
# if arch is set, we do not try to autodetect it
return if defined($conf->{arch});
@@ -130,11 +169,20 @@ sub restore_proxmox_backup_archive {
}
sub restore_tar_archive {
- my ($archive, $rootdir, $conf, $no_unpack_error, $bwlimit) = @_;
+ my ($archive, $rootdir, $conf, $no_unpack_error, $bwlimit, $orig_mps) = @_;
my ($id_map, $rootuid, $rootgid) = PVE::LXC::parse_id_maps($conf);
my $userns_cmd = PVE::LXC::userns_command($id_map);
+ PVE::LXC::Config->foreach_volume($conf, sub {
+ my ($name, $vol, undef) = @_;
+ my $orig_mp = $orig_mps->{$name};
+ if (defined($orig_mp->{mp}) && $orig_mp->{mp} ne $vol->{mp}) {
+ print "'$name': path differs from backed-up path, restore to backed-up path.\n";
+ print " If this is not intended, change the path after restore.\n";
+ }
+ });
+
my $archive_fh;
my $tar_input = '<&STDIN';
my @compression_opt;
diff --git a/src/PVE/VZDump/LXC.pm b/src/PVE/VZDump/LXC.pm
index c68a06f..0d411b8 100644
--- a/src/PVE/VZDump/LXC.pm
+++ b/src/PVE/VZDump/LXC.pm
@@ -381,11 +381,11 @@ sub archive {
push @$param, "fw.conf:$fw_conf";
}
- my $rootdir = $snapdir;
- push @$param, "root.pxar:$rootdir";
-
- foreach my $disk (@sources) {
- push @$param, '--include-dev', "$snapdir/$disk";
+ foreach my $disk (@$disks) {
+ my $name = $disk->{name};
+ # Needed for backwards compatibility with previous backups
+ $name = 'root' if $name eq 'rootfs';
+ push @$param, "$name.pxar:$snapdir/.$disk->{mp}";
}
push @$param, '--skip-lost-and-found' if $userns_cmd;
--
2.39.2
^ permalink raw reply [flat|nested] 2+ messages in thread
* Re: [pve-devel] [RFC pve-container] backup: do not delete not backed-up mps on restore
2023-10-17 12:38 [pve-devel] [RFC pve-container] backup: do not delete not backed-up mps on restore Christian Ebner
@ 2023-10-23 11:21 ` Christian Ebner
0 siblings, 0 replies; 2+ messages in thread
From: Christian Ebner @ 2023-10-23 11:21 UTC (permalink / raw)
To: pve-devel
There is an newer version of the patch, see https://lists.proxmox.com/pipermail/pve-devel/2023-October/059566.html
Please ignore this one.
> On 17.10.2023 14:38 CEST Christian Ebner <c.ebner@proxmox.com> wrote:
>
>
> The current behaviour of the restore is to recreate all backed up
> mountpoints and remove all previous ones, causing potential data loss on
> restore when the mountpoint was not included in the backup and the user
> not aware of this behaviour.
>
> By checking the mountpoint configuration from the backup, only recreate
> the disks which are included in the backup and add them as mountpoints.
> Leave all other mountpoints untouched and attach them as unused disks
> as final step of the restore.
>
> To facilitate selective restore from PBS backups, split the currently
> single root pxar archive into a pxar archive for each individual
> mountpoint, while remaining backwards compatible.
>
> Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
> ---
>
> I'm sending this as RFC since backwards compatibility for restore still
> suffers from the fact, that mountpoints are now expected to have their
> corresponding pxar archive for PBS backups, all previous backups however
> do not follow this scheme.
> For now, this is handled by treating restore errors of these archives as
> soft errors, meaning restore will continue. This is not ideal and I am
> very much open for suggestions on how this might be handled better.
>
> src/PVE/API2/LXC.pm | 26 ++++++++++++----
> src/PVE/LXC/Create.pm | 72 +++++++++++++++++++++++++++++++++++--------
> src/PVE/VZDump/LXC.pm | 10 +++---
> 3 files changed, 85 insertions(+), 23 deletions(-)
>
> diff --git a/src/PVE/API2/LXC.pm b/src/PVE/API2/LXC.pm
> index 28d14de..3c7b22d 100644
> --- a/src/PVE/API2/LXC.pm
> +++ b/src/PVE/API2/LXC.pm
> @@ -381,8 +381,10 @@ __PACKAGE__->register_method({
> my $vollist = [];
> eval {
> my $orig_mp_param; # only used if $restore
> + my $clear_mps;
> if ($restore) {
> die "can't overwrite running container\n" if PVE::LXC::check_running($vmid);
> +
> if ($archive ne '-') {
> my $orig_conf;
> print "recovering backed-up configuration from '$archive'\n";
> @@ -424,6 +426,14 @@ __PACKAGE__->register_method({
> if !defined($mp_param->{rootfs});
> PVE::LXC::Config->foreach_volume($mp_param, sub {
> my ($ms, $mountpoint) = @_;
> + if ($ms eq 'rootfs' || $mountpoint->{backup}) {
> + # backup conf contains the mp, clear for retsore
> + $clear_mps->{$ms} = $mountpoint;
> + } else {
> + # do not add as mp, will be attach as unused at the end
> + delete $mp_param->{$ms};
> + return;
> + }
> my $type = $mountpoint->{type};
> if ($type eq 'volume') {
> die "unable to detect disk size - please specify $ms (size)\n"
> @@ -459,18 +469,19 @@ __PACKAGE__->register_method({
>
> $vollist = PVE::LXC::create_disks($storage_cfg, $vmid, $mp_param, $conf);
>
> - # we always have the 'create' lock so check for more than 1 entry
> - if (scalar(keys %$old_conf) > 1) {
> - # destroy old container volumes
> - PVE::LXC::destroy_lxc_container($storage_cfg, $vmid, $old_conf, { lock => 'create' });
> - }
> + # Delete old mountpoints which are restored from backup.
> + PVE::LXC::Config->foreach_volume($old_conf, sub {
> + my ($name, $mountpoint, undef) = @_;
> + return if !defined($clear_mps->{$name});
> + PVE::LXC::delete_mountpoint_volume($storage_cfg, $vmid, $mountpoint->{volume});
> + });
>
> eval {
> my $rootdir = PVE::LXC::mount_all($vmid, $storage_cfg, $conf, 1);
> $bwlimit = PVE::Storage::get_bandwidth_limit('restore', [keys %used_storages], $bwlimit);
> print "restoring '$archive' now..\n"
> if $restore && $archive ne '-';
> - PVE::LXC::Create::restore_archive($storage_cfg, $archive, $rootdir, $conf, $ignore_unpack_errors, $bwlimit);
> + PVE::LXC::Create::restore_archive($storage_cfg, $archive, $rootdir, $conf, $ignore_unpack_errors, $bwlimit, $orig_mp_param);
>
> if ($restore) {
> print "merging backed-up and given configuration..\n";
> @@ -501,6 +512,9 @@ __PACKAGE__->register_method({
> $conf->{template} = 1;
> }
> PVE::LXC::Config->write_config($vmid, $conf);
> +
> + # Attach all additionally found mountpoints as unused disks.
> + PVE::LXC::rescan($vmid, 1, 0);
> };
> if (my $err = $@) {
> PVE::LXC::destroy_disks($storage_cfg, $vollist);
> diff --git a/src/PVE/LXC/Create.pm b/src/PVE/LXC/Create.pm
> index f4c3220..74d7954 100644
> --- a/src/PVE/LXC/Create.pm
> +++ b/src/PVE/LXC/Create.pm
> @@ -83,27 +83,33 @@ sub detect_architecture {
> }
>
> sub restore_archive {
> - my ($storage_cfg, $archive, $rootdir, $conf, $no_unpack_error, $bwlimit) = @_;
> + my ($storage_cfg, $archive, $rootdir, $conf, $no_unpack_error, $bwlimit, $orig_mp_param) = @_;
>
> + my $orig_mps;
> + PVE::LXC::Config->foreach_volume($orig_mp_param, sub {
> + my ($name, $vol, undef) = @_;
> + $orig_mps->{$name} = $vol;
> + });
> my ($storeid, $volname) = PVE::Storage::parse_volume_id($archive, 1);
> if (defined($storeid)) {
> my $scfg = PVE::Storage::storage_check_enabled($storage_cfg, $storeid);
> if ($scfg->{type} eq 'pbs') {
> - return restore_proxmox_backup_archive($storage_cfg, $archive, $rootdir, $conf, $no_unpack_error, $bwlimit);
> + return restore_proxmox_backup_archive(
> + $storage_cfg, $archive, $rootdir, $conf, $no_unpack_error, $bwlimit, $orig_mps);
> }
> }
>
> $archive = PVE::Storage::abs_filesystem_path($storage_cfg, $archive) if $archive ne '-';
> - restore_tar_archive($archive, $rootdir, $conf, $no_unpack_error, $bwlimit);
> + restore_tar_archive($archive, $rootdir, $conf, $no_unpack_error, $bwlimit, $orig_mps);
> }
>
> sub restore_proxmox_backup_archive {
> - my ($storage_cfg, $archive, $rootdir, $conf, $no_unpack_error, $bwlimit) = @_;
> + my ($storage_cfg, $archive, $rootdir, $conf, $no_unpack_error, $bwlimit, $orig_mps) = @_;
>
> my ($storeid, $volname) = PVE::Storage::parse_volume_id($archive);
> my $scfg = PVE::Storage::storage_config($storage_cfg, $storeid);
>
> - my ($vtype, $name, undef, undef, undef, undef, $format) =
> + my ($vtype, $snapshot, undef, undef, undef, undef, $format) =
> PVE::Storage::parse_volname($storage_cfg, $archive);
>
> die "got unexpected vtype '$vtype'\n" if $vtype ne 'backup';
> @@ -114,14 +120,47 @@ sub restore_proxmox_backup_archive {
> my $userns_cmd = PVE::LXC::userns_command($id_map);
>
> my $cmd = "restore";
> - my $param = [$name, "root.pxar", $rootdir, '--allow-existing-dirs'];
> + PVE::LXC::Config->foreach_volume($conf, sub {
> + my ($name, $vol, undef) = @_;
>
> - if ($no_unpack_error) {
> - push(@$param, '--ignore-extract-device-errors');
> - }
> + my $orig_mp = $orig_mps->{$name};
> + if (!defined($orig_mp)) {
> + print "'$name': mp not present in original backup.\n";
> + return;
> + }
>
> - PVE::Storage::PBSPlugin::run_raw_client_cmd(
> - $scfg, $storeid, $cmd, $param, userns_cmd => $userns_cmd);
> + if ($name ne 'rootfs' && (!defined($orig_mp->{backup}) || !$orig_mp->{backup})) {
> + print "'$name': mp not included in original backup.\n";
> + return;
> + }
> +
> + $name = 'root' if $name eq 'rootfs';
> + my $target = "$rootdir/.$vol->{mp}";
> + if ($orig_mp->{mp} ne $vol->{mp}) {
> + print "'$name': path differs from backed-up path, restore to backed-up path.\n";
> + print " If this is not intended, change the path after restore.\n";
> + $target = "$rootdir/.$orig_mp->{mp}";
> + }
> +
> + my $param = [$snapshot, "$name.pxar", $target, '--allow-existing-dirs'];
> +
> + if ($no_unpack_error) {
> + push(@$param, '--ignore-extract-device-errors');
> + }
> +
> + eval {
> + # This will fail for backups created without mp archive splitting
> + PVE::Storage::PBSPlugin::run_raw_client_cmd(
> + $scfg, $storeid, $cmd, $param, userns_cmd => $userns_cmd);
> + };
> + my $err = $@;
> + if ($err) {
> + # Only handle root restore failure as hard error
> + die $err if $name eq 'root';
> + print "extracting moutpoint '$name' failed:\n$err";
> + print "backup created with older version?\n";
> + }
> + });
>
> # if arch is set, we do not try to autodetect it
> return if defined($conf->{arch});
> @@ -130,11 +169,20 @@ sub restore_proxmox_backup_archive {
> }
>
> sub restore_tar_archive {
> - my ($archive, $rootdir, $conf, $no_unpack_error, $bwlimit) = @_;
> + my ($archive, $rootdir, $conf, $no_unpack_error, $bwlimit, $orig_mps) = @_;
>
> my ($id_map, $rootuid, $rootgid) = PVE::LXC::parse_id_maps($conf);
> my $userns_cmd = PVE::LXC::userns_command($id_map);
>
> + PVE::LXC::Config->foreach_volume($conf, sub {
> + my ($name, $vol, undef) = @_;
> + my $orig_mp = $orig_mps->{$name};
> + if (defined($orig_mp->{mp}) && $orig_mp->{mp} ne $vol->{mp}) {
> + print "'$name': path differs from backed-up path, restore to backed-up path.\n";
> + print " If this is not intended, change the path after restore.\n";
> + }
> + });
> +
> my $archive_fh;
> my $tar_input = '<&STDIN';
> my @compression_opt;
> diff --git a/src/PVE/VZDump/LXC.pm b/src/PVE/VZDump/LXC.pm
> index c68a06f..0d411b8 100644
> --- a/src/PVE/VZDump/LXC.pm
> +++ b/src/PVE/VZDump/LXC.pm
> @@ -381,11 +381,11 @@ sub archive {
> push @$param, "fw.conf:$fw_conf";
> }
>
> - my $rootdir = $snapdir;
> - push @$param, "root.pxar:$rootdir";
> -
> - foreach my $disk (@sources) {
> - push @$param, '--include-dev', "$snapdir/$disk";
> + foreach my $disk (@$disks) {
> + my $name = $disk->{name};
> + # Needed for backwards compatibility with previous backups
> + $name = 'root' if $name eq 'rootfs';
> + push @$param, "$name.pxar:$snapdir/.$disk->{mp}";
> }
>
> push @$param, '--skip-lost-and-found' if $userns_cmd;
> --
> 2.39.2
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2023-10-23 11:21 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-10-17 12:38 [pve-devel] [RFC pve-container] backup: do not delete not backed-up mps on restore Christian Ebner
2023-10-23 11:21 ` Christian Ebner
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox