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

[pbs-devel] [PATCH proxmox/proxmox-backup v2 0/8] 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. 

This depends on [1], without it the api process does not drop the file
handle on the `.lock` file which leads to the datastore being not
unmountable after sync jobs were triggered. (now thinking about it,
might have made sense to include it in this series directly, but it also
does make sense on its own)

v2, thanks @Chris:
- rebased onto master
- improve some docstrings
- move/fix config flag
- drop not-needed changes for the manager binary
- ui: move checkbox to advenced section + don't clear schedule field
- fix test
- actually check the configured flag when deciding if a job should
  run...

[1] https://lore.proxmox.com/pbs-devel/20250512125933.156192-1-h.laimer@proxmox.com/T/#u


proxmox:
Hannes Laimer (2):
  rest-server: add function that returns a join handle for spawn
  pbs-api-types: add run-on-mount flag to SyncJobConfig

 pbs-api-types/src/jobs.rs              |  8 ++++++++
 proxmox-rest-server/src/worker_task.rs | 24 ++++++++++++++++++++++--
 2 files changed, 30 insertions(+), 2 deletions(-)

proxmox-backup:
Hannes Laimer (6):
  api: config: sync: update run-on-mount correctly
  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

 src/api2/admin/datastore.rs                 | 97 +++++++++++++++++++--
 src/api2/admin/sync.rs                      |  2 +-
 src/api2/config/sync.rs                     |  9 ++
 src/bin/proxmox_backup_manager/datastore.rs | 41 ++++++---
 src/server/sync.rs                          |  7 +-
 www/Utils.js                                |  1 +
 www/window/SyncJobEdit.js                   | 13 ++-
 7 files changed, 148 insertions(+), 22 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 v2 1/8] 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 | 24 ++++++++++++++++++++++--
 1 file changed, 22 insertions(+), 2 deletions(-)

diff --git a/proxmox-rest-server/src/worker_task.rs b/proxmox-rest-server/src/worker_task.rs
index a3a65add..459728c3 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::{error, info, warn};
 
 use proxmox_daemon::command_socket::CommandSocket;
@@ -936,6 +937,25 @@ 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)
+    }
+
+    /// 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.
+    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>>,
@@ -944,12 +964,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

[pbs-devel] [PATCH proxmox v2 2/8] pbs-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 | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/pbs-api-types/src/jobs.rs b/pbs-api-types/src/jobs.rs
index 6ef13dc2..3eb61cde 100644
--- a/pbs-api-types/src/jobs.rs
+++ b/pbs-api-types/src/jobs.rs
@@ -536,6 +536,8 @@ pub const SYNC_ENCRYPTED_ONLY_SCHEMA: Schema =
     BooleanSchema::new("Only synchronize encrypted backup snapshots, exclude others.").schema();
 pub const SYNC_VERIFIED_ONLY_SCHEMA: Schema =
     BooleanSchema::new("Only synchronize verified backup snapshots, exclude others.").schema();
+pub const RUN_SYNC_ON_MOUNT_SCHEMA: Schema =
+    BooleanSchema::new("Run this job when a relevant datastore is mounted.").schema();
 
 #[api(
     properties: {
@@ -603,6 +605,10 @@ pub const SYNC_VERIFIED_ONLY_SCHEMA: Schema =
             schema: SYNC_VERIFIED_ONLY_SCHEMA,
             optional: true,
         },
+        "run-on-mount": {
+            schema: RUN_SYNC_ON_MOUNT_SCHEMA,
+            optional: true,
+        },
         "sync-direction": {
             type: SyncDirection,
             optional: true,
@@ -647,6 +653,8 @@ pub struct SyncJobConfig {
     #[serde(skip_serializing_if = "Option::is_none")]
     pub verified_only: Option<bool>,
     #[serde(skip_serializing_if = "Option::is_none")]
+    pub run_on_mount: Option<bool>,
+    #[serde(skip_serializing_if = "Option::is_none")]
     pub sync_direction: Option<SyncDirection>,
 }
 
-- 
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 v2 3/8] api: config: sync: update run-on-mount correctly

