[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: [pbs-devel] [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: [pbs-devel] [pve-devel] [PATCH v2 proxmox-backup 06/10] sync: add group filtering

[Thomas Lamprecht]

On 16.09.21 09:19, Thomas Lamprecht wrote:
>>     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.

I had a quick talk with Dominik which noticed me that there can be a list of filters.

So a separate tab-panel would seem OK to me. For UX I'd could imagine having a
"Add Filter" button that'd work out similarly to how we have the knet link add in PVE
cluster creation nowadays. In addition to that a preview window would be nice to have,
maybe similar to the "Prune now" one, but we only need the backup-groups here not the
full list of snapshots, so it would be relatively cheap to get.

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: [pbs-devel] [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: [pbs-devel] [pve-devel] [PATCH v2 proxmox-backup 06/10] sync: add group filtering

[Fabian Grünbichler]

On September 16, 2021 9:38 am, Thomas Lamprecht wrote:
> On 16.09.21 09:19, Thomas Lamprecht wrote:
>>>     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.
> 
> I had a quick talk with Dominik which noticed me that there can be a list of filters.
> 
> So a separate tab-panel would seem OK to me. For UX I'd could imagine having a
> "Add Filter" button that'd work out similarly to how we have the knet link add in PVE
> cluster creation nowadays. In addition to that a preview window would be nice to have,
> maybe similar to the "Prune now" one, but we only need the backup-groups here not the
> full list of snapshots, so it would be relatively cheap to get.
> 

yeah, that was kind of what I was going for - but with the added 
complexity of having different filter types with different 
"completion"/selection mechanisms
- the type filter has three static choices
- the regex is freeform (but maybe a dropdown with some common 
  suggestions would be nice? those could also live in the docs though)
- the group filter has a list of existing groups on the remote end to 
  choose from, but also requires freeform entry as option (a group might 
  not exist yet, or not exist any longer, but still needs to be filtered 
  for in the sync job, either to catch the group when it has been 
  created, or to clean it up when it was previously synced, or just for 
  handling the currently set filters correctly)

the last part I tried when writing v1, and it was quite the mess 
(Dominik's suggestion back then was to move it to a tab with a grid 
view, because the combobox selector was too limited IIRC).

a preview/dry-run for pull/sync would be nice in general, and shouldn't 
be too hard to add. it would need to run in a worker though (as it needs 
to query remote and local groups to decide what would be synced, even if 
the bulk work of actually downloading chunks is skipped). it could be 
done with the pull API without persisting the sync config, as those line 
up 1:1.

a preview of just the filterung with the already available (cached) 
remote groups is of course instant. could even be done GUI-side I guess, 
since the current filters are quite trivial to re-implement there. not 
sure how to fit that inside the already big dialogue tab though ;)

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',
> 

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

[Thomas Lamprecht]

On 16.09.21 09:57, Fabian Grünbichler wrote:
> On September 16, 2021 9:38 am, Thomas Lamprecht wrote:
>> On 16.09.21 09:19, Thomas Lamprecht wrote:
>>>>     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.
>>
>> I had a quick talk with Dominik which noticed me that there can be a list of filters.
>>
>> So a separate tab-panel would seem OK to me. For UX I'd could imagine having a
>> "Add Filter" button that'd work out similarly to how we have the knet link add in PVE
>> cluster creation nowadays. In addition to that a preview window would be nice to have,
>> maybe similar to the "Prune now" one, but we only need the backup-groups here not the
>> full list of snapshots, so it would be relatively cheap to get.
>>
> 
> yeah, that was kind of what I was going for - but with the added 
> complexity of having different filter types with different 
> "completion"/selection mechanisms
> - the type filter has three static choices
> - the regex is freeform (but maybe a dropdown with some common 
>   suggestions would be nice? those could also live in the docs though)

yes, can be similar in spirit of what we do for the calendar-event schedule
field.

> - the group filter has a list of existing groups on the remote end to 
>   choose from, but also requires freeform entry as option (a group might 
>   not exist yet, or not exist any longer, but still needs to be filtered 
>   for in the sync job, either to catch the group when it has been 
>   created, or to clean it up when it was previously synced, or just for 
>   handling the currently set filters correctly)

IMO that fits all in the approach with a [+] button, that could add a row that
looks somewhat like:

Type: [ Combobox ] | [ INPUT ]

Where the type combo-box allows to select for the regex-like filter or
a editabled group selection combobox where one multi-select the existing
but can also add others (i.e., only format not store/content validation).

> 
> the last part I tried when writing v1, and it was quite the mess 
> (Dominik's suggestion back then was to move it to a tab with a grid 
> view, because the combobox selector was too limited IIRC).
> 
> a preview/dry-run for pull/sync would be nice in general, and shouldn't 
> be too hard to add. it would need to run in a worker though (as it needs 
> to query remote and local groups to decide what would be synced, even if 
> the bulk work of actually downloading chunks is skipped). it could be 
> done with the pull API without persisting the sync config, as those line 
> up 1:1.

that's not required IMO, I would only show what the filter affects, that
stuff is then not synced if already present on the remote does not matters
much here on that level.

> a preview of just the filterung with the already available (cached) 
> remote groups is of course instant. could even be done GUI-side I guess, 
> since the current filters are quite trivial to re-implement there. not 
> sure how to fit that inside the already big dialogue tab though ;)

Yeah, replicating the filter on the client side would be nice, single thing
that could cause trouble is difference between JS regex engines and the rust
regex crate we use (the latter is probably much saner).