Re: [pbs-devel] [PATCH v3 proxmox-backup 19/33] api: sync jobs: expose optional `sync-direction` parameter

[Christian Ebner <c.ebner@proxmox.com>]

On 10/10/24 16:48, Fabian Grünbichler wrote:
> On September 12, 2024 4:33 pm, Christian Ebner wrote:
>> @@ -44,6 +49,7 @@ use crate::{
>>   /// List all sync jobs
>>   pub fn list_sync_jobs(
>>       store: Option<String>,
>> +    sync_direction: Option<SyncDirection>,
> 
> would be much nicer if the default were already encoded in the API
> schema

As discussed off-list, this is not possible at the moment because the 
`api` macro does not allow to set a default value for external types, 
therefore leaving these in place for the time being.

> 
>>       _param: Value,
>>       rpcenv: &mut dyn RpcEnvironment,
>>   ) -> Result<Vec<SyncJobStatus>, 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 {
>> @@ -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, ..)?;
>      }
> }

Agreed, I went for the suggested nested loop here, makes this a bit more 
compact.

> 
>>           }
>>           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..).

What do you mean with being renamed here?

I think a sync variant `Any` is not the right approach, as that could 
lead to issues with clashing id's as these are unique on a job config 
type level only?

So again, using the suggested loop over enum variants.

> 
>>           values.push(MatchableValue {
>>               field: "job-id".into(),
>>               value: job.config.id,
>> 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 ;)

Using the `FromStr` impl of the `SyncDirection` enum does not work here, 
as these are the config type keys for the job config, not the sync 
direction itself.
Given that, I opted for implementing a `from_config_type_str` for 
`SyncDirection` as counterpart for the `as_config_type_str` 
implementation and use that for getting the sync direction based on the 
config type. Error handling still is required, as all match cases must 
be covered (even if logically not possible because already checked 
somewhere else).

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



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