From: "Fabian Grünbichler" <f.gruenbichler@proxmox.com>
To: pbs-devel@lists.proxmox.com
Subject: [pbs-devel] [PATCH proxmox-backup 1/5] improve group/snapshot listing
Date: Thu, 12 Nov 2020 11:30:30 +0100 [thread overview]
Message-ID: <20201112103034.159849-2-f.gruenbichler@proxmox.com> (raw)
In-Reply-To: <20201112103034.159849-1-f.gruenbichler@proxmox.com>
by listing groups first, then filtering, then listing group snapshots.
this cuts down the number of openat/getdirents calls for users that just
have a partial view of the datastore.
Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
---
Notes:
error behaviour should remain the same, unless I missed something..
src/api2/admin/datastore.rs | 200 ++++++++++++++++++++----------------
src/backup/backup_info.rs | 28 +++--
2 files changed, 131 insertions(+), 97 deletions(-)
diff --git a/src/api2/admin/datastore.rs b/src/api2/admin/datastore.rs
index e76867c7..df666156 100644
--- a/src/api2/admin/datastore.rs
+++ b/src/api2/admin/datastore.rs
@@ -1,4 +1,4 @@
-use std::collections::{HashSet, HashMap};
+use std::collections::HashSet;
use std::ffi::OsStr;
use std::os::unix::ffi::OsStrExt;
use std::sync::{Arc, Mutex};
@@ -125,19 +125,6 @@ fn get_all_snapshot_files(
Ok((manifest, files))
}
-fn group_backups(backup_list: Vec<BackupInfo>) -> HashMap<String, Vec<BackupInfo>> {
-
- let mut group_hash = HashMap::new();
-
- for info in backup_list {
- let group_id = info.backup_dir.group().group_path().to_str().unwrap().to_owned();
- let time_list = group_hash.entry(group_id).or_insert(vec![]);
- time_list.push(info);
- }
-
- group_hash
-}
-
#[api(
input: {
properties: {
@@ -171,45 +158,61 @@ fn list_groups(
let user_privs = user_info.lookup_privs(&auth_id, &["datastore", &store]);
let datastore = DataStore::lookup_datastore(&store)?;
+ let list_all = (user_privs & PRIV_DATASTORE_AUDIT) != 0;
+
+ let backup_groups = BackupInfo::list_backup_groups(&datastore.base_path())?;
+
+ let group_info = backup_groups
+ .into_iter()
+ .fold(Vec::new(), |mut group_info, group| {
+ let owner = match datastore.get_owner(&group) {
+ Ok(auth_id) => auth_id,
+ Err(err) => {
+ println!("Failed to get owner of group '{}' - {}", group, err);
+ return group_info;
+ },
+ };
+ if !list_all && check_backup_owner(&owner, &auth_id).is_err() {
+ return group_info;
+ }
- let backup_list = BackupInfo::list_backups(&datastore.base_path())?;
-
- let group_hash = group_backups(backup_list);
-
- let mut groups = Vec::new();
-
- for (_group_id, mut list) in group_hash {
-
- BackupInfo::sort_list(&mut list, false);
-
- let info = &list[0];
+ let snapshots = match group.list_backups(&datastore.base_path()) {
+ Ok(snapshots) => snapshots,
+ Err(_) => {
+ return group_info;
+ },
+ };
- let group = info.backup_dir.group();
+ let backup_count: u64 = snapshots.len() as u64;
+ if backup_count == 0 {
+ return group_info;
+ }
- let list_all = (user_privs & PRIV_DATASTORE_AUDIT) != 0;
- let owner = match datastore.get_owner(group) {
- Ok(auth_id) => auth_id,
- Err(err) => {
- println!("Failed to get owner of group '{}' - {}", group, err);
- continue;
- },
- };
- if !list_all && check_backup_owner(&owner, &auth_id).is_err() {
- continue;
- }
+ let last_backup = snapshots
+ .iter()
+ .fold(&snapshots[0], |last, curr| {
+ if curr.is_finished()
+ && curr.backup_dir.backup_time() > last.backup_dir.backup_time() {
+ curr
+ } else {
+ last
+ }
+ })
+ .to_owned();
+
+ group_info.push(GroupListItem {
+ backup_type: group.backup_type().to_string(),
+ backup_id: group.backup_id().to_string(),
+ last_backup: last_backup.backup_dir.backup_time(),
+ owner: Some(owner),
+ backup_count,
+ files: last_backup.files,
+ });
- let result_item = GroupListItem {
- backup_type: group.backup_type().to_string(),
- backup_id: group.backup_id().to_string(),
- last_backup: info.backup_dir.backup_time(),
- backup_count: list.len() as u64,
- files: info.files.clone(),
- owner: Some(owner),
- };
- groups.push(result_item);
- }
+ group_info
+ });
- Ok(groups)
+ Ok(group_info)
}
#[api(
@@ -357,41 +360,38 @@ pub fn list_snapshots (
let user_info = CachedUserInfo::new()?;
let user_privs = user_info.lookup_privs(&auth_id, &["datastore", &store]);
+ let list_all = (user_privs & PRIV_DATASTORE_AUDIT) != 0;
+
let datastore = DataStore::lookup_datastore(&store)?;
let base_path = datastore.base_path();
- let backup_list = BackupInfo::list_backups(&base_path)?;
-
- let mut snapshots = vec![];
-
- for info in backup_list {
- let group = info.backup_dir.group();
- if let Some(ref backup_type) = backup_type {
- if backup_type != group.backup_type() { continue; }
- }
- if let Some(ref backup_id) = backup_id {
- if backup_id != group.backup_id() { continue; }
- }
-
- let list_all = (user_privs & PRIV_DATASTORE_AUDIT) != 0;
- let owner = match datastore.get_owner(group) {
- Ok(auth_id) => auth_id,
- Err(err) => {
- println!("Failed to get owner of group '{}' - {}", group, err);
- continue;
- },
- };
-
- if !list_all && check_backup_owner(&owner, &auth_id).is_err() {
- continue;
- }
+ let groups = match (backup_type, backup_id) {
+ (Some(backup_type), Some(backup_id)) => {
+ let mut groups = Vec::with_capacity(1);
+ groups.push(BackupGroup::new(backup_type, backup_id));
+ groups
+ },
+ (Some(backup_type), None) => {
+ BackupInfo::list_backup_groups(&base_path)?
+ .into_iter()
+ .filter(|group| group.backup_type() == backup_type)
+ .collect()
+ },
+ (None, Some(backup_id)) => {
+ BackupInfo::list_backup_groups(&base_path)?
+ .into_iter()
+ .filter(|group| group.backup_id() == backup_id)
+ .collect()
+ },
+ _ => BackupInfo::list_backup_groups(&base_path)?,
+ };
- let mut size = None;
+ let info_to_snapshot_list_item = |group: &BackupGroup, owner, info: BackupInfo| {
+ let backup_time = info.backup_dir.backup_time();
- let (comment, verification, files) = match get_all_snapshot_files(&datastore, &info) {
+ let (comment, verification, files, size) = match get_all_snapshot_files(&datastore, &info) {
Ok((manifest, files)) => {
- size = Some(files.iter().map(|x| x.size.unwrap_or(0)).sum());
// extract the first line from notes
let comment: Option<String> = manifest.unprotected["notes"]
.as_str()
@@ -407,7 +407,9 @@ pub fn list_snapshots (
}
};
- (comment, verify, files)
+ let size = Some(files.iter().map(|x| x.size.unwrap_or(0)).sum());
+
+ (comment, verify, files, size)
},
Err(err) => {
eprintln!("error during snapshot file listing: '{}'", err);
@@ -416,32 +418,58 @@ pub fn list_snapshots (
None,
info
.files
- .iter()
+ .into_iter()
.map(|x| BackupContent {
filename: x.to_string(),
size: None,
crypt_mode: None,
})
- .collect()
+ .collect(),
+ None,
)
},
};
- let result_item = SnapshotListItem {
+ SnapshotListItem {
backup_type: group.backup_type().to_string(),
backup_id: group.backup_id().to_string(),
- backup_time: info.backup_dir.backup_time(),
+ backup_time,
comment,
verification,
files,
size,
- owner: Some(owner),
- };
+ owner,
+ }
+ };
- snapshots.push(result_item);
- }
+ groups
+ .iter()
+ .try_fold(Vec::new(), |mut snapshots, group| {
+ let owner = match datastore.get_owner(group) {
+ Ok(auth_id) => auth_id,
+ Err(err) => {
+ eprintln!("Failed to get owner of group '{}/{}' - {}",
+ &store,
+ group,
+ err);
+ return Ok(snapshots);
+ },
+ };
+
+ if !list_all && check_backup_owner(&owner, &auth_id).is_err() {
+ return Ok(snapshots);
+ }
+
+ let group_backups = group.list_backups(&datastore.base_path())?;
+
+ snapshots.extend(
+ group_backups
+ .into_iter()
+ .map(|info| info_to_snapshot_list_item(&group, Some(owner.clone()), info))
+ );
- Ok(snapshots)
+ Ok(snapshots)
+ })
}
fn get_snapshots_count(store: &DataStore) -> Result<Counts, Error> {
diff --git a/src/backup/backup_info.rs b/src/backup/backup_info.rs
index 59849848..ebb5325b 100644
--- a/src/backup/backup_info.rs
+++ b/src/backup/backup_info.rs
@@ -327,29 +327,35 @@ impl BackupInfo {
Ok(files)
}
- pub fn list_backups(base_path: &Path) -> Result<Vec<BackupInfo>, Error> {
+ pub fn list_backup_groups(base_path: &Path) -> Result<Vec<BackupGroup>, Error> {
let mut list = Vec::new();
tools::scandir(libc::AT_FDCWD, base_path, &BACKUP_TYPE_REGEX, |l0_fd, backup_type, file_type| {
if file_type != nix::dir::Type::Directory { return Ok(()); }
- tools::scandir(l0_fd, backup_type, &BACKUP_ID_REGEX, |l1_fd, backup_id, file_type| {
+ tools::scandir(l0_fd, backup_type, &BACKUP_ID_REGEX, |_, backup_id, file_type| {
if file_type != nix::dir::Type::Directory { return Ok(()); }
- tools::scandir(l1_fd, backup_id, &BACKUP_DATE_REGEX, |l2_fd, backup_time_string, file_type| {
- if file_type != nix::dir::Type::Directory { return Ok(()); }
-
- let backup_dir = BackupDir::with_rfc3339(backup_type, backup_id, backup_time_string)?;
-
- let files = list_backup_files(l2_fd, backup_time_string)?;
- list.push(BackupInfo { backup_dir, files });
+ list.push(BackupGroup::new(backup_type, backup_id));
- Ok(())
- })
+ Ok(())
})
})?;
+
Ok(list)
}
+ pub fn list_backups(base_path: &Path) -> Result<Vec<BackupInfo>, Error> {
+ let groups = BackupInfo::list_backup_groups(base_path)?;
+
+ groups
+ .into_iter()
+ .try_fold(Vec::new(), |mut snapshots, group| {
+ let group_snapshots = group.list_backups(base_path)?;
+ snapshots.extend(group_snapshots);
+ Ok(snapshots)
+ })
+ }
+
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)
--
2.20.1
next prev parent reply other threads:[~2020-11-12 10:31 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-11-12 10:30 [pbs-devel] [PATCH proxmox-backup 0/5] improve listing API Fabian Grünbichler
2020-11-12 10:30 ` Fabian Grünbichler [this message]
2020-11-12 10:30 ` [pbs-devel] [PATCH proxmox-backup 2/5] api: make expensive parts of datastore status opt-in Fabian Grünbichler
2020-11-12 10:30 ` [pbs-devel] [PATCH proxmox-backup 3/5] api: filter snapshot counts Fabian Grünbichler
2020-11-12 10:30 ` [pbs-devel] [PATCH proxmox-backup 4/5] drop now unused BackupInfo::list_backups Fabian Grünbichler
2020-11-12 10:30 ` [pbs-devel] [PATCH proxmox-backup 5/5] api: include store in invalid owner errors Fabian Grünbichler
2020-11-18 10:14 ` [pbs-devel] applied series: [PATCH proxmox-backup 0/5] improve listing API Wolfgang Bumiller
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20201112103034.159849-2-f.gruenbichler@proxmox.com \
--to=f.gruenbichler@proxmox.com \
--cc=pbs-devel@lists.proxmox.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox