[pve-devel] [PATCH v2 proxmox-backup 00/10] pull/sync group filter

[pve-devel] [PATCH v2 proxmox-backup 00/10] pull/sync group filter

[Fabian Grünbichler]

this has been requested a few times on the forum, e.g. for a special
sync job for the most important groups, or seeding of a new datastore
with a partial view of an existing one.

while it's possible to achieve similar results with hacky workarounds
based on group ownership and reduced "visibility", implementing it
properly is not that complex.

possible future additions in a similar fashion:
    - exclude filters
    - filtering in other API calls (tape, listing groups/snapshots)
    - only sync/pull encrypted snapshots (less trusted off-site
      location)
    - only sync/pull latest snapshot in each group (fast seeding of new
      datastore)

changed since v1:
- reworked filter to support different types, rebased
- dropped last patch
- add docs patch

Fabian Grünbichler (10):
  api-types: add schema for backup group
  api: add GroupFilter(List) type
  BackupGroup: add filter helper
  pull: use BackupGroup consistently
  pull: allow pulling groups selectively
  sync: add group filtering
  remote: add backup group scanning
  manager: extend sync/pull completion
  manager: render group filter properly
  docs: mention group filter in sync docs

 docs/managing-remotes.rst              |   6 ++
 pbs-api-types/src/datastore.rs         |   5 ++
 pbs-api-types/src/jobs.rs              |  96 +++++++++++++++++++++
 pbs-datastore/src/backup_info.rs       |  10 +++
 src/api2/config/remote.rs              |  73 +++++++++++++++-
 src/api2/config/sync.rs                |   5 ++
 src/api2/pull.rs                       |  15 +++-
 src/bin/proxmox-backup-manager.rs      | 113 ++++++++++++++++++++++---
 src/bin/proxmox_backup_manager/sync.rs |  21 +++++
 src/server/pull.rs                     |  57 ++++++++++---
 www/config/SyncView.js                 |  13 ++-
 www/window/SyncJobEdit.js              |  12 +++
 12 files changed, 394 insertions(+), 32 deletions(-)

-- 
2.30.2

[pve-devel] [PATCH v2 proxmox-backup 01/10] api-types: add schema for backup group

[Fabian Grünbichler]

the regex was already there, and we need a simple type/schema for
passing in multiple groups as Vec/Array via the API.

Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
---
 pbs-api-types/src/datastore.rs | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/pbs-api-types/src/datastore.rs b/pbs-api-types/src/datastore.rs
index 75f82ea4..bf3fad73 100644
--- a/pbs-api-types/src/datastore.rs
+++ b/pbs-api-types/src/datastore.rs
@@ -43,6 +43,7 @@ pub const BACKUP_ARCHIVE_NAME_SCHEMA: Schema = StringSchema::new("Backup archive
     .schema();
 
 pub const BACKUP_ID_FORMAT: ApiStringFormat = ApiStringFormat::Pattern(&BACKUP_ID_REGEX);
+pub const BACKUP_GROUP_FORMAT: ApiStringFormat = ApiStringFormat::Pattern(&GROUP_PATH_REGEX);
 
 pub const BACKUP_ID_SCHEMA: Schema = StringSchema::new("Backup ID.")
     .format(&BACKUP_ID_FORMAT)
@@ -60,6 +61,10 @@ pub const BACKUP_TIME_SCHEMA: Schema = IntegerSchema::new("Backup time (Unix epo
     .minimum(1_547_797_308)
     .schema();
 
+pub const BACKUP_GROUP_SCHEMA: Schema = StringSchema::new("Backup Group")
+    .format(&BACKUP_GROUP_FORMAT)
+    .schema();
+
 pub const DATASTORE_SCHEMA: Schema = StringSchema::new("Datastore name.")
     .format(&PROXMOX_SAFE_ID_FORMAT)
     .min_length(3)
-- 
2.30.2

[pve-devel] [PATCH v2 proxmox-backup 02/10] api: add GroupFilter(List) type

[Fabian Grünbichler]

at the API level, this is a simple (wrapped) Vec of Strings with a
verifier function. all users should use the provided helper to get the
actual GroupFilter enum values, which can't be directly used in the API
schema because of restrictions of the api macro.

validation of the schema + parsing into the proper type uses the same fn
intentionally to avoid running out of sync, even if it means compiling
the REs twice.

Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
---
this is working around some api limitations
- updated doesn't support a Vec<String> type
- api-macro doesn't support enum with values

the validation fn and the parser/converted use the same code to avoid
getting out of sync, at the expense of parsing potentially expensive REs
twice..

 pbs-api-types/src/jobs.rs | 90 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 90 insertions(+)

diff --git a/pbs-api-types/src/jobs.rs b/pbs-api-types/src/jobs.rs
index 1526dbc4..94638fe5 100644
--- a/pbs-api-types/src/jobs.rs
+++ b/pbs-api-types/src/jobs.rs
@@ -1,3 +1,7 @@
+use anyhow::format_err;
+use std::str::FromStr;
+
+use regex::Regex;
 use serde::{Deserialize, Serialize};
 
 use proxmox::const_regex;
@@ -7,6 +11,7 @@ use proxmox::api::{api, schema::*};
 use crate::{
     Userid, Authid, REMOTE_ID_SCHEMA, DRIVE_NAME_SCHEMA, MEDIA_POOL_NAME_SCHEMA,
     SINGLE_LINE_COMMENT_SCHEMA, PROXMOX_SAFE_ID_FORMAT, DATASTORE_SCHEMA,
+    BACKUP_GROUP_SCHEMA, BACKUP_TYPE_SCHEMA,
 };
 
 const_regex!{
@@ -319,6 +324,91 @@ pub struct TapeBackupJobStatus {
     pub next_media_label: Option<String>,
 }
 
+#[derive(Clone, Debug)]
+/// Filter for matching `BackupGroup`s, for use with `BackupGroup::filter`.
+pub enum GroupFilter {
+    /// BackupGroup type - either `vm`, `ct`, or `host`.
+    BackupType(String),
+    /// Full identifier of BackupGroup, including type
+    Group(String),
+    /// A regular expression matched against the full identifier of the BackupGroup
+    Regex(Regex),
+}
+
+impl std::str::FromStr for GroupFilter {
+    type Err = anyhow::Error;
+
+    fn from_str(s: &str) -> Result<Self, Self::Err> {
+        match s.split_once(":") {
+            Some(("group", value)) => parse_simple_value(value, &BACKUP_GROUP_SCHEMA).map(|_| GroupFilter::Group(value.to_string())),
+            Some(("type", value)) => parse_simple_value(value, &BACKUP_TYPE_SCHEMA).map(|_| GroupFilter::BackupType(value.to_string())),
+            Some(("regex", value)) => Ok(GroupFilter::Regex(Regex::new(value)?)),
+            Some((ty, _value)) => Err(format_err!("expected 'group', 'type' or 'regex' prefix, got '{}'", ty)),
+            None => Err(format_err!("input doesn't match expected format '<group:GROUP||type:<vm|ct|host>|regex:REGEX>'")),
+        }.map_err(|err| format_err!("'{}' - {}", s, err))
+    }
+}
+
+// used for serializing below, caution!
+impl std::fmt::Display for GroupFilter {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        match self {
+            GroupFilter::BackupType(backup_type) => write!(f, "type:{}", backup_type),
+            GroupFilter::Group(backup_group) => write!(f, "group:{}", backup_group),
+            GroupFilter::Regex(regex) => write!(f, "regex:{}", regex.as_str()),
+        }
+    }
+}
+
+impl Serialize for GroupFilter {
+    fn serialize<S>(&self, serializer: S) -> Result<S::Ok, S::Error>
+    where
+        S: serde::Serializer,
+    {
+        serializer.serialize_str(&self.to_string())
+    }
+}
+
+impl<'de> Deserialize<'de> for GroupFilter {
+    fn deserialize<D>(deserializer: D) -> Result<GroupFilter, D::Error>
+    where
+        D: serde::Deserializer<'de>,
+    {
+        String::deserialize(deserializer).and_then(|string| {
+            GroupFilter::from_str(&string).map_err(serde::de::Error::custom)
+        })
+    }
+}
+
+fn verify_group_filter(input: &str) -> Result<(), anyhow::Error> {
+    GroupFilter::from_str(input).map(|_| ())
+}
+
+pub const GROUP_FILTER_SCHEMA: Schema = StringSchema::new(
+    "Group filter based on group identifier ('group:GROUP'), group type ('type:<vm|ct|host>'), or regex ('regex:RE').")
+    .format(&ApiStringFormat::VerifyFn(verify_group_filter))
+    .type_text("<type:<vm|ct|host>|group:GROUP|regex:RE>")
+    .schema();
+
+#[api(
+    type: Array,
+    description: "List of group filters.",
+    items: {
+        schema: GROUP_FILTER_SCHEMA,
+    },
+)]
+#[derive(Serialize, Deserialize, Clone, UpdaterType)]
+pub struct GroupFilterList(Vec<String>);
+
+impl GroupFilterList {
+    /// converts schema-validated Vec<String> into Vec<GroupFilter>
+    pub fn filters(self) -> Vec<GroupFilter> {
+        self.0.iter()
+             .map(|group_filter| GroupFilter::from_str(group_filter).unwrap())
+             .collect()
+    }
+}
+
 #[api(
     properties: {
         id: {
-- 
2.30.2

[pve-devel] [PATCH v2 proxmox-backup 03/10] BackupGroup: add filter helper

[Fabian Grünbichler]

to have a single implementation of "group is matched by group filter".

Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
---

Notes:
    there might be a better place for this if we want to support more complex
    filters in the future (like, exists in local datastore, or has > X snapshots,
    ..)

 pbs-datastore/src/backup_info.rs | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/pbs-datastore/src/backup_info.rs b/pbs-datastore/src/backup_info.rs
index f77098ee..416a5e32 100644
--- a/pbs-datastore/src/backup_info.rs
+++ b/pbs-datastore/src/backup_info.rs
@@ -1,5 +1,6 @@
 use std::os::unix::io::RawFd;
 use std::path::{Path, PathBuf};
+use std::str::FromStr;
 
 use anyhow::{bail, format_err, Error};
 
@@ -10,6 +11,7 @@ use pbs_api_types::{
     GROUP_PATH_REGEX,
     SNAPSHOT_PATH_REGEX,
     BACKUP_FILE_REGEX,
+    GroupFilter,
 };
 
 use super::manifest::MANIFEST_BLOB_NAME;
@@ -153,6 +155,14 @@ impl BackupGroup {
 
         Ok(last)
     }
+
+    pub fn filter(&self, filter: &GroupFilter) -> bool {
+        match filter {
+            GroupFilter::Group(backup_group) => &BackupGroup::from_str(&backup_group).unwrap() == self,
+            GroupFilter::BackupType(backup_type) => self.backup_type() == backup_type,
+            GroupFilter::Regex(regex) => regex.is_match(&format!("{}", self)),
+        }
+    }
 }
 
 impl std::fmt::Display for BackupGroup {
-- 
2.30.2

[pve-devel] [PATCH v2 proxmox-backup 04/10] pull: use BackupGroup consistently

[Fabian Grünbichler]

instead of `GroupListItem`s. we convert it anyway, so might as well do
that at the start.

Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
---
 src/server/pull.rs | 25 ++++++++++++++-----------
 1 file changed, 14 insertions(+), 11 deletions(-)

diff --git a/src/server/pull.rs b/src/server/pull.rs
index 5214a218..4e82df1a 100644
--- a/src/server/pull.rs
+++ b/src/server/pull.rs
@@ -655,28 +655,31 @@ pub async fn pull_store(
         }
     });
 
