* [pbs-devel] [PATCH proxmox v2 1/8] rest-server: add function that returns a join handle for spawn
2025-05-15 12:41 [pbs-devel] [PATCH proxmox/proxmox-backup v2 0/8] trigger sync jobs on mount Hannes Laimer
@ 2025-05-15 12:41 ` Hannes Laimer
2025-05-15 12:41 ` [pbs-devel] [PATCH proxmox v2 2/8] pbs-api-types: add run-on-mount flag to SyncJobConfig Hannes Laimer
` (7 subsequent siblings)
8 siblings, 0 replies; 14+ messages in thread
From: Hannes Laimer @ 2025-05-15 12:41 UTC (permalink / raw)
To: pbs-devel
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
^ permalink raw reply [flat|nested] 14+ messages in thread* [pbs-devel] [PATCH proxmox v2 2/8] pbs-api-types: add run-on-mount flag to SyncJobConfig
2025-05-15 12:41 [pbs-devel] [PATCH proxmox/proxmox-backup v2 0/8] trigger sync jobs on mount Hannes Laimer
2025-05-15 12:41 ` [pbs-devel] [PATCH proxmox v2 1/8] rest-server: add function that returns a join handle for spawn Hannes Laimer
@ 2025-05-15 12:41 ` Hannes Laimer
2025-05-15 12:41 ` [pbs-devel] [PATCH proxmox-backup v2 3/8] api: config: sync: update run-on-mount correctly Hannes Laimer
` (6 subsequent siblings)
8 siblings, 0 replies; 14+ messages in thread
From: Hannes Laimer @ 2025-05-15 12:41 UTC (permalink / raw)
To: pbs-devel
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
^ permalink raw reply [flat|nested] 14+ messages in thread* [pbs-devel] [PATCH proxmox-backup v2 3/8] api: config: sync: update run-on-mount correctly
2025-05-15 12:41 [pbs-devel] [PATCH proxmox/proxmox-backup v2 0/8] trigger sync jobs on mount Hannes Laimer
2025-05-15 12:41 ` [pbs-devel] [PATCH proxmox v2 1/8] rest-server: add function that returns a join handle for spawn Hannes Laimer
2025-05-15 12:41 ` [pbs-devel] [PATCH proxmox v2 2/8] pbs-api-types: add run-on-mount flag to SyncJobConfig Hannes Laimer
@ 2025-05-15 12:41 ` Hannes Laimer
2025-05-15 12:41 ` [pbs-devel] [PATCH proxmox-backup v2 4/8] api: admin: run configured sync jobs when a datastore is mounted Hannes Laimer
` (5 subsequent siblings)
8 siblings, 0 replies; 14+ messages in thread
From: Hannes Laimer @ 2025-05-15 12:41 UTC (permalink / raw)
To: pbs-devel
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
^ permalink raw reply [flat|nested] 14+ messages in thread* [pbs-devel] [PATCH proxmox-backup v2 4/8] api: admin: run configured sync jobs when a datastore is mounted
2025-05-15 12:41 [pbs-devel] [PATCH proxmox/proxmox-backup v2 0/8] trigger sync jobs on mount Hannes Laimer
` (2 preceding siblings ...)
2025-05-15 12:41 ` [pbs-devel] [PATCH proxmox-backup v2 3/8] api: config: sync: update run-on-mount correctly Hannes Laimer
@ 2025-05-15 12:41 ` Hannes Laimer
2025-05-30 10:02 ` Christian Ebner
2025-05-15 12:41 ` [pbs-devel] [PATCH proxmox-backup v2 5/8] api: admin: trigger sync jobs only on datastore mount Hannes Laimer
` (4 subsequent siblings)
8 siblings, 1 reply; 14+ messages in thread
From: Hannes Laimer @ 2025-05-15 12:41 UTC (permalink / raw)
To: pbs-devel
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
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [pbs-devel] [PATCH proxmox-backup v2 4/8] api: admin: run configured sync jobs when a datastore is mounted
2025-05-15 12:41 ` [pbs-devel] [PATCH proxmox-backup v2 4/8] api: admin: run configured sync jobs when a datastore is mounted Hannes Laimer
@ 2025-05-30 10:02 ` Christian Ebner
2025-05-30 11:45 ` Hannes Laimer
0 siblings, 1 reply; 14+ messages in thread
From: Christian Ebner @ 2025-05-30 10:02 UTC (permalink / raw)
To: Proxmox Backup Server development discussion, Hannes Laimer
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
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [pbs-devel] [PATCH proxmox-backup v2 4/8] api: admin: run configured sync jobs when a datastore is mounted
2025-05-30 10:02 ` Christian Ebner
@ 2025-05-30 11:45 ` Hannes Laimer
2025-05-30 13:08 ` Christian Ebner
0 siblings, 1 reply; 14+ messages in thread
From: Hannes Laimer @ 2025-05-30 11:45 UTC (permalink / raw)
To: Christian Ebner, Proxmox Backup Server development discussion
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
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [pbs-devel] [PATCH proxmox-backup v2 4/8] api: admin: run configured sync jobs when a datastore is mounted
2025-05-30 11:45 ` Hannes Laimer
@ 2025-05-30 13:08 ` Christian Ebner
2025-06-04 12:22 ` Hannes Laimer
0 siblings, 1 reply; 14+ messages in thread
From: Christian Ebner @ 2025-05-30 13:08 UTC (permalink / raw)
To: Hannes Laimer, Proxmox Backup Server development discussion
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
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [pbs-devel] [PATCH proxmox-backup v2 4/8] api: admin: run configured sync jobs when a datastore is mounted
2025-05-30 13:08 ` Christian Ebner
@ 2025-06-04 12:22 ` Hannes Laimer
0 siblings, 0 replies; 14+ messages in thread
From: Hannes Laimer @ 2025-06-04 12:22 UTC (permalink / raw)
To: Christian Ebner, Proxmox Backup Server development discussion
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* [pbs-devel] [PATCH proxmox-backup v2 5/8] api: admin: trigger sync jobs only on datastore mount
2025-05-15 12:41 [pbs-devel] [PATCH proxmox/proxmox-backup v2 0/8] trigger sync jobs on mount Hannes Laimer
` (3 preceding siblings ...)
2025-05-15 12:41 ` [pbs-devel] [PATCH proxmox-backup v2 4/8] api: admin: run configured sync jobs when a datastore is mounted Hannes Laimer
@ 2025-05-15 12:41 ` Hannes Laimer
2025-05-15 12:41 ` [pbs-devel] [PATCH proxmox-backup v2 6/8] bin: manager: run uuid_mount/mount tasks on the proxy Hannes Laimer
` (3 subsequent siblings)
8 siblings, 0 replies; 14+ messages in thread
From: Hannes Laimer @ 2025-05-15 12:41 UTC (permalink / raw)
To: pbs-devel
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
^ permalink raw reply [flat|nested] 14+ messages in thread* [pbs-devel] [PATCH proxmox-backup v2 6/8] bin: manager: run uuid_mount/mount tasks on the proxy
2025-05-15 12:41 [pbs-devel] [PATCH proxmox/proxmox-backup v2 0/8] trigger sync jobs on mount Hannes Laimer
` (4 preceding siblings ...)
2025-05-15 12:41 ` [pbs-devel] [PATCH proxmox-backup v2 5/8] api: admin: trigger sync jobs only on datastore mount Hannes Laimer
@ 2025-05-15 12:41 ` Hannes Laimer
2025-05-15 12:41 ` [pbs-devel] [PATCH proxmox-backup v2 7/8] ui: add run-on-mount checkbox to SyncJob form Hannes Laimer
` (2 subsequent siblings)
8 siblings, 0 replies; 14+ messages in thread
From: Hannes Laimer @ 2025-05-15 12:41 UTC (permalink / raw)
To: pbs-devel
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
^ permalink raw reply [flat|nested] 14+ messages in thread* [pbs-devel] [PATCH proxmox-backup v2 7/8] ui: add run-on-mount checkbox to SyncJob form
2025-05-15 12:41 [pbs-devel] [PATCH proxmox/proxmox-backup v2 0/8] trigger sync jobs on mount Hannes Laimer
` (5 preceding siblings ...)
2025-05-15 12:41 ` [pbs-devel] [PATCH proxmox-backup v2 6/8] bin: manager: run uuid_mount/mount tasks on the proxy Hannes Laimer
@ 2025-05-15 12:41 ` Hannes Laimer
2025-05-15 12:41 ` [pbs-devel] [PATCH proxmox-backup v2 8/8] ui: add task title for triggering sync jobs Hannes Laimer
2025-06-04 12:32 ` [pbs-devel] superseded: Re: [PATCH proxmox/proxmox-backup v2 0/8] trigger sync jobs on mount Hannes Laimer
8 siblings, 0 replies; 14+ messages in thread
From: Hannes Laimer @ 2025-05-15 12:41 UTC (permalink / raw)
To: pbs-devel
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
^ permalink raw reply [flat|nested] 14+ messages in thread* [pbs-devel] [PATCH proxmox-backup v2 8/8] ui: add task title for triggering sync jobs
2025-05-15 12:41 [pbs-devel] [PATCH proxmox/proxmox-backup v2 0/8] trigger sync jobs on mount Hannes Laimer
` (6 preceding siblings ...)
2025-05-15 12:41 ` [pbs-devel] [PATCH proxmox-backup v2 7/8] ui: add run-on-mount checkbox to SyncJob form Hannes Laimer
@ 2025-05-15 12:41 ` Hannes Laimer
2025-06-04 12:32 ` [pbs-devel] superseded: Re: [PATCH proxmox/proxmox-backup v2 0/8] trigger sync jobs on mount Hannes Laimer
8 siblings, 0 replies; 14+ messages in thread
From: Hannes Laimer @ 2025-05-15 12:41 UTC (permalink / raw)
To: pbs-devel
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
^ permalink raw reply [flat|nested] 14+ messages in thread* [pbs-devel] superseded: Re: [PATCH proxmox/proxmox-backup v2 0/8] trigger sync jobs on mount
2025-05-15 12:41 [pbs-devel] [PATCH proxmox/proxmox-backup v2 0/8] trigger sync jobs on mount Hannes Laimer
` (7 preceding siblings ...)
2025-05-15 12:41 ` [pbs-devel] [PATCH proxmox-backup v2 8/8] ui: add task title for triggering sync jobs Hannes Laimer
@ 2025-06-04 12:32 ` Hannes Laimer
8 siblings, 0 replies; 14+ messages in thread
From: Hannes Laimer @ 2025-06-04 12:32 UTC (permalink / raw)
To: pbs-devel
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
^ permalink raw reply [flat|nested] 14+ messages in thread