[Hannes Laimer]

Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
---
 src/api2/config/sync.rs | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/src/api2/config/sync.rs b/src/api2/config/sync.rs
index 6194d8653..358409b54 100644
--- a/src/api2/config/sync.rs
+++ b/src/api2/config/sync.rs
@@ -339,6 +339,8 @@ pub enum DeletableProperty {
     EncryptedOnly,
     /// Delete the verified_only property,
     VerifiedOnly,
+    /// Delete the run_on_mount property,
+    RunOnMount,
     /// Delete the sync_direction property,
     SyncDirection,
 }
@@ -458,6 +460,9 @@ pub fn update_sync_job(
                 DeletableProperty::VerifiedOnly => {
                     data.verified_only = None;
                 }
+                DeletableProperty::RunOnMount => {
+                    data.run_on_mount = None;
+                }
                 DeletableProperty::SyncDirection => {
                     data.sync_direction = None;
                 }
@@ -507,6 +512,9 @@ pub fn update_sync_job(
     if let Some(verified_only) = update.verified_only {
         data.verified_only = Some(verified_only);
     }
+    if let Some(run_on_mount) = update.run_on_mount {
+        data.run_on_mount = Some(run_on_mount);
+    }
     if let Some(sync_direction) = update.sync_direction {
         data.sync_direction = Some(sync_direction);
     }
@@ -683,6 +691,7 @@ acl:1:/remote/remote1/remotestore1:write@pbs:RemoteSyncOperator
         transfer_last: None,
         encrypted_only: None,
         verified_only: None,
+        run_on_mount: None,
         sync_direction: None, // use default
     };
 
-- 
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 v2 4/8] 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 | 91 +++++++++++++++++++++++++++++++++++--
 src/api2/admin/sync.rs      |  2 +-
 src/server/sync.rs          |  7 +--
 3 files changed, 91 insertions(+), 9 deletions(-)

diff --git a/src/api2/admin/datastore.rs b/src/api2/admin/datastore.rs
index 392494488..8463adb6a 100644
--- a/src/api2/admin/datastore.rs
+++ b/src/api2/admin/datastore.rs
@@ -42,8 +42,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,
@@ -2510,6 +2510,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: {
@@ -2541,12 +2586,48 @@ 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 (running on mount is enabled and) 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.run_on_mount.unwrap_or(false)
+                        && (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 09814ef0c..c45a8975e 100644
--- a/src/server/sync.rs
+++ b/src/server/sync.rs
@@ -12,6 +12,7 @@ use futures::{future::FutureExt, select};
 use hyper::http::StatusCode;
 use pbs_config::BackupLockGuard;
 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(),
@@ -730,7 +731,7 @@ pub fn do_sync_job(
         },
     )?;
 
-    Ok(upid_str)
+    Ok((upid_str, handle))
 }
 
 pub(super) fn ignore_not_verified_or_encrypted(
-- 
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 v2 5/8] 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 8463adb6a..a3ba82e21 100644
--- a/src/api2/admin/datastore.rs
+++ b/src/api2/admin/datastore.rs
@@ -2455,14 +2455,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}",
@@ -2507,7 +2507,7 @@ pub fn do_mount_device(datastore: DataStoreConfig) -> Result<(), Error> {
             datastore.name
         )
     }
-    Ok(())
+    Ok(true)
 }
 
 async fn do_sync_jobs(
@@ -2592,7 +2592,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

[pbs-devel] [PATCH proxmox-backup v2 6/8] 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 | 41 ++++++++++++++++-----
 1 file changed, 31 insertions(+), 10 deletions(-)

diff --git a/src/bin/proxmox_backup_manager/datastore.rs b/src/bin/proxmox_backup_manager/datastore.rs
index 1922a55a2..3fbb5fe5b 100644
--- a/src/bin/proxmox_backup_manager/datastore.rs
+++ b/src/bin/proxmox_backup_manager/datastore.rs
@@ -49,6 +49,10 @@ fn list_datastores(param: Value, rpcenv: &mut dyn RpcEnvironment) -> Result<Valu
             store: {
                 schema: DATASTORE_SCHEMA,
             },
+            "output-format": {
+                schema: OUTPUT_FORMAT,
+                optional: true,
+            },
             digest: {
                 optional: true,
                 schema: PROXMOX_CONFIG_DIGEST_SCHEMA,
@@ -57,16 +61,23 @@ fn list_datastores(param: Value, rpcenv: &mut dyn RpcEnvironment) -> Result<Valu
     },
 )]
 /// 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/{store}/mount").as_str(),
