From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <c.ebner@proxmox.com>
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 <pve-devel@lists.proxmox.com>; 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 <pve-devel@lists.proxmox.com>; 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 <pve-devel@lists.proxmox.com>; 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 <pve-devel@lists.proxmox.com>; Mon, 23 Oct 2023 13:21:20 +0200 (CEST)
Date: Mon, 23 Oct 2023 13:21:19 +0200 (CEST)
From: Christian Ebner <c.ebner@proxmox.com>
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 <pve-devel.lists.proxmox.com>
List-Unsubscribe: <https://lists.proxmox.com/cgi-bin/mailman/options/pve-devel>,
<mailto:pve-devel-request@lists.proxmox.com?subject=unsubscribe>
List-Archive: <http://lists.proxmox.com/pipermail/pve-devel/>
List-Post: <mailto:pve-devel@lists.proxmox.com>
List-Help: <mailto:pve-devel-request@lists.proxmox.com?subject=help>
List-Subscribe: <https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel>,
<mailto:pve-devel-request@lists.proxmox.com?subject=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 <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