+    let list:Vec<BackupGroup> = list
+        .into_iter()
+        .map(|item| BackupGroup::new(item.backup_type, item.backup_id))
+        .collect();
+
     let mut errors = false;
 
     let mut new_groups = std::collections::HashSet::new();
-    for item in list.iter() {
-        new_groups.insert(BackupGroup::new(&item.backup_type, &item.backup_id));
+    for group in list.iter() {
+        new_groups.insert(group.clone());
     }
 
     let mut progress = StoreProgress::new(list.len() as u64);
 
-    for (done, item) in list.into_iter().enumerate() {
+    for (done, group) in list.into_iter().enumerate() {
         progress.done_groups = done as u64;
         progress.done_snapshots = 0;
         progress.group_snapshots = 0;
 
-        let group = BackupGroup::new(&item.backup_type, &item.backup_id);
-
         let (owner, _lock_guard) = match tgt_store.create_locked_backup_group(&group, &auth_id) {
             Ok(result) => result,
             Err(err) => {
                 worker.log(format!(
-                    "sync group {}/{} failed - group lock failed: {}",
-                    item.backup_type, item.backup_id, err
+                    "sync group {} failed - group lock failed: {}",
+                    &group, err
                 ));
                 errors = true; // do not stop here, instead continue
                 continue;
@@ -687,8 +690,8 @@ pub async fn pull_store(
         if auth_id != owner {
             // only the owner is allowed to create additional snapshots
             worker.log(format!(
-                "sync group {}/{} failed - owner check failed ({} != {})",
-                item.backup_type, item.backup_id, auth_id, owner
+                "sync group {} failed - owner check failed ({} != {})",
+                &group, auth_id, owner
             ));
             errors = true; // do not stop here, instead continue
         } else if let Err(err) = pull_group(
@@ -703,8 +706,8 @@ pub async fn pull_store(
         .await
         {
             worker.log(format!(
-                "sync group {}/{} failed - {}",
-                item.backup_type, item.backup_id, err,
+                "sync group {} failed - {}",
+                &group, err,
             ));
             errors = true; // do not stop here, instead continue
         }
-- 
2.30.2

[pve-devel] [PATCH v2 proxmox-backup 05/10] pull: allow pulling groups selectively

[Fabian Grünbichler]

without requiring workarounds based on ownership and limited
visibility/access.

if a group filter is set, remove_vanished will only consider filtered
groups for removal to prevent concurrent disjunct filters from trashing
eachother's synced groups.

Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
---
other name options:
- `group-filter` (with `exclude` boolean as extension?)
- `include-groups` (with `exclude-groups` filter list as extension?)

 src/api2/pull.rs                  | 13 ++++++++++---
 src/bin/proxmox-backup-manager.rs | 10 ++++++++++
 src/server/pull.rs                | 32 ++++++++++++++++++++++++++++---
 3 files changed, 49 insertions(+), 6 deletions(-)

diff --git a/src/api2/pull.rs b/src/api2/pull.rs
index d7b155a1..dfa6a938 100644
--- a/src/api2/pull.rs
+++ b/src/api2/pull.rs
@@ -9,7 +9,7 @@ use proxmox::api::{ApiMethod, Router, RpcEnvironment, Permission};
 
 use pbs_client::{HttpClient, BackupRepository};
 use pbs_api_types::{
-    Remote, Authid, SyncJobConfig,
+    Remote, Authid, SyncJobConfig, GroupFilterList,
     DATASTORE_SCHEMA, REMOTE_ID_SCHEMA, REMOVE_VANISHED_BACKUPS_SCHEMA,
     PRIV_DATASTORE_BACKUP, PRIV_DATASTORE_PRUNE, PRIV_REMOTE_READ,
 };
@@ -102,7 +102,7 @@ pub fn do_sync_job(
                 worker.log(format!("Sync datastore '{}' from '{}/{}'",
                         sync_job.store, sync_job.remote, sync_job.remote_store));
 
-                pull_store(&worker, &client, &src_repo, tgt_store.clone(), delete, sync_owner).await?;
+                pull_store(&worker, &client, &src_repo, tgt_store.clone(), delete, sync_owner, None).await?;
 
                 worker.log(format!("sync job '{}' end", &job_id));
 
@@ -153,6 +153,10 @@ pub fn do_sync_job(
                 schema: REMOVE_VANISHED_BACKUPS_SCHEMA,
                 optional: true,
             },
+            "groups": {
+                type: GroupFilterList,
+                optional: true,
+            },
         },
     },
     access: {
@@ -170,6 +174,7 @@ async fn pull (
     remote: String,
     remote_store: String,
     remove_vanished: Option<bool>,
+    groups: Option<GroupFilterList>,
     _info: &ApiMethod,
     rpcenv: &mut dyn RpcEnvironment,
 ) -> Result<String, Error> {
@@ -177,6 +182,8 @@ async fn pull (
     let auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?;
     let delete = remove_vanished.unwrap_or(true);
 
+    let groups = groups.map(GroupFilterList::filters);
+
     check_pull_privs(&auth_id, &store, &remote, &remote_store, delete)?;
 
     let (client, src_repo, tgt_store) = get_pull_parameters(&store, &remote, &remote_store).await?;
@@ -186,7 +193,7 @@ async fn pull (
 
         worker.log(format!("sync datastore '{}' start", store));
 
-        let pull_future = pull_store(&worker, &client, &src_repo, tgt_store.clone(), delete, auth_id);
+        let pull_future = pull_store(&worker, &client, &src_repo, tgt_store.clone(), delete, auth_id, groups);
         let future = select!{
             success = pull_future.fuse() => success,
             abort = worker.abort_future().map(|_| Err(format_err!("pull aborted"))) => abort,
diff --git a/src/bin/proxmox-backup-manager.rs b/src/bin/proxmox-backup-manager.rs
index 689f44db..479ecf03 100644
--- a/src/bin/proxmox-backup-manager.rs
+++ b/src/bin/proxmox-backup-manager.rs
@@ -12,6 +12,7 @@ use pbs_tools::json::required_string_param;
 use pbs_api_types::{
     DATASTORE_SCHEMA, UPID_SCHEMA, REMOTE_ID_SCHEMA, REMOVE_VANISHED_BACKUPS_SCHEMA,
     IGNORE_VERIFIED_BACKUPS_SCHEMA, VERIFICATION_OUTDATED_AFTER_SCHEMA,
+    GroupFilterList,
 };
 
 use proxmox_backup::config;
@@ -234,6 +235,10 @@ fn task_mgmt_cli() -> CommandLineInterface {
                 schema: REMOVE_VANISHED_BACKUPS_SCHEMA,
                 optional: true,
             },
+            "groups": {
+                type: GroupFilterList,
+                optional: true,
+            },
             "output-format": {
                 schema: OUTPUT_FORMAT,
                 optional: true,
@@ -247,6 +252,7 @@ async fn pull_datastore(
     remote_store: String,
     local_store: String,
     remove_vanished: Option<bool>,
+    groups: Option<GroupFilterList>,
     param: Value,
 ) -> Result<Value, Error> {
 
@@ -260,6 +266,10 @@ async fn pull_datastore(
         "remote-store": remote_store,
     });
 
+    if groups.is_some() {
+        args["groups"] = json!(groups);
+    }
+
     if let Some(remove_vanished) = remove_vanished {
         args["remove-vanished"] = Value::from(remove_vanished);
     }
diff --git a/src/server/pull.rs b/src/server/pull.rs
index 4e82df1a..2910d280 100644
--- a/src/server/pull.rs
+++ b/src/server/pull.rs
@@ -12,7 +12,7 @@ use serde_json::json;
 
 use proxmox::api::error::{HttpError, StatusCode};
 
-use pbs_api_types::{Authid, SnapshotListItem, GroupListItem};
+use pbs_api_types::{Authid, SnapshotListItem, GroupListItem, GroupFilter};
 use pbs_datastore::{task_log, BackupInfo, BackupDir, BackupGroup, StoreProgress};
 use pbs_datastore::data_blob::DataBlob;
 use pbs_datastore::dynamic_index::DynamicIndexReader;
@@ -631,6 +631,7 @@ pub async fn pull_store(
     tgt_store: Arc<DataStore>,
     delete: bool,
     auth_id: Authid,
+    group_filter: Option<Vec<GroupFilter>>,
 ) -> Result<(), Error> {
     // explicit create shared lock to prevent GC on newly created chunks
     let _shared_store_lock = tgt_store.try_shared_chunk_store_lock()?;
@@ -644,8 +645,7 @@ pub async fn pull_store(
 
     let mut list: Vec<GroupListItem> = serde_json::from_value(result["data"].take())?;
 
-    worker.log(format!("found {} groups to sync", list.len()));
-
+    let total_count = list.len();
     list.sort_unstable_by(|a, b| {
         let type_order = a.backup_type.cmp(&b.backup_type);
         if type_order == std::cmp::Ordering::Equal {
@@ -655,11 +655,32 @@ pub async fn pull_store(
         }
     });
 
+    let apply_filters = |group: &BackupGroup, filters: &[GroupFilter]| -> bool {
+        filters
+            .iter()
+            .any(|filter| group.filter(filter))
+    };
+
     let list:Vec<BackupGroup> = list
         .into_iter()
         .map(|item| BackupGroup::new(item.backup_type, item.backup_id))
         .collect();
 
+    let list = if let Some(ref group_filter) = &group_filter {
+        let unfiltered_count = list.len();
+        let list:Vec<BackupGroup> = list
+            .into_iter()
+            .filter(|group| {
+                apply_filters(&group, group_filter)
+            })
+            .collect();
+        worker.log(format!("found {} groups to sync (out of {} total)", list.len(), unfiltered_count));
+        list
+    } else {
+        worker.log(format!("found {} groups to sync", total_count));
+        list
+    };
+
     let mut errors = false;
 
     let mut new_groups = std::collections::HashSet::new();
@@ -720,6 +741,11 @@ pub async fn pull_store(
                 if new_groups.contains(&local_group) {
                     continue;
                 }
+                if let Some(ref group_filter) = &group_filter {
+                    if !apply_filters(&local_group, group_filter) {
+                        continue;
+                    }
+                }
                 worker.log(format!(
                     "delete vanished group '{}/{}'",
                     local_group.backup_type(),
-- 
2.30.2

[pve-devel] [PATCH v2 proxmox-backup 06/10] sync: add group filtering

[Fabian Grünbichler]

like for manual pulls, but persisted in the sync job config and visible
in the relevant GUI parts.

Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
---

Notes:
    GUI is read-only for now (and defaults to no filtering on creation), as this is
    a rather advanced feature that requires a complex GUI to be user-friendly
    (regex-freeform, type-combobox, remote group scanning + selector with
    additional freeform input).
    
    I did test the API manually though to see whether it works as expected, and
    updating the filter list by overwriting with a new one passed in as multiple
    parameters works as expected.
    
    if we want to make this configurable over the GUI, we probably want to switch
    the job edit window to a tabpanel and add a second grid tab for selecting
    the groups.

 pbs-api-types/src/jobs.rs |  6 ++++++
 src/api2/config/sync.rs   |  5 +++++
 src/api2/pull.rs          |  4 +++-
 www/config/SyncView.js    | 13 ++++++++++++-
 www/window/SyncJobEdit.js | 12 ++++++++++++
 5 files changed, 38 insertions(+), 2 deletions(-)

diff --git a/pbs-api-types/src/jobs.rs b/pbs-api-types/src/jobs.rs
index 94638fe5..06a12131 100644
--- a/pbs-api-types/src/jobs.rs
+++ b/pbs-api-types/src/jobs.rs
@@ -439,6 +439,10 @@ impl GroupFilterList {
             optional: true,
             schema: SYNC_SCHEDULE_SCHEMA,
         },
+        groups: {
+            type: GroupFilterList,
+            optional: true,
+        },
     }
 )]
 #[derive(Serialize,Deserialize,Clone,Updater)]
@@ -458,6 +462,8 @@ pub struct SyncJobConfig {
     pub comment: Option<String>,
     #[serde(skip_serializing_if="Option::is_none")]
     pub schedule: Option<String>,
+    #[serde(skip_serializing_if="Option::is_none")]
+    pub groups: Option<GroupFilterList>,
 }
 
 #[api(
diff --git a/src/api2/config/sync.rs b/src/api2/config/sync.rs
index 3c2bfd78..cbd5b192 100644
--- a/src/api2/config/sync.rs
+++ b/src/api2/config/sync.rs
@@ -191,6 +191,8 @@ pub enum DeletableProperty {
     schedule,
     /// Delete the remove-vanished flag.
     remove_vanished,
+    /// Delete the groups property.
+    groups,
 }
 
 #[api(
@@ -253,6 +255,7 @@ pub fn update_sync_job(
                 DeletableProperty::comment => { data.comment = None; },
                 DeletableProperty::schedule => { data.schedule = None; },
                 DeletableProperty::remove_vanished => { data.remove_vanished = None; },
+                DeletableProperty::groups => { data.groups = None; },
             }
         }
     }
@@ -270,6 +273,7 @@ pub fn update_sync_job(
     if let Some(remote) = update.remote { data.remote = remote; }
     if let Some(remote_store) = update.remote_store { data.remote_store = remote_store; }
     if let Some(owner) = update.owner { data.owner = Some(owner); }
+    if let Some(groups) = update.groups { data.groups = Some(groups); }
 
     let schedule_changed = data.schedule != update.schedule;
     if update.schedule.is_some() { data.schedule = update.schedule; }
@@ -389,6 +393,7 @@ acl:1:/remote/remote1/remotestore1:write@pbs:RemoteSyncOperator
         owner: Some(write_auth_id.clone()),
         comment: None,
         remove_vanished: None,
+        groups: None,
         schedule: None,
     };
 
diff --git a/src/api2/pull.rs b/src/api2/pull.rs
index dfa6a938..86b618ed 100644
--- a/src/api2/pull.rs
+++ b/src/api2/pull.rs
@@ -102,7 +102,9 @@ pub fn do_sync_job(
                 worker.log(format!("Sync datastore '{}' from '{}/{}'",
                         sync_job.store, sync_job.remote, sync_job.remote_store));
 
-                pull_store(&worker, &client, &src_repo, tgt_store.clone(), delete, sync_owner, None).await?;
+                let sync_group_filter = sync_job.groups.map(GroupFilterList::filters);
+
+                pull_store(&worker, &client, &src_repo, tgt_store.clone(), delete, sync_owner, sync_group_filter).await?;
 
                 worker.log(format!("sync job '{}' end", &job_id));
 
diff --git a/www/config/SyncView.js b/www/config/SyncView.js
index 7d7e751c..d2a3954f 100644
--- a/www/config/SyncView.js
+++ b/www/config/SyncView.js
@@ -1,7 +1,7 @@
 Ext.define('pbs-sync-jobs-status', {
     extend: 'Ext.data.Model',
     fields: [
-	'id', 'owner', 'remote', 'remote-store', 'store', 'schedule',
+	'id', 'owner', 'remote', 'remote-store', 'store', 'schedule', 'groups',
 	'next-run', 'last-run-upid', 'last-run-state', 'last-run-endtime',
 	{
 	    name: 'duration',
@@ -106,6 +106,11 @@ Ext.define('PBS.config.SyncJobView', {
 	    return Ext.String.htmlEncode(value, metadata, record);
 	},
 
+	render_optional_groups: function(value, metadata, record) {
+	    if (!value) return gettext('All');
+	    return Ext.String.htmlEncode(value, metadata, record);
+	},
+
 	startStore: function() { this.getView().getStore().rstore.startUpdate(); },
 	stopStore: function() { this.getView().getStore().rstore.stopUpdate(); },
 
@@ -214,6 +219,12 @@ Ext.define('PBS.config.SyncJobView', {
 	    flex: 2,
 	    sortable: true,
 	},
+	{
+	    header: gettext('Backup Groups'),
+	    dataIndex: 'groups',
+	    renderer: 'render_optional_groups',
+	    width: 80,
+	},
 	{
 	    header: gettext('Schedule'),
 	    dataIndex: 'schedule',
diff --git a/www/window/SyncJobEdit.js b/www/window/SyncJobEdit.js
index 47e65ae3..2399f11f 100644
--- a/www/window/SyncJobEdit.js
+++ b/www/window/SyncJobEdit.js
@@ -199,6 +199,18 @@ Ext.define('PBS.window.SyncJobEdit', {
 	],
 
 	columnB: [
+	    {
+		fieldLabel: gettext('Backup Groups'),
+		xtype: 'displayfield',
+		name: 'groups',
+		renderer: function(value, metadata, record) {
+		    if (!value) return gettext('All');
+		    return Ext.String.htmlEncode(value, metadata, record);
+		},
+		cbind: {
+		    hidden: '{isCreate}',
+		},
+	    },
 	    {
 		fieldLabel: gettext('Comment'),
 		xtype: 'proxmoxtextfield',
-- 
2.30.2

[pve-devel] [PATCH v2 proxmox-backup 07/10] remote: add backup group scanning

[Fabian Grünbichler]

Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
---

Notes:
    used for bash completion (next patch) and potentially GUI (future work). could also be integrated into the 'remote' CLI/GUI..

 src/api2/config/remote.rs | 73 ++++++++++++++++++++++++++++++++++++---
 1 file changed, 69 insertions(+), 4 deletions(-)

diff --git a/src/api2/config/remote.rs b/src/api2/config/remote.rs
index acf7cfcf..ceca8109 100644
--- a/src/api2/config/remote.rs
+++ b/src/api2/config/remote.rs
@@ -3,13 +3,14 @@ use serde_json::Value;
 use ::serde::{Deserialize, Serialize};
 
 use proxmox::api::{api, ApiMethod, Router, RpcEnvironment, Permission};
-use proxmox::http_err;
+use proxmox::api::router::SubdirMap;
+use proxmox::{http_err, list_subdirs_api_method, sortable};
 
 use pbs_client::{HttpClient, HttpClientOptions};
 use pbs_api_types::{
     REMOTE_ID_SCHEMA, REMOTE_PASSWORD_SCHEMA, Remote, RemoteConfig, RemoteConfigUpdater,
-    Authid, PROXMOX_CONFIG_DIGEST_SCHEMA, DataStoreListItem, SyncJobConfig,
-    PRIV_REMOTE_AUDIT, PRIV_REMOTE_MODIFY,
+    Authid, PROXMOX_CONFIG_DIGEST_SCHEMA, DATASTORE_SCHEMA, GroupListItem,
+    DataStoreListItem, SyncJobConfig, PRIV_REMOTE_AUDIT, PRIV_REMOTE_MODIFY,
 };
 use pbs_config::sync;
 
@@ -340,8 +341,72 @@ pub async fn scan_remote_datastores(name: String) -> Result<Vec<DataStoreListIte
     }
 }
 
+#[api(
+    input: {
+        properties: {
+            name: {
+                schema: REMOTE_ID_SCHEMA,
+            },
+            store: {
+                schema: DATASTORE_SCHEMA,
+            },
+        },
+    },
+    access: {
+        permission: &Permission::Privilege(&["remote", "{name}"], PRIV_REMOTE_AUDIT, false),
+    },
+    returns: {
+        description: "Lists the accessible backup groups in a remote datastore.",
+        type: Array,
+        items: { type: GroupListItem },
+    },
+)]
+/// List groups of a remote.cfg entry's datastore
+pub async fn scan_remote_groups(name: String, store: String) -> Result<Vec<GroupListItem>, Error> {
+    let (remote_config, _digest) = pbs_config::remote::config()?;
+    let remote: Remote = remote_config.lookup("remote", &name)?;
+
+    let map_remote_err = |api_err| {
+        http_err!(INTERNAL_SERVER_ERROR,
+                  "failed to scan remote '{}' - {}",
+                  &name,
+                  api_err)
+    };
+
+    let client = remote_client(remote)
+        .await
+        .map_err(map_remote_err)?;
+    let api_res = client
+        .get(&format!("api2/json/admin/datastore/{}/groups", store), None)
+        .await
+        .map_err(map_remote_err)?;
+    let parse_res = match api_res.get("data") {
+        Some(data) => serde_json::from_value::<Vec<GroupListItem>>(data.to_owned()),
+        None => bail!("remote {} did not return any group list data", &name),
+    };
+
+    match parse_res {
+        Ok(parsed) => Ok(parsed),
+        Err(_) => bail!("Failed to parse remote scan api result."),
+    }
+}
+
+#[sortable]
+const DATASTORE_SCAN_SUBDIRS: SubdirMap = &[
+    (
+        "groups",
+        &Router::new()
+            .get(&API_METHOD_SCAN_REMOTE_GROUPS)
+    ),
+];
+
+const DATASTORE_SCAN_ROUTER: Router = Router::new()
+    .get(&list_subdirs_api_method!(DATASTORE_SCAN_SUBDIRS))
+    .subdirs(DATASTORE_SCAN_SUBDIRS);
+
 const SCAN_ROUTER: Router = Router::new()
-    .get(&API_METHOD_SCAN_REMOTE_DATASTORES);
+    .get(&API_METHOD_SCAN_REMOTE_DATASTORES)
+    .match_all("store", &DATASTORE_SCAN_ROUTER);
 
 const ITEM_ROUTER: Router = Router::new()
     .get(&API_METHOD_READ_REMOTE)
-- 
2.30.2

[pve-devel] [PATCH v2 proxmox-backup 08/10] manager: extend sync/pull completion

[Fabian Grünbichler]

complete groups by scanning the remote store if available, and query the
sync job config if remote or remote-store is not given on the current
command-line (e.g., when updating a job config).

unfortunately groups already given on the current command line are not
passed to the completion helper, so we can't filter those out..

other filter types are completed statically.

Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
---
 src/bin/proxmox-backup-manager.rs      | 105 ++++++++++++++++++++++---
 src/bin/proxmox_backup_manager/sync.rs |   2 +
 2 files changed, 96 insertions(+), 11 deletions(-)

diff --git a/src/bin/proxmox-backup-manager.rs b/src/bin/proxmox-backup-manager.rs
index 479ecf03..0c8f2dc7 100644
--- a/src/bin/proxmox-backup-manager.rs
+++ b/src/bin/proxmox-backup-manager.rs
@@ -1,18 +1,19 @@
 use std::collections::HashMap;
 use std::io::{self, Write};
 
-use anyhow::{format_err, Error};
+use anyhow::Error;
 use serde_json::{json, Value};
 
 use proxmox::api::{api, cli::*, RpcEnvironment};
 
 use pbs_client::{connect_to_localhost, display_task_log, view_task_result};
+use pbs_config::sync;
 use pbs_tools::percent_encoding::percent_encode_component;
 use pbs_tools::json::required_string_param;
 use pbs_api_types::{
     DATASTORE_SCHEMA, UPID_SCHEMA, REMOTE_ID_SCHEMA, REMOVE_VANISHED_BACKUPS_SCHEMA,
     IGNORE_VERIFIED_BACKUPS_SCHEMA, VERIFICATION_OUTDATED_AFTER_SCHEMA,
-    GroupFilterList,
+    GroupFilterList, SyncJobConfig,
 };
 
 use proxmox_backup::config;
@@ -396,6 +397,7 @@ fn main() {
                 .completion_cb("local-store", pbs_config::datastore::complete_datastore_name)
                 .completion_cb("remote", pbs_config::remote::complete_remote_name)
                 .completion_cb("remote-store", complete_remote_datastore_name)
+                .completion_cb("groups", complete_remote_datastore_group_filter)
         )
         .insert(
             "verify",
@@ -418,24 +420,105 @@ fn main() {
    pbs_runtime::main(run_async_cli_command(cmd_def, rpcenv));
 }
 
+fn get_sync_job(id: &String) -> Result<SyncJobConfig, Error> {
+    let (config, _digest) = sync::config()?;
+
+    config.lookup("sync", id)
+}
+
+fn get_remote(param: &HashMap<String, String>) -> Option<String> {
+    param
+        .get("remote")
+        .map(|r| r.to_owned())
+        .or_else(|| {
+            if let Some(id) = param.get("id") {
+                if let Ok(job) = get_sync_job(id) {
+                    return Some(job.remote.clone());
+                }
+            }
+            None
+        })
+}
+
+fn get_remote_store(param: &HashMap<String, String>) -> Option<(String, String)> {
+    let mut job: Option<SyncJobConfig> = None;
+
+    let remote = param
+        .get("remote")
+        .map(|r| r.to_owned())
+        .or_else(|| {
+            if let Some(id) = param.get("id") {
+                job = get_sync_job(id).ok();
+                if let Some(ref job) = job {
+                    return Some(job.remote.clone());
+                }
+            }
+            None
+        });
+
+    if let Some(remote) = remote {
+        let store = param
+            .get("remote-store")
+            .map(|r| r.to_owned())
+            .or_else(|| job.map(|job| job.remote_store.clone()));
+
+        if let Some(store) = store {
+            return Some((remote, store))
+        }
+    }
+
+    None
+}
+
 // shell completion helper
 pub fn complete_remote_datastore_name(_arg: &str, param: &HashMap<String, String>) -> Vec<String> {
 
     let mut list = Vec::new();
 
-    let _ = proxmox::try_block!({
-        let remote = param.get("remote").ok_or_else(|| format_err!("no remote"))?;
+    if let Some(remote) = get_remote(param) {
+        if let Ok(data) = pbs_runtime::block_on(async move {
+                crate::api2::config::remote::scan_remote_datastores(remote).await
+            }) {
 
-        let data = pbs_runtime::block_on(async move {
-            crate::api2::config::remote::scan_remote_datastores(remote.clone()).await
-        })?;
+            for item in data {
+                list.push(item.store);
+            }
+        }
+    }
 
-        for item in data {
-            list.push(item.store);
+    list
+}
+
+// shell completion helper
+pub fn complete_remote_datastore_group(_arg: &str, param: &HashMap<String, String>) -> Vec<String> {
+
+    let mut list = Vec::new();
+
+    if let Some((remote, remote_store)) = get_remote_store(param) {
+        if let Ok(data) = pbs_runtime::block_on(async move {
+            crate::api2::config::remote::scan_remote_groups(remote.clone(), remote_store.clone()).await
+        }) {
+
+            for item in data {
+                list.push(format!("{}/{}", item.backup_type, item.backup_id));
+            }
         }
+    }
+
+    list
+}
+
+// shell completion helper
+pub fn complete_remote_datastore_group_filter(_arg: &str, param: &HashMap<String, String>) -> Vec<String> {
+
+    let mut list = Vec::new();
+
+    list.push("regex:".to_string());
+    list.push("type:ct".to_string());
+    list.push("type:host".to_string());
+    list.push("type:vm".to_string());
 
-        Ok(())
-    }).map_err(|_err: Error| { /* ignore */ });
+    list.extend(complete_remote_datastore_group(_arg, param).iter().map(|group| format!("group:{}", group)));
 
     list
 }
diff --git a/src/bin/proxmox_backup_manager/sync.rs b/src/bin/proxmox_backup_manager/sync.rs
index 7a1e8718..2dbef119 100644
--- a/src/bin/proxmox_backup_manager/sync.rs
+++ b/src/bin/proxmox_backup_manager/sync.rs
@@ -88,6 +88,7 @@ pub fn sync_job_commands() -> CommandLineInterface {
                 .completion_cb("store", pbs_config::datastore::complete_datastore_name)
                 .completion_cb("remote", pbs_config::remote::complete_remote_name)
                 .completion_cb("remote-store", crate::complete_remote_datastore_name)
+                .completion_cb("groups", crate::complete_remote_datastore_group_filter)
         )
         .insert("update",
                 CliCommand::new(&api2::config::sync::API_METHOD_UPDATE_SYNC_JOB)
@@ -96,6 +97,7 @@ pub fn sync_job_commands() -> CommandLineInterface {
                 .completion_cb("schedule", pbs_config::datastore::complete_calendar_event)
                 .completion_cb("store", pbs_config::datastore::complete_datastore_name)
                 .completion_cb("remote-store", crate::complete_remote_datastore_name)
+                .completion_cb("groups", crate::complete_remote_datastore_group_filter)
         )
         .insert("remove",
                 CliCommand::new(&api2::config::sync::API_METHOD_DELETE_SYNC_JOB)
-- 
2.30.2

[pve-devel] [PATCH v2 proxmox-backup 09/10] manager: render group filter properly

[Fabian Grünbichler]

Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
---
 src/bin/proxmox_backup_manager/sync.rs | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/src/bin/proxmox_backup_manager/sync.rs b/src/bin/proxmox_backup_manager/sync.rs
index 2dbef119..fd312e9e 100644
--- a/src/bin/proxmox_backup_manager/sync.rs
+++ b/src/bin/proxmox_backup_manager/sync.rs
@@ -7,6 +7,18 @@ use pbs_api_types::JOB_ID_SCHEMA;
 
 use proxmox_backup::api2;
 
+fn render_group_filter(value: &Value, _record: &Value) -> Result<String, Error> {
+    if let Some(group_filters) = value.as_array() {
+        let group_filters:Vec<&str> = group_filters
+            .iter()
+            .filter_map(Value::as_str)
+            .collect();
+        Ok(group_filters.join(" OR "))
+    } else {
+        Ok(String::from("all"))
+    }
+}
+
 #[api(
     input: {
         properties: {
@@ -34,6 +46,7 @@ fn list_sync_jobs(param: Value, rpcenv: &mut dyn RpcEnvironment) -> Result<Value
         .column(ColumnConfig::new("remote"))
         .column(ColumnConfig::new("remote-store"))
         .column(ColumnConfig::new("schedule"))
+        .column(ColumnConfig::new("groups").renderer(render_group_filter))
         .column(ColumnConfig::new("comment"));
 
     format_and_print_result_full(&mut data, &info.returns, &output_format, &options);
@@ -65,6 +78,12 @@ fn show_sync_job(param: Value, rpcenv: &mut dyn RpcEnvironment) -> Result<Value,
         _ => unreachable!(),
     };
 
+    if let Some(groups) = data.get_mut("groups") {
+        if let Ok(rendered) = render_group_filter(groups, groups) {
+            *groups = Value::String(rendered);
+        }
+    }
+
     let options = default_table_format_options();
     format_and_print_result_full(&mut data, &info.returns, &output_format, &options);
 
-- 
2.30.2

[pve-devel] [PATCH v2 proxmox-backup 10/10] docs: mention group filter in sync docs

[Fabian Grünbichler]

Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
---
 docs/managing-remotes.rst | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/docs/managing-remotes.rst b/docs/managing-remotes.rst
index 88ab3ba2..7968248b 100644
--- a/docs/managing-remotes.rst
+++ b/docs/managing-remotes.rst
@@ -89,6 +89,12 @@ the local datastore as well. If the ``owner`` option is not set (defaulting to
 ``root@pam``) or set to something other than the configuring user,
 ``Datastore.Modify`` is required as well.
 
+If the ``groups`` option is set, only backup groups matching at least one of
+the specified criteria (backup type, full group identifier, or a regular
+expression matched against the full group identifier) are synced. The same
+filter is applied to local groups for handling of the ``remove-vanished``
+option.
+
 .. note:: A sync job can only sync backup groups that the configured remote's
   user/API token can read. If a remote is configured with a user/API token that
   only has ``Datastore.Backup`` privileges, only the limited set of accessible
-- 
2.30.2

Re: [pve-devel] [PATCH v2 proxmox-backup 06/10] sync: add group filtering

[Thomas Lamprecht]

FYI, it seems you sent the series to PVE devel by mistake.

On 15.09.21 15:41, Fabian Grünbichler wrote:
> like for manual pulls, but persisted in the sync job config and visible
> in the relevant GUI parts.
> 
> Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
> ---
> 
> Notes:
>     GUI is read-only for now (and defaults to no filtering on creation), as this is
>     a rather advanced feature that requires a complex GUI to be user-friendly
>     (regex-freeform, type-combobox, remote group scanning + selector with
>     additional freeform input).

above paragraph would belong into the commit message IMO.

>     
>     I did test the API manually though to see whether it works as expected, and
>     updating the filter list by overwriting with a new one passed in as multiple
>     parameters works as expected.
>     
>     if we want to make this configurable over the GUI, we probably want to switch
>     the job edit window to a tabpanel and add a second grid tab for selecting
>     the groups

we could also get away by adding it in the advanced section for now.
> diff --git a/www/window/SyncJobEdit.js b/www/window/SyncJobEdit.js
> index 47e65ae3..2399f11f 100644
> --- a/www/window/SyncJobEdit.js
> +++ b/www/window/SyncJobEdit.js
> @@ -199,6 +199,18 @@ Ext.define('PBS.window.SyncJobEdit', {
>  	],
>  
>  	columnB: [
> +	    {
> +		fieldLabel: gettext('Backup Groups'),
> +		xtype: 'displayfield',
> +		name: 'groups',
> +		renderer: function(value, metadata, record) {
> +		    if (!value) return gettext('All');
> +		    return Ext.String.htmlEncode(value, metadata, record);

Ext.String.htmlEncode only takes a single parameter
https://docs.sencha.com/extjs/7.0.0/classic/Ext.String.html#method-htmlEncode

besides that you could use a arrow fn here, the following would seem quite ok for me for
a renderer:

renderer: v => v ? Ext.String.htmlEncode(v) : gettext('All'),

> +		},
> +		cbind: {
> +		    hidden: '{isCreate}',
> +		},
> +	    },
>  	    {
>  		fieldLabel: gettext('Comment'),
>  		xtype: 'proxmoxtextfield',
> 

Re: [pve-devel] [PATCH v2 proxmox-backup 06/10] sync: add group filtering

[Fabian Grünbichler]

On September 16, 2021 9:19 am, Thomas Lamprecht wrote:
> FYI, it seems you sent the series to PVE devel by mistake.

dang, yeah, shell history got me there I guess.

> 
> On 15.09.21 15:41, Fabian Grünbichler wrote:
>> like for manual pulls, but persisted in the sync job config and visible
>> in the relevant GUI parts.
>> 
>> Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
>> ---
>> 
>> Notes:
>>     GUI is read-only for now (and defaults to no filtering on creation), as this is
>>     a rather advanced feature that requires a complex GUI to be user-friendly
>>     (regex-freeform, type-combobox, remote group scanning + selector with
>>     additional freeform input).
> 
> above paragraph would belong into the commit message IMO.
> 
>>     
>>     I did test the API manually though to see whether it works as expected, and
>>     updating the filter list by overwriting with a new one passed in as multiple
>>     parameters works as expected.
>>     
>>     if we want to make this configurable over the GUI, we probably want to switch
>>     the job edit window to a tabpanel and add a second grid tab for selecting
>>     the groups
> 
> we could also get away by adding it in the advanced section for now.

it has the problem of "adding an arbitrary amount of filters", which 
might take up a lot of space, which might make it nicer to move to a 
separate tab. I am not yet sure which direction to go for this.

>> diff --git a/www/window/SyncJobEdit.js b/www/window/SyncJobEdit.js
>> index 47e65ae3..2399f11f 100644
>> --- a/www/window/SyncJobEdit.js
>> +++ b/www/window/SyncJobEdit.js
>> @@ -199,6 +199,18 @@ Ext.define('PBS.window.SyncJobEdit', {
>>  	],
>>  
>>  	columnB: [
>> +	    {
>> +		fieldLabel: gettext('Backup Groups'),
>> +		xtype: 'displayfield',
>> +		name: 'groups',
>> +		renderer: function(value, metadata, record) {
>> +		    if (!value) return gettext('All');
>> +		    return Ext.String.htmlEncode(value, metadata, record);
> 
> Ext.String.htmlEncode only takes a single parameter
> https://docs.sencha.com/extjs/7.0.0/classic/Ext.String.html#method-htmlEncode

seems like I copied that from an earlier mistake - render_optional_owner 
- made by me as well ;)

> besides that you could use a arrow fn here, the following would seem quite ok for me for
> a renderer:
> 
> renderer: v => v ? Ext.String.htmlEncode(v) : gettext('All'),

yeah, that looks cleaner.

>> +		},
>> +		cbind: {
>> +		    hidden: '{isCreate}',
>> +		},
>> +	    },
>>  	    {
>>  		fieldLabel: gettext('Comment'),
>>  		xtype: 'proxmoxtextfield',
>> 
> 
> 

Re: [pve-devel] [PATCH v2 proxmox-backup 02/10] api: add GroupFilter(List) type

[Dominik Csapak]

On 9/15/21 15:41, Fabian Grünbichler wrote:
> at the API level, this is a simple (wrapped) Vec of Strings with a
> verifier function. all users should use the provided helper to get the
> actual GroupFilter enum values, which can't be directly used in the API
> schema because of restrictions of the api macro.
> 
> validation of the schema + parsing into the proper type uses the same fn
> intentionally to avoid running out of sync, even if it means compiling
> the REs twice.
> 
> Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
> ---
> this is working around some api limitations
> - updated doesn't support a Vec<String> type
> - api-macro doesn't support enum with values
> 
> the validation fn and the parser/converted use the same code to avoid
> getting out of sync, at the expense of parsing potentially expensive REs
> twice..
> 
>   pbs-api-types/src/jobs.rs | 90 +++++++++++++++++++++++++++++++++++++++
>   1 file changed, 90 insertions(+)
> 
> diff --git a/pbs-api-types/src/jobs.rs b/pbs-api-types/src/jobs.rs
> index 1526dbc4..94638fe5 100644
> --- a/pbs-api-types/src/jobs.rs
> +++ b/pbs-api-types/src/jobs.rs
> @@ -1,3 +1,7 @@
> +use anyhow::format_err;
> +use std::str::FromStr;
> +
> +use regex::Regex;
>   use serde::{Deserialize, Serialize};
>   
>   use proxmox::const_regex;
> @@ -7,6 +11,7 @@ use proxmox::api::{api, schema::*};
>   use crate::{
>       Userid, Authid, REMOTE_ID_SCHEMA, DRIVE_NAME_SCHEMA, MEDIA_POOL_NAME_SCHEMA,
>       SINGLE_LINE_COMMENT_SCHEMA, PROXMOX_SAFE_ID_FORMAT, DATASTORE_SCHEMA,
> +    BACKUP_GROUP_SCHEMA, BACKUP_TYPE_SCHEMA,
>   };
>   
>   const_regex!{
> @@ -319,6 +324,91 @@ pub struct TapeBackupJobStatus {
>       pub next_media_label: Option<String>,
>   }
>   
> +#[derive(Clone, Debug)]
> +/// Filter for matching `BackupGroup`s, for use with `BackupGroup::filter`.
> +pub enum GroupFilter {
> +    /// BackupGroup type - either `vm`, `ct`, or `host`.
> +    BackupType(String),
> +    /// Full identifier of BackupGroup, including type
> +    Group(String),
> +    /// A regular expression matched against the full identifier of the BackupGroup
> +    Regex(Regex),
> +}

Would it not make sense to have an already parsed "BackupGroup" in
the Group Variant instead of a string? we would have
to move the pub struct BackupGroup here, but i think the impl block can 
stay in pbs-datastore

> +
> +impl std::str::FromStr for GroupFilter {
> +    type Err = anyhow::Error;
> +
> +    fn from_str(s: &str) -> Result<Self, Self::Err> {
> +        match s.split_once(":") {
> +            Some(("group", value)) => parse_simple_value(value, &BACKUP_GROUP_SCHEMA).map(|_| GroupFilter::Group(value.to_string())),

here you could parse it directly to a 'BackupGroup' then we would not
need the BACKUP_GROUP_SCHEMA

> +            Some(("type", value)) => parse_simple_value(value, &BACKUP_TYPE_SCHEMA).map(|_| GroupFilter::BackupType(value.to_string())),

nit: would it not be enough to match the regex instead of the schema?
(not that i think it'd make a difference)

> +            Some(("regex", value)) => Ok(GroupFilter::Regex(Regex::new(value)?)),
> +            Some((ty, _value)) => Err(format_err!("expected 'group', 'type' or 'regex' prefix, got '{}'", ty)),
> +            None => Err(format_err!("input doesn't match expected format '<group:GROUP||type:<vm|ct|host>|regex:REGEX>'")),
> +        }.map_err(|err| format_err!("'{}' - {}", s, err))
> +    }
> +}
> +
> +// used for serializing below, caution!
> +impl std::fmt::Display for GroupFilter {
> +    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
> +        match self {
> +            GroupFilter::BackupType(backup_type) => write!(f, "type:{}", backup_type),
> +            GroupFilter::Group(backup_group) => write!(f, "group:{}", backup_group),
> +            GroupFilter::Regex(regex) => write!(f, "regex:{}", regex.as_str()),
> +        }
> +    }
> +}
> +
> +impl Serialize for GroupFilter {
> +    fn serialize<S>(&self, serializer: S) -> Result<S::Ok, S::Error>
> +    where
> +        S: serde::Serializer,
> +    {
> +        serializer.serialize_str(&self.to_string())
> +    }
> +}
> +
> +impl<'de> Deserialize<'de> for GroupFilter {
> +    fn deserialize<D>(deserializer: D) -> Result<GroupFilter, D::Error>
> +    where
> +        D: serde::Deserializer<'de>,
> +    {
> +        String::deserialize(deserializer).and_then(|string| {
> +            GroupFilter::from_str(&string).map_err(serde::de::Error::custom)
> +        })
> +    }
> +}
> +
> +fn verify_group_filter(input: &str) -> Result<(), anyhow::Error> {
> +    GroupFilter::from_str(input).map(|_| ())
> +}
> +
> +pub const GROUP_FILTER_SCHEMA: Schema = StringSchema::new(
> +    "Group filter based on group identifier ('group:GROUP'), group type ('type:<vm|ct|host>'), or regex ('regex:RE').")
> +    .format(&ApiStringFormat::VerifyFn(verify_group_filter))
> +    .type_text("<type:<vm|ct|host>|group:GROUP|regex:RE>")
> +    .schema();
> +
> +#[api(
> +    type: Array,
> +    description: "List of group filters.",
> +    items: {
> +        schema: GROUP_FILTER_SCHEMA,
> +    },
> +)]
> +#[derive(Serialize, Deserialize, Clone, UpdaterType)]
> +pub struct GroupFilterList(Vec<String>);
> +
> +impl GroupFilterList {
> +    /// converts schema-validated Vec<String> into Vec<GroupFilter>
> +    pub fn filters(self) -> Vec<GroupFilter> {
> +        self.0.iter()
> +             .map(|group_filter| GroupFilter::from_str(group_filter).unwrap())
> +             .collect()
> +    }
> +}
> +

IMHO that has to be easier:

instead of the custom type, i'd do:

const GROUP_FILTER_LIST_SCHEMA: Schema = ArraySchema::new("",
	&GROUP_FILTER_SCHEMA).schema()

the only thing you'd have to do is to convice the updater that
there is an updaterType for it: (in proxmox/src/api/schema.rs of the 
proxmox crate)

impl<T> UpdaterType for Vec<T> {
     type Updater = Option<Self>;
}

that implies though that a user always has to give the complete list
(so no single removal from the list), though i think that's what
we want anyway..

i tested it here, and AFAICS, this works (i can update/list the sync job 
properly)

with that you can also remove the 'filters' fn and can use
Vec<GroupFilter> as the api call parameters.

makes the code a little easier to read

>   #[api(
>       properties: {
>           id: {
> 

Re: [pve-devel] [PATCH v2 proxmox-backup 03/10] BackupGroup: add filter helper

[Dominik Csapak]

On 9/15/21 15:41, Fabian Grünbichler wrote:
> to have a single implementation of "group is matched by group filter".
> 
> Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
> ---
> 
> Notes:
>      there might be a better place for this if we want to support more complex
>      filters in the future (like, exists in local datastore, or has > X snapshots,
>      ..)
> 
>   pbs-datastore/src/backup_info.rs | 10 ++++++++++
>   1 file changed, 10 insertions(+)
> 
> diff --git a/pbs-datastore/src/backup_info.rs b/pbs-datastore/src/backup_info.rs
> index f77098ee..416a5e32 100644
> --- a/pbs-datastore/src/backup_info.rs
> +++ b/pbs-datastore/src/backup_info.rs
> @@ -1,5 +1,6 @@
>   use std::os::unix::io::RawFd;
>   use std::path::{Path, PathBuf};
> +use std::str::FromStr;
>   
>   use anyhow::{bail, format_err, Error};
>   
> @@ -10,6 +11,7 @@ use pbs_api_types::{
>       GROUP_PATH_REGEX,
>       SNAPSHOT_PATH_REGEX,
>       BACKUP_FILE_REGEX,
> +    GroupFilter,
>   };
>   
>   use super::manifest::MANIFEST_BLOB_NAME;
> @@ -153,6 +155,14 @@ impl BackupGroup {
>   
>           Ok(last)
>       }
> +
> +    pub fn filter(&self, filter: &GroupFilter) -> bool {
> +        match filter {
> +            GroupFilter::Group(backup_group) => &BackupGroup::from_str(&backup_group).unwrap() == self,

if you'd parse the backup group already in the api type, this would not 
be necessary

> +            GroupFilter::BackupType(backup_type) => self.backup_type() == backup_type,
> +            GroupFilter::Regex(regex) => regex.is_match(&format!("{}", self)),

wouldn't &self.to_string() not better?

> +        }
> +    }
>   }
>   
>   impl std::fmt::Display for BackupGroup {
> 

Re: [pve-devel] [PATCH v2 proxmox-backup 02/10] api: add GroupFilter(List) type

[Fabian Grünbichler]

On September 16, 2021 4:46 pm, Dominik Csapak wrote:
> On 9/15/21 15:41, Fabian Grünbichler wrote:
>> at the API level, this is a simple (wrapped) Vec of Strings with a
>> verifier function. all users should use the provided helper to get the
>> actual GroupFilter enum values, which can't be directly used in the API
>> schema because of restrictions of the api macro.
>> 
>> validation of the schema + parsing into the proper type uses the same fn
>> intentionally to avoid running out of sync, even if it means compiling
>> the REs twice.
>> 
>> Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
>> ---
>> this is working around some api limitations
>> - updated doesn't support a Vec<String> type
>> - api-macro doesn't support enum with values
>> 
>> the validation fn and the parser/converted use the same code to avoid
>> getting out of sync, at the expense of parsing potentially expensive REs
>> twice..
>> 
>>   pbs-api-types/src/jobs.rs | 90 +++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 90 insertions(+)
>> 
>> diff --git a/pbs-api-types/src/jobs.rs b/pbs-api-types/src/jobs.rs
>> index 1526dbc4..94638fe5 100644
>> --- a/pbs-api-types/src/jobs.rs
>> +++ b/pbs-api-types/src/jobs.rs
>> @@ -1,3 +1,7 @@
>> +use anyhow::format_err;
>> +use std::str::FromStr;
>> +
>> +use regex::Regex;
>>   use serde::{Deserialize, Serialize};
>>   
>>   use proxmox::const_regex;
>> @@ -7,6 +11,7 @@ use proxmox::api::{api, schema::*};
>>   use crate::{
>>       Userid, Authid, REMOTE_ID_SCHEMA, DRIVE_NAME_SCHEMA, MEDIA_POOL_NAME_SCHEMA,
>>       SINGLE_LINE_COMMENT_SCHEMA, PROXMOX_SAFE_ID_FORMAT, DATASTORE_SCHEMA,
>> +    BACKUP_GROUP_SCHEMA, BACKUP_TYPE_SCHEMA,
>>   };
>>   
>>   const_regex!{
>> @@ -319,6 +324,91 @@ pub struct TapeBackupJobStatus {
>>       pub next_media_label: Option<String>,
>>   }
>>   
>> +#[derive(Clone, Debug)]
>> +/// Filter for matching `BackupGroup`s, for use with `BackupGroup::filter`.
>> +pub enum GroupFilter {
>> +    /// BackupGroup type - either `vm`, `ct`, or `host`.
>> +    BackupType(String),
>> +    /// Full identifier of BackupGroup, including type
>> +    Group(String),
>> +    /// A regular expression matched against the full identifier of the BackupGroup
>> +    Regex(Regex),
>> +}
> 
> Would it not make sense to have an already parsed "BackupGroup" in
> the Group Variant instead of a string? we would have
> to move the pub struct BackupGroup here, but i think the impl block can 
> stay in pbs-datastore

not moving it was the reason why I did it that way, but if moving is 
okay, moving is probably better ;)

> 
>> +
>> +impl std::str::FromStr for GroupFilter {
>> +    type Err = anyhow::Error;
>> +
>> +    fn from_str(s: &str) -> Result<Self, Self::Err> {
>> +        match s.split_once(":") {
>> +            Some(("group", value)) => parse_simple_value(value, &BACKUP_GROUP_SCHEMA).map(|_| GroupFilter::Group(value.to_string())),
> 
> here you could parse it directly to a 'BackupGroup' then we would not
> need the BACKUP_GROUP_SCHEMA
> 
>> +            Some(("type", value)) => parse_simple_value(value, &BACKUP_TYPE_SCHEMA).map(|_| GroupFilter::BackupType(value.to_string())),
> 
> nit: would it not be enough to match the regex instead of the schema?
> (not that i think it'd make a difference)

using the schema we get the schema error message, which is probably what we 
want.

> 
>> +            Some(("regex", value)) => Ok(GroupFilter::Regex(Regex::new(value)?)),
>> +            Some((ty, _value)) => Err(format_err!("expected 'group', 'type' or 'regex' prefix, got '{}'", ty)),
>> +            None => Err(format_err!("input doesn't match expected format '<group:GROUP||type:<vm|ct|host>|regex:REGEX>'")),
>> +        }.map_err(|err| format_err!("'{}' - {}", s, err))
>> +    }
>> +}
>> +
>> +// used for serializing below, caution!
>> +impl std::fmt::Display for GroupFilter {
>> +    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
>> +        match self {
>> +            GroupFilter::BackupType(backup_type) => write!(f, "type:{}", backup_type),
>> +            GroupFilter::Group(backup_group) => write!(f, "group:{}", backup_group),
>> +            GroupFilter::Regex(regex) => write!(f, "regex:{}", regex.as_str()),
>> +        }
>> +    }
>> +}
>> +
>> +impl Serialize for GroupFilter {
>> +    fn serialize<S>(&self, serializer: S) -> Result<S::Ok, S::Error>
>> +    where
>> +        S: serde::Serializer,
>> +    {
>> +        serializer.serialize_str(&self.to_string())
>> +    }
>> +}
>> +
>> +impl<'de> Deserialize<'de> for GroupFilter {
>> +    fn deserialize<D>(deserializer: D) -> Result<GroupFilter, D::Error>
>> +    where
>> +        D: serde::Deserializer<'de>,
>> +    {
>> +        String::deserialize(deserializer).and_then(|string| {
>> +            GroupFilter::from_str(&string).map_err(serde::de::Error::custom)
>> +        })
>> +    }
>> +}
>> +
>> +fn verify_group_filter(input: &str) -> Result<(), anyhow::Error> {
>> +    GroupFilter::from_str(input).map(|_| ())
>> +}
>> +
>> +pub const GROUP_FILTER_SCHEMA: Schema = StringSchema::new(
>> +    "Group filter based on group identifier ('group:GROUP'), group type ('type:<vm|ct|host>'), or regex ('regex:RE').")
>> +    .format(&ApiStringFormat::VerifyFn(verify_group_filter))
>> +    .type_text("<type:<vm|ct|host>|group:GROUP|regex:RE>")
>> +    .schema();
>> +
>> +#[api(
>> +    type: Array,
>> +    description: "List of group filters.",
>> +    items: {
>> +        schema: GROUP_FILTER_SCHEMA,
>> +    },
>> +)]
>> +#[derive(Serialize, Deserialize, Clone, UpdaterType)]
>> +pub struct GroupFilterList(Vec<String>);
>> +
>> +impl GroupFilterList {
>> +    /// converts schema-validated Vec<String> into Vec<GroupFilter>
>> +    pub fn filters(self) -> Vec<GroupFilter> {
>> +        self.0.iter()
>> +             .map(|group_filter| GroupFilter::from_str(group_filter).unwrap())
>> +             .collect()
>> +    }
>> +}
>> +
> 
> IMHO that has to be easier:
> 
> instead of the custom type, i'd do:
> 
> const GROUP_FILTER_LIST_SCHEMA: Schema = ArraySchema::new("",
> 	&GROUP_FILTER_SCHEMA).schema()
> 
> the only thing you'd have to do is to convice the updater that
> there is an updaterType for it: (in proxmox/src/api/schema.rs of the 
> proxmox crate)
> 
> impl<T> UpdaterType for Vec<T> {
>      type Updater = Option<Self>;
> }
> 
> that implies though that a user always has to give the complete list
> (so no single removal from the list), though i think that's what
> we want anyway..
> 
> i tested it here, and AFAICS, this works (i can update/list the sync job 
> properly)
> 
> with that you can also remove the 'filters' fn and can use
> Vec<GroupFilter> as the api call parameters.
> 
> makes the code a little easier to read

indeed it does :) only question is whether we want to enable this 
updater behaviour in general for Vec (and if we ever need "add/remove 
element(s)" semantics we do that via a custom type with custom Updater?)

> 
>>   #[api(
>>       properties: {
>>           id: {
>> 
> 
> 
> 

Re: [pve-devel] [PATCH v2 proxmox-backup 06/10] sync: add group filtering

[Dominik Csapak]

On 9/15/21 3:41 PM, Fabian Grünbichler wrote:
> like for manual pulls, but persisted in the sync job config and visible
> in the relevant GUI parts.
> 
> Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
> ---
> 
> Notes:
>      GUI is read-only for now (and defaults to no filtering on creation), as this is
>      a rather advanced feature that requires a complex GUI to be user-friendly
>      (regex-freeform, type-combobox, remote group scanning + selector with
>      additional freeform input).
>      
>      I did test the API manually though to see whether it works as expected, and
>      updating the filter list by overwriting with a new one passed in as multiple
>      parameters works as expected.
>      
>      if we want to make this configurable over the GUI, we probably want to switch
>      the job edit window to a tabpanel and add a second grid tab for selecting
>      the groups.
> 
>   pbs-api-types/src/jobs.rs |  6 ++++++
>   src/api2/config/sync.rs   |  5 +++++
>   src/api2/pull.rs          |  4 +++-
>   www/config/SyncView.js    | 13 ++++++++++++-
>   www/window/SyncJobEdit.js | 12 ++++++++++++
>   5 files changed, 38 insertions(+), 2 deletions(-)
> 
> diff --git a/pbs-api-types/src/jobs.rs b/pbs-api-types/src/jobs.rs
> index 94638fe5..06a12131 100644
> --- a/pbs-api-types/src/jobs.rs
> +++ b/pbs-api-types/src/jobs.rs
> @@ -439,6 +439,10 @@ impl GroupFilterList {
>               optional: true,
>               schema: SYNC_SCHEDULE_SCHEMA,
>           },
> +        groups: {
> +            type: GroupFilterList,
> +            optional: true,
> +        },
>       }
>   )]
>   #[derive(Serialize,Deserialize,Clone,Updater)]
> @@ -458,6 +462,8 @@ pub struct SyncJobConfig {
>       pub comment: Option<String>,
>       #[serde(skip_serializing_if="Option::is_none")]
>       pub schedule: Option<String>,
> +    #[serde(skip_serializing_if="Option::is_none")]
> +    pub groups: Option<GroupFilterList>,
>   }
>   
>   #[api(
> diff --git a/src/api2/config/sync.rs b/src/api2/config/sync.rs
> index 3c2bfd78..cbd5b192 100644
> --- a/src/api2/config/sync.rs
> +++ b/src/api2/config/sync.rs
> @@ -191,6 +191,8 @@ pub enum DeletableProperty {
>       schedule,
>       /// Delete the remove-vanished flag.
>       remove_vanished,
> +    /// Delete the groups property.
> +    groups,
>   }
>   
>   #[api(
> @@ -253,6 +255,7 @@ pub fn update_sync_job(
>                   DeletableProperty::comment => { data.comment = None; },
>                   DeletableProperty::schedule => { data.schedule = None; },
>                   DeletableProperty::remove_vanished => { data.remove_vanished = None; },
> +                DeletableProperty::groups => { data.groups = None; },
>               }
>           }
>       }
> @@ -270,6 +273,7 @@ pub fn update_sync_job(
>       if let Some(remote) = update.remote { data.remote = remote; }
>       if let Some(remote_store) = update.remote_store { data.remote_store = remote_store; }
>       if let Some(owner) = update.owner { data.owner = Some(owner); }
> +    if let Some(groups) = update.groups { data.groups = Some(groups); }
>   
>       let schedule_changed = data.schedule != update.schedule;
>       if update.schedule.is_some() { data.schedule = update.schedule; }
> @@ -389,6 +393,7 @@ acl:1:/remote/remote1/remotestore1:write@pbs:RemoteSyncOperator
>           owner: Some(write_auth_id.clone()),
>           comment: None,
>           remove_vanished: None,
> +        groups: None,
>           schedule: None,
>       };
>   
> diff --git a/src/api2/pull.rs b/src/api2/pull.rs
> index dfa6a938..86b618ed 100644
> --- a/src/api2/pull.rs
> +++ b/src/api2/pull.rs
> @@ -102,7 +102,9 @@ pub fn do_sync_job(
>                   worker.log(format!("Sync datastore '{}' from '{}/{}'",
>                           sync_job.store, sync_job.remote, sync_job.remote_store));
>   
> -                pull_store(&worker, &client, &src_repo, tgt_store.clone(), delete, sync_owner, None).await?;
> +                let sync_group_filter = sync_job.groups.map(GroupFilterList::filters);
> +
> +                pull_store(&worker, &client, &src_repo, tgt_store.clone(), delete, sync_owner, sync_group_filter).await?;

imho at this point, i'd look into giving pull_store the syncJobConfig 
directly and creating one for the 'pull' api... (just a struct in memory
to pass it to pull_store)

or, if the config contains too much, create a smaller struct and use that.

>   
>                   worker.log(format!("sync job '{}' end", &job_id));
>   
> diff --git a/www/config/SyncView.js b/www/config/SyncView.js
> index 7d7e751c..d2a3954f 100644
> --- a/www/config/SyncView.js
> +++ b/www/config/SyncView.js
> @@ -1,7 +1,7 @@
>   Ext.define('pbs-sync-jobs-status', {
>       extend: 'Ext.data.Model',
>       fields: [
> -	'id', 'owner', 'remote', 'remote-store', 'store', 'schedule',
> +	'id', 'owner', 'remote', 'remote-store', 'store', 'schedule', 'groups',
>   	'next-run', 'last-run-upid', 'last-run-state', 'last-run-endtime',
>   	{
>   	    name: 'duration',
> @@ -106,6 +106,11 @@ Ext.define('PBS.config.SyncJobView', {
>   	    return Ext.String.htmlEncode(value, metadata, record);
>   	},
>   
> +	render_optional_groups: function(value, metadata, record) {
> +	    if (!value) return gettext('All');
> +	    return Ext.String.htmlEncode(value, metadata, record);
> +	},
> +
>   	startStore: function() { this.getView().getStore().rstore.startUpdate(); },
>   	stopStore: function() { this.getView().getStore().rstore.stopUpdate(); },
>   
> @@ -214,6 +219,12 @@ Ext.define('PBS.config.SyncJobView', {
>   	    flex: 2,
>   	    sortable: true,
>   	},
> +	{
> +	    header: gettext('Backup Groups'),
> +	    dataIndex: 'groups',
> +	    renderer: 'render_optional_groups',
> +	    width: 80,
> +	},
>   	{
>   	    header: gettext('Schedule'),
>   	    dataIndex: 'schedule',
> diff --git a/www/window/SyncJobEdit.js b/www/window/SyncJobEdit.js
> index 47e65ae3..2399f11f 100644
> --- a/www/window/SyncJobEdit.js
> +++ b/www/window/SyncJobEdit.js
> @@ -199,6 +199,18 @@ Ext.define('PBS.window.SyncJobEdit', {
>   	],
>   
>   	columnB: [
> +	    {
> +		fieldLabel: gettext('Backup Groups'),
> +		xtype: 'displayfield',
> +		name: 'groups',
> +		renderer: function(value, metadata, record) {
> +		    if (!value) return gettext('All');
> +		    return Ext.String.htmlEncode(value, metadata, record);
> +		},
> +		cbind: {
> +		    hidden: '{isCreate}',
> +		},
> +	    },
>   	    {
>   		fieldLabel: gettext('Comment'),
>   		xtype: 'proxmoxtextfield',
>