diff --git a/src/api2/admin/datastore.rs b/src/api2/admin/datastore.rs index a0f71eba..c720231c 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) } @@ -661,7 +661,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(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}; -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)?; } }