[pbs-devel] [PATCH proxmox/proxmox-backup 0/7] trigger sync jobs on mount

[pbs-devel] [PATCH proxmox/proxmox-backup 0/7] trigger sync jobs on mount

[Hannes Laimer]

Sync jobs now have a run-on-mount flag, that, if set, runs the job whenever
a relevant removable datastore is mounted. 


* proxmox:
Hannes Laimer (1):
  rest-server: add function that returns a join handle for spawn

 proxmox-rest-server/src/worker_task.rs | 21 +++++++++++++++++++--
 1 file changed, 19 insertions(+), 2 deletions(-)
 

* proxmox-backup:
Hannes Laimer (6):
  api types: add run-on-mount flag to SyncJobConfig
  api: admin: run configured sync jobs when a datastore is mounted
  api: admin: trigger sync jobs only on datastore mount
  bin: manager: run uuid_mount/mount tasks on the proxy
  ui: add run-on-mount checkbox to SyncJob form
  ui: add task title for triggering sync jobs

 pbs-api-types/src/jobs.rs                   |  3 +
 src/api2/admin/datastore.rs                 | 96 +++++++++++++++++++--
 src/api2/admin/sync.rs                      |  2 +-
 src/bin/proxmox_backup_manager/datastore.rs | 42 +++++----
 src/server/sync.rs                          |  7 +-
 www/Utils.js                                |  1 +
 www/window/SyncJobEdit.js                   | 23 ++++-
 7 files changed, 147 insertions(+), 27 deletions(-)

-- 
2.39.5



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

[pbs-devel] [PATCH proxmox 1/7] rest-server: add function that returns a join handle for spawn

[Hannes Laimer]

We need this handle when we want to start multiple jobs sequentially
from a 'manage' task. With the handle we can avoid polling for the
task status with its upid.

Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
---
 proxmox-rest-server/src/worker_task.rs | 21 +++++++++++++++++++--
 1 file changed, 19 insertions(+), 2 deletions(-)

diff --git a/proxmox-rest-server/src/worker_task.rs b/proxmox-rest-server/src/worker_task.rs
index beec691e..020bdb95 100644
--- a/proxmox-rest-server/src/worker_task.rs
+++ b/proxmox-rest-server/src/worker_task.rs
@@ -14,6 +14,7 @@ use serde::{Deserialize, Serialize};
 use serde_json::{json, Value};
 use tokio::signal::unix::SignalKind;
 use tokio::sync::{oneshot, watch};
+use tokio::task::JoinHandle;
 use tracing::{info, warn};
 
 use proxmox_daemon::command_socket::CommandSocket;
@@ -942,6 +943,22 @@ impl WorkerTask {
         to_stdout: bool,
         f: F,
     ) -> Result<String, Error>
+    where
+        F: Send + 'static + FnOnce(Arc<WorkerTask>) -> T,
+        T: Send + 'static + Future<Output = Result<(), Error>>,
+    {
+        let (upid_str, _) = Self::spawn_with_handle(worker_type, worker_id, auth_id, to_stdout, f)?;
+        Ok(upid_str)
+    }
+
+    /// Needed when a 'management' task starts multiple tasks sequentially
+    pub fn spawn_with_handle<F, T>(
+        worker_type: &str,
+        worker_id: Option<String>,
+        auth_id: String,
+        to_stdout: bool,
+        f: F,
+    ) -> Result<(String, JoinHandle<()>), Error>
     where
         F: Send + 'static + FnOnce(Arc<WorkerTask>) -> T,
         T: Send + 'static + Future<Output = Result<(), Error>>,
