[pbs-devel] [PATCH proxmox-backup v2] datastore: remove datastore from internal cache based on maintenance mode

[pbs-devel] [PATCH proxmox-backup v2] datastore: remove datastore from internal cache based on maintenance mode

[Hannes Laimer]

We keep a DataStore cache, so ChunkStore's and lock files are kept by
the proxy process and don't have to be reopened every time. However, for
specific maintenance modes, e.g. 'offline', our process should not keep
file in that datastore open. This clears the cache entry of a datastore
if it is in a specific maintanance mode and the last task finished, which
also drops any files still open by the process.

Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
Tested-by: Gabriel Goller <g.goller@proxmox.com>
Reviewed-by: Gabriel Goller <g.goller@proxmox.com>
---

v2, thanks @Gabriel:
 - improve comments
 - remove not needed &'s and .clone()'s

 pbs-api-types/src/maintenance.rs   |  6 +++++
 pbs-datastore/src/datastore.rs     | 41 ++++++++++++++++++++++++++++--
 pbs-datastore/src/task_tracking.rs | 23 ++++++++++-------
 src/api2/config/datastore.rs       | 18 +++++++++++++
 src/bin/proxmox-backup-proxy.rs    |  8 ++++++
 5 files changed, 85 insertions(+), 11 deletions(-)

diff --git a/pbs-api-types/src/maintenance.rs b/pbs-api-types/src/maintenance.rs
index 1b03ca94..a1564031 100644
--- a/pbs-api-types/src/maintenance.rs
+++ b/pbs-api-types/src/maintenance.rs
@@ -77,6 +77,12 @@ pub struct MaintenanceMode {
 }
 
 impl MaintenanceMode {
+    /// Used for deciding whether the datastore is cleared from the internal cache after the last
+    /// task finishes, so all open files are closed.
+    pub fn clear_from_cache(&self) -> bool {
+        self.ty == MaintenanceType::Offline
+    }
+
     pub fn check(&self, operation: Option<Operation>) -> Result<(), Error> {
         if self.ty == MaintenanceType::Delete {
             bail!("datastore is being deleted");
diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/datastore.rs
index 2f0e5279..f26dff83 100644
--- a/pbs-datastore/src/datastore.rs
+++ b/pbs-datastore/src/datastore.rs
@@ -104,8 +104,27 @@ impl Clone for DataStore {
 impl Drop for DataStore {
     fn drop(&mut self) {
         if let Some(operation) = self.operation {
-            if let Err(e) = update_active_operations(self.name(), operation, -1) {
-                log::error!("could not update active operations - {}", e);
+            let mut last_task = false;
+            match update_active_operations(self.name(), operation, -1) {
+                Err(e) => log::error!("could not update active operations - {}", e),
+                Ok(updated_operations) => {
+                    last_task = updated_operations.read + updated_operations.write == 0;
+                }
+            }
+
+            // remove datastore from cache iff 
+            //  - last task finished, and
+            //  - datastore is in a maintenance mode that mandates it
+            let remove_from_cache = last_task
+                && pbs_config::datastore::config()
+                    .and_then(|(s, _)| s.lookup::<DataStoreConfig>("datastore", self.name()))
+                    .map_or(false, |c| {
+                        c.get_maintenance_mode()
+                            .map_or(false, |m| m.clear_from_cache())
+                    });
+
+            if remove_from_cache {
+                DATASTORE_MAP.lock().unwrap().remove(self.name());
             }
         }
     }
@@ -193,6 +212,24 @@ impl DataStore {
         Ok(())
     }
 
+    /// trigger clearing cache entries based on maintenance mode. Entries will only
+    /// be cleared iff there is no other task running, if there is, the end of the
+    /// last running task will trigger the clearing of the cache entry.
+    pub fn update_datastore_cache() -> Result<(), Error> {
+        let (config, _digest) = pbs_config::datastore::config()?;
+        for (store, (_, _)) in &config.sections {
+            let datastore: DataStoreConfig = config.lookup("datastore", store)?;
+            if datastore
+                .get_maintenance_mode()
+                .map_or(false, |m| m.clear_from_cache())
+            {
+                let _ = DataStore::lookup_datastore(store, Some(Operation::Lookup));
+            }
+        }
+
+        Ok(())
+    }
+
     /// Open a raw database given a name and a path.
     ///
     /// # Safety
diff --git a/pbs-datastore/src/task_tracking.rs b/pbs-datastore/src/task_tracking.rs
index 18fbd4ba..ec06a0bc 100644
--- a/pbs-datastore/src/task_tracking.rs
+++ b/pbs-datastore/src/task_tracking.rs
@@ -91,15 +91,23 @@ pub fn get_active_operations_locked(
     Ok((data, lock.unwrap()))
 }
 
-pub fn update_active_operations(name: &str, operation: Operation, count: i64) -> Result<(), Error> {
+pub fn update_active_operations(
+    name: &str,
+    operation: Operation,
+    count: i64,
+) -> Result<ActiveOperationStats, Error> {
     let path = PathBuf::from(format!("{}/{}", crate::ACTIVE_OPERATIONS_DIR, name));
 
     let (_lock, options) = open_lock_file(name)?;
 
     let pid = std::process::id();
     let starttime = procfs::PidStat::read_from_pid(Pid::from_raw(pid as pid_t))?.starttime;
-    let mut updated = false;
 
+    let mut updated_active_operations = match operation {
+        Operation::Read => ActiveOperationStats { read: 1, write: 0 },
+        Operation::Write => ActiveOperationStats { read: 0, write: 1 },
+        Operation::Lookup => ActiveOperationStats { read: 0, write: 0 },
+    };
     let mut updated_tasks: Vec<TaskOperations> = match file_read_optional_string(&path)? {
         Some(data) => serde_json::from_str::<Vec<TaskOperations>>(&data)?
             .iter_mut()
@@ -108,12 +116,12 @@ pub fn update_active_operations(name: &str, operation: Operation, count: i64) ->
                     Some(stat) if pid == task.pid && stat.starttime != task.starttime => None,
                     Some(_) => {
                         if pid == task.pid {
-                            updated = true;
                             match operation {
                                 Operation::Read => task.active_operations.read += count,
                                 Operation::Write => task.active_operations.write += count,
                                 Operation::Lookup => (), // no IO must happen there
                             };
+                            updated_active_operations = task.active_operations;
                         }
                         Some(task.clone())
                     }
@@ -124,15 +132,11 @@ pub fn update_active_operations(name: &str, operation: Operation, count: i64) ->
         None => Vec::new(),
     };
 
-    if !updated {
+    if updated_tasks.is_empty() {
         updated_tasks.push(TaskOperations {
             pid,
             starttime,
-            active_operations: match operation {
-                Operation::Read => ActiveOperationStats { read: 1, write: 0 },
-                Operation::Write => ActiveOperationStats { read: 0, write: 1 },
-                Operation::Lookup => ActiveOperationStats { read: 0, write: 0 },
-            },
+            active_operations: updated_active_operations,
         })
     }
     replace_file(
@@ -141,4 +145,5 @@ pub fn update_active_operations(name: &str, operation: Operation, count: i64) ->
         options,
         false,
     )
+    .map(|_| updated_active_operations)
 }
diff --git a/src/api2/config/datastore.rs b/src/api2/config/datastore.rs
index d571334c..519fadc9 100644
--- a/src/api2/config/datastore.rs
+++ b/src/api2/config/datastore.rs
@@ -389,7 +389,9 @@ pub fn update_datastore(
         data.tuning = update.tuning;
     }
 
+    let mut maintenance_mode_changed = false;
     if update.maintenance_mode.is_some() {
+        maintenance_mode_changed = data.maintenance_mode != update.maintenance_mode;
         data.maintenance_mode = update.maintenance_mode;
     }
 
@@ -403,6 +405,22 @@ pub fn update_datastore(
         jobstate::update_job_last_run_time("garbage_collection", &name)?;
     }
 
+    // tell the proxy it might have to clear a cache entry
+    if maintenance_mode_changed {
+        tokio::spawn(async {
+            if let Ok(proxy_pid) =
+                proxmox_rest_server::read_pid(pbs_buildcfg::PROXMOX_BACKUP_PROXY_PID_FN)
+            {
+                let sock = proxmox_rest_server::ctrl_sock_from_pid(proxy_pid);
+                let _ = proxmox_rest_server::send_raw_command(
+                    sock,
+                    "{\"command\":\"update-datastore-cache\"}\n",
+                )
+                .await;
+            }
+        });
+    }
+
     Ok(())
 }
 
diff --git a/src/bin/proxmox-backup-proxy.rs b/src/bin/proxmox-backup-proxy.rs
index 5d5bcdd3..d5e3e810 100644
--- a/src/bin/proxmox-backup-proxy.rs
+++ b/src/bin/proxmox-backup-proxy.rs
@@ -289,6 +289,14 @@ async fn run() -> Result<(), Error> {
         Ok(Value::Null)
     })?;
 
+    // clear cache entries for datastores that are in a specific maintenance mode
+    command_sock.register_command("update-datastore-cache".to_string(), |_value| {
+        if let Err(err) = DataStore::update_datastore_cache() {
+            log::error!("could not trigger update datastore cache: {err}");
+        }
+        Ok(Value::Null)
+    })?;
+
     let connections = proxmox_rest_server::connection::AcceptBuilder::new()
         .debug(debug)
         .rate_limiter_lookup(Arc::new(lookup_rate_limiter))
-- 
2.39.2

Re: [pbs-devel] [PATCH proxmox-backup v2] datastore: remove datastore from internal cache based on maintenance mode

[Thomas Lamprecht]

Am 01/03/2024 um 16:03 schrieb Hannes Laimer:
> We keep a DataStore cache, so ChunkStore's and lock files are kept by
> the proxy process and don't have to be reopened every time. However, for
> specific maintenance modes, e.g. 'offline', our process should not keep
> file in that datastore open. This clears the cache entry of a datastore
> if it is in a specific maintanance mode and the last task finished, which
> also drops any files still open by the process.

One always asks themselves if command sockets are the right approach, but
for this it seems alright.

Some code style comments inline.

> Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
> Tested-by: Gabriel Goller <g.goller@proxmox.com>
> Reviewed-by: Gabriel Goller <g.goller@proxmox.com>
> ---
> 
> v2, thanks @Gabriel:
>  - improve comments
>  - remove not needed &'s and .clone()'s
> 
>  pbs-api-types/src/maintenance.rs   |  6 +++++
>  pbs-datastore/src/datastore.rs     | 41 ++++++++++++++++++++++++++++--
>  pbs-datastore/src/task_tracking.rs | 23 ++++++++++-------
>  src/api2/config/datastore.rs       | 18 +++++++++++++
>  src/bin/proxmox-backup-proxy.rs    |  8 ++++++
>  5 files changed, 85 insertions(+), 11 deletions(-)
> 
> diff --git a/pbs-api-types/src/maintenance.rs b/pbs-api-types/src/maintenance.rs
> index 1b03ca94..a1564031 100644
> --- a/pbs-api-types/src/maintenance.rs
> +++ b/pbs-api-types/src/maintenance.rs
> @@ -77,6 +77,12 @@ pub struct MaintenanceMode {
>  }
>  
>  impl MaintenanceMode {
> +    /// Used for deciding whether the datastore is cleared from the internal cache after the last
> +    /// task finishes, so all open files are closed.
> +    pub fn clear_from_cache(&self) -> bool {

that function name makes it sound like calling it does actively clears it,
but this is only for checking if a required condition for clearing is met.

So maybe use a name that better convey that and maybe even avoid coupling
this to an action that a user of ours executes, as this might have some use
for other call sites too.

From top of my head one could use `is_offline` as name, adding a note to
the doc-comment that this is e.g. used to check if a datastore can be
removed from the cache would still be fine though.

> +        self.ty == MaintenanceType::Offline
> +    }
> +
>      pub fn check(&self, operation: Option<Operation>) -> Result<(), Error> {
>          if self.ty == MaintenanceType::Delete {
>              bail!("datastore is being deleted");
> diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/datastore.rs
> index 2f0e5279..f26dff83 100644
> --- a/pbs-datastore/src/datastore.rs
> +++ b/pbs-datastore/src/datastore.rs
> @@ -104,8 +104,27 @@ impl Clone for DataStore {
>  impl Drop for DataStore {
>      fn drop(&mut self) {
>          if let Some(operation) = self.operation {
> -            if let Err(e) = update_active_operations(self.name(), operation, -1) {
> -                log::error!("could not update active operations - {}", e);
> +            let mut last_task = false;
> +            match update_active_operations(self.name(), operation, -1) {
> +                Err(e) => log::error!("could not update active operations - {}", e),
> +                Ok(updated_operations) => {
> +                    last_task = updated_operations.read + updated_operations.write == 0;
> +                }
> +            }
> +
> +            // remove datastore from cache iff 
> +            //  - last task finished, and
> +            //  - datastore is in a maintenance mode that mandates it
> +            let remove_from_cache = last_task
> +                && pbs_config::datastore::config()
> +                    .and_then(|(s, _)| s.lookup::<DataStoreConfig>("datastore", self.name()))
> +                    .map_or(false, |c| {
> +                        c.get_maintenance_mode()
> +                            .map_or(false, |m| m.clear_from_cache())
> +                    });
> +
> +            if remove_from_cache {
> +                DATASTORE_MAP.lock().unwrap().remove(self.name());
>              }
>          }
>      }
> @@ -193,6 +212,24 @@ impl DataStore {
>          Ok(())
>      }
>  
> +    /// trigger clearing cache entries based on maintenance mode. Entries will only
> +    /// be cleared iff there is no other task running, if there is, the end of the
> +    /// last running task will trigger the clearing of the cache entry.
> +    pub fn update_datastore_cache() -> Result<(), Error> {

why does this work on all but not a single datastore, after all we always want to
remove a specific one?

> +        let (config, _digest) = pbs_config::datastore::config()?;
> +        for (store, (_, _)) in &config.sections {
> +            let datastore: DataStoreConfig = config.lookup("datastore", store)?;
> +            if datastore
> +                .get_maintenance_mode()
> +                .map_or(false, |m| m.clear_from_cache())
> +            {
> +                let _ = DataStore::lookup_datastore(store, Some(Operation::Lookup));

A comment that the actual removal from the cache happens through the drop handler
would be good, as this is a bit to subtle for my taste, if one stumbles over this
in a few months down the line it might cause a bit to much easily to avoid head
scratching...

Alternatively, factor the actual check-maintenance-mode-and-remove-from-cache out
of the drop handler and call that explicit here, all you need of outside info is
the name there anyway.

Re: [pbs-devel] [PATCH proxmox-backup v2] datastore: remove datastore from internal cache based on maintenance mode

[Hannes Laimer]

On Mon Mar 4, 2024 at 11:42 AM CET, Thomas Lamprecht wrote:
> Am 01/03/2024 um 16:03 schrieb Hannes Laimer:
> > We keep a DataStore cache, so ChunkStore's and lock files are kept by
> > the proxy process and don't have to be reopened every time. However, for
> > specific maintenance modes, e.g. 'offline', our process should not keep
> > file in that datastore open. This clears the cache entry of a datastore
> > if it is in a specific maintanance mode and the last task finished, which
> > also drops any files still open by the process.
>
> One always asks themselves if command sockets are the right approach, but
> for this it seems alright.
>
> Some code style comments inline.
>
> > Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
> > Tested-by: Gabriel Goller <g.goller@proxmox.com>
> > Reviewed-by: Gabriel Goller <g.goller@proxmox.com>
> > ---
> > 
> > v2, thanks @Gabriel:
> >  - improve comments
> >  - remove not needed &'s and .clone()'s
> > 
> >  pbs-api-types/src/maintenance.rs   |  6 +++++
> >  pbs-datastore/src/datastore.rs     | 41 ++++++++++++++++++++++++++++--
> >  pbs-datastore/src/task_tracking.rs | 23 ++++++++++-------
> >  src/api2/config/datastore.rs       | 18 +++++++++++++
> >  src/bin/proxmox-backup-proxy.rs    |  8 ++++++
> >  5 files changed, 85 insertions(+), 11 deletions(-)
> > 
> > diff --git a/pbs-api-types/src/maintenance.rs b/pbs-api-types/src/maintenance.rs
> > index 1b03ca94..a1564031 100644
> > --- a/pbs-api-types/src/maintenance.rs
> > +++ b/pbs-api-types/src/maintenance.rs
> > @@ -77,6 +77,12 @@ pub struct MaintenanceMode {
> >  }
> >  
> >  impl MaintenanceMode {
> > +    /// Used for deciding whether the datastore is cleared from the internal cache after the last
> > +    /// task finishes, so all open files are closed.
> > +    pub fn clear_from_cache(&self) -> bool {
>
> that function name makes it sound like calling it does actively clears it,
> but this is only for checking if a required condition for clearing is met.
>
> So maybe use a name that better convey that and maybe even avoid coupling
> this to an action that a user of ours executes, as this might have some use
> for other call sites too.
>
> From top of my head one could use `is_offline` as name, adding a note to
> the doc-comment that this is e.g. used to check if a datastore can be
> removed from the cache would still be fine though.
>

I agree, the name is somewhat misleading. The idea was to make it easy
to potentially add more modes here in the future, so maybe something
a little more general like `is_accessible` would make sense?

> > +        self.ty == MaintenanceType::Offline
> > +    }
> > +
> >      pub fn check(&self, operation: Option<Operation>) -> Result<(), Error> {
> >          if self.ty == MaintenanceType::Delete {
> >              bail!("datastore is being deleted");
> > diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/datastore.rs
> > index 2f0e5279..f26dff83 100644
> > --- a/pbs-datastore/src/datastore.rs
> > +++ b/pbs-datastore/src/datastore.rs
> > @@ -104,8 +104,27 @@ impl Clone for DataStore {
> >  impl Drop for DataStore {
> >      fn drop(&mut self) {
> >          if let Some(operation) = self.operation {
> > -            if let Err(e) = update_active_operations(self.name(), operation, -1) {
> > -                log::error!("could not update active operations - {}", e);
> > +            let mut last_task = false;
> > +            match update_active_operations(self.name(), operation, -1) {
> > +                Err(e) => log::error!("could not update active operations - {}", e),
> > +                Ok(updated_operations) => {
> > +                    last_task = updated_operations.read + updated_operations.write == 0;
> > +                }
> > +            }
> > +
> > +            // remove datastore from cache iff 
> > +            //  - last task finished, and
> > +            //  - datastore is in a maintenance mode that mandates it
> > +            let remove_from_cache = last_task
> > +                && pbs_config::datastore::config()
> > +                    .and_then(|(s, _)| s.lookup::<DataStoreConfig>("datastore", self.name()))
> > +                    .map_or(false, |c| {
> > +                        c.get_maintenance_mode()
> > +                            .map_or(false, |m| m.clear_from_cache())
> > +                    });
> > +
> > +            if remove_from_cache {
> > +                DATASTORE_MAP.lock().unwrap().remove(self.name());
> >              }
> >          }
> >      }
> > @@ -193,6 +212,24 @@ impl DataStore {
> >          Ok(())
> >      }
> >  
> > +    /// trigger clearing cache entries based on maintenance mode. Entries will only
> > +    /// be cleared iff there is no other task running, if there is, the end of the
> > +    /// last running task will trigger the clearing of the cache entry.
> > +    pub fn update_datastore_cache() -> Result<(), Error> {
>
> why does this work on all but not a single datastore, after all we always want to
> remove a specific one?
>

Actually just missed that our command_socket also does args, will update
this in v3.

> > +        let (config, _digest) = pbs_config::datastore::config()?;
> > +        for (store, (_, _)) in &config.sections {
> > +            let datastore: DataStoreConfig = config.lookup("datastore", store)?;
> > +            if datastore
> > +                .get_maintenance_mode()
> > +                .map_or(false, |m| m.clear_from_cache())
> > +            {
> > +                let _ = DataStore::lookup_datastore(store, Some(Operation::Lookup));
>
> A comment that the actual removal from the cache happens through the drop handler
> would be good, as this is a bit to subtle for my taste, if one stumbles over this
> in a few months down the line it might cause a bit to much easily to avoid head
> scratching...
>
> Alternatively, factor the actual check-maintenance-mode-and-remove-from-cache out
> of the drop handler and call that explicit here, all you need of outside info is
> the name there anyway.

I think that would entail having to open the file twice in the drop
handler, once for updating it, and once for reading it. But just
reading it here and explicitly clearing it from the cache seems
reasonable, it makes it way clrearer what's happening. I'll change that
in a v3.

Thanks for the review!

Re: [pbs-devel] [PATCH proxmox-backup v2] datastore: remove datastore from internal cache based on maintenance mode

[Thomas Lamprecht]

Am 04/03/2024 um 12:12 schrieb Hannes Laimer:
> On Mon Mar 4, 2024 at 11:42 AM CET, Thomas Lamprecht wrote:
>> Am 01/03/2024 um 16:03 schrieb Hannes Laimer:
>>> @@ -77,6 +77,12 @@ pub struct MaintenanceMode {
>>>  }
>>>  
>>>  impl MaintenanceMode {
>>> +    /// Used for deciding whether the datastore is cleared from the internal cache after the last
>>> +    /// task finishes, so all open files are closed.
>>> +    pub fn clear_from_cache(&self) -> bool {
>>
>> that function name makes it sound like calling it does actively clears it,
>> but this is only for checking if a required condition for clearing is met.
>>
>> So maybe use a name that better convey that and maybe even avoid coupling
>> this to an action that a user of ours executes, as this might have some use
>> for other call sites too.
>>
>> From top of my head one could use `is_offline` as name, adding a note to
>> the doc-comment that this is e.g. used to check if a datastore can be
>> removed from the cache would still be fine though.
>>
> 
> I agree, the name is somewhat misleading. The idea was to make it easy
> to potentially add more modes here in the future, so maybe something
> a little more general like `is_accessible` would make sense?

is_accessible is the negative of is_offline, can be fine, but if you're
explicitly check for != offline it could be better to avoid the double
negative for now, and whenever this is extended see if there's a better
fitting name for the existing and new uses cases.

in short, can be fine, but personally I'd prefer is_offline for this
use case.

>>> +        self.ty == MaintenanceType::Offline
>>> +    }
>>> +
>>>      pub fn check(&self, operation: Option<Operation>) -> Result<(), Error> {
>>>          if self.ty == MaintenanceType::Delete {
>>>              bail!("datastore is being deleted");


>>> +        let (config, _digest) = pbs_config::datastore::config()?;
>>> +        for (store, (_, _)) in &config.sections {
>>> +            let datastore: DataStoreConfig = config.lookup("datastore", store)?;
>>> +            if datastore
>>> +                .get_maintenance_mode()
>>> +                .map_or(false, |m| m.clear_from_cache())
>>> +            {
>>> +                let _ = DataStore::lookup_datastore(store, Some(Operation::Lookup));
>>
>> A comment that the actual removal from the cache happens through the drop handler
>> would be good, as this is a bit to subtle for my taste, if one stumbles over this
>> in a few months down the line it might cause a bit to much easily to avoid head
>> scratching...
>>
>> Alternatively, factor the actual check-maintenance-mode-and-remove-from-cache out
>> of the drop handler and call that explicit here, all you need of outside info is
>> the name there anyway.
> 
> I think that would entail having to open the file twice in the drop
> handler, once for updating it, and once for reading it. But just
> reading it here and explicitly clearing it from the cache seems
> reasonable, it makes it way clrearer what's happening. I'll change that
> in a v3.


Yeah, but not a must IMO.
You can also just add a comment here about reusing the drop handler for
its cache-dropping handling, it just should be conveyed rather explicit.