+            None,
+        )
+        .await?;
+
+    view_task_result(&client, result, &output_format).await?;
 
-    crate::wait_for_local_worker(result.as_str().unwrap()).await?;
     Ok(())
 }
 
@@ -260,7 +271,8 @@ async fn update_datastore(name: String, mut param: Value) -> Result<(), Error> {
     },
 )]
 /// Try mounting a removable datastore given the UUID.
-async fn uuid_mount(param: Value, _rpcenv: &mut dyn RpcEnvironment) -> Result<Value, Error> {
+async fn uuid_mount(mut param: Value, _rpcenv: &mut dyn RpcEnvironment) -> Result<Value, Error> {
+    let output_format = extract_output_format(&mut param);
     let uuid = param["uuid"]
         .as_str()
         .ok_or_else(|| format_err!("uuid has to be specified"))?;
@@ -282,7 +294,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, &output_format).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

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

[Hannes Laimer]

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

diff --git a/www/window/SyncJobEdit.js b/www/window/SyncJobEdit.js
index 4cef9a1d1..73a489f1d 100644
--- a/www/window/SyncJobEdit.js
+++ b/www/window/SyncJobEdit.js
@@ -195,7 +195,7 @@ Ext.define('PBS.window.SyncJobEdit', {
 			xtype: 'pbsCalendarEvent',
 			name: 'schedule',
 			fieldLabel: gettext('Sync Schedule'),
-			emptyText: gettext('none (disabled)'),
+			emptyText: gettext('none'),
 			cbind: {
 			    deleteEmpty: '{!isCreate}',
 			    value: '{scheduleValue}',
@@ -451,6 +451,17 @@ Ext.define('PBS.window.SyncJobEdit', {
 			uncheckedValue: false,
 			value: false,
 		    },
+		    {
+			xtype: 'proxmoxcheckbox',
+			name: 'run-on-mount',
+			fieldLabel: gettext('Run when mounted'),
+			autoEl: {
+			    tag: 'div',
+			    'data-qtip': gettext('Run this job when a relevant removable datastore is mounted.'),
+			},
+			uncheckedValue: false,
+			value: false,
+		    },
 		],
 	    },
 	    {
-- 
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 v2 8/8] 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 9dcde6941..eb4943010 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-backup v2 4/8] api: admin: run configured sync jobs when a datastore is mounted

[Christian Ebner]

please see some comments inline

On 5/15/25 14:41, 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 | 91 +++++++++++++++++++++++++++++++++++--
>   src/api2/admin/sync.rs      |  2 +-
>   src/server/sync.rs          |  7 +--
>   3 files changed, 91 insertions(+), 9 deletions(-)
> 
> diff --git a/src/api2/admin/datastore.rs b/src/api2/admin/datastore.rs
> index 392494488..8463adb6a 100644
> --- a/src/api2/admin/datastore.rs
> +++ b/src/api2/admin/datastore.rs
> @@ -42,8 +42,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,
> @@ -2510,6 +2510,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(", ")
> +    );

nit:

above can be rewritten without cloning the job ids and `count` in-lined as:
```
     info!(
         "will run {count} sync jobs: {}",
         jobs_to_run
             .iter()
             .map(|sync_job_config| sync_job_config.id.as_str())
             .collect::<Vec<&str>>()
             .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 {

nit: this should log an error/warning and the status progress, as this 
will fail if the job lock cannot be acquired. That is something which is 
of interest for debugging.

> +            continue;
> +        };
> +        let auth_id = Authid::root_auth_id().clone();

nit: auth_id does not need to be cloned here ...

> +        info!("[{}/{count}] starting '{job_id}'...", i + 1);
> +        match crate::server::do_sync_job(
> +            job,
> +            job_config,
> +            &auth_id,

... since only passed as reference here.

> +            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}"); },

question: should this be logged as error rather than a warning ...

> +                    _abort = worker.abort_future() => bail!("aborted due to user request"),
> +                };
> +            }
> +            Err(err) => warn!("unable to start sync job {job_id} - {err}"),

... same here?

> +        }
> +    }
> +
> +    Ok(())
> +}
> +
>   #[api(
>       protected: true,
>       input: {
> @@ -2541,12 +2586,48 @@ 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(

question: is it okay to run this on the same thread here? 
`do_mount_device` does some blocking calls after all?

>           "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 (running on mount is enabled and) 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.run_on_mount.unwrap_or(false)
> +                        && (job.remote.is_none() && job.remote_store == datastore.name
> +                            || job.store == datastore.name)
> +                })
> +                .collect();
> +            if !jobs_to_run.is_empty() {

comment: an additional log info to the mount task log would be nice, so 
one sees from it as well that some sync jobs were triggered.

> +                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(())
> +        },

comment: all of above is executed within a api endpoint flagged as 
protected! This however leads to the sync job to be executed by the 
proxmox-backup-proxy, all the synced contents therefore written and 
owner by the root user instead of the backup user.

>       )?;
>   
>       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 09814ef0c..c45a8975e 100644
> --- a/src/server/sync.rs
> +++ b/src/server/sync.rs
> @@ -12,6 +12,7 @@ use futures::{future::FutureExt, select};
>   use hyper::http::StatusCode;
>   use pbs_config::BackupLockGuard;
>   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(),
> @@ -730,7 +731,7 @@ pub fn do_sync_job(
>           },
>       )?;
>   
> -    Ok(upid_str)
> +    Ok((upid_str, handle))
>   }
>   
>   pub(super) fn ignore_not_verified_or_encrypted(



_______________________________________________
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 v2 4/8] api: admin: run configured sync jobs when a datastore is mounted

[Hannes Laimer]

added some inline comments, I'll probably send a v3 addressing some of
the things you mentioned

On 5/30/25 12:02, Christian Ebner wrote:
> please see some comments inline
> 
> On 5/15/25 14:41, 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 | 91 +++++++++++++++++++++++++++++++++++--
>>   src/api2/admin/sync.rs      |  2 +-
>>   src/server/sync.rs          |  7 +--
>>   3 files changed, 91 insertions(+), 9 deletions(-)
>>
>> diff --git a/src/api2/admin/datastore.rs b/src/api2/admin/datastore.rs
>> index 392494488..8463adb6a 100644
>> --- a/src/api2/admin/datastore.rs
>> +++ b/src/api2/admin/datastore.rs
>> @@ -42,8 +42,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,
>> @@ -2510,6 +2510,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(", ")
>> +    );
> 
> nit:
> 
> above can be rewritten without cloning the job ids and `count` in-lined as:
> ```
>      info!(
>          "will run {count} sync jobs: {}",
>          jobs_to_run
>              .iter()
>              .map(|sync_job_config| sync_job_config.id.as_str())
>              .collect::<Vec<&str>>()
>              .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 {
> 
> nit: this should log an error/warning and the status progress, as this 
> will fail if the job lock cannot be acquired. That is something which is 
> of interest for debugging.
> 
makes sense, will do

>> +            continue;
>> +        };
>> +        let auth_id = Authid::root_auth_id().clone();
> 
> nit: auth_id does not need to be cloned here ...
> 
>> +        info!("[{}/{count}] starting '{job_id}'...", i + 1);
>> +        match crate::server::do_sync_job(
>> +            job,
>> +            job_config,
>> +            &auth_id,
> 
> ... since only passed as reference here.
> 
>> +            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}"); },
> 
> question: should this be logged as error rather than a warning ...
> 

hmm, maybe. But it doesn't mean anything necessarily failed, the sync
itself will continue. We'll just start the next one without waiting.

But I'm not super sure exactly when this would fail, maybe there are
some cases where an error would be warranted. But with my current
understanding it is not really.

>> +                    _abort = worker.abort_future() => bail!("aborted 
>> due to user request"),
>> +                };
>> +            }
>> +            Err(err) => warn!("unable to start sync job {job_id} - 
>> {err}"),
> 
> ... same here?
> 

in my head error means task could not continue, which is not the case
here, we'll just skip this jobs. But no hard feelings, could also make
an error here.

>> +        }
>> +    }
>> +
>> +    Ok(())
>> +}
>> +
>>   #[api(
>>       protected: true,
>>       input: {
>> @@ -2541,12 +2586,48 @@ 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(
> 
> question: is it okay to run this on the same thread here? 
> `do_mount_device` does some blocking calls after all?
> 

yes, there's no real reason to look for job configs unless mounting is
successful and done. So the blocking is not a problem, actually quite
the opposite.

>>           "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 (running on mount is enabled and) 
>> 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.run_on_mount.unwrap_or(false)
>> +                        && (job.remote.is_none() && job.remote_store 
>> == datastore.name
>> +                            || job.store == datastore.name)
>> +                })
>> +                .collect();
>> +            if !jobs_to_run.is_empty() {
> 
> comment: an additional log info to the mount task log would be nice, so 
> one sees from it as well that some sync jobs were triggered.
> 

sure

>> +                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(())
>> +        },
> 
> comment: all of above is executed within a api endpoint flagged as 
> protected! This however leads to the sync job to be executed by the 
> proxmox-backup-proxy, all the synced contents therefore written and 
> owner by the root user instead of the backup user.
> 

actually true, good catch! I didn't even check that, I just assumed we'd 
explicitly set the owner whenever we create dirs/files during sync. We 
do this for garbage collection and in basically all other places IIRC.

So, I think we should also do that for syncs, can you think of a reason
to not do that? If not, I'll prepare a patch for that.

>>       )?;
>>       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 09814ef0c..c45a8975e 100644
>> --- a/src/server/sync.rs
>> +++ b/src/server/sync.rs
>> @@ -12,6 +12,7 @@ use futures::{future::FutureExt, select};
>>   use hyper::http::StatusCode;
>>   use pbs_config::BackupLockGuard;
>>   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(),
>> @@ -730,7 +731,7 @@ pub fn do_sync_job(
>>           },
>>       )?;
>> -    Ok(upid_str)
>> +    Ok((upid_str, handle))
>>   }
>>   pub(super) fn ignore_not_verified_or_encrypted(
> 



_______________________________________________
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 v2 4/8] api: admin: run configured sync jobs when a datastore is mounted

[Christian Ebner]

On 5/30/25 13:45, Hannes Laimer wrote:
> added some inline comments, I'll probably send a v3 addressing some of
> the things you mentioned
> 
> On 5/30/25 12:02, Christian Ebner wrote:
>> please see some comments inline
>>
>> On 5/15/25 14:41, 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>
>>> +                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(())
>>> +        },
>>
>> comment: all of above is executed within a api endpoint flagged as 
>> protected! This however leads to the sync job to be executed by the 
>> proxmox-backup-proxy, all the synced contents therefore written and 
>> owner by the root user instead of the backup user.
>>
> 
> actually true, good catch! I didn't even check that, I just assumed we'd 
> explicitly set the owner whenever we create dirs/files during sync. We 
> do this for garbage collection and in basically all other places IIRC.
> 
> So, I think we should also do that for syncs, can you think of a reason
> to not do that? If not, I'll prepare a patch for that.

