[pbs-devel] [PATCH proxmox-backup 0/4] use same config section type for all sync jobs

[pbs-devel] [PATCH proxmox-backup 0/4] use same config section type for all sync jobs

[Christian Ebner]

This patch series drops the `sync-push` config section type in favor of
using the same `sync` for both, sync jobs in push and pull direction.
Instead, encode the sync direction as optional parameter in the sync job
config, defaulting to sync in pull direction. This reduces complexity by
allowing to drop the optional parameter for most function calls.
For api methods, the default remains to only show sync directions in
pull direction, if no ListSyncDirection::All is passed, or the direction
explicitly selected. This allows to default to show both directions in
future Proxmox Backup Server version.

This patch series depends on Dominik's patch series found here:
https://lore.proxmox.com/pbs-devel/377618fd-0ea9-46ba-9aec-a47387eca50d@proxmox.com/T

Christian Ebner (4):
  config: sync: use same config section type `sync` for push and pull
  api: admin/config: introduce sync direction as job config parameter
  bin: show direction in sync job list output
  api types: drop unused config type helpers for sync direction

 pbs-api-types/src/jobs.rs              |  25 ++--
 pbs-config/src/sync.rs                 |  17 +--
 src/api2/admin/sync.rs                 |  18 +--
 src/api2/config/datastore.rs           |  16 +--
 src/api2/config/notifications/mod.rs   |  19 ++--
 src/api2/config/sync.rs                | 151 ++++++++-----------------
 src/bin/proxmox-backup-proxy.rs        |  22 +---
 src/bin/proxmox_backup_manager/sync.rs |   6 +-
 src/server/sync.rs                     |   2 +-
 9 files changed, 88 insertions(+), 188 deletions(-)

-- 
2.39.5



_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

[pbs-devel] [PATCH proxmox-backup 1/4] config: sync: use same config section type `sync` for push and pull

[Christian Ebner]

Use `sync` as config section type string for both, sync jobs in push
and pull direction, renaming the now combined config plugin to sync
plugin.

Commit bcd80bf9 ("api types/config: add `sync-push` config type for
push sync jobs") introduced the additional config type with the
intend to reduce possible misconfiguration. Partially revert this to
use the same config type string again, since the misconfiguration
can happen nevertheless (by editing the config type) and currently
sync job configs are only listed partially when fetched via the
config api endpoint. The filtering based on the additional api
parameter is however retained.

Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
---
 pbs-config/src/sync.rs | 17 ++++-------------
 1 file changed, 4 insertions(+), 13 deletions(-)

diff --git a/pbs-config/src/sync.rs b/pbs-config/src/sync.rs
index 7fc977e77..10f528b5e 100644
--- a/pbs-config/src/sync.rs
+++ b/pbs-config/src/sync.rs
@@ -6,7 +6,7 @@ use anyhow::Error;
 use proxmox_schema::{ApiType, Schema};
 use proxmox_section_config::{SectionConfig, SectionConfigData, SectionConfigPlugin};
 
-use pbs_api_types::{SyncDirection, SyncJobConfig, JOB_ID_SCHEMA};
+use pbs_api_types::{SyncJobConfig, JOB_ID_SCHEMA};
 
 use crate::{open_backup_lockfile, replace_backup_config, BackupLockGuard};
 
@@ -18,19 +18,10 @@ fn init() -> SectionConfig {
         _ => unreachable!(),
     };
 
-    let pull_plugin = SectionConfigPlugin::new(
-        SyncDirection::Pull.as_config_type_str().to_string(),
-        Some(String::from("id")),
-        obj_schema,
-    );
-    let push_plugin = SectionConfigPlugin::new(
-        SyncDirection::Push.as_config_type_str().to_string(),
-        Some(String::from("id")),
-        obj_schema,
-    );
+    let sync_plugin =
+        SectionConfigPlugin::new("sync".to_string(), Some(String::from("id")), obj_schema);
     let mut config = SectionConfig::new(&JOB_ID_SCHEMA);
-    config.register_plugin(pull_plugin);
-    config.register_plugin(push_plugin);
+    config.register_plugin(sync_plugin);
 
     config
 }
-- 
2.39.5



_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

[pbs-devel] [PATCH proxmox-backup 2/4] api: admin/config: introduce sync direction as job config parameter

[Christian Ebner]

Add the sync direction for the sync job as optional config parameter
and refrain from using the config section type for conditional
direction check, as they are now the same (see previous commit).

Use the configured sync job parameter instead of passing it to the
various methods as function parameter and only filter based on sync
direction if an optional api parameter to distingush/filter based on
direction is given.

Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
---
 pbs-api-types/src/jobs.rs              |   8 +-
 src/api2/admin/sync.rs                 |  18 +--
 src/api2/config/datastore.rs           |  16 +--
 src/api2/config/notifications/mod.rs   |  19 ++--
 src/api2/config/sync.rs                | 151 ++++++++-----------------
 src/bin/proxmox-backup-proxy.rs        |  22 +---
 src/bin/proxmox_backup_manager/sync.rs |   5 +-
 src/server/sync.rs                     |   2 +-
 8 files changed, 83 insertions(+), 158 deletions(-)

diff --git a/pbs-api-types/src/jobs.rs b/pbs-api-types/src/jobs.rs
index e18197fb1..4a85378ce 100644
--- a/pbs-api-types/src/jobs.rs
+++ b/pbs-api-types/src/jobs.rs
@@ -597,7 +597,11 @@ pub const RESYNC_CORRUPT_SCHEMA: Schema =
         "resync-corrupt": {
             schema: RESYNC_CORRUPT_SCHEMA,
             optional: true,
-        }
+        },
+        "sync-direction": {
+            type: SyncDirection,
+            optional: true,
+        },
     }
 )]
 #[derive(Serialize, Deserialize, Clone, Updater, PartialEq)]
@@ -633,6 +637,8 @@ pub struct SyncJobConfig {
     pub transfer_last: Option<usize>,
     #[serde(skip_serializing_if = "Option::is_none")]
     pub resync_corrupt: Option<bool>,
+    #[serde(skip_serializing_if = "Option::is_none")]
+    pub sync_direction: Option<SyncDirection>,
 }
 
 impl SyncJobConfig {
diff --git a/src/api2/admin/sync.rs b/src/api2/admin/sync.rs
index 2b8fce484..965be8d06 100644
--- a/src/api2/admin/sync.rs
+++ b/src/api2/admin/sync.rs
@@ -85,9 +85,9 @@ pub fn list_config_sync_jobs(
     let sync_direction = sync_direction.unwrap_or_default();
 
     let mut list = Vec::with_capacity(config.sections.len());
-    for (_, (sync_type, job)) in config.sections.into_iter() {
+    for (_, (_, job)) in config.sections.into_iter() {
         let job: SyncJobConfig = serde_json::from_value(job)?;
-        let direction = SyncDirection::from_config_type_str(&sync_type)?;
+        let direction = job.sync_direction.unwrap_or_default();
 
         match &store {
             Some(store) if &job.store != store => continue,
@@ -100,7 +100,7 @@ pub fn list_config_sync_jobs(
             _ => {}
         }
 
-        if !check_sync_job_read_access(&user_info, &auth_id, &job, direction) {
+        if !check_sync_job_read_access(&user_info, &auth_id, &job) {
             continue;
         }
 
@@ -144,15 +144,9 @@ pub fn run_sync_job(
     let user_info = CachedUserInfo::new()?;
 
     let (config, _digest) = sync::config()?;
-    let (config_type, config_section) = config
-        .sections
-        .get(&id)
-        .ok_or_else(|| format_err!("No sync job with id '{id}' found in config"))?;
+    let sync_job: SyncJobConfig = config.lookup("sync", &id)?;
 
-    let sync_direction = SyncDirection::from_config_type_str(config_type)?;
-    let sync_job = SyncJobConfig::deserialize(config_section)?;
-
-    if !check_sync_job_modify_access(&user_info, &auth_id, &sync_job, sync_direction) {
+    if !check_sync_job_modify_access(&user_info, &auth_id, &sync_job) {
         bail!("permission check failed, '{auth_id}' is missing access");
     }
 
@@ -160,7 +154,7 @@ pub fn run_sync_job(
 
     let to_stdout = rpcenv.env_type() == RpcEnvironmentType::CLI;
 
-    let upid_str = do_sync_job(job, sync_job, &auth_id, None, sync_direction, to_stdout)?;
+    let upid_str = do_sync_job(job, sync_job, &auth_id, None, to_stdout)?;
 
     Ok(upid_str)
 }
diff --git a/src/api2/config/datastore.rs b/src/api2/config/datastore.rs
index 8c307a233..1b0728a22 100644
--- a/src/api2/config/datastore.rs
+++ b/src/api2/config/datastore.rs
@@ -13,9 +13,8 @@ use proxmox_uuid::Uuid;
 
 use pbs_api_types::{
     Authid, DataStoreConfig, DataStoreConfigUpdater, DatastoreNotify, DatastoreTuning, KeepOptions,
-    MaintenanceMode, PruneJobConfig, PruneJobOptions, SyncDirection, DATASTORE_SCHEMA,
-    PRIV_DATASTORE_ALLOCATE, PRIV_DATASTORE_AUDIT, PRIV_DATASTORE_MODIFY,
-    PROXMOX_CONFIG_DIGEST_SCHEMA, UPID_SCHEMA,
+    MaintenanceMode, PruneJobConfig, PruneJobOptions, DATASTORE_SCHEMA, PRIV_DATASTORE_ALLOCATE,
+    PRIV_DATASTORE_AUDIT, PRIV_DATASTORE_MODIFY, PROXMOX_CONFIG_DIGEST_SCHEMA, UPID_SCHEMA,
 };
 use pbs_config::BackupLockGuard;
 use pbs_datastore::chunk_store::ChunkStore;
@@ -525,15 +524,8 @@ pub async fn delete_datastore(
         for job in list_verification_jobs(Some(name.clone()), Value::Null, rpcenv)? {
             delete_verification_job(job.config.id, None, rpcenv)?
         }
-        for direction in [SyncDirection::Pull, SyncDirection::Push] {
-            for job in list_config_sync_jobs(
-                Some(name.clone()),
-                Some(direction.into()),
-                Value::Null,
-                rpcenv,
-            )? {
-                delete_sync_job(job.config.id, None, rpcenv)?
-            }
+        for job in list_config_sync_jobs(Some(name.clone()), None, Value::Null, rpcenv)? {
+            delete_sync_job(job.config.id, None, rpcenv)?
         }
         for job in list_prune_jobs(Some(name.clone()), Value::Null, rpcenv)? {
             delete_prune_job(job.config.id, None, rpcenv)?
diff --git a/src/api2/config/notifications/mod.rs b/src/api2/config/notifications/mod.rs
index 2081b7b75..89b2ba8a4 100644
--- a/src/api2/config/notifications/mod.rs
+++ b/src/api2/config/notifications/mod.rs
@@ -9,7 +9,7 @@ use proxmox_schema::api;
 use proxmox_sortable_macro::sortable;
 
 use crate::api2::admin::datastore::get_datastore_list;
-use pbs_api_types::{SyncDirection, PRIV_SYS_AUDIT};
+use pbs_api_types::PRIV_SYS_AUDIT;
 
 use crate::api2::admin::prune::list_prune_jobs;
 use crate::api2::admin::sync::list_config_sync_jobs;
@@ -154,15 +154,13 @@ pub fn get_values(
         });
     }
 
-    for direction in [SyncDirection::Pull, SyncDirection::Push] {
-        let sync_jobs = list_config_sync_jobs(None, Some(direction.into()), param.clone(), rpcenv)?;
-        for job in sync_jobs {
-            values.push(MatchableValue {
-                field: "job-id".into(),
-                value: job.config.id,
-                comment: job.config.comment,
-            });
-        }
+    let sync_jobs = list_config_sync_jobs(None, None, param.clone(), rpcenv)?;
+    for job in sync_jobs {
+        values.push(MatchableValue {
+            field: "job-id".into(),
+            value: job.config.id,
+            comment: job.config.comment,
+        });
     }
 
     let verify_jobs = list_verification_jobs(None, param.clone(), rpcenv)?;
@@ -186,7 +184,6 @@ pub fn get_values(
         "package-updates",
         "prune",
         "sync",
-        "sync-push",
         "system-mail",
         "tape-backup",
         "tape-load",
diff --git a/src/api2/config/sync.rs b/src/api2/config/sync.rs
index afaa0d5e4..e8a1ad076 100644
--- a/src/api2/config/sync.rs
+++ b/src/api2/config/sync.rs
@@ -17,12 +17,12 @@ use pbs_config::sync;
 
 use pbs_config::CachedUserInfo;
 use pbs_datastore::check_backup_owner;
+use crate::api2::admin::sync::ListSyncDirection;
 
 pub fn check_sync_job_read_access(
     user_info: &CachedUserInfo,
     auth_id: &Authid,
     job: &SyncJobConfig,
-    sync_direction: SyncDirection,
 ) -> bool {
     // check for audit access on datastore/namespace, applies for pull and push direction
     let ns_anchor_privs = user_info.lookup_privs(auth_id, &job.acl_path());
@@ -30,6 +30,7 @@ pub fn check_sync_job_read_access(
         return false;
     }
 
+    let sync_direction = job.sync_direction.unwrap_or_default();
     match sync_direction {
         SyncDirection::Pull => {
             if let Some(remote) = &job.remote {
@@ -71,8 +72,8 @@ pub fn check_sync_job_modify_access(
     user_info: &CachedUserInfo,
     auth_id: &Authid,
     job: &SyncJobConfig,
-    sync_direction: SyncDirection,
 ) -> bool {
+    let sync_direction = job.sync_direction.unwrap_or_default();
     match sync_direction {
         SyncDirection::Pull => {
             let ns_anchor_privs = user_info.lookup_privs(auth_id, &job.acl_path());
@@ -150,7 +151,7 @@ pub fn check_sync_job_modify_access(
     input: {
         properties: {
             "sync-direction": {
-                type: SyncDirection,
+                type: ListSyncDirection,
                 optional: true,
             },
         },
@@ -168,23 +169,29 @@ pub fn check_sync_job_modify_access(
 /// List all sync jobs
 pub fn list_sync_jobs(
     _param: Value,
-    sync_direction: Option<SyncDirection>,
+    sync_direction: Option<ListSyncDirection>,
     rpcenv: &mut dyn RpcEnvironment,
 ) -> Result<Vec<SyncJobConfig>, Error> {
     let auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?;
     let user_info = CachedUserInfo::new()?;
+    let sync_direction = sync_direction.unwrap_or_default();
 
     let (config, digest) = sync::config()?;
 
-    let sync_direction = sync_direction.unwrap_or_default();
-    let list = config.convert_to_typed_array(sync_direction.as_config_type_str())?;
+    let list: Vec<SyncJobConfig> = config.convert_to_typed_array("sync")?;
 
     rpcenv["digest"] = hex::encode(digest).into();
 
     let list = list
         .into_iter()
         .filter(|sync_job| {
-            check_sync_job_read_access(&user_info, &auth_id, sync_job, sync_direction)
+            let direction = sync_job.sync_direction.unwrap_or_default();
+            match &sync_direction {
+                ListSyncDirection::Pull if direction != SyncDirection::Pull => return false,
+                ListSyncDirection::Push if direction != SyncDirection::Push => return false,
+                _ => {}
+            }
+            check_sync_job_read_access(&user_info, &auth_id, sync_job)
         })
         .collect();
     Ok(list)
@@ -198,10 +205,6 @@ pub fn list_sync_jobs(
                 type: SyncJobConfig,
                 flatten: true,
             },
-            "sync-direction": {
-                type: SyncDirection,
-                optional: true,
-            },
         },
     },
     access: {
@@ -212,16 +215,14 @@ pub fn list_sync_jobs(
 /// Create a new sync job.
 pub fn create_sync_job(
     config: SyncJobConfig,
-    sync_direction: Option<SyncDirection>,
     rpcenv: &mut dyn RpcEnvironment,
 ) -> Result<(), Error> {
     let auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?;
     let user_info = CachedUserInfo::new()?;
-    let sync_direction = sync_direction.unwrap_or_default();
 
     let _lock = sync::lock_config()?;
 
-    if !check_sync_job_modify_access(&user_info, &auth_id, &config, sync_direction) {
+    if !check_sync_job_modify_access(&user_info, &auth_id, &config) {
         bail!("permission check failed");
     }
 
@@ -229,6 +230,7 @@ pub fn create_sync_job(
         bail!("source and target datastore can't be the same");
     }
 
+    let sync_direction = config.sync_direction.unwrap_or_default();
     if sync_direction == SyncDirection::Push && config.resync_corrupt.is_some() {
         bail!("push jobs do not support resync-corrupt option");
     }
@@ -248,7 +250,7 @@ pub fn create_sync_job(
         param_bail!("id", "job '{}' already exists.", config.id);
     }
 
-    section_config.set_data(&config.id, sync_direction.as_config_type_str(), &config)?;
+    section_config.set_data(&config.id, "sync", &config)?;
 
     sync::save_config(&section_config)?;
 
@@ -278,17 +280,9 @@ pub fn read_sync_job(id: String, rpcenv: &mut dyn RpcEnvironment) -> Result<Sync
 
     let (config, digest) = sync::config()?;
 
-    let (sync_job, sync_direction) =
-        if let Some((config_type, config_section)) = config.sections.get(&id) {
-            (
-                SyncJobConfig::deserialize(config_section)?,
-                SyncDirection::from_config_type_str(config_type)?,
-            )
-        } else {
-            http_bail!(NOT_FOUND, "job '{id}' does not exist.")
-        };
+    let sync_job = config.lookup("sync", &id)?;
 
-    if !check_sync_job_read_access(&user_info, &auth_id, &sync_job, sync_direction) {
+    if !check_sync_job_read_access(&user_info, &auth_id, &sync_job) {
         bail!("permission check failed");
     }
 
@@ -330,6 +324,8 @@ pub enum DeletableProperty {
     MaxDepth,
     /// Delete the transfer_last property,
     TransferLast,
+    /// Delete the sync_direction property,
+    SyncDirection,
 }
 
 #[api(
@@ -383,17 +379,11 @@ pub fn update_sync_job(
         crate::tools::detect_modified_configuration_file(&digest, &expected_digest)?;
     }
 
-    let (mut data, sync_direction) =
-        if let Some((config_type, config_section)) = config.sections.get(&id) {
-            (
-                SyncJobConfig::deserialize(config_section)?,
-                SyncDirection::from_config_type_str(config_type)?,
-            )
-        } else {
-            http_bail!(NOT_FOUND, "job '{id}' does not exist.")
-        };
+    let mut data: SyncJobConfig = config.lookup("sync", &id)?;
 
-    if sync_direction == SyncDirection::Push && update.resync_corrupt.is_some() {
+    if update.sync_direction.unwrap_or_default() == SyncDirection::Push
+        && update.resync_corrupt.is_some()
+    {
         bail!("push jobs do not support resync-corrupt option");
     }
 
@@ -442,6 +432,9 @@ pub fn update_sync_job(
                 DeletableProperty::TransferLast => {
                     data.transfer_last = None;
                 }
+                DeletableProperty::SyncDirection => {
+                    data.sync_direction = None;
+                }
             }
         }
     }
@@ -482,6 +475,9 @@ pub fn update_sync_job(
     if let Some(resync_corrupt) = update.resync_corrupt {
         data.resync_corrupt = Some(resync_corrupt);
     }
+    if let Some(sync_direction) = update.sync_direction {
+        data.sync_direction = Some(sync_direction);
+    }
 
     if update.limit.rate_in.is_some() {
         data.limit.rate_in = update.limit.rate_in;
@@ -519,11 +515,11 @@ pub fn update_sync_job(
         }
     }
 
-    if !check_sync_job_modify_access(&user_info, &auth_id, &data, sync_direction) {
+    if !check_sync_job_modify_access(&user_info, &auth_id, &data) {
         bail!("permission check failed");
     }
 
-    config.set_data(&id, sync_direction.as_config_type_str(), &data)?;
+    config.set_data(&id, "sync", &data)?;
 
     sync::save_config(&config)?;
 
@@ -570,15 +566,16 @@ pub fn delete_sync_job(
         crate::tools::detect_modified_configuration_file(&digest, &expected_digest)?;
     }
 
-    if let Some((config_type, config_section)) = config.sections.get(&id) {
-        let sync_direction = SyncDirection::from_config_type_str(config_type)?;
-        let job = SyncJobConfig::deserialize(config_section)?;
-        if !check_sync_job_modify_access(&user_info, &auth_id, &job, sync_direction) {
-            bail!("permission check failed");
+    match config.lookup("sync", &id) {
+        Ok(job) => {
+            if !check_sync_job_modify_access(&user_info, &auth_id, &job) {
+                bail!("permission check failed");
+            }
+            config.sections.remove(&id);
+        }
+        Err(_) => {
+            http_bail!(NOT_FOUND, "job '{}' does not exist.", id)
         }
-        config.sections.remove(&id);
-    } else {
-        http_bail!(NOT_FOUND, "job '{}' does not exist.", id)
     }
 
     sync::save_config(&config)?;
@@ -647,62 +644,36 @@ acl:1:/remote/remote1/remotestore1:write@pbs:RemoteSyncOperator
         schedule: None,
         limit: pbs_api_types::RateLimitConfig::default(), // no limit
         transfer_last: None,
+        sync_direction: None, // use default
     };
 
     // should work without ACLs
-    assert!(check_sync_job_read_access(
-        &user_info,
-        root_auth_id,
-        &job,
-        SyncDirection::Pull,
-    ));
-    assert!(check_sync_job_modify_access(
-        &user_info,
-        root_auth_id,
-        &job,
-        SyncDirection::Pull,
-    ));
+    assert!(check_sync_job_read_access(&user_info, root_auth_id, &job));
+    assert!(check_sync_job_modify_access(&user_info, root_auth_id, &job,));
 
     // user without permissions must fail
     assert!(!check_sync_job_read_access(
         &user_info,
         &no_perm_auth_id,
         &job,
-        SyncDirection::Pull,
     ));
     assert!(!check_sync_job_modify_access(
         &user_info,
         &no_perm_auth_id,
         &job,
-        SyncDirection::Pull,
     ));
 
     // reading without proper read permissions on either remote or local must fail
-    assert!(!check_sync_job_read_access(
-        &user_info,
-        &read_auth_id,
-        &job,
-        SyncDirection::Pull,
-    ));
+    assert!(!check_sync_job_read_access(&user_info, &read_auth_id, &job,));
 
     // reading without proper read permissions on local end must fail
     job.remote = Some("remote1".to_string());
-    assert!(!check_sync_job_read_access(
-        &user_info,
-        &read_auth_id,
-        &job,
-        SyncDirection::Pull,
-    ));
+    assert!(!check_sync_job_read_access(&user_info, &read_auth_id, &job,));
 
     // reading without proper read permissions on remote end must fail
     job.remote = Some("remote0".to_string());
     job.store = "localstore1".to_string();
-    assert!(!check_sync_job_read_access(
-        &user_info,
-        &read_auth_id,
-        &job,
-        SyncDirection::Pull,
-    ));
+    assert!(!check_sync_job_read_access(&user_info, &read_auth_id, &job,));
 
     // writing without proper write permissions on either end must fail
     job.store = "localstore0".to_string();
@@ -710,7 +681,6 @@ acl:1:/remote/remote1/remotestore1:write@pbs:RemoteSyncOperator
         &user_info,
         &write_auth_id,
         &job,
-        SyncDirection::Pull,
     ));
 
     // writing without proper write permissions on local end must fail
@@ -723,53 +693,38 @@ acl:1:/remote/remote1/remotestore1:write@pbs:RemoteSyncOperator
         &user_info,
         &write_auth_id,
         &job,
-        SyncDirection::Pull,
     ));
 
     // reset remote to one where users have access
     job.remote = Some("remote1".to_string());
 
     // user with read permission can only read, but not modify/run
-    assert!(check_sync_job_read_access(
-        &user_info,
-        &read_auth_id,
-        &job,
-        SyncDirection::Pull,
-    ));
+    assert!(check_sync_job_read_access(&user_info, &read_auth_id, &job,));
     job.owner = Some(read_auth_id.clone());
     assert!(!check_sync_job_modify_access(
         &user_info,
         &read_auth_id,
         &job,
-        SyncDirection::Pull,
     ));
     job.owner = None;
     assert!(!check_sync_job_modify_access(
         &user_info,
         &read_auth_id,
         &job,
-        SyncDirection::Pull,
     ));
     job.owner = Some(write_auth_id.clone());
     assert!(!check_sync_job_modify_access(
         &user_info,
         &read_auth_id,
         &job,
-        SyncDirection::Pull,
     ));
 
     // user with simple write permission can modify/run
-    assert!(check_sync_job_read_access(
-        &user_info,
-        &write_auth_id,
-        &job,
-        SyncDirection::Pull,
-    ));
+    assert!(check_sync_job_read_access(&user_info, &write_auth_id, &job,));
     assert!(check_sync_job_modify_access(
         &user_info,
         &write_auth_id,
         &job,
-        SyncDirection::Pull,
     ));
 
     // but can't modify/run with deletion
@@ -778,7 +733,6 @@ acl:1:/remote/remote1/remotestore1:write@pbs:RemoteSyncOperator
         &user_info,
         &write_auth_id,
         &job,
-        SyncDirection::Pull,
     ));
 
     // unless they have Datastore.Prune as well
@@ -787,7 +741,6 @@ acl:1:/remote/remote1/remotestore1:write@pbs:RemoteSyncOperator
         &user_info,
         &write_auth_id,
         &job,
-        SyncDirection::Pull,
     ));
 
     // changing owner is not possible
@@ -796,7 +749,6 @@ acl:1:/remote/remote1/remotestore1:write@pbs:RemoteSyncOperator
         &user_info,
         &write_auth_id,
         &job,
-        SyncDirection::Pull,
     ));
 
     // also not to the default 'root@pam'
@@ -805,7 +757,6 @@ acl:1:/remote/remote1/remotestore1:write@pbs:RemoteSyncOperator
         &user_info,
         &write_auth_id,
         &job,
-        SyncDirection::Pull,
     ));
 
     // unless they have Datastore.Modify as well
@@ -815,14 +766,12 @@ acl:1:/remote/remote1/remotestore1:write@pbs:RemoteSyncOperator
         &user_info,
         &write_auth_id,
         &job,
-        SyncDirection::Pull,
     ));
     job.owner = None;
     assert!(check_sync_job_modify_access(
         &user_info,
         &write_auth_id,
         &job,
-        SyncDirection::Pull,
     ));
 
     Ok(())
diff --git a/src/bin/proxmox-backup-proxy.rs b/src/bin/proxmox-backup-proxy.rs
index 70283510d..ec3df856a 100644
--- a/src/bin/proxmox-backup-proxy.rs
+++ b/src/bin/proxmox-backup-proxy.rs
@@ -40,8 +40,8 @@ use pbs_buildcfg::configdir;
 use proxmox_time::CalendarEvent;
 
 use pbs_api_types::{
-    Authid, DataStoreConfig, Operation, PruneJobConfig, SyncDirection, SyncJobConfig,
-    TapeBackupJobConfig, VerificationJobConfig,
+    Authid, DataStoreConfig, Operation, PruneJobConfig, SyncJobConfig, TapeBackupJobConfig,
+    VerificationJobConfig,
 };
 
 use proxmox_backup::auth_helpers::*;
@@ -589,14 +589,7 @@ async fn schedule_datastore_sync_jobs() {
         Ok((config, _digest)) => config,
     };
 
-    for (job_id, (job_type, job_config)) in config.sections {
-        let sync_direction = match SyncDirection::from_config_type_str(&job_type) {
-            Ok(direction) => direction,
-            Err(err) => {
-                eprintln!("unexpected config type in sync job config - {err}");
-                continue;
-            }
-        };
+    for (job_id, (_, job_config)) in config.sections {
         let job_config: SyncJobConfig = match serde_json::from_value(job_config) {
             Ok(c) => c,
             Err(err) => {
@@ -618,14 +611,7 @@ async fn schedule_datastore_sync_jobs() {
             };
 
             let auth_id = Authid::root_auth_id().clone();
-            if let Err(err) = do_sync_job(
-                job,
-                job_config,
-                &auth_id,
-                Some(event_str),
-                sync_direction,
-                false,
-            ) {
+            if let Err(err) = do_sync_job(job, job_config, &auth_id, Some(event_str), false) {
                 eprintln!("unable to start datastore sync job {job_id} - {err}");
             }
         };
diff --git a/src/bin/proxmox_backup_manager/sync.rs b/src/bin/proxmox_backup_manager/sync.rs
index b08bfb58b..3ccaae943 100644
--- a/src/bin/proxmox_backup_manager/sync.rs
+++ b/src/bin/proxmox_backup_manager/sync.rs
@@ -4,9 +4,10 @@ use serde_json::Value;
 use proxmox_router::{cli::*, ApiHandler, RpcEnvironment};
 use proxmox_schema::api;
 
-use pbs_api_types::{SyncDirection, JOB_ID_SCHEMA};
+use pbs_api_types::JOB_ID_SCHEMA;
 
 use proxmox_backup::api2;
+use crate::api2::admin::sync::ListSyncDirection;
 
 fn render_group_filter(value: &Value, _record: &Value) -> Result<String, Error> {
     if let Some(group_filters) = value.as_array() {
@@ -21,7 +22,7 @@ fn render_group_filter(value: &Value, _record: &Value) -> Result<String, Error>
     input: {
         properties: {
             "sync-direction": {
-                type: SyncDirection,
+                type: ListSyncDirection,
                 optional: true,
             },
             "output-format": {
diff --git a/src/server/sync.rs b/src/server/sync.rs
index 4c6b43d24..0bd7a7a85 100644
--- a/src/server/sync.rs
+++ b/src/server/sync.rs
@@ -597,7 +597,6 @@ pub fn do_sync_job(
     sync_job: SyncJobConfig,
     auth_id: &Authid,
     schedule: Option<String>,
-    sync_direction: SyncDirection,
     to_stdout: bool,
 ) -> Result<String, Error> {
     let job_id = format!(
@@ -609,6 +608,7 @@ pub fn do_sync_job(
         job.jobname(),
     );
     let worker_type = job.jobtype().to_string();
+    let sync_direction = sync_job.sync_direction.unwrap_or_default();
 
     if sync_job.remote.is_none() && sync_job.store == sync_job.remote_store {
         bail!("can't sync to same datastore");
-- 
2.39.5



_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

[pbs-devel] [PATCH proxmox-backup 3/4] bin: show direction in sync job list output

[Christian Ebner]

As the WebUI also lists the sync direction, display the direction in
the cli output as well.

Examplary output:
```
┌─────────────────┬────────────────┬───────────┬────────┬───────────────────┬──────────┬──────────────┬─────────┬─────────┐
│ id              │ sync-direction │ store     │ remote │ remote-store      │ schedule │ group-filter │ rate-in │ comment │
╞═════════════════╪════════════════╪═══════════╪════════╪═══════════════════╪══════════╪══════════════╪═════════╪═════════╡
│ s-6c16fab2-9e85 │                │ datastore │        │ datastore         │ hourly   │ all          │         │         │
├─────────────────┼────────────────┼───────────┼────────┼───────────────────┼──────────┼──────────────┼─────────┼─────────┤
│ s-8764c440-3a6c │ push           │ datastore │ local  │ push-target-store │ hourly   │ all          │         │         │
└─────────────────┴────────────────┴───────────┴────────┴───────────────────┴──────────┴──────────────┴─────────┴─────────┘
```

Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
---
 src/bin/proxmox_backup_manager/sync.rs | 1 +
 1 file changed, 1 insertion(+)

diff --git a/src/bin/proxmox_backup_manager/sync.rs b/src/bin/proxmox_backup_manager/sync.rs
index 3ccaae943..42df08d30 100644
--- a/src/bin/proxmox_backup_manager/sync.rs
+++ b/src/bin/proxmox_backup_manager/sync.rs
@@ -44,6 +44,7 @@ fn list_sync_jobs(param: Value, rpcenv: &mut dyn RpcEnvironment) -> Result<Value
 
     let options = default_table_format_options()
         .column(ColumnConfig::new("id"))
+        .column(ColumnConfig::new("sync-direction"))
         .column(ColumnConfig::new("store"))
         .column(ColumnConfig::new("remote"))
         .column(ColumnConfig::new("remote-store"))
-- 
2.39.5



_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

[pbs-devel] [PATCH proxmox-backup 4/4] api types: drop unused config type helpers for sync direction

[Christian Ebner]

Jobs for both sync directions are now stored using the same `sync`
config section type, so drop the outdated helpers.

Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
---
 pbs-api-types/src/jobs.rs | 17 -----------------
 1 file changed, 17 deletions(-)

diff --git a/pbs-api-types/src/jobs.rs b/pbs-api-types/src/jobs.rs
index 4a85378ce..16b16dd84 100644
--- a/pbs-api-types/src/jobs.rs
+++ b/pbs-api-types/src/jobs.rs
@@ -519,23 +519,6 @@ impl std::fmt::Display for SyncDirection {
     }
 }
 
-impl SyncDirection {
-    pub fn as_config_type_str(&self) -> &'static str {
-        match self {
-            SyncDirection::Pull => "sync",
-            SyncDirection::Push => "sync-push",
-        }
-    }
-
-    pub fn from_config_type_str(config_type: &str) -> Result<Self, anyhow::Error> {
-        match config_type {
-            "sync" => Ok(SyncDirection::Pull),
-            "sync-push" => Ok(SyncDirection::Push),
-            _ => bail!("invalid config type for sync job"),
-        }
-    }
-}
-
 pub const RESYNC_CORRUPT_SCHEMA: Schema =
     BooleanSchema::new("If the verification failed for a local snapshot, try to pull it again.")
         .schema();
-- 
2.39.5



_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

[pbs-devel] [PATCH proxmox-backup] (List)SyncDirection: extract match check into impl fn

[Fabian Grünbichler]

in case we add another direction or another call site, doing it without a
wildcard match arm seems cleaner.

Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
---
small cleanup as potential follow-up

 src/api2/admin/sync.rs  | 16 ++++++++++++----
 src/api2/config/sync.rs | 11 +++--------
 2 files changed, 15 insertions(+), 12 deletions(-)

diff --git a/src/api2/admin/sync.rs b/src/api2/admin/sync.rs
index 965be8d06..089e6f50d 100644
--- a/src/api2/admin/sync.rs
+++ b/src/api2/admin/sync.rs
@@ -47,6 +47,16 @@ impl From<SyncDirection> for ListSyncDirection {
     }
 }
 
+impl ListSyncDirection {
+    /// Checks whether a `ListSyncDirection` matches a given `SyncDirection`
+    pub fn matches(&self, other: SyncDirection) -> bool {
+        if *self == ListSyncDirection::All {
+            return true;
+        }
+        *self == other.into()
+    }
+}
+
 #[api(
     input: {
         properties: {
@@ -94,10 +104,8 @@ pub fn list_config_sync_jobs(
             _ => {}
         }
 
-        match &sync_direction {
-            ListSyncDirection::Pull if direction != SyncDirection::Pull => continue,
-            ListSyncDirection::Push if direction != SyncDirection::Push => continue,
-            _ => {}
+        if !sync_direction.matches(direction) {
+            continue;
         }
 
         if !check_sync_job_read_access(&user_info, &auth_id, &job) {
diff --git a/src/api2/config/sync.rs b/src/api2/config/sync.rs
index e8a1ad076..bc012744a 100644
--- a/src/api2/config/sync.rs
+++ b/src/api2/config/sync.rs
@@ -15,9 +15,9 @@ use pbs_api_types::{
 };
 use pbs_config::sync;
 
+use crate::api2::admin::sync::ListSyncDirection;
 use pbs_config::CachedUserInfo;
 use pbs_datastore::check_backup_owner;
-use crate::api2::admin::sync::ListSyncDirection;
 
 pub fn check_sync_job_read_access(
     user_info: &CachedUserInfo,
@@ -185,13 +185,8 @@ pub fn list_sync_jobs(
     let list = list
         .into_iter()
         .filter(|sync_job| {
-            let direction = sync_job.sync_direction.unwrap_or_default();
-            match &sync_direction {
-                ListSyncDirection::Pull if direction != SyncDirection::Pull => return false,
-                ListSyncDirection::Push if direction != SyncDirection::Push => return false,
-                _ => {}
-            }
-            check_sync_job_read_access(&user_info, &auth_id, sync_job)
+            sync_direction.matches(sync_job.sync_direction.unwrap_or_default())
+                && check_sync_job_read_access(&user_info, &auth_id, sync_job)
         })
         .collect();
     Ok(list)
-- 
2.39.5



_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

Re: [pbs-devel] [PATCH proxmox-backup 0/4] use same config section type for all sync jobs

[Fabian Grünbichler]

FWIW, did a quick do-over and testing and found no issues with these two
series combined.

On November 25, 2024 6:40 pm, Christian Ebner wrote:
> This patch series drops the `sync-push` config section type in favor of
> using the same `sync` for both, sync jobs in push and pull direction.
> Instead, encode the sync direction as optional parameter in the sync job
> config, defaulting to sync in pull direction. This reduces complexity by
> allowing to drop the optional parameter for most function calls.
> For api methods, the default remains to only show sync directions in
> pull direction, if no ListSyncDirection::All is passed, or the direction
> explicitly selected. This allows to default to show both directions in
> future Proxmox Backup Server version.
> 
> This patch series depends on Dominik's patch series found here:
> https://lore.proxmox.com/pbs-devel/377618fd-0ea9-46ba-9aec-a47387eca50d@proxmox.com/T
> 
> Christian Ebner (4):
>   config: sync: use same config section type `sync` for push and pull
>   api: admin/config: introduce sync direction as job config parameter
>   bin: show direction in sync job list output
>   api types: drop unused config type helpers for sync direction
> 
>  pbs-api-types/src/jobs.rs              |  25 ++--
>  pbs-config/src/sync.rs                 |  17 +--
>  src/api2/admin/sync.rs                 |  18 +--
>  src/api2/config/datastore.rs           |  16 +--
>  src/api2/config/notifications/mod.rs   |  19 ++--
>  src/api2/config/sync.rs                | 151 ++++++++-----------------
>  src/bin/proxmox-backup-proxy.rs        |  22 +---
>  src/bin/proxmox_backup_manager/sync.rs |   6 +-
>  src/server/sync.rs                     |   2 +-
>  9 files changed, 88 insertions(+), 188 deletions(-)
> 
> -- 
> 2.39.5
> 
> 
> 
> _______________________________________________
> pbs-devel mailing list
> pbs-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
> 
> 
>


_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

[pbs-devel] [PATCH proxmox-backup] sync jobs: remove superfluous direction property

[Dominik Csapak]

since the SyncJobConfig struct now contains a 'sync-direction' property, we can
omit the 'direction' property of the SyncJobStatus struct. This makes a
few adaptions in the ui necessary:

* use the correct field
* handle 'pull' as default (since we don't necessarily get a
  'sync-direction' in that case)

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
based on:

https://lore.proxmox.com/pbs-devel/20241125174012.678523-1-c.ebner@proxmox.com/
and
https://lore.proxmox.com/pbs-devel/20241126092029.207319-1-f.gruenbichler@proxmox.com/


 pbs-api-types/src/jobs.rs |  6 ------
 src/api2/admin/sync.rs    |  1 -
 www/config/SyncView.js    | 16 ++++++++--------
 3 files changed, 8 insertions(+), 15 deletions(-)

diff --git a/pbs-api-types/src/jobs.rs b/pbs-api-types/src/jobs.rs
index 16b16dd84..04631d920 100644
--- a/pbs-api-types/src/jobs.rs
+++ b/pbs-api-types/src/jobs.rs
@@ -649,9 +649,6 @@ impl SyncJobConfig {
         status: {
             type: JobScheduleStatus,
         },
-        direction: {
-            type: SyncDirection,
-        },
     },
 )]
 #[derive(Serialize, Deserialize, Clone, PartialEq)]
@@ -662,9 +659,6 @@ pub struct SyncJobStatus {
     pub config: SyncJobConfig,
     #[serde(flatten)]
     pub status: JobScheduleStatus,
-
-    /// The direction of the job
-    pub direction: SyncDirection,
 }
 
 /// These are used separately without `ns`/`max-depth` sometimes in the API, specifically in the API
diff --git a/src/api2/admin/sync.rs b/src/api2/admin/sync.rs
index 089e6f50d..6722ebea0 100644
--- a/src/api2/admin/sync.rs
+++ b/src/api2/admin/sync.rs
@@ -120,7 +120,6 @@ pub fn list_config_sync_jobs(
         list.push(SyncJobStatus {
             config: job,
             status,
-            direction,
         });
     }
 
diff --git a/www/config/SyncView.js b/www/config/SyncView.js
index ca1f7ecd6..503bdc6df 100644
--- a/www/config/SyncView.js
+++ b/www/config/SyncView.js
@@ -45,7 +45,7 @@ Ext.define('PBS.config.SyncJobView', {
 
 	    store.clearFilter();
 
-	    let fieldsToSearch = ['direction', 'id', 'remote', 'remote-store', 'owner'];
+	    let fieldsToSearch = ['sync-direction', 'id', 'remote', 'remote-store', 'owner'];
 	    if (!view.datastore) {
 		fieldsToSearch.push('store');
 	    }
@@ -96,7 +96,7 @@ Ext.define('PBS.config.SyncJobView', {
             Ext.create('PBS.window.SyncJobEdit', {
 		datastore: view.datastore,
                 id: selection[0].data.id,
-		syncDirection: selection[0].data.direction,
+		syncDirection: selection[0].data['sync-direction'],
 		listeners: {
 		    destroy: function() {
 			me.reload();
@@ -174,7 +174,7 @@ Ext.define('PBS.config.SyncJobView', {
 	type: 'diff',
 	autoDestroy: true,
 	autoDestroyRstore: true,
-	sorters: ['store', 'direction', 'id'],
+	sorters: ['store', 'sync-direction', 'id'],
 	rstore: {
 	    type: 'update',
 	    storeid: 'pbs-sync-jobs-status',
@@ -277,15 +277,15 @@ Ext.define('PBS.config.SyncJobView', {
 	},
 	{
 	    header: gettext('Direction'),
-	    dataIndex: 'direction',
+	    dataIndex: 'sync-direction',
 	    renderer: function(value) {
 		let iconCls, text;
-		if (value === 'pull') {
-		    iconCls = 'download';
-		    text = gettext('Pull');
-		} else {
+		if (value === 'push') {
 		    iconCls = 'upload';
 		    text = gettext('Push');
+		} else {
+		    iconCls = 'download';
+		    text = gettext('Pull');
 		}
 		return `<i class="fa fa-fw fa-${iconCls}"></i> ${text}`;
 	    },
-- 
2.39.5



_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

Re: [pbs-devel] [PATCH proxmox-backup 0/4] use same config section type for all sync jobs

[Dominik Csapak]

look good to me beside one small issue with the leftover direction
property of the status, but sent a follow up for that:

https://lore.proxmox.com/pbs-devel/20241126144734.2858189-1-d.csapak@proxmox.com/

(both christians series and fabians patch should be applied)

with that consider christians/fabians patches:

Reviewed-by: Dominik Csapak <d.csapak@proxmox.com>
Tested-by: Dominik Csapak <d.csapak@proxmox.com>




_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

[pbs-devel] applied-series: [PATCH proxmox-backup 0/4] use same config section type for all sync jobs

[Thomas Lamprecht]

Am 25.11.24 um 18:40 schrieb Christian Ebner:
> This patch series drops the `sync-push` config section type in favor of
> using the same `sync` for both, sync jobs in push and pull direction.
> Instead, encode the sync direction as optional parameter in the sync job
> config, defaulting to sync in pull direction. This reduces complexity by
> allowing to drop the optional parameter for most function calls.
> For api methods, the default remains to only show sync directions in
> pull direction, if no ListSyncDirection::All is passed, or the direction
> explicitly selected. This allows to default to show both directions in
> future Proxmox Backup Server version.
> 
> This patch series depends on Dominik's patch series found here:
> https://lore.proxmox.com/pbs-devel/377618fd-0ea9-46ba-9aec-a47387eca50d@proxmox.com/T
> 
> Christian Ebner (4):
>   config: sync: use same config section type `sync` for push and pull
>   api: admin/config: introduce sync direction as job config parameter
>   bin: show direction in sync job list output
>   api types: drop unused config type helpers for sync direction
> 
>  pbs-api-types/src/jobs.rs              |  25 ++--
>  pbs-config/src/sync.rs                 |  17 +--
>  src/api2/admin/sync.rs                 |  18 +--
>  src/api2/config/datastore.rs           |  16 +--
>  src/api2/config/notifications/mod.rs   |  19 ++--
>  src/api2/config/sync.rs                | 151 ++++++++-----------------
>  src/bin/proxmox-backup-proxy.rs        |  22 +---
>  src/bin/proxmox_backup_manager/sync.rs |   6 +-
>  src/server/sync.rs                     |   2 +-
>  9 files changed, 88 insertions(+), 188 deletions(-)
> 


applied series with both Fabian's and Dominik's follow-ups and from the latter
also his review trailers, thanks!


_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel