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

[Fabian Grünbichler]

On October 14, 2024 10:10 am, Christian Ebner wrote:
> On 10/10/24 16:48, Fabian Grünbichler wrote:
>> On September 12, 2024 4:33 pm, Christian Ebner wrote:
>>>           }
>>>           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?

there's two "list_sync_jobs" fns, which can be confusing when reading
code/patches..

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

yes, the Any variant would only be usable for querying the config, not
for defining a job or persisting it.. that's what I meant with "worth it
for just the two list functions" ;)

> So again, using the suggested loop over enum variants.

ack!

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

right. if only we had given the original sync job entries a section type
of "pull" ;) in any case, IMHO it's a good idea to have
serializing/deserializing constructs like that in a single place.