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 4F09C672A2 for ; Thu, 30 Jul 2020 08:40:58 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 46FC01303B for ; Thu, 30 Jul 2020 08:40:28 +0200 (CEST) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [212.186.127.180]) (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 id E272113031 for ; Thu, 30 Jul 2020 08:40:26 +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 B0A27433F0 for ; Thu, 30 Jul 2020 08:40:26 +0200 (CEST) Date: Thu, 30 Jul 2020 08:40:19 +0200 From: Fabian =?iso-8859-1?q?Gr=FCnbichler?= To: Proxmox Backup Server development discussion References: <20200729123314.10049-1-s.reiter@proxmox.com> <20200729123314.10049-3-s.reiter@proxmox.com> In-Reply-To: <20200729123314.10049-3-s.reiter@proxmox.com> MIME-Version: 1.0 User-Agent: astroid/0.15.0 (https://github.com/astroidmail/astroid) Message-Id: <1596091108.tpwl0nigkw.astroid@nora.none> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-SPAM-LEVEL: Spam detection results: 0 AWL 0.049 Adjusted score from AWL reputation of From: address KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment RCVD_IN_DNSWL_MED -2.3 Sender listed at https://www.dnswl.org/, medium trust SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Subject: Re: [pbs-devel] [PATCH proxmox-backup 2/5] datastore: prevent deletion of snaps in use as "previous backup" X-BeenThere: pbs-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox Backup Server development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 30 Jul 2020 06:40:58 -0000 for the record, this is https://bugzilla.proxmox.com/show_bug.cgi?id=3D2881 On July 29, 2020 2:33 pm, Stefan Reiter wrote: > To prevent a race with a background GC operation, do not allow deletion > of backups who's index might currently be referenced as the "known chunk > list" for successive backups. Otherwise the GC could delete chunks it > thinks are no longer referenced, while at the same time telling the > client that it doesn't need to upload said chunks because they already > exist. >=20 > Additionally, prevent deletion of whole backup groups, if there are > snapshots contained that appear to be currently in-progress. This is > currently unlikely to trigger, as that function is only used for sync > jobs, but it's a useful safeguard either way. >=20 > Deleting a single snapshot has a 'force' parameter, which is necessary > to allow deleting incomplete snapshots on an aborted backup. Pruning > also sets force=3Dtrue to avoid the check, since it calculates which > snapshots to keep on its own. >=20 > To avoid code duplication, the is_finished method is factored out. >=20 > Signed-off-by: Stefan Reiter > --- > src/api2/admin/datastore.rs | 4 +-- > src/api2/backup/environment.rs | 2 +- > src/backup/backup_info.rs | 7 +++++- > src/backup/datastore.rs | 43 +++++++++++++++++++++++++++++++-- > src/backup/prune.rs | 2 +- > src/bin/proxmox-backup-proxy.rs | 2 +- > src/client/pull.rs | 4 +-- > 7 files changed, 54 insertions(+), 10 deletions(-) >=20 > diff --git a/src/api2/admin/datastore.rs b/src/api2/admin/datastore.rs > index 17e31f1e..f207e5ea 100644 > --- a/src/api2/admin/datastore.rs > +++ b/src/api2/admin/datastore.rs > @@ -272,7 +272,7 @@ fn delete_snapshot( > let allowed =3D (user_privs & PRIV_DATASTORE_MODIFY) !=3D 0; > if !allowed { check_backup_owner(&datastore, snapshot.group(), &user= name)?; } > =20 > - datastore.remove_backup_dir(&snapshot)?; > + datastore.remove_backup_dir(&snapshot, false)?; > =20 > Ok(Value::Null) > } > @@ -660,7 +660,7 @@ fn prune( > })); > =20 > if !(dry_run || keep) { > - datastore.remove_backup_dir(&info.backup_dir)?; > + datastore.remove_backup_dir(&info.backup_dir, true)?; > } > } > =20 > diff --git a/src/api2/backup/environment.rs b/src/api2/backup/environment= .rs > index 2a3632a4..f2ebd24f 100644 > --- a/src/api2/backup/environment.rs > +++ b/src/api2/backup/environment.rs > @@ -479,7 +479,7 @@ impl BackupEnvironment { > let mut state =3D self.state.lock().unwrap(); > state.finished =3D true; > =20 > - self.datastore.remove_backup_dir(&self.backup_dir)?; > + self.datastore.remove_backup_dir(&self.backup_dir, true)?; > =20 > Ok(()) > } > diff --git a/src/backup/backup_info.rs b/src/backup/backup_info.rs > index f8ea11bd..b4f671bd 100644 > --- a/src/backup/backup_info.rs > +++ b/src/backup/backup_info.rs > @@ -173,7 +173,7 @@ impl std::str::FromStr for BackupGroup { > /// Uniquely identify a Backup (relative to data store) > /// > /// We also call this a backup snaphost. > -#[derive(Debug, Clone)] > +#[derive(Debug, Eq, PartialEq, Clone)] > pub struct BackupDir { > /// Backup group > group: BackupGroup, > @@ -317,6 +317,11 @@ impl BackupInfo { > })?; > Ok(list) > } > + > + pub fn is_finished(&self) -> bool { > + // backup is considered unfinished if there is no manifest > + self.files.iter().any(|name| name =3D=3D super::MANIFEST_BLOB_NA= ME) > + } > } > =20 > fn list_backup_files(dirfd: RawFd, path: &P) -= > Result, Error> { > diff --git a/src/backup/datastore.rs b/src/backup/datastore.rs > index 92f7b069..9c2f2851 100644 > --- a/src/backup/datastore.rs > +++ b/src/backup/datastore.rs > @@ -8,7 +8,7 @@ use anyhow::{bail, format_err, Error}; > use lazy_static::lazy_static; > use chrono::{DateTime, Utc}; > =20 > -use super::backup_info::{BackupGroup, BackupDir}; > +use super::backup_info::{BackupGroup, BackupDir, BackupInfo}; > use super::chunk_store::ChunkStore; > use super::dynamic_index::{DynamicIndexReader, DynamicIndexWriter}; > use super::fixed_index::{FixedIndexReader, FixedIndexWriter}; > @@ -197,6 +197,20 @@ impl DataStore { > =20 > let full_path =3D self.group_path(backup_group); > =20 > + let mut snap_list =3D backup_group.list_backups(&self.base_path(= ))?; > + BackupInfo::sort_list(&mut snap_list, false); > + for snap in snap_list { > + if snap.is_finished() { > + break; > + } else { > + bail!( > + "cannot remove backup group {:?}, contains potential= ly running backup: {}", > + full_path, > + snap.backup_dir > + ); > + } > + } > + > log::info!("removing backup group {:?}", full_path); > std::fs::remove_dir_all(&full_path) > .map_err(|err| { > @@ -211,10 +225,35 @@ impl DataStore { > } > =20 > /// Remove a backup directory including all content > - pub fn remove_backup_dir(&self, backup_dir: &BackupDir) -> Result<(= ), Error> { > + pub fn remove_backup_dir(&self, backup_dir: &BackupDir, force: bool)= -> Result<(), Error> { > =20 > let full_path =3D self.snapshot_path(backup_dir); > =20 > + if !force { > + let mut snap_list =3D backup_dir.group().list_backups(&self.= base_path())?; > + BackupInfo::sort_list(&mut snap_list, false); > + let mut prev_snap_finished =3D true; > + for snap in snap_list { > + let cur_snap_finished =3D snap.is_finished(); > + if &snap.backup_dir =3D=3D backup_dir { > + if !cur_snap_finished { > + bail!( > + "cannot remove currently running snapshot: {= :?}", > + backup_dir > + ); > + } > + if !prev_snap_finished { > + bail!( > + "cannot remove snapshot {:?}, successor is c= urrently running and potentially based on it", > + backup_dir > + ); > + } > + break; > + } > + prev_snap_finished =3D cur_snap_finished; > + } > + } > + > log::info!("removing backup snapshot {:?}", full_path); > std::fs::remove_dir_all(&full_path) > .map_err(|err| { > diff --git a/src/backup/prune.rs b/src/backup/prune.rs > index bd7cf3b1..f7a87c5a 100644 > --- a/src/backup/prune.rs > +++ b/src/backup/prune.rs > @@ -53,7 +53,7 @@ fn remove_incomplete_snapshots( > let mut keep_unfinished =3D true; > for info in list.iter() { > // backup is considered unfinished if there is no manifest > - if info.files.iter().any(|name| name =3D=3D super::MANIFEST_BLOB= _NAME) { > + if info.is_finished() { > // There is a new finished backup, so there is no need > // to keep older unfinished backups. > keep_unfinished =3D false; > diff --git a/src/bin/proxmox-backup-proxy.rs b/src/bin/proxmox-backup-pro= xy.rs > index a3033916..dc0d16d2 100644 > --- a/src/bin/proxmox-backup-proxy.rs > +++ b/src/bin/proxmox-backup-proxy.rs > @@ -455,7 +455,7 @@ async fn schedule_datastore_prune() { > BackupDir::backup_time_to_string(info.backup= _dir.backup_time()))); > =20 > if !keep { > - datastore.remove_backup_dir(&info.backup_dir= )?; > + datastore.remove_backup_dir(&info.backup_dir= , true)?; > } > } > } > diff --git a/src/client/pull.rs b/src/client/pull.rs > index 758cb574..c44cb9f6 100644 > --- a/src/client/pull.rs > +++ b/src/client/pull.rs > @@ -277,7 +277,7 @@ pub async fn pull_snapshot_from( > worker.log(format!("sync snapshot {:?}", snapshot.relative_path(= ))); > =20 > if let Err(err) =3D pull_snapshot(worker, reader, tgt_store.clon= e(), &snapshot).await { > - if let Err(cleanup_err) =3D tgt_store.remove_backup_dir(&sna= pshot) { > + if let Err(cleanup_err) =3D tgt_store.remove_backup_dir(&sna= pshot, true) { > worker.log(format!("cleanup error - {}", cleanup_err)); > } > return Err(err); > @@ -362,7 +362,7 @@ pub async fn pull_group( > let backup_time =3D info.backup_dir.backup_time(); > if remote_snapshots.contains(&backup_time) { continue; } > worker.log(format!("delete vanished snapshot {:?}", info.bac= kup_dir.relative_path())); > - tgt_store.remove_backup_dir(&info.backup_dir)?; > + tgt_store.remove_backup_dir(&info.backup_dir, false)?; > } > } > =20 > --=20 > 2.20.1 >=20 >=20 >=20 > _______________________________________________ > pbs-devel mailing list > pbs-devel@lists.proxmox.com > https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel >=20 >=20 >=20 =