[pve-devel] [RFC pve-container] backup: do not delete not backed-up mps on restore
Christian Ebner
c.ebner at proxmox.com
Mon Oct 23 13:21:19 CEST 2023
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 at 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 at 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
More information about the pve-devel
mailing list