As discussed off-list a bit, IMHO running the whole sync job code paths 
in the privileged api server is not ideal and would weaken the privilege 
separation. But I do agree that this is already been done in some places 
and would be probably easy to approach.

An option would be to add an (unprivileged) api endpoint which allows to 
pass a list of sync jobs to be executed sequentially, e.g. `POST 
/api2/json/admin/sync/run-jobs`.

Given that we found no better approach other then the two proposed ones,
maybe somebody else has an opinion/suggestion on this?


_______________________________________________
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 v2 4/8] api: admin: run configured sync jobs when a datastore is mounted

[Hannes Laimer]

On 5/30/25 15:08, Christian Ebner wrote:
> On 5/30/25 13:45, Hannes Laimer wrote:
>> added some inline comments, I'll probably send a v3 addressing some of
>> the things you mentioned
>>
>> On 5/30/25 12:02, Christian Ebner wrote:
>>> please see some comments inline
>>>
>>> On 5/15/25 14:41, 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>
>>>> +                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(())
>>>> +        },
>>>
>>> comment: all of above is executed within a api endpoint flagged as 
>>> protected! This however leads to the sync job to be executed by the 
>>> proxmox-backup-proxy, all the synced contents therefore written and 
>>> owner by the root user instead of the backup user.
>>>
>>
>> actually true, good catch! I didn't even check that, I just assumed 
>> we'd explicitly set the owner whenever we create dirs/files during 
>> sync. We do this for garbage collection and in basically all other 
>> places IIRC.
>>
>> So, I think we should also do that for syncs, can you think of a reason
>> to not do that? If not, I'll prepare a patch for that.
> 
> As discussed off-list a bit, IMHO running the whole sync job code paths 
> in the privileged api server is not ideal and would weaken the privilege 
> separation. But I do agree that this is already been done in some places 
> and would be probably easy to approach.
> 
> An option would be to add an (unprivileged) api endpoint which allows to 
> pass a list of sync jobs to be executed sequentially, e.g. `POST /api2/ 
> json/admin/sync/run-jobs`.
> 
> Given that we found no better approach other then the two proposed ones,
> maybe somebody else has an opinion/suggestion on this?

For the sake of completeness, spoke with @Thomas off-list, and I'll use 
the existing run_sync_job endpoint to start the jobs, and the UPID to do
polling on the status. This is in line with what we do in a few places
already and it does keep privileged and non-privileged stuff strictly
separated.



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

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

[Hannes Laimer]

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

On 5/15/25 14:41, 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.
> 
> This depends on [1], without it the api process does not drop the file
> handle on the `.lock` file which leads to the datastore being not
> unmountable after sync jobs were triggered. (now thinking about it,
> might have made sense to include it in this series directly, but it also
> does make sense on its own)
> 
> v2, thanks @Chris:
> - rebased onto master
> - improve some docstrings
> - move/fix config flag
> - drop not-needed changes for the manager binary
> - ui: move checkbox to advenced section + don't clear schedule field
> - fix test
> - actually check the configured flag when deciding if a job should
>    run...
> 
> [1] https://lore.proxmox.com/pbs-devel/20250512125933.156192-1-h.laimer@proxmox.com/T/#u
> 
> 
> proxmox:
> Hannes Laimer (2):
>    rest-server: add function that returns a join handle for spawn
>    pbs-api-types: add run-on-mount flag to SyncJobConfig
> 
>   pbs-api-types/src/jobs.rs              |  8 ++++++++
>   proxmox-rest-server/src/worker_task.rs | 24 ++++++++++++++++++++++--
>   2 files changed, 30 insertions(+), 2 deletions(-)
> 
> proxmox-backup:
> Hannes Laimer (6):
>    api: config: sync: update run-on-mount correctly
>    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
> 
>   src/api2/admin/datastore.rs                 | 97 +++++++++++++++++++--
>   src/api2/admin/sync.rs                      |  2 +-
>   src/api2/config/sync.rs                     |  9 ++
>   src/bin/proxmox_backup_manager/datastore.rs | 41 ++++++---
>   src/server/sync.rs                          |  7 +-
>   www/Utils.js                                |  1 +
>   www/window/SyncJobEdit.js                   | 13 ++-
>   7 files changed, 148 insertions(+), 22 deletions(-)
> 



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