Re: [pbs-devel] [PATCH v3 proxmox-backup 16/33] config: jobs: add `sync-push` config type for push sync jobs

[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:
>> In order for sync jobs to be either pull or push jobs, allow to
>> configure the direction of the job.
>>
>> Adds an additional config type `sync-push` to the sync job config, to
>> clearly distinguish sync jobs configured in pull and in push
>> direction.
>>
>> This approach was chosen in order to limit possible misconfiguration,
>> as unintentionally switching the sync direction could potentially
>> delete still required snapshots.
>>
>> Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
>> ---
>> changes since version 2:
>> - no changes
>>
>>   pbs-api-types/src/jobs.rs | 52 +++++++++++++++++++++++++++++++++++++++
>>   pbs-config/src/sync.rs    | 11 +++++++--
>>   2 files changed, 61 insertions(+), 2 deletions(-)
>>
>> diff --git a/pbs-api-types/src/jobs.rs b/pbs-api-types/src/jobs.rs
>> index 868702bc0..12b39782c 100644
>> --- a/pbs-api-types/src/jobs.rs
>> +++ b/pbs-api-types/src/jobs.rs
>> @@ -20,6 +20,8 @@ const_regex! {
>>       pub VERIFICATION_JOB_WORKER_ID_REGEX = concatcp!(r"^(", PROXMOX_SAFE_ID_REGEX_STR, r"):");
>>       /// Regex for sync jobs '(REMOTE|\-):REMOTE_DATASTORE:LOCAL_DATASTORE:(?:LOCAL_NS_ANCHOR:)ACTUAL_JOB_ID'
>>       pub SYNC_JOB_WORKER_ID_REGEX = concatcp!(r"^(", PROXMOX_SAFE_ID_REGEX_STR, r"|\-):(", PROXMOX_SAFE_ID_REGEX_STR, r"):(", PROXMOX_SAFE_ID_REGEX_STR, r")(?::(", BACKUP_NS_RE, r"))?:");
>> +    /// Regex for sync direction'(pull|push)'
>> +    pub SYNC_DIRECTION_REGEX = r"^(pull|push)$";
>>   }
>>   
>>   pub const JOB_ID_SCHEMA: Schema = StringSchema::new("Job ID.")
>> @@ -498,6 +500,56 @@ pub const TRANSFER_LAST_SCHEMA: Schema =
>>           .minimum(1)
>>           .schema();
>>   
>> +pub const SYNC_DIRECTION_SCHEMA: Schema = StringSchema::new("Sync job direction (pull|push)")
>> +    .format(&ApiStringFormat::Pattern(&SYNC_DIRECTION_REGEX))
>> +    .schema();
>> +
>> +/// Direction of the sync job, push or pull
>> +#[derive(Clone, Debug, Default, Eq, PartialEq, Ord, PartialOrd, Hash, UpdaterType)]
>> +pub enum SyncDirection {
>> +    #[default]
>> +    Pull,
>> +    Push,
>> +}
>> +
>> +impl ApiType for SyncDirection {
>> +    const API_SCHEMA: Schema = SYNC_DIRECTION_SCHEMA;
>> +}
>> +
>> +// used for serialization using `proxmox_serde::forward_serialize_to_display` macro
>> +impl std::fmt::Display for SyncDirection {
>> +    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
>> +        match self {
>> +            SyncDirection::Pull => f.write_str("pull"),
>> +            SyncDirection::Push => f.write_str("push"),
>> +        }
>> +    }
>> +}
>> +
>> +impl std::str::FromStr for SyncDirection {
>> +    type Err = anyhow::Error;
>> +
>> +    fn from_str(s: &str) -> Result<Self, Self::Err> {
>> +        match s {
>> +            "pull" => Ok(SyncDirection::Pull),
>> +            "push" => Ok(SyncDirection::Push),
>> +            _ => bail!("invalid sync direction"),
>> +        }
>> +    }
>> +}
>> +
>> +proxmox_serde::forward_deserialize_to_from_str!(SyncDirection);
>> +proxmox_serde::forward_serialize_to_display!(SyncDirection);
> 
> wouldn't just implementing Display, and deriving Serialize/Deserialize
> be enough?

I opted for using the `api` macro on the `SyncDirection` enum, so all 
this is auto generated and neither schema, nor regex are required to be 
implemented explicitly, reducing the amount of code for this significantly.

> 
>> +
>> +impl SyncDirection {
>> +    pub fn as_config_type_str(&self) -> &'static str {
>> +        match self {
>> +            SyncDirection::Pull => "sync",
>> +            SyncDirection::Push => "sync-push",
>> +        }
>> +    }
>> +}
>> +
>>   #[api(
>>       properties: {
>>           id: {
>> diff --git a/pbs-config/src/sync.rs b/pbs-config/src/sync.rs
>> index 45453abb1..143b73e78 100644
>> --- a/pbs-config/src/sync.rs
>> +++ b/pbs-config/src/sync.rs
>> @@ -18,9 +18,16 @@ fn init() -> SectionConfig {
>>           _ => unreachable!(),
>>       };
>>   
>> -    let plugin = SectionConfigPlugin::new("sync".to_string(), Some(String::from("id")), obj_schema);
>> +    let pull_plugin =
>> +        SectionConfigPlugin::new("sync".to_string(), Some(String::from("id")), obj_schema);
>> +    let push_plugin = SectionConfigPlugin::new(
>> +        "sync-push".to_string(),
> 
> these here should probably use SyncDirection's as_config_type_str to
> make the connection obvious when adapting that at some point in the
> future?

Agreed, that will be fixed in the next version of the patches.

> 
>> +        Some(String::from("id")),
>> +        obj_schema,
>> +    );
>>       let mut config = SectionConfig::new(&JOB_ID_SCHEMA);
>> -    config.register_plugin(plugin);
>> +    config.register_plugin(pull_plugin);
>> +    config.register_plugin(push_plugin);
>>   
>>       config
>>   }
>> -- 
>> 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