* [pbs-devel] [PATCH proxmox-backup 1/2] api: tape/restore: fix wrong datastore locking
@ 2022-05-09 10:41 Dominik Csapak
2022-05-09 10:41 ` [pbs-devel] [PATCH proxmox-backup 2/2] api: tape/restore: skip snapshot if owner check failed Dominik Csapak
2022-05-09 11:57 ` [pbs-devel] applied-series: Re: [PATCH proxmox-backup 1/2] api: tape/restore: fix wrong datastore locking Thomas Lamprecht
0 siblings, 2 replies; 3+ messages in thread
From: Dominik Csapak @ 2022-05-09 10:41 UTC (permalink / raw)
To: pbs-devel
used_datastores returned the 'target', but in the full_restore_worker,
we interpreted it as the source and searched for a mapping
(which we then locked)
since we cannot return a HashSet of Arc<T> (missing Hash trait on DataStore),
we have now a map of source -> target
Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
src/api2/tape/restore.rs | 28 +++++++++++++---------------
1 file changed, 13 insertions(+), 15 deletions(-)
diff --git a/src/api2/tape/restore.rs b/src/api2/tape/restore.rs
index 93803f14..f24ae23c 100644
--- a/src/api2/tape/restore.rs
+++ b/src/api2/tape/restore.rs
@@ -88,17 +88,17 @@ impl TryFrom<String> for DataStoreMap {
}
impl DataStoreMap {
- fn used_datastores<'a>(&self) -> HashSet<&str> {
- let mut set = HashSet::new();
- for store in self.map.values() {
- set.insert(store.name());
+ fn used_datastores<'a>(&self) -> HashMap<&str, Arc<DataStore>> {
+ let mut map = HashMap::new();
+ for (source, target) in self.map.iter() {
+ map.insert(source.as_str(), Arc::clone(target));
}
if let Some(ref store) = self.default {
- set.insert(store.name());
+ map.insert("", Arc::clone(store));
}
- set
+ map
}
fn get_datastore(&self, source: &str) -> Option<Arc<DataStore>> {
@@ -200,8 +200,8 @@ pub fn restore(
bail!("no datastores given");
}
- for store in used_datastores.iter() {
- check_datastore_privs(&user_info, store, &auth_id, &owner)?;
+ for (_, target) in used_datastores.iter() {
+ check_datastore_privs(&user_info, target.name(), &auth_id, &owner)?;
}
let privs = user_info.lookup_privs(&auth_id, &["tape", "drive", &drive]);
@@ -233,7 +233,7 @@ pub fn restore(
let taskid = used_datastores
.iter()
- .map(|s| s.to_string())
+ .map(|(_, t)| t.name().to_string())
.collect::<Vec<String>>()
.join(", ");
@@ -349,7 +349,7 @@ fn restore_full_worker(
store_map
.used_datastores()
.into_iter()
- .map(String::from)
+ .map(|(_, t)| String::from(t.name()))
.collect::<Vec<String>>()
.join(", "),
);
@@ -366,12 +366,10 @@ fn restore_full_worker(
);
let mut datastore_locks = Vec::new();
- for store_name in store_map.used_datastores() {
+ for (_, target) in store_map.used_datastores() {
// explicit create shared lock to prevent GC on newly created chunks
- if let Some(store) = store_map.get_datastore(store_name) {
- let shared_store_lock = store.try_shared_chunk_store_lock()?;
- datastore_locks.push(shared_store_lock);
- }
+ let shared_store_lock = target.try_shared_chunk_store_lock()?;
+ datastore_locks.push(shared_store_lock);
}
let mut checked_chunks_map = HashMap::new();
--
2.30.2
^ permalink raw reply [flat|nested] 3+ messages in thread
* [pbs-devel] [PATCH proxmox-backup 2/2] api: tape/restore: skip snapshot if owner check failed
2022-05-09 10:41 [pbs-devel] [PATCH proxmox-backup 1/2] api: tape/restore: fix wrong datastore locking Dominik Csapak
@ 2022-05-09 10:41 ` Dominik Csapak
2022-05-09 11:57 ` [pbs-devel] applied-series: Re: [PATCH proxmox-backup 1/2] api: tape/restore: fix wrong datastore locking Thomas Lamprecht
1 sibling, 0 replies; 3+ messages in thread
From: Dominik Csapak @ 2022-05-09 10:41 UTC (permalink / raw)
To: pbs-devel
instead of aborting the whole restore
Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
src/api2/tape/restore.rs | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/src/api2/tape/restore.rs b/src/api2/tape/restore.rs
index f24ae23c..a7ed119a 100644
--- a/src/api2/tape/restore.rs
+++ b/src/api2/tape/restore.rs
@@ -433,12 +433,14 @@ fn restore_list_worker(
datastore.create_locked_backup_group(backup_dir.as_ref(), restore_owner)?;
if restore_owner != &owner {
// only the owner is allowed to create additional snapshots
- bail!(
+ task_warn!(
+ worker,
"restore '{}' failed - owner check failed ({} != {})",
snapshot,
restore_owner,
owner
);
+ continue;
}
let (media_id, file_num) = if let Some((media_uuid, file_num)) =
--
2.30.2
^ permalink raw reply [flat|nested] 3+ messages in thread
* [pbs-devel] applied-series: Re: [PATCH proxmox-backup 1/2] api: tape/restore: fix wrong datastore locking
2022-05-09 10:41 [pbs-devel] [PATCH proxmox-backup 1/2] api: tape/restore: fix wrong datastore locking Dominik Csapak
2022-05-09 10:41 ` [pbs-devel] [PATCH proxmox-backup 2/2] api: tape/restore: skip snapshot if owner check failed Dominik Csapak
@ 2022-05-09 11:57 ` Thomas Lamprecht
1 sibling, 0 replies; 3+ messages in thread
From: Thomas Lamprecht @ 2022-05-09 11:57 UTC (permalink / raw)
To: Proxmox Backup Server development discussion, Dominik Csapak
On 09/05/2022 12:41, Dominik Csapak wrote:
> used_datastores returned the 'target', but in the full_restore_worker,
> we interpreted it as the source and searched for a mapping
> (which we then locked)
>
> since we cannot return a HashSet of Arc<T> (missing Hash trait on DataStore),
> we have now a map of source -> target
>
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
> src/api2/tape/restore.rs | 28 +++++++++++++---------------
> 1 file changed, 13 insertions(+), 15 deletions(-)
>
>
applied both patches, thanks!
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2022-05-09 11:57 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-05-09 10:41 [pbs-devel] [PATCH proxmox-backup 1/2] api: tape/restore: fix wrong datastore locking Dominik Csapak
2022-05-09 10:41 ` [pbs-devel] [PATCH proxmox-backup 2/2] api: tape/restore: skip snapshot if owner check failed Dominik Csapak
2022-05-09 11:57 ` [pbs-devel] applied-series: Re: [PATCH proxmox-backup 1/2] api: tape/restore: fix wrong datastore locking Thomas Lamprecht
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox