From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) by lore.proxmox.com (Postfix) with ESMTPS id 7576A1FF163 for ; Thu, 10 Oct 2024 16:48:24 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id EA2741C77E; Thu, 10 Oct 2024 16:48:51 +0200 (CEST) Date: Thu, 10 Oct 2024 16:48:40 +0200 From: Fabian =?iso-8859-1?q?Gr=FCnbichler?= To: Proxmox Backup Server development discussion References: <20240912143322.548839-1-c.ebner@proxmox.com> <20240912143322.548839-20-c.ebner@proxmox.com> In-Reply-To: <20240912143322.548839-20-c.ebner@proxmox.com> MIME-Version: 1.0 User-Agent: astroid/0.16.0 (https://github.com/astroidmail/astroid) Message-Id: <1728563155.hqqja90k1h.astroid@yuna.none> X-SPAM-LEVEL: Spam detection results: 0 AWL 0.049 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Subject: Re: [pbs-devel] [PATCH v3 proxmox-backup 19/33] api: sync jobs: expose optional `sync-direction` parameter X-BeenThere: pbs-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox Backup Server development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: Proxmox Backup Server development discussion Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: pbs-devel-bounces@lists.proxmox.com Sender: "pbs-devel" On September 12, 2024 4:33 pm, Christian Ebner wrote: > Exposes and switch the config type for sync job operations based > on the `sync-direction` parameter. If not set, the default config > type is `sync` and the default sync direction is `pull` for full > backwards compatibility. > > Signed-off-by: Christian Ebner > --- > changes since version 2: > - no changes > > src/api2/admin/sync.rs | 28 +++++++++------ > src/api2/config/datastore.rs | 22 +++++++++--- > src/api2/config/notifications/mod.rs | 15 ++++++-- > src/api2/config/sync.rs | 53 +++++++++++++++++++++++----- > src/bin/proxmox-backup-proxy.rs | 12 +++++-- > 5 files changed, 101 insertions(+), 29 deletions(-) > > diff --git a/src/api2/admin/sync.rs b/src/api2/admin/sync.rs > index be324564c..bdbc06a8e 100644 > --- a/src/api2/admin/sync.rs > +++ b/src/api2/admin/sync.rs > @@ -12,6 +12,7 @@ use proxmox_sortable_macro::sortable; > > use pbs_api_types::{ > Authid, SyncDirection, SyncJobConfig, SyncJobStatus, DATASTORE_SCHEMA, JOB_ID_SCHEMA, > + SYNC_DIRECTION_SCHEMA, > }; > use pbs_config::sync; > use pbs_config::CachedUserInfo; > @@ -29,6 +30,10 @@ use crate::{ > schema: DATASTORE_SCHEMA, > optional: true, > }, > + "sync-direction": { > + schema: SYNC_DIRECTION_SCHEMA, > + optional: true, > + }, > }, > }, > returns: { > @@ -44,6 +49,7 @@ use crate::{ > /// List all sync jobs > pub fn list_sync_jobs( > store: Option, > + sync_direction: Option, would be much nicer if the default were already encoded in the API schema > _param: Value, > rpcenv: &mut dyn RpcEnvironment, > ) -> Result, Error> { > @@ -51,9 +57,10 @@ pub fn list_sync_jobs( > let user_info = CachedUserInfo::new()?; > > let (config, digest) = sync::config()?; > + let sync_direction = sync_direction.unwrap_or_default(); instead of unwrapping here.. > > let job_config_iter = config > - .convert_to_typed_array("sync")? > + .convert_to_typed_array(sync_direction.as_config_type_str())? > .into_iter() > .filter(|job: &SyncJobConfig| { > if let Some(store) = &store { > @@ -88,7 +95,11 @@ pub fn list_sync_jobs( > properties: { > id: { > schema: JOB_ID_SCHEMA, > - } > + }, > + "sync-direction": { > + schema: SYNC_DIRECTION_SCHEMA, > + optional: true, > + }, > } > }, > access: { > @@ -99,6 +110,7 @@ pub fn list_sync_jobs( > /// Runs the sync jobs manually. > pub fn run_sync_job( > id: String, > + sync_direction: Option, > _info: &ApiMethod, > rpcenv: &mut dyn RpcEnvironment, > ) -> Result { > @@ -106,7 +118,8 @@ pub fn run_sync_job( > let user_info = CachedUserInfo::new()?; > > let (config, _digest) = sync::config()?; > - let sync_job: SyncJobConfig = config.lookup("sync", &id)?; > + let sync_direction = sync_direction.unwrap_or_default(); same here > + let sync_job: SyncJobConfig = config.lookup(sync_direction.as_config_type_str(), &id)?; > > if !check_sync_job_modify_access(&user_info, &auth_id, &sync_job) { > bail!("permission check failed"); > @@ -116,14 +129,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, > - SyncDirection::Pull, > - to_stdout, > - )?; > + let upid_str = do_sync_job(job, sync_job, &auth_id, None, sync_direction, to_stdout)?; > > Ok(upid_str) > } > diff --git a/src/api2/config/datastore.rs b/src/api2/config/datastore.rs > index ca6edf05a..a01d26cad 100644 > --- a/src/api2/config/datastore.rs > +++ b/src/api2/config/datastore.rs > @@ -13,8 +13,9 @@ use proxmox_uuid::Uuid; > > use pbs_api_types::{ > Authid, DataStoreConfig, DataStoreConfigUpdater, DatastoreNotify, DatastoreTuning, KeepOptions, > - MaintenanceMode, PruneJobConfig, PruneJobOptions, DATASTORE_SCHEMA, PRIV_DATASTORE_ALLOCATE, > - PRIV_DATASTORE_AUDIT, PRIV_DATASTORE_MODIFY, PROXMOX_CONFIG_DIGEST_SCHEMA, UPID_SCHEMA, > + MaintenanceMode, PruneJobConfig, PruneJobOptions, SyncDirection, 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; > @@ -498,8 +499,21 @@ 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 job in list_sync_jobs(Some(name.clone()), Value::Null, rpcenv)? { > - delete_sync_job(job.config.id, None, rpcenv)? > + for job in list_sync_jobs( > + Some(name.clone()), > + Some(SyncDirection::Pull), > + Value::Null, > + rpcenv, > + )? { > + delete_sync_job(job.config.id, Some(SyncDirection::Pull), None, rpcenv)? > + } > + for job in list_sync_jobs( > + Some(name.clone()), > + Some(SyncDirection::Push), > + Value::Null, > + rpcenv, > + )? { > + delete_sync_job(job.config.id, Some(SyncDirection::Push), None, rpcenv)? this looks a bit weird, but I guess it's a side-effect we have to live with if we want to separate both types of sync jobs somewhat.. could still be a nested loop though for brevity? for direction in .. { for job in list_sync_jobs(.. , direction, ..)? { delete_sync_job(.. , direction, ..)?; } } > } > 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 dfe82ed03..9622d43ee 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::PRIV_SYS_AUDIT; > +use pbs_api_types::{SyncDirection, PRIV_SYS_AUDIT}; > > use crate::api2::admin::prune::list_prune_jobs; > use crate::api2::admin::sync::list_sync_jobs; > @@ -154,8 +154,16 @@ pub fn get_values( > }); > } > > - let sync_jobs = list_sync_jobs(None, param.clone(), rpcenv)?; > - for job in sync_jobs { > + let sync_jobs_pull = list_sync_jobs(None, Some(SyncDirection::Pull), param.clone(), rpcenv)?; > + for job in sync_jobs_pull { > + values.push(MatchableValue { > + field: "job-id".into(), > + value: job.config.id, > + comment: job.config.comment, > + }); > + } > + let sync_jobs_push = list_sync_jobs(None, Some(SyncDirection::Push), param.clone(), rpcenv)?; > + for job in sync_jobs_push { here as well? or alternatively, all a third SyncDirection variant Any, but not sure if it's worth it just for those two list_sync_jobs functions (btw, one of those might benefit from being renamed while we are at it..). > values.push(MatchableValue { > field: "job-id".into(), > value: job.config.id, > @@ -184,6 +192,7 @@ 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 6fdc69a9e..a21e0bd6f 100644 > --- a/src/api2/config/sync.rs > +++ b/src/api2/config/sync.rs > @@ -1,6 +1,7 @@ > use ::serde::{Deserialize, Serialize}; > use anyhow::{bail, Error}; > use hex::FromHex; > +use pbs_api_types::SyncDirection; > use serde_json::Value; > > use proxmox_router::{http_bail, Permission, Router, RpcEnvironment}; > @@ -9,7 +10,7 @@ use proxmox_schema::{api, param_bail}; > use pbs_api_types::{ > Authid, SyncJobConfig, SyncJobConfigUpdater, JOB_ID_SCHEMA, PRIV_DATASTORE_AUDIT, > PRIV_DATASTORE_BACKUP, PRIV_DATASTORE_MODIFY, PRIV_DATASTORE_PRUNE, PRIV_REMOTE_AUDIT, > - PRIV_REMOTE_READ, PROXMOX_CONFIG_DIGEST_SCHEMA, > + PRIV_REMOTE_READ, PROXMOX_CONFIG_DIGEST_SCHEMA, SYNC_DIRECTION_SCHEMA, > }; > use pbs_config::sync; > > @@ -77,7 +78,12 @@ pub fn check_sync_job_modify_access( > > #[api( > input: { > - properties: {}, > + properties: { > + "sync-direction": { > + schema: SYNC_DIRECTION_SCHEMA, > + optional: true, > + }, > + }, > }, > returns: { > description: "List configured jobs.", > @@ -92,6 +98,7 @@ pub fn check_sync_job_modify_access( > /// List all sync jobs > pub fn list_sync_jobs( > _param: Value, > + sync_direction: Option, > rpcenv: &mut dyn RpcEnvironment, > ) -> Result, Error> { > let auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?; > @@ -99,7 +106,8 @@ pub fn list_sync_jobs( > > let (config, digest) = sync::config()?; > > - let list = config.convert_to_typed_array("sync")?; > + let sync_direction = sync_direction.unwrap_or_default(); this unwrap_or_default would also be better off being encoded in the schema.. > + let list = config.convert_to_typed_array(sync_direction.as_config_type_str())?; > > rpcenv["digest"] = hex::encode(digest).into(); > > @@ -118,6 +126,10 @@ pub fn list_sync_jobs( > type: SyncJobConfig, > flatten: true, > }, > + "sync-direction": { > + schema: SYNC_DIRECTION_SCHEMA, > + optional: true, > + }, > }, > }, > access: { > @@ -128,6 +140,7 @@ pub fn list_sync_jobs( > /// Create a new sync job. > pub fn create_sync_job( > config: SyncJobConfig, > + sync_direction: Option, > rpcenv: &mut dyn RpcEnvironment, > ) -> Result<(), Error> { > let auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?; > @@ -158,7 +171,8 @@ pub fn create_sync_job( > param_bail!("id", "job '{}' already exists.", config.id); > } > > - section_config.set_data(&config.id, "sync", &config)?; > + let sync_direction = sync_direction.unwrap_or_default(); same here > + section_config.set_data(&config.id, sync_direction.as_config_type_str(), &config)?; > > sync::save_config(§ion_config)?; > > @@ -173,6 +187,10 @@ pub fn create_sync_job( > id: { > schema: JOB_ID_SCHEMA, > }, > + "sync-direction": { > + schema: SYNC_DIRECTION_SCHEMA, > + optional: true, > + }, > }, > }, > returns: { type: SyncJobConfig }, > @@ -182,13 +200,18 @@ pub fn create_sync_job( > }, > )] > /// Read a sync job configuration. > -pub fn read_sync_job(id: String, rpcenv: &mut dyn RpcEnvironment) -> Result { > +pub fn read_sync_job( > + id: String, > + sync_direction: Option, > + rpcenv: &mut dyn RpcEnvironment, > +) -> Result { > let auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?; > let user_info = CachedUserInfo::new()?; > > let (config, digest) = sync::config()?; > > - let sync_job = config.lookup("sync", &id)?; > + let sync_direction = sync_direction.unwrap_or_default(); and here > + let sync_job = config.lookup(sync_direction.as_config_type_str(), &id)?; > if !check_sync_job_read_access(&user_info, &auth_id, &sync_job) { > bail!("permission check failed"); > } > @@ -252,6 +275,10 @@ pub enum DeletableProperty { > type: DeletableProperty, > } > }, > + "sync-direction": { > + schema: SYNC_DIRECTION_SCHEMA, > + optional: true, > + }, > digest: { > optional: true, > schema: PROXMOX_CONFIG_DIGEST_SCHEMA, > @@ -269,6 +296,7 @@ pub fn update_sync_job( > id: String, > update: SyncJobConfigUpdater, > delete: Option>, > + sync_direction: Option, > digest: Option, > rpcenv: &mut dyn RpcEnvironment, > ) -> Result<(), Error> { > @@ -284,7 +312,8 @@ pub fn update_sync_job( > crate::tools::detect_modified_configuration_file(&digest, &expected_digest)?; > } > > - let mut data: SyncJobConfig = config.lookup("sync", &id)?; > + let sync_direction = sync_direction.unwrap_or_default(); and here > + let mut data: SyncJobConfig = config.lookup(sync_direction.as_config_type_str(), &id)?; > > if let Some(delete) = delete { > for delete_prop in delete { > @@ -409,7 +438,7 @@ pub fn update_sync_job( > bail!("permission check failed"); > } > > - config.set_data(&id, "sync", &data)?; > + config.set_data(&id, sync_direction.as_config_type_str(), &data)?; > > sync::save_config(&config)?; > > @@ -427,6 +456,10 @@ pub fn update_sync_job( > id: { > schema: JOB_ID_SCHEMA, > }, > + "sync-direction": { > + schema: SYNC_DIRECTION_SCHEMA, > + optional: true, > + }, > digest: { > optional: true, > schema: PROXMOX_CONFIG_DIGEST_SCHEMA, > @@ -441,6 +474,7 @@ pub fn update_sync_job( > /// Remove a sync job configuration > pub fn delete_sync_job( > id: String, > + sync_direction: Option, > digest: Option, > rpcenv: &mut dyn RpcEnvironment, > ) -> Result<(), Error> { > @@ -456,7 +490,8 @@ pub fn delete_sync_job( > crate::tools::detect_modified_configuration_file(&digest, &expected_digest)?; > } > > - match config.lookup("sync", &id) { > + let sync_direction = sync_direction.unwrap_or_default(); and here > + match config.lookup(sync_direction.as_config_type_str(), &id) { > Ok(job) => { > if !check_sync_job_modify_access(&user_info, &auth_id, &job) { > bail!("permission check failed"); > diff --git a/src/bin/proxmox-backup-proxy.rs b/src/bin/proxmox-backup-proxy.rs > index 4409234b2..2b6f1c133 100644 > --- a/src/bin/proxmox-backup-proxy.rs > +++ b/src/bin/proxmox-backup-proxy.rs > @@ -608,7 +608,15 @@ async fn schedule_datastore_sync_jobs() { > Ok((config, _digest)) => config, > }; > > - for (job_id, (_, job_config)) in config.sections { > + for (job_id, (job_type, job_config)) in config.sections { > + let sync_direction = match job_type.as_str() { > + "sync" => SyncDirection::Pull, > + "sync-push" => SyncDirection::Push, > + _ => { > + eprintln!("unexpected config type in sync job config - {job_type}"); > + continue; > + } > + }; can this even happen? we don't allow unknown section types in the SyncJobConfig.. arguably, this should have used the `FromStr` implementation, and might be an argument for keeping it around instead of dropping it ;) > let job_config: SyncJobConfig = match serde_json::from_value(job_config) { > Ok(c) => c, > Err(err) => { > @@ -635,7 +643,7 @@ async fn schedule_datastore_sync_jobs() { > job_config, > &auth_id, > Some(event_str), > - SyncDirection::Pull, > + sync_direction, > false, > ) { > eprintln!("unable to start datastore sync job {job_id} - {err}"); > -- > 2.39.2 > > > > _______________________________________________ > 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