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

[Hannes Laimer <h.laimer@proxmox.com>]

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

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

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



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