Re: [pbs-devel] [PATCH proxmox-backup v3 1/6] api2: make Remote for SyncJob optional

[Lukas Wagner <l.wagner@proxmox.com>]

Comments inline:

On 8/8/23 14:13, Hannes Laimer wrote:
> diff --git a/src/api2/pull.rs b/src/api2/pull.rs
> index daeba7cf..664ecce5 100644
> --- a/src/api2/pull.rs
> +++ b/src/api2/pull.rs
> @@ -8,7 +8,7 @@ use proxmox_sys::task_log;
>   
>   use pbs_api_types::{
>       Authid, BackupNamespace, GroupFilter, RateLimitConfig, SyncJobConfig, DATASTORE_SCHEMA,
> -    GROUP_FILTER_LIST_SCHEMA, NS_MAX_DEPTH_REDUCED_SCHEMA, PRIV_DATASTORE_BACKUP,
> +    GROUP_FILTER_LIST_SCHEMA, MAX_NAMESPACE_DEPTH, NS_MAX_DEPTH_REDUCED_SCHEMA, PRIV_DATASTORE_BACKUP,
>       PRIV_DATASTORE_PRUNE, PRIV_REMOTE_READ, REMOTE_ID_SCHEMA, REMOVE_VANISHED_BACKUPS_SCHEMA,
>       TRANSFER_LAST_SCHEMA,
>   };
> @@ -22,7 +22,7 @@ pub fn check_pull_privs(
>       auth_id: &Authid,
>       store: &str,
>       ns: Option<&str>,
> -    remote: &str,
> +    remote: Option<&str>,
>       remote_store: &str,
>       delete: bool,
>   ) -> Result<(), Error> {
> @@ -39,12 +39,22 @@ pub fn check_pull_privs(
>           PRIV_DATASTORE_BACKUP,
>           false,
>       )?;
> -    user_info.check_privs(
> -        auth_id,
> -        &["remote", remote, remote_store],
> -        PRIV_REMOTE_READ,
> -        false,
> -    )?;
> +
> +    if let Some(remote) = remote {
> +        user_info.check_privs(
> +            auth_id,
> +            &["remote", remote, remote_store],
> +            PRIV_REMOTE_READ,
> +            false,
> +        )?;
> +    } else {
> +        user_info.check_privs(
> +            auth_id,
> +            &["datastore", remote_store],
> +            PRIV_DATASTORE_BACKUP,
> +            false,
> +        )?;
> +    }
>   
>       if delete {
>           user_info.check_privs(
> @@ -65,7 +75,7 @@ impl TryFrom<&SyncJobConfig> for PullParameters {
>           PullParameters::new(
>               &sync_job.store,
>               sync_job.ns.clone().unwrap_or_default(),
> -            &sync_job.remote,
> +            sync_job.remote.as_deref().unwrap_or("local"),
>               &sync_job.remote_store,
>               sync_job.remote_ns.clone().unwrap_or_default(),
>               sync_job
> @@ -91,7 +101,7 @@ pub fn do_sync_job(
>   ) -> Result<String, Error> {
>       let job_id = format!(
>           "{}:{}:{}:{}:{}",
> -        sync_job.remote,
> +        sync_job.remote.clone().unwrap_or("localhost".to_string()),
>           sync_job.remote_store,
>           sync_job.store,
>           sync_job.ns.clone().unwrap_or_default(),
> @@ -124,11 +134,28 @@ pub fn do_sync_job(
>                       worker,
>                       "sync datastore '{}' from '{}/{}'",
>                       sync_job.store,
> -                    sync_job.remote,
> +                    sync_job.remote.clone().unwrap_or("local".to_string()),
>                       sync_job.remote_store,
>                   );
>   
> -                pull_store(&worker, &client, pull_params).await?;
> +                if sync_job.remote.is_some() {
> +                    pull_store(&worker, &client, pull_params).await?;
> +                } else {
> +                    if let (Some(target_ns), Some(source_ns)) = (sync_job.ns, sync_job.remote_ns) {
> +                        if target_ns.path().starts_with(source_ns.path())
> +                            && sync_job.store == sync_job.remote_store
> +                            && sync_job.max_depth.map_or(true, |sync_depth| {
> +                            target_ns.depth() + sync_depth > MAX_NAMESPACE_DEPTH
> +                        }) {
> +                            task_log!(
> +                                worker,
> +                                "Can't sync namespace into one of its sub-namespaces, would exceed maximum namespace depth, skipping"
> +                            );
> +                        }
> +                    } else {
> +                        pull_store(&worker, &client, pull_params).await?;
> +                    }
> +                }
>   
>                   task_log!(worker, "sync job '{}' end", &job_id);
>   
> @@ -180,6 +207,7 @@ pub fn do_sync_job(
>               },
>               remote: {
>                   schema: REMOTE_ID_SCHEMA,
> +                optional: true,
>               },
>               "remote-store": {
>                   schema: DATASTORE_SCHEMA,
> @@ -224,7 +252,7 @@ The delete flag additionally requires the Datastore.Prune privilege on '/datasto
>   async fn pull(
>       store: String,
>       ns: Option<BackupNamespace>,
> -    remote: String,
> +    remote: Option<String>,
>       remote_store: String,
>       remote_ns: Option<BackupNamespace>,
>       remove_vanished: Option<bool>,
> @@ -248,7 +276,7 @@ async fn pull(
>           &auth_id,
>           &store,
>           ns_str.as_deref(),
> -        &remote,
> +        remote.as_deref(),
>           &remote_store,
>           delete,
>       )?;
> @@ -256,7 +284,7 @@ async fn pull(
>       let pull_params = PullParameters::new(
>           &store,
>           ns,
> -        &remote,
> +        remote.as_deref().unwrap_or("local"),
>           &remote_store,
>           remote_ns.unwrap_or_default(),
>           auth_id.clone(),
> @@ -280,7 +308,7 @@ async fn pull(
>                   worker,
>                   "pull datastore '{}' from '{}/{}'",
>                   store,
> -                remote,
> +                remote.as_deref().unwrap_or("localhost"),
>                   remote_store,
>               );
>   
> @@ -299,4 +327,4 @@ async fn pull(
>       Ok(upid_str)
>   }
>   
> -pub const ROUTER: Router = Router::new().post(&API_METHOD_PULL);
> +pub const ROUTER: Router = Router::new().post(&API_METHOD_PULL);
> \ No newline at end of file

You stripped the newline at the end of file (and added it back a few 
patches later)

> diff --git a/src/server/email_notifications.rs b/src/server/email_notifications.rs
> index ea1476d7..18881782 100644
> --- a/src/server/email_notifications.rs
> +++ b/src/server/email_notifications.rs
> @@ -484,15 +484,17 @@ pub fn send_sync_status(
>           }
>       };
>   
> +    let tmp_src_string;
> +    let source_str = if let Some(remote) = &job.remote {
> +        tmp_src_string = format!("Sync remote '{}'", remote);
> +        &tmp_src_string
> +    } else {
> +        "Sync local"
> +    };
> +

nit: considering that this is not in a hot loop or anything, I'd
just do a

let source_str = if let Some(...) {
   format!(...)
} else {
   "Sync Local".into()
}

instead of using the temporary variable. Looks far nicer and I guess we 
can spare a couple extra µs when sending the notification email. ;)
No hard feelings tho.

-- 
- Lukas