Re: [pbs-devel] [PATCH v2 proxmox-backup 7/7] prune: also check backup snapshot locks

[Dietmar Maurer <dietmar@proxmox.com>]

I think this is not required. The old code also works now.

> On 08/11/2020 10:50 AM Stefan Reiter <s.reiter@proxmox.com> wrote:
> 
>  
> ...to avoid pruning running backups or backups used as base for others.
> 
> Unfinished backups that have no lock are considered broken and will
> always be pruned.
> 
> The ignore_locks parameter is used for testing, since flocks are not
> available in test environment.
> 
> Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>
> ---
> 
> Once again, feel free to remove my comments, I just can't wrap my head around
> that logic without them ;)
> 
>  src/api2/admin/datastore.rs     |  2 +-
>  src/backup/prune.rs             | 50 ++++++++++++++++++++++-----------
>  src/bin/proxmox-backup-proxy.rs |  2 +-
>  tests/prune.rs                  |  6 ++--
>  4 files changed, 39 insertions(+), 21 deletions(-)
> 
> diff --git a/src/api2/admin/datastore.rs b/src/api2/admin/datastore.rs
> index fe72ea32..a987416e 100644
> --- a/src/api2/admin/datastore.rs
> +++ b/src/api2/admin/datastore.rs
> @@ -636,7 +636,7 @@ fn prune(
>  
>      let list = group.list_backups(&datastore.base_path())?;
>  
> -    let mut prune_info = compute_prune_info(list, &prune_options)?;
> +    let mut prune_info = compute_prune_info(list, &prune_options, &datastore.base_path(), false)?;
>  
>      prune_info.reverse(); // delete older snapshots first
>  
> diff --git a/src/backup/prune.rs b/src/backup/prune.rs
> index f7a87c5a..f642e7b3 100644
> --- a/src/backup/prune.rs
> +++ b/src/backup/prune.rs
> @@ -1,12 +1,13 @@
>  use anyhow::{Error};
>  use std::collections::{HashMap, HashSet};
> -use std::path::PathBuf;
> +use std::path::{PathBuf, Path};
>  
>  use chrono::{DateTime, Timelike, Datelike, Local};
>  
>  use super::{BackupDir, BackupInfo};
> +use crate::tools::fs::lock_dir_noblock;
>  
> -enum PruneMark { Keep, KeepPartial, Remove }
> +enum PruneMark { Keep, Ignored, Remove }
>  
>  fn mark_selections<F: Fn(DateTime<Local>, &BackupInfo) -> String> (
>      mark: &mut HashMap<PathBuf, PruneMark>,
> @@ -45,26 +46,39 @@ fn mark_selections<F: Fn(DateTime<Local>, &BackupInfo) -> String> (
>      }
>  }
>  
> -fn remove_incomplete_snapshots(
> +fn mark_incomplete_and_in_use_snapshots(
>      mark: &mut HashMap<PathBuf, PruneMark>,
>      list: &Vec<BackupInfo>,
> +    base_path: &Path,
> +    ignore_locks: bool,
>  ) {
>  
> -    let mut keep_unfinished = true;
>      for info in list.iter() {
> -        // backup is considered unfinished if there is no manifest
> -        if info.is_finished() {
> -            // There is a new finished backup, so there is no need
> -            // to keep older unfinished backups.
> -            keep_unfinished = false;
> -        } else {
> -            let backup_id = info.backup_dir.relative_path();
> -            if keep_unfinished { // keep first unfinished
> -                mark.insert(backup_id, PruneMark::KeepPartial);
> -            } else {
> +        let backup_id = info.backup_dir.relative_path();
> +        let mut full_path = base_path.to_owned();
> +        full_path.push(backup_id.clone());
> +
> +        if ignore_locks || lock_dir_noblock(&full_path, "snapshot", "").is_ok() {
> +            if !info.is_finished() {
> +                // incomplete backup, but we can lock it, so it's not running -
> +                // always remove to clean up broken backups
>                  mark.insert(backup_id, PruneMark::Remove);
>              }
> -            keep_unfinished = false;
> +            // if locking succeeds and the backup is complete, let the regular
> +            // marking logic figure it out
> +            // note that we don't need to keep any locks either - any newly
> +            // started backup can only ever use the latest finished backup as
> +            // base, which is kept anyway (since we always keep the latest, and
> +            // this function already filters out any unfinished ones)
> +        } else {
> +            // backups we can't lock we have to ignore - note that this means in
> +            // case a backup is running in a group, the first three snapshots
> +            // will always be kept
> +            // we cannot special case that though, since we don't know if a lock
> +            // is in place for backup or forget, where in the latter case we
> +            // must not mark it as "Keep", as otherwise we might end up with
> +            // no backup for a given selection period
> +            mark.insert(backup_id, PruneMark::Ignored);
>          }
>      }
>  }
> @@ -173,13 +187,15 @@ impl PruneOptions {
>  pub fn compute_prune_info(
>      mut list: Vec<BackupInfo>,
>      options: &PruneOptions,
> +    base_path: &Path,
> +    ignore_locks: bool,
>  ) -> Result<Vec<(BackupInfo, bool)>, Error> {
>  
>      let mut mark = HashMap::new();
>  
>      BackupInfo::sort_list(&mut list, false);
>  
> -    remove_incomplete_snapshots(&mut mark, &list);
> +    mark_incomplete_and_in_use_snapshots(&mut mark, &list, base_path, ignore_locks);
>  
>      if let Some(keep_last) = options.keep_last {
>          mark_selections(&mut mark, &list, keep_last as usize, |_local_time, info| {
> @@ -227,7 +243,7 @@ pub fn compute_prune_info(
>              let backup_id = info.backup_dir.relative_path();
>              let keep = match mark.get(&backup_id) {
>                  Some(PruneMark::Keep) => true,
> -                Some(PruneMark::KeepPartial) => true,
> +                Some(PruneMark::Ignored) => true,
>                 _ => false,
>              };
>              (info, keep)
> diff --git a/src/bin/proxmox-backup-proxy.rs b/src/bin/proxmox-backup-proxy.rs
> index 3f7bf3ec..8cd2e105 100644
> --- a/src/bin/proxmox-backup-proxy.rs
> +++ b/src/bin/proxmox-backup-proxy.rs
> @@ -442,7 +442,7 @@ async fn schedule_datastore_prune() {
>                  let groups = BackupGroup::list_groups(&base_path)?;
>                  for group in groups {
>                      let list = group.list_backups(&base_path)?;
> -                    let mut prune_info = compute_prune_info(list, &prune_options)?;
> +                    let mut prune_info = compute_prune_info(list, &prune_options, &base_path, false)?;
>                      prune_info.reverse(); // delete older snapshots first
>  
>                      worker.log(format!("Starting prune on store \"{}\" group \"{}/{}\"",
> diff --git a/tests/prune.rs b/tests/prune.rs
> index d9758ea7..31f48970 100644
> --- a/tests/prune.rs
> +++ b/tests/prune.rs
> @@ -1,5 +1,5 @@
>  use anyhow::{Error};
> -use std::path::PathBuf;
> +use std::path::{PathBuf, Path};
>  
>  use proxmox_backup::backup::*;
>  
> @@ -9,7 +9,9 @@ fn get_prune_list(
>      options: &PruneOptions,
>  ) -> Vec<PathBuf> {
>  
> -    let mut prune_info = compute_prune_info(list, options).unwrap();
> +    // ignore locks, would always fail in test environment
> +    // base_path is only used for locking so leave empty
> +    let mut prune_info = compute_prune_info(list, options, &Path::new(""), true).unwrap();
>  
>      prune_info.reverse();
>  
> -- 
> 2.20.1
> 
> 
> 
> _______________________________________________
> pbs-devel mailing list
> pbs-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel