From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id AE03B9C0EA for ; Mon, 23 Oct 2023 13:21:22 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 91A1712DD2 for ; Mon, 23 Oct 2023 13:21:22 +0200 (CEST) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [94.136.29.106]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS for ; Mon, 23 Oct 2023 13:21:21 +0200 (CEST) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id C814944580 for ; Mon, 23 Oct 2023 13:21:20 +0200 (CEST) Date: Mon, 23 Oct 2023 13:21:19 +0200 (CEST) From: Christian Ebner To: pve-devel@lists.proxmox.com Message-ID: <447752374.1198.1698060079244@webmail.proxmox.com> In-Reply-To: <20231017123827.313014-1-c.ebner@proxmox.com> References: <20231017123827.313014-1-c.ebner@proxmox.com> MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Priority: 3 Importance: Normal X-Mailer: Open-Xchange Mailer v7.10.6-Rev53 X-Originating-Client: open-xchange-appsuite X-SPAM-LEVEL: Spam detection results: 0 AWL 0.026 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment PROLO_LEO2 0.1 Meta Catches all Leo drug variations so far SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Subject: Re: [pve-devel] [RFC pve-container] backup: do not delete not backed-up mps on restore X-BeenThere: pve-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox VE development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 23 Oct 2023 11:21:22 -0000 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 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 > --- > > 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