[pbs-devel] [PATCH v2 proxmox-backup 7/7] prune: also check backup snapshot locks
Dietmar Maurer
dietmar at proxmox.com
Tue Aug 11 11:35:04 CEST 2020
I think this is not required. The old code also works now.
> On 08/11/2020 10:50 AM Stefan Reiter <s.reiter at 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 at 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 at lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
More information about the pbs-devel
mailing list