@@ -950,12 +967,12 @@ impl WorkerTask {
         let upid_str = worker.upid.to_string();
         let f = f(worker.clone());
 
-        tokio::spawn(LogContext::new(logger).scope(async move {
+        let handle = tokio::spawn(LogContext::new(logger).scope(async move {
             let result = f.await;
             worker.log_result(&result);
         }));
 
-        Ok(upid_str)
+        Ok((upid_str, handle))
     }
 
     /// Create a new worker thread.
-- 
2.39.5



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

Re: [pbs-devel] [PATCH proxmox 1/7] rest-server: add function that returns a join handle for spawn

[Christian Ebner]

nit inline

On 1/16/25 07:45, Hannes Laimer wrote:
> We need this handle when we want to start multiple jobs sequentially
> from a 'manage' task. With the handle we can avoid polling for the
> task status with its upid.
> 
> Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
> ---
>   proxmox-rest-server/src/worker_task.rs | 21 +++++++++++++++++++--
>   1 file changed, 19 insertions(+), 2 deletions(-)
> 
> diff --git a/proxmox-rest-server/src/worker_task.rs b/proxmox-rest-server/src/worker_task.rs
> index beec691e..020bdb95 100644
> --- a/proxmox-rest-server/src/worker_task.rs
> +++ b/proxmox-rest-server/src/worker_task.rs
> @@ -14,6 +14,7 @@ use serde::{Deserialize, Serialize};
>   use serde_json::{json, Value};
>   use tokio::signal::unix::SignalKind;
>   use tokio::sync::{oneshot, watch};
> +use tokio::task::JoinHandle;
>   use tracing::{info, warn};
>   
>   use proxmox_daemon::command_socket::CommandSocket;
> @@ -942,6 +943,22 @@ impl WorkerTask {
>           to_stdout: bool,
>           f: F,
>       ) -> Result<String, Error>
> +    where
> +        F: Send + 'static + FnOnce(Arc<WorkerTask>) -> T,
> +        T: Send + 'static + Future<Output = Result<(), Error>>,
> +    {
> +        let (upid_str, _) = Self::spawn_with_handle(worker_type, worker_id, auth_id, to_stdout, f)?;
> +        Ok(upid_str)
> +    }
> +

nit: docstring for `spawn_with_handle` should primarily tell what it 
does, only secondarily what it is used for.

Maybe something like:
```
/// Spawn a new worker task with log context like `spawn`, but return 
the tasks join handle in addition to its upid.
/// Allows for a management tasks to handle multiple worker tasks.
```

> +    /// Needed when a 'management' task starts multiple tasks sequentially
> +    pub fn spawn_with_handle<F, T>(
> +        worker_type: &str,
> +        worker_id: Option<String>,
> +        auth_id: String,
> +        to_stdout: bool,
> +        f: F,
> +    ) -> Result<(String, JoinHandle<()>), Error>
>       where
>           F: Send + 'static + FnOnce(Arc<WorkerTask>) -> T,
>           T: Send + 'static + Future<Output = Result<(), Error>>,
> @@ -950,12 +967,12 @@ impl WorkerTask {
>           let upid_str = worker.upid.to_string();
>           let f = f(worker.clone());
>   
> -        tokio::spawn(LogContext::new(logger).scope(async move {
> +        let handle = tokio::spawn(LogContext::new(logger).scope(async move {
>               let result = f.await;
>               worker.log_result(&result);
>           }));
>   
> -        Ok(upid_str)
> +        Ok((upid_str, handle))
>       }
>   
>       /// Create a new worker thread.



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

[pbs-devel] [PATCH proxmox-backup 2/7] api types: add run-on-mount flag to SyncJobConfig

[Hannes Laimer]

Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
---
 pbs-api-types/src/jobs.rs | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/pbs-api-types/src/jobs.rs b/pbs-api-types/src/jobs.rs
index 04631d920..da2565e07 100644
--- a/pbs-api-types/src/jobs.rs
+++ b/pbs-api-types/src/jobs.rs
@@ -622,6 +622,9 @@ pub struct SyncJobConfig {
     pub resync_corrupt: Option<bool>,
     #[serde(skip_serializing_if = "Option::is_none")]
     pub sync_direction: Option<SyncDirection>,
+    /// Run job when datastore is mounted
+    #[serde(default)]
+    pub run_on_mount: bool,
 }
 
 impl SyncJobConfig {
-- 
2.39.5



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

Re: [pbs-devel] [PATCH proxmox-backup 2/7] api types: add run-on-mount flag to SyncJobConfig

[Christian Ebner]

Should now be relocated to the pbs-api-types in `proxmox` repo, but 
still applies cleanly.

further comments inline.
On 1/16/25 07:45, Hannes Laimer wrote:
> Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
> ---
>   pbs-api-types/src/jobs.rs | 3 +++
>   1 file changed, 3 insertions(+)
> 
> diff --git a/pbs-api-types/src/jobs.rs b/pbs-api-types/src/jobs.rs
> index 04631d920..da2565e07 100644
> --- a/pbs-api-types/src/jobs.rs
> +++ b/pbs-api-types/src/jobs.rs
> @@ -622,6 +622,9 @@ pub struct SyncJobConfig {
>       pub resync_corrupt: Option<bool>,
>       #[serde(skip_serializing_if = "Option::is_none")]
>       pub sync_direction: Option<SyncDirection>,
> +    /// Run job when datastore is mounted
> +    #[serde(default)]
> +    pub run_on_mount: bool,

This should rather be an optional flag (like e.g. the resync corrupted) 
in order to:
1. not fail for pre-existing configs
2. limit the number of properties if only the default ones are set

Further, it needs to be included also in the api schema description just 
before the struct declaration.

>   }
>   
>   impl SyncJobConfig {



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

Re: [pbs-devel] [PATCH proxmox-backup 2/7] api types: add run-on-mount flag to SyncJobConfig

[Hannes Laimer]

On 2/4/25 15:24, Christian Ebner wrote:
> Should now be relocated to the pbs-api-types in `proxmox` repo, but 
> still applies cleanly.
> 
> further comments inline.
> On 1/16/25 07:45, Hannes Laimer wrote:
>> Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
>> ---
>>   pbs-api-types/src/jobs.rs | 3 +++
>>   1 file changed, 3 insertions(+)
>>
>> diff --git a/pbs-api-types/src/jobs.rs b/pbs-api-types/src/jobs.rs
>> index 04631d920..da2565e07 100644
>> --- a/pbs-api-types/src/jobs.rs
>> +++ b/pbs-api-types/src/jobs.rs
>> @@ -622,6 +622,9 @@ pub struct SyncJobConfig {
>>       pub resync_corrupt: Option<bool>,
>>       #[serde(skip_serializing_if = "Option::is_none")]
>>       pub sync_direction: Option<SyncDirection>,
>> +    /// Run job when datastore is mounted
>> +    #[serde(default)]
>> +    pub run_on_mount: bool,
> 
> This should rather be an optional flag (like e.g. the resync corrupted) 
> in order to:
> 1. not fail for pre-existing configs
> 2. limit the number of properties if only the default ones are set
> 
> Further, it needs to be included also in the api schema description just 
> before the struct declaration.
> 

not sure why, but I assumed `#[serde(default)]` would to that, anyway,
will fix in next version

>>   }
>>   impl SyncJobConfig {
> 



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

[pbs-devel] [PATCH proxmox-backup 3/7] api: admin: run configured sync jobs when a datastore is mounted

[Hannes Laimer]

When a datastore is mounted, spawn a new task to run all sync jobs
marked with `run-on-mount`. These jobs run sequentially and include
any job for which the mounted datastore is:

- The source or target in a local push/pull job
- The source in a push job to a remote datastore
- The target in a pull job from a remote datastore

Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
---
 src/api2/admin/datastore.rs | 90 ++++++++++++++++++++++++++++++++++---
 src/api2/admin/sync.rs      |  2 +-
 src/server/sync.rs          |  7 +--
 3 files changed, 90 insertions(+), 9 deletions(-)

diff --git a/src/api2/admin/datastore.rs b/src/api2/admin/datastore.rs
index f5d80d610..21b58391d 100644
--- a/src/api2/admin/datastore.rs
+++ b/src/api2/admin/datastore.rs
@@ -41,8 +41,8 @@ use pbs_api_types::{
     DataStoreConfig, DataStoreListItem, DataStoreMountStatus, DataStoreStatus,
     GarbageCollectionJobStatus, GroupListItem, JobScheduleStatus, KeepOptions, MaintenanceMode,
     MaintenanceType, Operation, PruneJobOptions, SnapshotListItem, SnapshotVerifyState,
-    BACKUP_ARCHIVE_NAME_SCHEMA, BACKUP_ID_SCHEMA, BACKUP_NAMESPACE_SCHEMA, BACKUP_TIME_SCHEMA,
-    BACKUP_TYPE_SCHEMA, CATALOG_NAME, CLIENT_LOG_BLOB_NAME, DATASTORE_SCHEMA,
+    SyncJobConfig, BACKUP_ARCHIVE_NAME_SCHEMA, BACKUP_ID_SCHEMA, BACKUP_NAMESPACE_SCHEMA,
+    BACKUP_TIME_SCHEMA, BACKUP_TYPE_SCHEMA, CATALOG_NAME, CLIENT_LOG_BLOB_NAME, DATASTORE_SCHEMA,
     IGNORE_VERIFIED_BACKUPS_SCHEMA, MANIFEST_BLOB_NAME, MAX_NAMESPACE_DEPTH, NS_MAX_DEPTH_SCHEMA,
     PRIV_DATASTORE_AUDIT, PRIV_DATASTORE_BACKUP, PRIV_DATASTORE_MODIFY, PRIV_DATASTORE_PRUNE,
     PRIV_DATASTORE_READ, PRIV_DATASTORE_VERIFY, PRIV_SYS_MODIFY, UPID, UPID_SCHEMA,
@@ -2500,6 +2500,51 @@ pub fn do_mount_device(datastore: DataStoreConfig) -> Result<(), Error> {
     Ok(())
 }
 
+async fn do_sync_jobs(
+    jobs_to_run: Vec<SyncJobConfig>,
+    worker: Arc<WorkerTask>,
+) -> Result<(), Error> {
+    let count = jobs_to_run.len();
+    info!(
+        "will run {} sync jobs: {}",
+        count,
+        jobs_to_run
+            .iter()
+            .map(|j| j.id.clone())
+            .collect::<Vec<String>>()
+            .join(", ")
+    );
+
+    for (i, job_config) in jobs_to_run.into_iter().enumerate() {
+        if worker.abort_requested() {
+            bail!("aborted due to user request");
+        }
+        let job_id = job_config.id.clone();
+        let Ok(job) = Job::new("syncjob", &job_id) else {
+            continue;
+        };
+        let auth_id = Authid::root_auth_id().clone();
+        info!("[{}/{count}] starting '{job_id}'...", i + 1);
+        match crate::server::do_sync_job(
+            job,
+            job_config,
+            &auth_id,
+            Some("mount".to_string()),
+            false,
+        ) {
+            Ok((_upid, handle)) => {
+                tokio::select! {
+                    sync_done = handle.fuse() => if let Err(err) = sync_done { warn!("could not wait for job to finish: {err}"); },
+                    _abort = worker.abort_future() => bail!("aborted due to user request"),
+                };
+            }
+            Err(err) => warn!("unable to start sync job {job_id} - {err}"),
+        }
+    }
+
+    Ok(())
+}
+
 #[api(
     protected: true,
     input: {
@@ -2531,12 +2576,47 @@ pub fn mount(store: String, rpcenv: &mut dyn RpcEnvironment) -> Result<Value, Er
     let auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?;
     let to_stdout = rpcenv.env_type() == RpcEnvironmentType::CLI;
 
-    let upid = WorkerTask::new_thread(
+    let upid = WorkerTask::spawn(
         "mount-device",
-        Some(store),
+        Some(store.clone()),
         auth_id.to_string(),
         to_stdout,
-        move |_worker| do_mount_device(datastore),
+        move |_worker| async move {
+            do_mount_device(datastore.clone())?;
+            let Ok((sync_config, _digest)) = pbs_config::sync::config() else {
+                warn!("unable to read sync job config, won't run any sync jobs");
+                return Ok(());
+            };
+            let Ok(list) = sync_config.convert_to_typed_array("sync") else {
+                warn!("unable to parse sync job config, won't run any sync jobs");
+                return Ok(());
+            };
+            let jobs_to_run: Vec<SyncJobConfig> = list
+                .into_iter()
+                .filter(|job: &SyncJobConfig| {
+                    // add job iff any of these apply
+                    //   - the jobs is local and we are source or target
+                    //   - we are the source of a push to a remote
+                    //   - we are the target of a pull from a remote
+                    //
+                    // `job.store == datastore.name` iff we are the target for pull from remote or we
+                    // are the source for push to remote, therefore we don't have to check for the
+                    // direction of the job.
+                    job.remote.is_none() && job.remote_store == datastore.name
+                        || job.store == datastore.name
+                })
+                .collect();
+            if !jobs_to_run.is_empty() {
+                let _ = WorkerTask::spawn(
+                    "mount-sync-jobs",
+                    Some(store),
+                    auth_id.to_string(),
+                    false,
+                    move |worker| async move { do_sync_jobs(jobs_to_run, worker).await },
+                );
+            }
+            Ok(())
+        },
     )?;
 
     Ok(json!(upid))
diff --git a/src/api2/admin/sync.rs b/src/api2/admin/sync.rs
index 6722ebea0..01dea5126 100644
--- a/src/api2/admin/sync.rs
+++ b/src/api2/admin/sync.rs
@@ -161,7 +161,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, to_stdout)?;
+    let (upid_str, _) = do_sync_job(job, sync_job, &auth_id, None, to_stdout)?;
 
     Ok(upid_str)
 }
diff --git a/src/server/sync.rs b/src/server/sync.rs
index 0bd7a7a85..2a290a8ee 100644
--- a/src/server/sync.rs
+++ b/src/server/sync.rs
@@ -11,6 +11,7 @@ use anyhow::{bail, format_err, Context, Error};
 use futures::{future::FutureExt, select};
 use http::StatusCode;
 use serde_json::json;
+use tokio::task::JoinHandle;
 use tracing::{info, warn};
 
 use proxmox_human_byte::HumanByte;
@@ -598,7 +599,7 @@ pub fn do_sync_job(
     auth_id: &Authid,
     schedule: Option<String>,
     to_stdout: bool,
-) -> Result<String, Error> {
+) -> Result<(String, JoinHandle<()>), Error> {
     let job_id = format!(
         "{}:{}:{}:{}:{}",
         sync_job.remote.as_deref().unwrap_or("-"),
@@ -614,7 +615,7 @@ pub fn do_sync_job(
         bail!("can't sync to same datastore");
     }
 
-    let upid_str = WorkerTask::spawn(
+    let (upid_str, handle) = WorkerTask::spawn_with_handle(
         &worker_type,
         Some(job_id.clone()),
         auth_id.to_string(),
@@ -728,5 +729,5 @@ pub fn do_sync_job(
         },
     )?;
 
-    Ok(upid_str)
+    Ok((upid_str, handle))
 }
-- 
2.39.5



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

Re: [pbs-devel] [PATCH proxmox-backup 3/7] api: admin: run configured sync jobs when a datastore is mounted

[Christian Ebner]

This patch needs to be rebased on current master.

High level comment:
Mixing the mount api handler with the execution logic for the sync jobs 
is not the best approach here I think. This mixes unrelated code and 
makes it harder to further extend  this to e.g. also run verify, prune 
or garbage collection in the future.

Just an idea: Maybe it would be possible to let the mount worker task 
signal the mount event via e.g. a std::sync::mpsc::channel() or the 
command socket (already used to request datastore cache updates on 
unmount) to schedule a managing task in the proxmox-backup-proxy?

further comments inline.

On 1/16/25 07:45, Hannes Laimer wrote:
> When a datastore is mounted, spawn a new task to run all sync jobs
> marked with `run-on-mount`. These jobs run sequentially and include
> any job for which the mounted datastore is:
> 
> - The source or target in a local push/pull job
> - The source in a push job to a remote datastore
> - The target in a pull job from a remote datastore
> 
> Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
> ---
>   src/api2/admin/datastore.rs | 90 ++++++++++++++++++++++++++++++++++---
>   src/api2/admin/sync.rs      |  2 +-
>   src/server/sync.rs          |  7 +--
>   3 files changed, 90 insertions(+), 9 deletions(-)
> 
> diff --git a/src/api2/admin/datastore.rs b/src/api2/admin/datastore.rs
> index f5d80d610..21b58391d 100644
> --- a/src/api2/admin/datastore.rs
> +++ b/src/api2/admin/datastore.rs
> @@ -41,8 +41,8 @@ use pbs_api_types::{
>       DataStoreConfig, DataStoreListItem, DataStoreMountStatus, DataStoreStatus,
>       GarbageCollectionJobStatus, GroupListItem, JobScheduleStatus, KeepOptions, MaintenanceMode,
>       MaintenanceType, Operation, PruneJobOptions, SnapshotListItem, SnapshotVerifyState,
> -    BACKUP_ARCHIVE_NAME_SCHEMA, BACKUP_ID_SCHEMA, BACKUP_NAMESPACE_SCHEMA, BACKUP_TIME_SCHEMA,
> -    BACKUP_TYPE_SCHEMA, CATALOG_NAME, CLIENT_LOG_BLOB_NAME, DATASTORE_SCHEMA,
> +    SyncJobConfig, BACKUP_ARCHIVE_NAME_SCHEMA, BACKUP_ID_SCHEMA, BACKUP_NAMESPACE_SCHEMA,
> +    BACKUP_TIME_SCHEMA, BACKUP_TYPE_SCHEMA, CATALOG_NAME, CLIENT_LOG_BLOB_NAME, DATASTORE_SCHEMA,
>       IGNORE_VERIFIED_BACKUPS_SCHEMA, MANIFEST_BLOB_NAME, MAX_NAMESPACE_DEPTH, NS_MAX_DEPTH_SCHEMA,
>       PRIV_DATASTORE_AUDIT, PRIV_DATASTORE_BACKUP, PRIV_DATASTORE_MODIFY, PRIV_DATASTORE_PRUNE,
>       PRIV_DATASTORE_READ, PRIV_DATASTORE_VERIFY, PRIV_SYS_MODIFY, UPID, UPID_SCHEMA,
> @@ -2500,6 +2500,51 @@ pub fn do_mount_device(datastore: DataStoreConfig) -> Result<(), Error> {
>       Ok(())
>   }
>   
> +async fn do_sync_jobs(

nit: this helper function makes more sense to be placed into 
`src/api2/admin/sync.rs` I would say, as it purely handles execution of 
sync jobs.

> +    jobs_to_run: Vec<SyncJobConfig>,
> +    worker: Arc<WorkerTask>,
> +) -> Result<(), Error> {
> +    let count = jobs_to_run.len();
> +    info!(
> +        "will run {} sync jobs: {}",
> +        count,

nit: this variable can be in-lined into the format string.

> +        jobs_to_run
> +            .iter()
> +            .map(|j| j.id.clone())
> +            .collect::<Vec<String>>()
> +            .join(", ")

nit: not sure if it is required to list all sync jobs a-priori, 
especially since that line might get rather long if many sync jobs are 
to be executed? Also, that would avoid the double iteration and id cloning.

> +    );
> +
> +    for (i, job_config) in jobs_to_run.into_iter().enumerate() {
> +        if worker.abort_requested() {
> +            bail!("aborted due to user request");
> +        }
> +        let job_id = job_config.id.clone();
> +        let Ok(job) = Job::new("syncjob", &job_id) else {
> +            continue;
> +        };
> +        let auth_id = Authid::root_auth_id().clone();
> +        info!("[{}/{count}] starting '{job_id}'...", i + 1);
> +        match crate::server::do_sync_job(
> +            job,
> +            job_config,
> +            &auth_id,
> +            Some("mount".to_string()),
> +            false,
> +        ) {
> +            Ok((_upid, handle)) => {
> +                tokio::select! {
> +                    sync_done = handle.fuse() => if let Err(err) = sync_done { warn!("could not wait for job to finish: {err}"); },
> +                    _abort = worker.abort_future() => bail!("aborted due to user request"),
> +                };
> +            }
> +            Err(err) => warn!("unable to start sync job {job_id} - {err}"),
> +        }
> +    }
> +
> +    Ok(())
> +}
> +
>   #[api(
>       protected: true,
>       input: {
> @@ -2531,12 +2576,47 @@ pub fn mount(store: String, rpcenv: &mut dyn RpcEnvironment) -> Result<Value, Er
>       let auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?;
>       let to_stdout = rpcenv.env_type() == RpcEnvironmentType::CLI;
>   
> -    let upid = WorkerTask::new_thread(
> +    let upid = WorkerTask::spawn(
>           "mount-device",
> -        Some(store),
> +        Some(store.clone()),
>           auth_id.to_string(),
>           to_stdout,
> -        move |_worker| do_mount_device(datastore),
> +        move |_worker| async move {
> +            do_mount_device(datastore.clone())?;
> +            let Ok((sync_config, _digest)) = pbs_config::sync::config() else {
> +                warn!("unable to read sync job config, won't run any sync jobs");
> +                return Ok(());
> +            };
> +            let Ok(list) = sync_config.convert_to_typed_array("sync") else {
> +                warn!("unable to parse sync job config, won't run any sync jobs");
> +                return Ok(());
> +            };
> +            let jobs_to_run: Vec<SyncJobConfig> = list
> +                .into_iter()
> +                .filter(|job: &SyncJobConfig| {
> +                    // add job iff any of these apply
> +                    //   - the jobs is local and we are source or target
> +                    //   - we are the source of a push to a remote
> +                    //   - we are the target of a pull from a remote
> +                    //
> +                    // `job.store == datastore.name` iff we are the target for pull from remote or we
> +                    // are the source for push to remote, therefore we don't have to check for the
> +                    // direction of the job.

This misses a check for the `run_on_mount` flag here I think, as it is 
never read in the rest of the patch series?

> +                    job.remote.is_none() && job.remote_store == datastore.name
> +                        || job.store == datastore.name
> +                })
> +                .collect();
> +            if !jobs_to_run.is_empty() {
> +                let _ = WorkerTask::spawn(
> +                    "mount-sync-jobs",
> +                    Some(store),
> +                    auth_id.to_string(),
> +                    false,
> +                    move |worker| async move { do_sync_jobs(jobs_to_run, worker).await },
> +                );
> +            }
> +            Ok(())
> +        },
>       )?;
>   
>       Ok(json!(upid))
> diff --git a/src/api2/admin/sync.rs b/src/api2/admin/sync.rs
> index 6722ebea0..01dea5126 100644
> --- a/src/api2/admin/sync.rs
> +++ b/src/api2/admin/sync.rs
> @@ -161,7 +161,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, to_stdout)?;
> +    let (upid_str, _) = do_sync_job(job, sync_job, &auth_id, None, to_stdout)?;
>   
>       Ok(upid_str)
>   }
> diff --git a/src/server/sync.rs b/src/server/sync.rs
> index 0bd7a7a85..2a290a8ee 100644
> --- a/src/server/sync.rs
> +++ b/src/server/sync.rs
> @@ -11,6 +11,7 @@ use anyhow::{bail, format_err, Context, Error};
>   use futures::{future::FutureExt, select};
>   use http::StatusCode;
>   use serde_json::json;
> +use tokio::task::JoinHandle;
>   use tracing::{info, warn};
>   
>   use proxmox_human_byte::HumanByte;
> @@ -598,7 +599,7 @@ pub fn do_sync_job(
>       auth_id: &Authid,
>       schedule: Option<String>,
>       to_stdout: bool,
> -) -> Result<String, Error> {
> +) -> Result<(String, JoinHandle<()>), Error> {
>       let job_id = format!(
>           "{}:{}:{}:{}:{}",
>           sync_job.remote.as_deref().unwrap_or("-"),
> @@ -614,7 +615,7 @@ pub fn do_sync_job(
>           bail!("can't sync to same datastore");
>       }
>   
> -    let upid_str = WorkerTask::spawn(
> +    let (upid_str, handle) = WorkerTask::spawn_with_handle(
>           &worker_type,
>           Some(job_id.clone()),
>           auth_id.to_string(),
> @@ -728,5 +729,5 @@ pub fn do_sync_job(
>           },
>       )?;
>   
> -    Ok(upid_str)
> +    Ok((upid_str, handle))
>   }



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

Re: [pbs-devel] [PATCH proxmox-backup 3/7] api: admin: run configured sync jobs when a datastore is mounted

[Hannes Laimer]

On 2/4/25 15:33, Christian Ebner wrote:
> This patch needs to be rebased on current master.
> 
> High level comment:
> Mixing the mount api handler with the execution logic for the sync jobs 
> is not the best approach here I think. This mixes unrelated code and 
> makes it harder to further extend  this to e.g. also run verify, prune 
> or garbage collection in the future.
> 

The endpoint starts the task that does the mounting, and it starts the
tasks that manages possible sync jobs. I don't think this really does
anything to the logic of the sync job itself. Sure, this
"run these jobs in sequence" would make sense, but currently we can't do
that, we don't have an interface for jobs. Not even job-ids are unique
across different job types. I am not sure weather there would be a lot
of value in trying to have a more abstract job things. The only thing
I could think of is jobs triggering each other. But I feel like that
might be out of scope for this here.


> Just an idea: Maybe it would be possible to let the mount worker task 
> signal the mount event via e.g. a std::sync::mpsc::channel() or the 
> command socket (already used to request datastore cache updates on 
> unmount) to schedule a managing task in the proxmox-backup-proxy?
> 

This is a privileged endpoint, it runs on the api process, so we
can't really use channels here. And it don't think the command socket
is the right tool for this, one reason being it is more of a hit or miss
(technically) which is not really what we want here. Also having the api
process handle this is IMHO not what we want.


> further comments inline.
> 
> On 1/16/25 07:45, Hannes Laimer wrote:
>> When a datastore is mounted, spawn a new task to run all sync jobs
>> marked with `run-on-mount`. These jobs run sequentially and include
>> any job for which the mounted datastore is:
>>
>> - The source or target in a local push/pull job
>> - The source in a push job to a remote datastore
>> - The target in a pull job from a remote datastore
>>
>> Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
>> ---
>>   src/api2/admin/datastore.rs | 90 ++++++++++++++++++++++++++++++++++---
>>   src/api2/admin/sync.rs      |  2 +-
>>   src/server/sync.rs          |  7 +--
>>   3 files changed, 90 insertions(+), 9 deletions(-)
>>
>> diff --git a/src/api2/admin/datastore.rs b/src/api2/admin/datastore.rs
>> index f5d80d610..21b58391d 100644
>> --- a/src/api2/admin/datastore.rs
>> +++ b/src/api2/admin/datastore.rs
>> @@ -41,8 +41,8 @@ use pbs_api_types::{
>>       DataStoreConfig, DataStoreListItem, DataStoreMountStatus, 
>> DataStoreStatus,
>>       GarbageCollectionJobStatus, GroupListItem, JobScheduleStatus, 
>> KeepOptions, MaintenanceMode,
>>       MaintenanceType, Operation, PruneJobOptions, SnapshotListItem, 
>> SnapshotVerifyState,
>> -    BACKUP_ARCHIVE_NAME_SCHEMA, BACKUP_ID_SCHEMA, 
>> BACKUP_NAMESPACE_SCHEMA, BACKUP_TIME_SCHEMA,
>> -    BACKUP_TYPE_SCHEMA, CATALOG_NAME, CLIENT_LOG_BLOB_NAME, 
>> DATASTORE_SCHEMA,
>> +    SyncJobConfig, BACKUP_ARCHIVE_NAME_SCHEMA, BACKUP_ID_SCHEMA, 
>> BACKUP_NAMESPACE_SCHEMA,
>> +    BACKUP_TIME_SCHEMA, BACKUP_TYPE_SCHEMA, CATALOG_NAME, 
>> CLIENT_LOG_BLOB_NAME, DATASTORE_SCHEMA,
>>       IGNORE_VERIFIED_BACKUPS_SCHEMA, MANIFEST_BLOB_NAME, 
>> MAX_NAMESPACE_DEPTH, NS_MAX_DEPTH_SCHEMA,
>>       PRIV_DATASTORE_AUDIT, PRIV_DATASTORE_BACKUP, 
>> PRIV_DATASTORE_MODIFY, PRIV_DATASTORE_PRUNE,
>>       PRIV_DATASTORE_READ, PRIV_DATASTORE_VERIFY, PRIV_SYS_MODIFY, 
>> UPID, UPID_SCHEMA,
>> @@ -2500,6 +2500,51 @@ pub fn do_mount_device(datastore: 
>> DataStoreConfig) -> Result<(), Error> {
>>       Ok(())
>>   }
>> +async fn do_sync_jobs(
> 
> nit: this helper function makes more sense to be placed into `src/api2/ 
> admin/sync.rs` I would say, as it purely handles execution of sync jobs.
> 
>> +    jobs_to_run: Vec<SyncJobConfig>,
>> +    worker: Arc<WorkerTask>,
>> +) -> Result<(), Error> {
>> +    let count = jobs_to_run.len();
>> +    info!(
>> +        "will run {} sync jobs: {}",
>> +        count,
> 
> nit: this variable can be in-lined into the format string.
> 
>> +        jobs_to_run
>> +            .iter()
>> +            .map(|j| j.id.clone())
>> +            .collect::<Vec<String>>()
>> +            .join(", ")
> 
> nit: not sure if it is required to list all sync jobs a-priori, 
> especially since that line might get rather long if many sync jobs are 
> to be executed? Also, that would avoid the double iteration and id cloning.
> 
>> +    );
>> +
>> +    for (i, job_config) in jobs_to_run.into_iter().enumerate() {
>> +        if worker.abort_requested() {
>> +            bail!("aborted due to user request");
>> +        }
>> +        let job_id = job_config.id.clone();
>> +        let Ok(job) = Job::new("syncjob", &job_id) else {
>> +            continue;
>> +        };
>> +        let auth_id = Authid::root_auth_id().clone();
>> +        info!("[{}/{count}] starting '{job_id}'...", i + 1);
>> +        match crate::server::do_sync_job(
>> +            job,
>> +            job_config,
>> +            &auth_id,
>> +            Some("mount".to_string()),
>> +            false,
>> +        ) {
>> +            Ok((_upid, handle)) => {
>> +                tokio::select! {
>> +                    sync_done = handle.fuse() => if let Err(err) = 
>> sync_done { warn!("could not wait for job to finish: {err}"); },
>> +                    _abort = worker.abort_future() => bail!("aborted 
>> due to user request"),
>> +                };
>> +            }
>> +            Err(err) => warn!("unable to start sync job {job_id} - 
>> {err}"),
>> +        }
>> +    }
>> +
>> +    Ok(())
>> +}
>> +
>>   #[api(
>>       protected: true,
>>       input: {
>> @@ -2531,12 +2576,47 @@ pub fn mount(store: String, rpcenv: &mut dyn 
>> RpcEnvironment) -> Result<Value, Er
>>       let auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?;
>>       let to_stdout = rpcenv.env_type() == RpcEnvironmentType::CLI;
>> -    let upid = WorkerTask::new_thread(
>> +    let upid = WorkerTask::spawn(
>>           "mount-device",
>> -        Some(store),
>> +        Some(store.clone()),
>>           auth_id.to_string(),
>>           to_stdout,
>> -        move |_worker| do_mount_device(datastore),
>> +        move |_worker| async move {
>> +            do_mount_device(datastore.clone())?;
>> +            let Ok((sync_config, _digest)) = 
>> pbs_config::sync::config() else {
>> +                warn!("unable to read sync job config, won't run any 
>> sync jobs");
>> +                return Ok(());
>> +            };
>> +            let Ok(list) = sync_config.convert_to_typed_array("sync") 
>> else {
>> +                warn!("unable to parse sync job config, won't run any 
>> sync jobs");
>> +                return Ok(());
>> +            };
>> +            let jobs_to_run: Vec<SyncJobConfig> = list
>> +                .into_iter()
>> +                .filter(|job: &SyncJobConfig| {
>> +                    // add job iff any of these apply
>> +                    //   - the jobs is local and we are source or target
>> +                    //   - we are the source of a push to a remote
>> +                    //   - we are the target of a pull from a remote
>> +                    //
>> +                    // `job.store == datastore.name` iff we are the 
>> target for pull from remote or we
>> +                    // are the source for push to remote, therefore 
>> we don't have to check for the
>> +                    // direction of the job.
> 
> This misses a check for the `run_on_mount` flag here I think, as it is 
> never read in the rest of the patch series?
> 

I think you are right, I actually missed checking that...

>> +                    job.remote.is_none() && job.remote_store == 
>> datastore.name
>> +                        || job.store == datastore.name
>> +                })
>> +                .collect();
>> +            if !jobs_to_run.is_empty() {
>> +                let _ = WorkerTask::spawn(
>> +                    "mount-sync-jobs",
>> +                    Some(store),
>> +                    auth_id.to_string(),
>> +                    false,
>> +                    move |worker| async move 
>> { do_sync_jobs(jobs_to_run, worker).await },
>> +                );
>> +            }
>> +            Ok(())
>> +        },
>>       )?;
>>       Ok(json!(upid))
>> diff --git a/src/api2/admin/sync.rs b/src/api2/admin/sync.rs
>> index 6722ebea0..01dea5126 100644
>> --- a/src/api2/admin/sync.rs
>> +++ b/src/api2/admin/sync.rs
>> @@ -161,7 +161,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, 
>> to_stdout)?;
>> +    let (upid_str, _) = do_sync_job(job, sync_job, &auth_id, None, 
>> to_stdout)?;
>>       Ok(upid_str)
>>   }
>> diff --git a/src/server/sync.rs b/src/server/sync.rs
>> index 0bd7a7a85..2a290a8ee 100644
>> --- a/src/server/sync.rs
>> +++ b/src/server/sync.rs
>> @@ -11,6 +11,7 @@ use anyhow::{bail, format_err, Context, Error};
>>   use futures::{future::FutureExt, select};
>>   use http::StatusCode;
>>   use serde_json::json;
>> +use tokio::task::JoinHandle;
>>   use tracing::{info, warn};
>>   use proxmox_human_byte::HumanByte;
>> @@ -598,7 +599,7 @@ pub fn do_sync_job(
>>       auth_id: &Authid,
>>       schedule: Option<String>,
>>       to_stdout: bool,
>> -) -> Result<String, Error> {
>> +) -> Result<(String, JoinHandle<()>), Error> {
>>       let job_id = format!(
>>           "{}:{}:{}:{}:{}",
>>           sync_job.remote.as_deref().unwrap_or("-"),
>> @@ -614,7 +615,7 @@ pub fn do_sync_job(
>>           bail!("can't sync to same datastore");
>>       }
>> -    let upid_str = WorkerTask::spawn(
>> +    let (upid_str, handle) = WorkerTask::spawn_with_handle(
>>           &worker_type,
>>           Some(job_id.clone()),
>>           auth_id.to_string(),
>> @@ -728,5 +729,5 @@ pub fn do_sync_job(
>>           },
>>       )?;
>> -    Ok(upid_str)
>> +    Ok((upid_str, handle))
>>   }
> 



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

Re: [pbs-devel] [PATCH proxmox-backup 3/7] api: admin: run configured sync jobs when a datastore is mounted

[Christian Ebner]

On 2/28/25 09:18, Hannes Laimer wrote:
> 
> 
> On 2/4/25 15:33, Christian Ebner wrote:
>> This patch needs to be rebased on current master.
>>
>> High level comment:
>> Mixing the mount api handler with the execution logic for the sync 
>> jobs is not the best approach here I think. This mixes unrelated code 
>> and makes it harder to further extend  this to e.g. also run verify, 
>> prune or garbage collection in the future.
>>
> 
> The endpoint starts the task that does the mounting, and it starts the
> tasks that manages possible sync jobs. I don't think this really does
> anything to the logic of the sync job itself. Sure, this
> "run these jobs in sequence" would make sense, but currently we can't do
> that, we don't have an interface for jobs. Not even job-ids are unique
> across different job types. I am not sure weather there would be a lot
> of value in trying to have a more abstract job things. The only thing
> I could think of is jobs triggering each other. But I feel like that
> might be out of scope for this here.

Right, therefore having the job scheduler queuing and executing the jobs 
seemed better to me. But you are right, there is currently no 
implementation that would achieve this.

> 
>> Just an idea: Maybe it would be possible to let the mount worker task 
>> signal the mount event via e.g. a std::sync::mpsc::channel() or the 
>> command socket (already used to request datastore cache updates on 
>> unmount) to schedule a managing task in the proxmox-backup-proxy?
>>
> 
> This is a privileged endpoint, it runs on the api process, so we
> can't really use channels here. And it don't think the command socket
> is the right tool for this, one reason being it is more of a hit or miss
> (technically) which is not really what we want here. Also having the api
> process handle this is IMHO not what we want.

Right, fair point, was just throwing ideas at you ;).

In lack of better approaches one might stick with the current 
implementation for executing the jobs. But I think this is rather 
unflexible and will be harder to extend if needed, getting messy with a 
job executing jobs, which themself might execute yet another job?

Opinions or suggestions of other devs on this?

> 
>> further comments inline.
>>
>> On 1/16/25 07:45, Hannes Laimer wrote:
>>> When a datastore is mounted, spawn a new task to run all sync jobs
>>> marked with `run-on-mount`. These jobs run sequentially and include
>>> any job for which the mounted datastore is:
>>>
>>> - The source or target in a local push/pull job
>>> - The source in a push job to a remote datastore
>>> - The target in a pull job from a remote datastore
>>>
>>> Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
>>> ---
>>>   src/api2/admin/datastore.rs | 90 ++++++++++++++++++++++++++++++++++---
>>>   src/api2/admin/sync.rs      |  2 +-
>>>   src/server/sync.rs          |  7 +--
>>>   3 files changed, 90 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/src/api2/admin/datastore.rs b/src/api2/admin/datastore.rs
>>> index f5d80d610..21b58391d 100644
>>> --- a/src/api2/admin/datastore.rs
>>> +++ b/src/api2/admin/datastore.rs
>>> @@ -41,8 +41,8 @@ use pbs_api_types::{
>>>       DataStoreConfig, DataStoreListItem, DataStoreMountStatus, 
>>> DataStoreStatus,
>>>       GarbageCollectionJobStatus, GroupListItem, JobScheduleStatus, 
>>> KeepOptions, MaintenanceMode,
>>>       MaintenanceType, Operation, PruneJobOptions, SnapshotListItem, 
>>> SnapshotVerifyState,
>>> -    BACKUP_ARCHIVE_NAME_SCHEMA, BACKUP_ID_SCHEMA, 
>>> BACKUP_NAMESPACE_SCHEMA, BACKUP_TIME_SCHEMA,
>>> -    BACKUP_TYPE_SCHEMA, CATALOG_NAME, CLIENT_LOG_BLOB_NAME, 
>>> DATASTORE_SCHEMA,
>>> +    SyncJobConfig, BACKUP_ARCHIVE_NAME_SCHEMA, BACKUP_ID_SCHEMA, 
>>> BACKUP_NAMESPACE_SCHEMA,
>>> +    BACKUP_TIME_SCHEMA, BACKUP_TYPE_SCHEMA, CATALOG_NAME, 
>>> CLIENT_LOG_BLOB_NAME, DATASTORE_SCHEMA,
>>>       IGNORE_VERIFIED_BACKUPS_SCHEMA, MANIFEST_BLOB_NAME, 
>>> MAX_NAMESPACE_DEPTH, NS_MAX_DEPTH_SCHEMA,
>>>       PRIV_DATASTORE_AUDIT, PRIV_DATASTORE_BACKUP, 
>>> PRIV_DATASTORE_MODIFY, PRIV_DATASTORE_PRUNE,
>>>       PRIV_DATASTORE_READ, PRIV_DATASTORE_VERIFY, PRIV_SYS_MODIFY, 
>>> UPID, UPID_SCHEMA,
>>> @@ -2500,6 +2500,51 @@ pub fn do_mount_device(datastore: 
>>> DataStoreConfig) -> Result<(), Error> {
>>>       Ok(())
>>>   }
>>> +async fn do_sync_jobs(
>>
>> nit: this helper function makes more sense to be placed into `src/ 
>> api2/ admin/sync.rs` I would say, as it purely handles execution of 
>> sync jobs.
>>
>>> +    jobs_to_run: Vec<SyncJobConfig>,
>>> +    worker: Arc<WorkerTask>,
>>> +) -> Result<(), Error> {
>>> +    let count = jobs_to_run.len();
>>> +    info!(
>>> +        "will run {} sync jobs: {}",
>>> +        count,
>>
>> nit: this variable can be in-lined into the format string.
>>
>>> +        jobs_to_run
>>> +            .iter()
>>> +            .map(|j| j.id.clone())
>>> +            .collect::<Vec<String>>()
>>> +            .join(", ")
>>
>> nit: not sure if it is required to list all sync jobs a-priori, 
>> especially since that line might get rather long if many sync jobs are 
>> to be executed? Also, that would avoid the double iteration and id 
>> cloning.
>>
>>> +    );
>>> +
>>> +    for (i, job_config) in jobs_to_run.into_iter().enumerate() {
>>> +        if worker.abort_requested() {
>>> +            bail!("aborted due to user request");
>>> +        }
>>> +        let job_id = job_config.id.clone();
>>> +        let Ok(job) = Job::new("syncjob", &job_id) else {
>>> +            continue;
>>> +        };
>>> +        let auth_id = Authid::root_auth_id().clone();
>>> +        info!("[{}/{count}] starting '{job_id}'...", i + 1);
>>> +        match crate::server::do_sync_job(
>>> +            job,
>>> +            job_config,
>>> +            &auth_id,
>>> +            Some("mount".to_string()),
>>> +            false,
>>> +        ) {
>>> +            Ok((_upid, handle)) => {
>>> +                tokio::select! {
>>> +                    sync_done = handle.fuse() => if let Err(err) = 
>>> sync_done { warn!("could not wait for job to finish: {err}"); },
>>> +                    _abort = worker.abort_future() => bail!("aborted 
>>> due to user request"),
>>> +                };
>>> +            }
>>> +            Err(err) => warn!("unable to start sync job {job_id} - 
>>> {err}"),
>>> +        }
>>> +    }
>>> +
>>> +    Ok(())
>>> +}
>>> +
>>>   #[api(
>>>       protected: true,
>>>       input: {
>>> @@ -2531,12 +2576,47 @@ pub fn mount(store: String, rpcenv: &mut dyn 
>>> RpcEnvironment) -> Result<Value, Er
>>>       let auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?;
>>>       let to_stdout = rpcenv.env_type() == RpcEnvironmentType::CLI;
>>> -    let upid = WorkerTask::new_thread(
>>> +    let upid = WorkerTask::spawn(
>>>           "mount-device",
>>> -        Some(store),
>>> +        Some(store.clone()),
>>>           auth_id.to_string(),
>>>           to_stdout,
>>> -        move |_worker| do_mount_device(datastore),
>>> +        move |_worker| async move {
>>> +            do_mount_device(datastore.clone())?;
>>> +            let Ok((sync_config, _digest)) = 
>>> pbs_config::sync::config() else {
>>> +                warn!("unable to read sync job config, won't run any 
>>> sync jobs");
>>> +                return Ok(());
>>> +            };
>>> +            let Ok(list) = 
>>> sync_config.convert_to_typed_array("sync") else {
>>> +                warn!("unable to parse sync job config, won't run 
>>> any sync jobs");
>>> +                return Ok(());
>>> +            };
>>> +            let jobs_to_run: Vec<SyncJobConfig> = list
>>> +                .into_iter()
>>> +                .filter(|job: &SyncJobConfig| {
>>> +                    // add job iff any of these apply
>>> +                    //   - the jobs is local and we are source or 
>>> target
>>> +                    //   - we are the source of a push to a remote
>>> +                    //   - we are the target of a pull from a remote
>>> +                    //
>>> +                    // `job.store == datastore.name` iff we are the 
>>> target for pull from remote or we
>>> +                    // are the source for push to remote, therefore 
>>> we don't have to check for the
>>> +                    // direction of the job.
>>
>> This misses a check for the `run_on_mount` flag here I think, as it is 
>> never read in the rest of the patch series?
>>
> 
> I think you are right, I actually missed checking that...
> 
>>> +                    job.remote.is_none() && job.remote_store == 
>>> datastore.name
>>> +                        || job.store == datastore.name
>>> +                })
>>> +                .collect();
>>> +            if !jobs_to_run.is_empty() {
>>> +                let _ = WorkerTask::spawn(
>>> +                    "mount-sync-jobs",
>>> +                    Some(store),
>>> +                    auth_id.to_string(),
>>> +                    false,
>>> +                    move |worker| async move 
>>> { do_sync_jobs(jobs_to_run, worker).await },
>>> +                );
>>> +            }
>>> +            Ok(())
>>> +        },
>>>       )?;
>>>       Ok(json!(upid))
>>> diff --git a/src/api2/admin/sync.rs b/src/api2/admin/sync.rs
>>> index 6722ebea0..01dea5126 100644
>>> --- a/src/api2/admin/sync.rs
>>> +++ b/src/api2/admin/sync.rs
>>> @@ -161,7 +161,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, 
>>> to_stdout)?;
>>> +    let (upid_str, _) = do_sync_job(job, sync_job, &auth_id, None, 
>>> to_stdout)?;
>>>       Ok(upid_str)
>>>   }
>>> diff --git a/src/server/sync.rs b/src/server/sync.rs
>>> index 0bd7a7a85..2a290a8ee 100644
>>> --- a/src/server/sync.rs
>>> +++ b/src/server/sync.rs
>>> @@ -11,6 +11,7 @@ use anyhow::{bail, format_err, Context, Error};
>>>   use futures::{future::FutureExt, select};
>>>   use http::StatusCode;
>>>   use serde_json::json;
>>> +use tokio::task::JoinHandle;
>>>   use tracing::{info, warn};
>>>   use proxmox_human_byte::HumanByte;
>>> @@ -598,7 +599,7 @@ pub fn do_sync_job(
>>>       auth_id: &Authid,
>>>       schedule: Option<String>,
>>>       to_stdout: bool,
>>> -) -> Result<String, Error> {
>>> +) -> Result<(String, JoinHandle<()>), Error> {
>>>       let job_id = format!(
>>>           "{}:{}:{}:{}:{}",
>>>           sync_job.remote.as_deref().unwrap_or("-"),
>>> @@ -614,7 +615,7 @@ pub fn do_sync_job(
>>>           bail!("can't sync to same datastore");
>>>       }
>>> -    let upid_str = WorkerTask::spawn(
>>> +    let (upid_str, handle) = WorkerTask::spawn_with_handle(
>>>           &worker_type,
>>>           Some(job_id.clone()),
>>>           auth_id.to_string(),
>>> @@ -728,5 +729,5 @@ pub fn do_sync_job(
>>>           },
>>>       )?;
>>> -    Ok(upid_str)
>>> +    Ok((upid_str, handle))
>>>   }
>>
> 



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

[pbs-devel] [PATCH proxmox-backup 4/7] api: admin: trigger sync jobs only on datastore mount

[Hannes Laimer]

Ensure sync jobs are triggered only when the datastore is actually
mounted. If the datastore is already mounted, we don't fail,
but sync jobs should not be re-triggered unnecessarily. This change
prevents redundant sync job execution.

Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
---
 src/api2/admin/datastore.rs | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/src/api2/admin/datastore.rs b/src/api2/admin/datastore.rs
index 21b58391d..e29ff9b99 100644
--- a/src/api2/admin/datastore.rs
+++ b/src/api2/admin/datastore.rs
@@ -2445,14 +2445,14 @@ fn setup_mounted_device(datastore: &DataStoreConfig, tmp_mount_path: &str) -> Re
 /// The reason for the randomized device mounting paths is to avoid two tasks trying to mount to
 /// the same path, this is *very* unlikely since the device is only mounted really shortly, but
 /// technically possible.
-pub fn do_mount_device(datastore: DataStoreConfig) -> Result<(), Error> {
+pub fn do_mount_device(datastore: DataStoreConfig) -> Result<bool, Error> {
     if let Some(uuid) = datastore.backing_device.as_ref() {
         if pbs_datastore::get_datastore_mount_status(&datastore) == Some(true) {
             info!(
                 "device is already mounted at '{}'",
                 datastore.absolute_path()
             );
-            return Ok(());
+            return Ok(false);
         }
         let tmp_mount_path = format!(
             "{}/{:x}",
@@ -2497,7 +2497,7 @@ pub fn do_mount_device(datastore: DataStoreConfig) -> Result<(), Error> {
             datastore.name
         )
     }
-    Ok(())
+    Ok(true)
 }
 
 async fn do_sync_jobs(
@@ -2582,7 +2582,9 @@ pub fn mount(store: String, rpcenv: &mut dyn RpcEnvironment) -> Result<Value, Er
         auth_id.to_string(),
         to_stdout,
         move |_worker| async move {
-            do_mount_device(datastore.clone())?;
+            if !do_mount_device(datastore.clone())? {
+                return Ok(());
+            }
             let Ok((sync_config, _digest)) = pbs_config::sync::config() else {
                 warn!("unable to read sync job config, won't run any sync jobs");
                 return Ok(());
-- 
2.39.5



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

Re: [pbs-devel] [PATCH proxmox-backup 4/7] api: admin: trigger sync jobs only on datastore mount

[Christian Ebner]

This patch should rather be placed as preparatory patch before patch 3 
and changes to `do_sync_jobs` directly included in the previous patch.

On 1/16/25 07:45, Hannes Laimer wrote:
> Ensure sync jobs are triggered only when the datastore is actually
> mounted. If the datastore is already mounted, we don't fail,
> but sync jobs should not be re-triggered unnecessarily. This change
> prevents redundant sync job execution.
> 
> Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
> ---
>   src/api2/admin/datastore.rs | 10 ++++++----
>   1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/src/api2/admin/datastore.rs b/src/api2/admin/datastore.rs
> index 21b58391d..e29ff9b99 100644
> --- a/src/api2/admin/datastore.rs
> +++ b/src/api2/admin/datastore.rs
> @@ -2445,14 +2445,14 @@ fn setup_mounted_device(datastore: &DataStoreConfig, tmp_mount_path: &str) -> Re
>   /// The reason for the randomized device mounting paths is to avoid two tasks trying to mount to
>   /// the same path, this is *very* unlikely since the device is only mounted really shortly, but
>   /// technically possible.
> -pub fn do_mount_device(datastore: DataStoreConfig) -> Result<(), Error> {
> +pub fn do_mount_device(datastore: DataStoreConfig) -> Result<bool, Error> {
>       if let Some(uuid) = datastore.backing_device.as_ref() {
>           if pbs_datastore::get_datastore_mount_status(&datastore) == Some(true) {
>               info!(
>                   "device is already mounted at '{}'",
>                   datastore.absolute_path()
>               );
> -            return Ok(());
> +            return Ok(false);
>           }
>           let tmp_mount_path = format!(
>               "{}/{:x}",
> @@ -2497,7 +2497,7 @@ pub fn do_mount_device(datastore: DataStoreConfig) -> Result<(), Error> {
>               datastore.name
>           )
>       }
> -    Ok(())
> +    Ok(true)
>   }
>   
>   async fn do_sync_jobs(
> @@ -2582,7 +2582,9 @@ pub fn mount(store: String, rpcenv: &mut dyn RpcEnvironment) -> Result<Value, Er
>           auth_id.to_string(),
>           to_stdout,
>           move |_worker| async move {
> -            do_mount_device(datastore.clone())?;
> +            if !do_mount_device(datastore.clone())? {
> +                return Ok(());
> +            }
>               let Ok((sync_config, _digest)) = pbs_config::sync::config() else {
>                   warn!("unable to read sync job config, won't run any sync jobs");
>                   return Ok(());



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

Re: [pbs-devel] [PATCH proxmox-backup 4/7] api: admin: trigger sync jobs only on datastore mount

[Hannes Laimer]

On 2/4/25 15:34, Christian Ebner wrote:
> This patch should rather be placed as preparatory patch before patch 3 
> and changes to `do_sync_jobs` directly included in the previous patch.
> 

Maybe, but these changes only really make sense in this context. If done
before it would be something like
```
           move |_worker| async move {
-            do_mount_device(datastore.clone())?;
+            if !do_mount_device(datastore.clone())? {
+                return Ok(());
+            }
	  }
```
I feel like having this change in the context of where it is needed does
add some value. Especially when one tries to find out later why this was
needed. Having code under the `return Ok(())` is pretty clear in terms
of what it is we try to skip here in some cases. Without I could imagine
being like "th was that good for?", then having to go through the commit
message. IMOH, having the extra context here is better than a possibly
better order. But no hard feelings though, if you'd really like this one
before I'll move it for v2.

> On 1/16/25 07:45, Hannes Laimer wrote:
>> Ensure sync jobs are triggered only when the datastore is actually
>> mounted. If the datastore is already mounted, we don't fail,
>> but sync jobs should not be re-triggered unnecessarily. This change
>> prevents redundant sync job execution.
>>
>> Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
>> ---
>>   src/api2/admin/datastore.rs | 10 ++++++----
>>   1 file changed, 6 insertions(+), 4 deletions(-)
>>
>> diff --git a/src/api2/admin/datastore.rs b/src/api2/admin/datastore.rs
>> index 21b58391d..e29ff9b99 100644
>> --- a/src/api2/admin/datastore.rs
>> +++ b/src/api2/admin/datastore.rs
>> @@ -2445,14 +2445,14 @@ fn setup_mounted_device(datastore: 
>> &DataStoreConfig, tmp_mount_path: &str) -> Re
>>   /// The reason for the randomized device mounting paths is to avoid 
>> two tasks trying to mount to
>>   /// the same path, this is *very* unlikely since the device is only 
>> mounted really shortly, but
>>   /// technically possible.
>> -pub fn do_mount_device(datastore: DataStoreConfig) -> Result<(), 
>> Error> {
>> +pub fn do_mount_device(datastore: DataStoreConfig) -> Result<bool, 
>> Error> {
>>       if let Some(uuid) = datastore.backing_device.as_ref() {
>>           if pbs_datastore::get_datastore_mount_status(&datastore) == 
>> Some(true) {
>>               info!(
>>                   "device is already mounted at '{}'",
>>                   datastore.absolute_path()
>>               );
>> -            return Ok(());
>> +            return Ok(false);
>>           }
>>           let tmp_mount_path = format!(
>>               "{}/{:x}",
>> @@ -2497,7 +2497,7 @@ pub fn do_mount_device(datastore: 
>> DataStoreConfig) -> Result<(), Error> {
>>               datastore.name
>>           )
>>       }
>> -    Ok(())
>> +    Ok(true)
>>   }
>>   async fn do_sync_jobs(
>> @@ -2582,7 +2582,9 @@ pub fn mount(store: String, rpcenv: &mut dyn 
>> RpcEnvironment) -> Result<Value, Er
>>           auth_id.to_string(),
>>           to_stdout,
>>           move |_worker| async move {
>> -            do_mount_device(datastore.clone())?;
>> +            if !do_mount_device(datastore.clone())? {
>> +                return Ok(());
>> +            }
>>               let Ok((sync_config, _digest)) = 
>> pbs_config::sync::config() else {
>>                   warn!("unable to read sync job config, won't run any 
>> sync jobs");
>>                   return Ok(());
> 



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

Re: [pbs-devel] [PATCH proxmox-backup 4/7] api: admin: trigger sync jobs only on datastore mount

[Christian Ebner]

On 5/15/25 13:33, Hannes Laimer wrote:
> 
> 
> On 2/4/25 15:34, Christian Ebner wrote:
>> This patch should rather be placed as preparatory patch before patch 3 
>> and changes to `do_sync_jobs` directly included in the previous patch.
>>
> 
> Maybe, but these changes only really make sense in this context. If done
> before it would be something like
> ```
>            move |_worker| async move {
> -            do_mount_device(datastore.clone())?;
> +            if !do_mount_device(datastore.clone())? {
> +                return Ok(());
> +            }
>        }
> ```
> I feel like having this change in the context of where it is needed does
> add some value. Especially when one tries to find out later why this was
> needed. Having code under the `return Ok(())` is pretty clear in terms
> of what it is we try to skip here in some cases. Without I could imagine
> being like "th was that good for?", then having to go through the commit
> message. IMOH, having the extra context here is better than a possibly
> better order. But no hard feelings though, if you'd really like this one
> before I'll move it for v2.

No, fine by me to keep as is given your reasoning!


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

[pbs-devel] [PATCH proxmox-backup 5/7] bin: manager: run uuid_mount/mount tasks on the proxy

[Hannes Laimer]

Use the API instead of running uuid_mount/mount directly in the CLI binary.
This ensures that all triggered tasks are handled by the proxy process.

Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
---
 src/bin/proxmox_backup_manager/datastore.rs | 42 +++++++++++++--------
 1 file changed, 27 insertions(+), 15 deletions(-)

diff --git a/src/bin/proxmox_backup_manager/datastore.rs b/src/bin/proxmox_backup_manager/datastore.rs
index 1922a55a2..50a07c03a 100644
--- a/src/bin/proxmox_backup_manager/datastore.rs
+++ b/src/bin/proxmox_backup_manager/datastore.rs
@@ -49,24 +49,31 @@ fn list_datastores(param: Value, rpcenv: &mut dyn RpcEnvironment) -> Result<Valu
             store: {
                 schema: DATASTORE_SCHEMA,
             },
-            digest: {
+            "output-format": {
+                schema: OUTPUT_FORMAT,
                 optional: true,
-                schema: PROXMOX_CONFIG_DIGEST_SCHEMA,
             },
         },
     },
 )]
 /// Mount a removable datastore.
-async fn mount_datastore(mut param: Value, rpcenv: &mut dyn RpcEnvironment) -> Result<(), Error> {
-    param["node"] = "localhost".into();
+async fn mount_datastore(
+    store: String,
+    mut param: Value,
+    _rpcenv: &mut dyn RpcEnvironment,
+) -> Result<(), Error> {
+    let output_format = extract_output_format(&mut param);
 
-    let info = &api2::admin::datastore::API_METHOD_MOUNT;
-    let result = match info.handler {
-        ApiHandler::Sync(handler) => (handler)(param, info, rpcenv)?,
-        _ => unreachable!(),
-    };
+    let client = connect_to_localhost()?;
+    let result = client
+        .post(
+            format!("api2/json/admin/datastore/{}/mount", store).as_str(),
+            None,
+        )
+        .await?;
+
+    view_task_result(&client, result, &output_format).await?;
 
-    crate::wait_for_local_worker(result.as_str().unwrap()).await?;
     Ok(())
 }
 
@@ -252,10 +259,6 @@ async fn update_datastore(name: String, mut param: Value) -> Result<(), Error> {
                 type: String,
                 description: "The UUID of the device that should be mounted",
             },
-            "output-format": {
-                schema: OUTPUT_FORMAT,
-                optional: true,
-            },
         },
     },
 )]
@@ -282,7 +285,16 @@ async fn uuid_mount(param: Value, _rpcenv: &mut dyn RpcEnvironment) -> Result<Va
     }
 
     if let Some(store) = matching_stores.first() {
-        api2::admin::datastore::do_mount_device(store.clone())?;
+        let client = connect_to_localhost()?;
+        let result = client
+            .post(
+                format!("api2/json/admin/datastore/{}/mount", store.name).as_str(),
+                None,
+            )
+            .await?;
+
+        view_task_result(&client, result, "json").await?;
+        return Ok(Value::Null);
     }
 
     // we don't want to fail for UUIDs that are not associated with datastores, as that produces
-- 
2.39.5



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

Re: [pbs-devel] [PATCH proxmox-backup 5/7] bin: manager: run uuid_mount/mount tasks on the proxy

[Christian Ebner]

comments inline

On 1/16/25 07:45, Hannes Laimer wrote:
> Use the API instead of running uuid_mount/mount directly in the CLI binary.
> This ensures that all triggered tasks are handled by the proxy process.
> 
> Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
> ---
>   src/bin/proxmox_backup_manager/datastore.rs | 42 +++++++++++++--------
>   1 file changed, 27 insertions(+), 15 deletions(-)
> 
> diff --git a/src/bin/proxmox_backup_manager/datastore.rs b/src/bin/proxmox_backup_manager/datastore.rs
> index 1922a55a2..50a07c03a 100644
> --- a/src/bin/proxmox_backup_manager/datastore.rs
> +++ b/src/bin/proxmox_backup_manager/datastore.rs
> @@ -49,24 +49,31 @@ fn list_datastores(param: Value, rpcenv: &mut dyn RpcEnvironment) -> Result<Valu
>               store: {
>                   schema: DATASTORE_SCHEMA,
>               },
> -            digest: {

Why is digest removed here? This parameter should stay as otherwise it 
is a breaking api change. If this has to be removed, the reason should 
be mentioned in the commit message.

> +            "output-format": {
> +                schema: OUTPUT_FORMAT,
>                   optional: true,
> -                schema: PROXMOX_CONFIG_DIGEST_SCHEMA,
>               },
>           },
>       },
>   )]
>   /// Mount a removable datastore.
> -async fn mount_datastore(mut param: Value, rpcenv: &mut dyn RpcEnvironment) -> Result<(), Error> {
> -    param["node"] = "localhost".into();
> +async fn mount_datastore(
> +    store: String,
> +    mut param: Value,
> +    _rpcenv: &mut dyn RpcEnvironment,
> +) -> Result<(), Error> {
> +    let output_format = extract_output_format(&mut param);
>   
> -    let info = &api2::admin::datastore::API_METHOD_MOUNT;
> -    let result = match info.handler {
> -        ApiHandler::Sync(handler) => (handler)(param, info, rpcenv)?,
> -        _ => unreachable!(),
> -    };
> +    let client = connect_to_localhost()?;
> +    let result = client
> +        .post(
> +            format!("api2/json/admin/datastore/{}/mount", store).as_str(),

nit: store can be in-lined into the format string

> +            None,
> +        )
> +        .await?;
> +
> +    view_task_result(&client, result, &output_format).await?;
>   
> -    crate::wait_for_local_worker(result.as_str().unwrap()).await?;
>       Ok(())
>   }
>   
> @@ -252,10 +259,6 @@ async fn update_datastore(name: String, mut param: Value) -> Result<(), Error> {
>                   type: String,
>                   description: "The UUID of the device that should be mounted",
>               },
> -            "output-format": {

Breaking change, can be avoided?

> -                schema: OUTPUT_FORMAT,
> -                optional: true,
> -            },
>           },
>       },
>   )]
> @@ -282,7 +285,16 @@ async fn uuid_mount(param: Value, _rpcenv: &mut dyn RpcEnvironment) -> Result<Va
>       }
>   
>       if let Some(store) = matching_stores.first() {
> -        api2::admin::datastore::do_mount_device(store.clone())?;
> +        let client = connect_to_localhost()?;
> +        let result = client
> +            .post(
> +                format!("api2/json/admin/datastore/{}/mount", store.name).as_str(),
> +                None,
> +            )
> +            .await?;
> +
> +        view_task_result(&client, result, "json").await?;

this could get the output format as obtained via the get_output_format 
helper instead of being json only? Analog to the other api endpoints.

> +        return Ok(Value::Null);
>       }
>   
>       // we don't want to fail for UUIDs that are not associated with datastores, as that produces



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

[pbs-devel] [PATCH proxmox-backup 6/7] ui: add run-on-mount checkbox to SyncJob form

[Hannes Laimer]

Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
---
 www/window/SyncJobEdit.js | 23 ++++++++++++++++++++++-
 1 file changed, 22 insertions(+), 1 deletion(-)

diff --git a/www/window/SyncJobEdit.js b/www/window/SyncJobEdit.js
index bcd2f2fb2..2b3660ed9 100644
--- a/www/window/SyncJobEdit.js
+++ b/www/window/SyncJobEdit.js
@@ -177,7 +177,7 @@ Ext.define('PBS.window.SyncJobEdit', {
 			fieldLabel: gettext('Sync Schedule'),
 			xtype: 'pbsCalendarEvent',
 			name: 'schedule',
-			emptyText: gettext('none (disabled)'),
+			emptyText: gettext('none'),
 			cbind: {
 			    deleteEmpty: '{!isCreate}',
 			    value: '{scheduleValue}',
@@ -191,6 +191,27 @@ Ext.define('PBS.window.SyncJobEdit', {
 			submitAutoScaledSizeUnit: true,
 			// NOTE: handle deleteEmpty in onGetValues due to bandwidth field having a cbind too
 		    },
+		    {
+			fieldLabel: gettext('Run when mounted'),
+			xtype: 'proxmoxcheckbox',
+			name: 'run-on-mount',
+			autoEl: {
+			    tag: 'div',
+			    'data-qtip': gettext('Run this job when a relevant removable datastore is mounted'),
+			},
+			uncheckedValue: false,
+			value: false,
+			listeners: {
+			    change: function(checkbox, runOnMount) {
+				let me = this;
+				let form = me.up('pbsSyncJobEdit');
+				let scheduleField = form.down('field[name=schedule]');
+				if (runOnMount) {
+				    scheduleField.clearValue();
+				}
+			    },
+			},
+		    },
 		],
 
 		column2: [
-- 
2.39.5



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

Re: [pbs-devel] [PATCH proxmox-backup 6/7] ui: add run-on-mount checkbox to SyncJob form

[Christian Ebner]

On 1/16/25 07:45, Hannes Laimer wrote:
> Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
> ---
>   www/window/SyncJobEdit.js | 23 ++++++++++++++++++++++-
>   1 file changed, 22 insertions(+), 1 deletion(-)
> 
> diff --git a/www/window/SyncJobEdit.js b/www/window/SyncJobEdit.js
> index bcd2f2fb2..2b3660ed9 100644
> --- a/www/window/SyncJobEdit.js
> +++ b/www/window/SyncJobEdit.js
> @@ -177,7 +177,7 @@ Ext.define('PBS.window.SyncJobEdit', {
>   			fieldLabel: gettext('Sync Schedule'),
>   			xtype: 'pbsCalendarEvent',
>   			name: 'schedule',
> -			emptyText: gettext('none (disabled)'),
> +			emptyText: gettext('none'),
>   			cbind: {
>   			    deleteEmpty: '{!isCreate}',
>   			    value: '{scheduleValue}',
> @@ -191,6 +191,27 @@ Ext.define('PBS.window.SyncJobEdit', {
>   			submitAutoScaledSizeUnit: true,
>   			// NOTE: handle deleteEmpty in onGetValues due to bandwidth field having a cbind too
>   		    },
> +		    {
> +			fieldLabel: gettext('Run when mounted'),
> +			xtype: 'proxmoxcheckbox',
> +			name: 'run-on-mount',
> +			autoEl: {
> +			    tag: 'div',
> +			    'data-qtip': gettext('Run this job when a relevant removable datastore is mounted'),
> +			},
> +			uncheckedValue: false,
> +			value: false,
> +			listeners: {
> +			    change: function(checkbox, runOnMount) {
> +				let me = this;
> +				let form = me.up('pbsSyncJobEdit');
> +				let scheduleField = form.down('field[name=schedule]');
> +				if (runOnMount) {
> +				    scheduleField.clearValue();

clearing the schedule field's value is not enough. Clicking the checkbox 
should also disable that field, as otherwise one can re-add a schedule 
after setting the flag. Further, if this is a requirement, setting and 
getting the sync job config should also include a check for that?

Although I'm not sure it is really required for a sync job which runs on 
mount to not be also be able to run on schedule?

> +				}
> +			    },
> +			},
> +		    },
>   		],
>   
>   		column2: [



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

Re: [pbs-devel] [PATCH proxmox-backup 6/7] ui: add run-on-mount checkbox to SyncJob form

[Hannes Laimer]

On 2/4/25 15:46, Christian Ebner wrote:
> On 1/16/25 07:45, Hannes Laimer wrote:
>> Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
>> ---
>>   www/window/SyncJobEdit.js | 23 ++++++++++++++++++++++-
>>   1 file changed, 22 insertions(+), 1 deletion(-)
>>
>> diff --git a/www/window/SyncJobEdit.js b/www/window/SyncJobEdit.js
>> index bcd2f2fb2..2b3660ed9 100644
>> --- a/www/window/SyncJobEdit.js
>> +++ b/www/window/SyncJobEdit.js
>> @@ -177,7 +177,7 @@ Ext.define('PBS.window.SyncJobEdit', {
>>               fieldLabel: gettext('Sync Schedule'),
>>               xtype: 'pbsCalendarEvent',
>>               name: 'schedule',
>> -            emptyText: gettext('none (disabled)'),
>> +            emptyText: gettext('none'),
>>               cbind: {
>>                   deleteEmpty: '{!isCreate}',
>>                   value: '{scheduleValue}',
>> @@ -191,6 +191,27 @@ Ext.define('PBS.window.SyncJobEdit', {
>>               submitAutoScaledSizeUnit: true,
>>               // NOTE: handle deleteEmpty in onGetValues due to 
>> bandwidth field having a cbind too
>>               },
>> +            {
>> +            fieldLabel: gettext('Run when mounted'),
>> +            xtype: 'proxmoxcheckbox',
>> +            name: 'run-on-mount',
>> +            autoEl: {
>> +                tag: 'div',
>> +                'data-qtip': gettext('Run this job when a relevant 
>> removable datastore is mounted'),
>> +            },
>> +            uncheckedValue: false,
>> +            value: false,
>> +            listeners: {
>> +                change: function(checkbox, runOnMount) {
>> +                let me = this;
>> +                let form = me.up('pbsSyncJobEdit');
>> +                let scheduleField = form.down('field[name=schedule]');
>> +                if (runOnMount) {
>> +                    scheduleField.clearValue();
> 
> clearing the schedule field's value is not enough. Clicking the checkbox 
> should also disable that field, as otherwise one can re-add a schedule 
> after setting the flag. Further, if this is a requirement, setting and 
> getting the sync job config should also include a check for that?
> 
> Although I'm not sure it is really required for a sync job which runs on 
> mount to not be also be able to run on schedule?
> 

it is not required, it was more of a "you probably don't need that
anymore" thought. No hard feelings about not clearing it.

>> +                }
>> +                },
>> +            },
>> +            },
>>           ],
>>           column2: [
> 



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

Re: [pbs-devel] [PATCH proxmox-backup 6/7] ui: add run-on-mount checkbox to SyncJob form

[Christian Ebner]

Forgot to mention that the run on mount checkbox should be placed in the 
advanced options IMO, as this only makes sense for sync jobs which are 
related to removable datastores.


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

[pbs-devel] [PATCH proxmox-backup 7/7] ui: add task title for triggering sync jobs

[Hannes Laimer]

Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
---
 www/Utils.js | 1 +
 1 file changed, 1 insertion(+)

diff --git a/www/Utils.js b/www/Utils.js
index 2746ef0b5..e8f82e6da 100644
--- a/www/Utils.js
+++ b/www/Utils.js
@@ -422,6 +422,7 @@ Ext.define('PBS.Utils', {
 	    prunejob: (type, id) => PBS.Utils.render_prune_job_worker_id(id, gettext('Prune Job')),
 	    reader: (type, id) => PBS.Utils.render_datastore_worker_id(id, gettext('Read Objects')),
 	    'rewind-media': [gettext('Drive'), gettext('Rewind Media')],
+	    'mount-sync-jobs': [gettext('Datastore'), gettext('trigger sync jobs')],
 	    sync: ['Datastore', gettext('Remote Sync')],
 	    syncjob: [gettext('Sync Job'), gettext('Remote Sync')],
 	    'tape-backup': (type, id) => PBS.Utils.render_tape_backup_id(id, gettext('Tape Backup')),
-- 
2.39.5



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

Re: [pbs-devel] [PATCH proxmox/proxmox-backup 0/7] trigger sync jobs on mount

[Christian Ebner]

On 1/16/25 07:45, Hannes Laimer wrote:
> Sync jobs now have a run-on-mount flag, that, if set, runs the job whenever
> a relevant removable datastore is mounted.
> 
> 
> * proxmox:
> Hannes Laimer (1):
>    rest-server: add function that returns a join handle for spawn
> 
>   proxmox-rest-server/src/worker_task.rs | 21 +++++++++++++++++++--
>   1 file changed, 19 insertions(+), 2 deletions(-)
>   
> 
> * proxmox-backup:
> Hannes Laimer (6):
>    api types: add run-on-mount flag to SyncJobConfig
>    api: admin: run configured sync jobs when a datastore is mounted
>    api: admin: trigger sync jobs only on datastore mount
>    bin: manager: run uuid_mount/mount tasks on the proxy
>    ui: add run-on-mount checkbox to SyncJob form
>    ui: add task title for triggering sync jobs
> 
>   pbs-api-types/src/jobs.rs                   |  3 +
>   src/api2/admin/datastore.rs                 | 96 +++++++++++++++++++--
>   src/api2/admin/sync.rs                      |  2 +-
>   src/bin/proxmox_backup_manager/datastore.rs | 42 +++++----
>   src/server/sync.rs                          |  7 +-
>   www/Utils.js                                |  1 +
>   www/window/SyncJobEdit.js                   | 23 ++++-
>   7 files changed, 147 insertions(+), 27 deletions(-)
> 

High level comments:
As mentioned in the comments to patch 3, decoupling the mount logic from 
the sync job logic would be desirable for better maintainability and 
possible future extension to other jobs such as prune, verify, garbage 
collection, ecc.

The run on mount flag should be an optional parameter of the sync job 
config, as otherwise per-existing configs are not valid anymore.
Also, clearing the schedule for the sync jobs when the run on mount flag 
is set is not really necessary in my opinion, as a sync job may be 
configured just fine with both.

Further, this still needs a
```
diff --git a/src/api2/config/sync.rs b/src/api2/config/sync.rs
index a8ea93465..f356bc588 100644
--- a/src/api2/config/sync.rs
+++ b/src/api2/config/sync.rs
@@ -666,6 +666,7 @@ 
acl:1:/remote/remote1/remotestore1:write@pbs:RemoteSyncOperator
          limit: pbs_api_types::RateLimitConfig::default(), // no limit
          transfer_last: None,
          sync_direction: None, // use default
+        run_on_mount: false,
      };

      // should work without ACLs
```
for the tests to build and pass successfully.


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

Re: [pbs-devel] [PATCH proxmox/proxmox-backup 0/7] trigger sync jobs on mount

[Hannes Laimer]

On 2/4/25 15:54, Christian Ebner wrote:
> On 1/16/25 07:45, Hannes Laimer wrote:
>> Sync jobs now have a run-on-mount flag, that, if set, runs the job 
>> whenever
>> a relevant removable datastore is mounted.
>>
>>
>> * proxmox:
>> Hannes Laimer (1):
>>    rest-server: add function that returns a join handle for spawn
>>
>>   proxmox-rest-server/src/worker_task.rs | 21 +++++++++++++++++++--
>>   1 file changed, 19 insertions(+), 2 deletions(-)
>>
>> * proxmox-backup:
>> Hannes Laimer (6):
>>    api types: add run-on-mount flag to SyncJobConfig
>>    api: admin: run configured sync jobs when a datastore is mounted
>>    api: admin: trigger sync jobs only on datastore mount
>>    bin: manager: run uuid_mount/mount tasks on the proxy
>>    ui: add run-on-mount checkbox to SyncJob form
>>    ui: add task title for triggering sync jobs
>>
>>   pbs-api-types/src/jobs.rs                   |  3 +
>>   src/api2/admin/datastore.rs                 | 96 +++++++++++++++++++--
>>   src/api2/admin/sync.rs                      |  2 +-
>>   src/bin/proxmox_backup_manager/datastore.rs | 42 +++++----
>>   src/server/sync.rs                          |  7 +-
>>   www/Utils.js                                |  1 +
>>   www/window/SyncJobEdit.js                   | 23 ++++-
>>   7 files changed, 147 insertions(+), 27 deletions(-)
>>
> 
> High level comments:
> As mentioned in the comments to patch 3, decoupling the mount logic from 
> the sync job logic would be desirable for better maintainability and 
> possible future extension to other jobs such as prune, verify, garbage 
> collection, ecc.
> 
> The run on mount flag should be an optional parameter of the sync job 
> config, as otherwise per-existing configs are not valid anymore.
> Also, clearing the schedule for the sync jobs when the run on mount flag 
> is set is not really necessary in my opinion, as a sync job may be 
> configured just fine with both.
> 
> Further, this still needs a
> ```
> diff --git a/src/api2/config/sync.rs b/src/api2/config/sync.rs
> index a8ea93465..f356bc588 100644
> --- a/src/api2/config/sync.rs
> +++ b/src/api2/config/sync.rs
> @@ -666,6 +666,7 @@ acl:1:/remote/remote1/ 
> remotestore1:write@pbs:RemoteSyncOperator
>           limit: pbs_api_types::RateLimitConfig::default(), // no limit
>           transfer_last: None,
>           sync_direction: None, // use default
> +        run_on_mount: false,
>       };
> 
>       // should work without ACLs
> ```
> for the tests to build and pass successfully.

Thanks a lot for taking a look and the feedback!


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

Re: [pbs-devel] [PATCH proxmox/proxmox-backup 0/7] trigger sync jobs on mount

[Hannes Laimer]

superseded by: 
https://lore.proxmox.com/pbs-devel/20250515124138.55436-1-h.laimer@proxmox.com/T/#t

On 1/16/25 07:45, Hannes Laimer wrote:
> Sync jobs now have a run-on-mount flag, that, if set, runs the job whenever
> a relevant removable datastore is mounted.
> 
> 
> * proxmox:
> Hannes Laimer (1):
>    rest-server: add function that returns a join handle for spawn
> 
>   proxmox-rest-server/src/worker_task.rs | 21 +++++++++++++++++++--
>   1 file changed, 19 insertions(+), 2 deletions(-)
>   
> 
> * proxmox-backup:
> Hannes Laimer (6):
>    api types: add run-on-mount flag to SyncJobConfig
>    api: admin: run configured sync jobs when a datastore is mounted
>    api: admin: trigger sync jobs only on datastore mount
>    bin: manager: run uuid_mount/mount tasks on the proxy
>    ui: add run-on-mount checkbox to SyncJob form
>    ui: add task title for triggering sync jobs
> 
>   pbs-api-types/src/jobs.rs                   |  3 +
>   src/api2/admin/datastore.rs                 | 96 +++++++++++++++++++--
>   src/api2/admin/sync.rs                      |  2 +-
>   src/bin/proxmox_backup_manager/datastore.rs | 42 +++++----
>   src/server/sync.rs                          |  7 +-
>   www/Utils.js                                |  1 +
>   www/window/SyncJobEdit.js                   | 23 ++++-
>   7 files changed, 147 insertions(+), 27 deletions(-)
> 



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

[pbs-devel] [PATCH proxmox-backup 0/7] trigger sync jobs on mount

[Hannes Laimer]

Sync jobs now have a run-on-mount flag, that, if set, runs the job whenever
a relevant removable datastore is mounted. More details on when a job is 
triggered in the commit message of patch #3

proxmox-rest-server has to be bumped since patch #3
relies on the new function added in patch #1 

 
* proxmox
Hannes Laimer (1):
  rest-server: add function that returns a join handle for spawn

 proxmox-rest-server/src/worker_task.rs | 21 +++++++++++++++++++--
 1 file changed, 19 insertions(+), 2 deletions(-)

* proxmox-backup
Hannes Laimer (6):
  api types: add run-on-mount flag to SyncJobConfig
  api: admin: run configured sync jobs when a datastore is mounted
  api: admin: trigger sync jobs only on datastore mount
  bin: manager: run uuid_mount/mount tasks on the proxy
  ui: add run-on-mount checkbox to SyncJob form
  ui: add task title for triggering sync jobs

 pbs-api-types/src/jobs.rs                   |  3 +
 src/api2/admin/datastore.rs                 | 96 +++++++++++++++++++--
 src/api2/admin/sync.rs                      |  2 +-
 src/bin/proxmox_backup_manager/datastore.rs | 42 +++++----
 src/server/sync.rs                          |  7 +-
 www/Utils.js                                |  1 +
 www/window/SyncJobEdit.js                   | 23 ++++-
 7 files changed, 147 insertions(+), 27 deletions(-)

-- 
2.39.5



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

[pbs-devel] [PATCH proxmox-backup 5/7] bin: manager: run uuid_mount/mount tasks on the proxy

[Hannes Laimer]

Use the API instead of running uuid_mount/mount directly in the CLI binary.
This ensures that all triggered tasks are handled by the proxy process.

Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
---
 src/bin/proxmox_backup_manager/datastore.rs | 42 +++++++++++++--------
 1 file changed, 27 insertions(+), 15 deletions(-)

diff --git a/src/bin/proxmox_backup_manager/datastore.rs b/src/bin/proxmox_backup_manager/datastore.rs
index 0918af3a4..e9fc1b93e 100644
--- a/src/bin/proxmox_backup_manager/datastore.rs
+++ b/src/bin/proxmox_backup_manager/datastore.rs
@@ -49,24 +49,31 @@ fn list_datastores(param: Value, rpcenv: &mut dyn RpcEnvironment) -> Result<Valu
             store: {
                 schema: DATASTORE_SCHEMA,
             },
-            digest: {
+            "output-format": {
+                schema: OUTPUT_FORMAT,
                 optional: true,
-                schema: PROXMOX_CONFIG_DIGEST_SCHEMA,
             },
         },
     },
 )]
 /// Mount a removable datastore.
-async fn mount_datastore(mut param: Value, rpcenv: &mut dyn RpcEnvironment) -> Result<(), Error> {
-    param["node"] = "localhost".into();
+async fn mount_datastore(
+    store: String,
+    mut param: Value,
+    _rpcenv: &mut dyn RpcEnvironment,
+) -> Result<(), Error> {
+    let output_format = extract_output_format(&mut param);
 
-    let info = &api2::admin::datastore::API_METHOD_MOUNT;
-    let result = match info.handler {
-        ApiHandler::Sync(handler) => (handler)(param, info, rpcenv)?,
-        _ => unreachable!(),
-    };
+    let client = connect_to_localhost()?;
+    let result = client
+        .post(
+            format!("api2/json/admin/datastore/{}/mount", store).as_str(),
+            None,
+        )
+        .await?;
+
+    view_task_result(&client, result, &output_format).await?;
 
-    crate::wait_for_local_worker(result.as_str().unwrap()).await?;
     Ok(())
 }
 
@@ -252,10 +259,6 @@ async fn update_datastore(name: String, mut param: Value) -> Result<(), Error> {
                 type: String,
                 description: "The UUID of the device that should be mounted",
             },
-            "output-format": {
-                schema: OUTPUT_FORMAT,
-                optional: true,
-            },
         },
     },
 )]
@@ -282,7 +285,16 @@ async fn uuid_mount(param: Value, _rpcenv: &mut dyn RpcEnvironment) -> Result<Va
     }
 
     if let Some(store) = matching_stores.first() {
-        api2::admin::datastore::do_mount_device(store.clone())?;
+        let client = connect_to_localhost()?;
+        let result = client
+            .post(
+                format!("api2/json/admin/datastore/{}/mount", store.name).as_str(),
+                None,
+            )
+            .await?;
+
+        view_task_result(&client, result, "json").await?;
+        return Ok(Value::Null);
     }
 
     // we don't want to fail for UUIDs that are not associated with datastores, as that produces
-- 
2.39.5



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