Re: [pbs-devel] [PATCH proxmox-backup 2/5] datastore: prevent deletion of snaps in use as "previous backup"

["Fabian Grünbichler" <f.gruenbichler@proxmox.com>]

for the record, this is https://bugzilla.proxmox.com/show_bug.cgi?id=2881

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.
> 
> 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.
> 
> Deleting a single snapshot has a 'force' parameter, which is necessary
> to allow deleting incomplete snapshots on an aborted backup. Pruning
> also sets force=true to avoid the check, since it calculates which
> snapshots to keep on its own.
> 
> To avoid code duplication, the is_finished method is factored out.
> 
> Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>
> ---
>  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(-)
> 
> 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 = (user_privs & PRIV_DATASTORE_MODIFY) != 0;
>      if !allowed { check_backup_owner(&datastore, snapshot.group(), &username)?; }
>  
> -    datastore.remove_backup_dir(&snapshot)?;
> +    datastore.remove_backup_dir(&snapshot, false)?;
>  
>      Ok(Value::Null)
>  }
> @@ -660,7 +660,7 @@ fn prune(
>              }));
>  
>              if !(dry_run || keep) {
> -                datastore.remove_backup_dir(&info.backup_dir)?;
> +                datastore.remove_backup_dir(&info.backup_dir, true)?;
>              }
>          }
>  
> 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 = self.state.lock().unwrap();
>          state.finished = true;
>  
> -        self.datastore.remove_backup_dir(&self.backup_dir)?;
> +        self.datastore.remove_backup_dir(&self.backup_dir, true)?;
>  
>          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 == super::MANIFEST_BLOB_NAME)
> +    }
>  }
>  
>  fn list_backup_files<P: ?Sized + nix::NixPath>(dirfd: RawFd, path: &P) -> Result<Vec<String>, 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};
>  
> -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 {
>  
>          let full_path = self.group_path(backup_group);
>  
> +        let mut snap_list = 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 potentially 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 {
>      }
>  
>      /// 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> {
>  
>          let full_path = self.snapshot_path(backup_dir);
>  
> +        if !force {
> +            let mut snap_list = backup_dir.group().list_backups(&self.base_path())?;
> +            BackupInfo::sort_list(&mut snap_list, false);
> +            let mut prev_snap_finished = true;
> +            for snap in snap_list {
> +                let cur_snap_finished = snap.is_finished();
> +                if &snap.backup_dir == backup_dir {
> +                    if !cur_snap_finished {
> +                        bail!(
> +                            "cannot remove currently running snapshot: {:?}",
> +                            backup_dir
> +                        );
> +                    }
> +                    if !prev_snap_finished {
> +                        bail!(
> +                            "cannot remove snapshot {:?}, successor is currently running and potentially based on it",
> +                            backup_dir
> +                        );
> +                    }
> +                    break;
> +                }
> +                prev_snap_finished = 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 = true;
>      for info in list.iter() {
>          // backup is considered unfinished if there is no manifest
> -        if info.files.iter().any(|name| name == 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 = false;
> diff --git a/src/bin/proxmox-backup-proxy.rs b/src/bin/proxmox-backup-proxy.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())));
>  
>                          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()));
>  
>          if let Err(err) = pull_snapshot(worker, reader, tgt_store.clone(), &snapshot).await {
> -            if let Err(cleanup_err) = tgt_store.remove_backup_dir(&snapshot) {
> +            if let Err(cleanup_err) = tgt_store.remove_backup_dir(&snapshot, true) {
>                  worker.log(format!("cleanup error - {}", cleanup_err));
>              }
>              return Err(err);
> @@ -362,7 +362,7 @@ pub async fn pull_group(
>              let backup_time = info.backup_dir.backup_time();
>              if remote_snapshots.contains(&backup_time) { continue; }
>              worker.log(format!("delete vanished snapshot {:?}", info.backup_dir.relative_path()));
> -            tgt_store.remove_backup_dir(&info.backup_dir)?;
> +            tgt_store.remove_backup_dir(&info.backup_dir, false)?;
>          }
>      }
>  
> -- 
> 2.20.1
> 
> 
> 
> _______________________________________________
> pbs-devel mailing list
> pbs-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
> 
> 
>