[pve-devel] [PATCH v2 storage/proxmox{, -perl-rs} 0/7] cache storage plugin status for pvestatd/API status update calls

[pve-devel] [PATCH v2 storage/proxmox{, -perl-rs} 0/7] cache storage plugin status for pvestatd/API status update calls

[Lukas Wagner]

This patch series introduces a caching mechanism for expensive status
update calls made from pvestatd.

As a first step, I introduced the cache to the arguably
most expensive call, namely `storage_info` from pve-storage. Instead
of caching the results of the `storage_info` function as a whole, we
only cache the results from status updates from individual storage
plugins.

Regarding the cache implementation: I really tried it keep it as simple
as possible. Cached values are not queried terribly often and are
rather small, so I ended up with with simply creating a json-file per
cache key in a directory in tmpfs.

Changes from v1 -> v2:
  - Incorporated changes from the first review (thx @Wolfgang, thx @Max)
  - Made sure to invalidate the cache for storage plugin if necessary:
     - When changing storage config in some way
     - When allocating volumes
     - When uploading ISOs/Images (might change the amount of free space)
  - This allows to increase the expiration time - I've set it now to 60s
  - Added a flag to storage status API/CLI that allows one to ignore cached values,
    in case somebody needs up-to-date values at all cost (--ignore-cache)
  - Added functions that allow the user of proxmox-cache to lock cache entries
    (locking is accomplished using lock files and `flock`)
  - Added get_or_update helper - this one tries to get the cached value, and if 
    it does not exist/is expired computes the new value from a closure/function
    passed in as a parameter, all while holding the lock on the cache entry.
    Locking while updating the value eliminates possible race conditions if 
    multiple processes want to update the value at the same time.

Versions of this patch series:
  - v1: https://lists.proxmox.com/pipermail/pve-devel/2023-August/058806.html



proxmox:

Lukas Wagner (5):
  sys: fs: remove unnecessary clippy allow directive
  sys: fs: let CreateOptions::apply_to take RawFd instead of File
  sys: fs: use inline formatting for bail! macro
  sys: add make_tmp_dir
  cache: add new crate 'proxmox-shared-cache'

 Cargo.toml                                   |   1 +
 proxmox-shared-cache/Cargo.toml              |  18 +
 proxmox-shared-cache/debian/changelog        |   5 +
 proxmox-shared-cache/debian/control          |  53 ++
 proxmox-shared-cache/debian/copyright        |  18 +
 proxmox-shared-cache/debian/debcargo.toml    |   7 +
 proxmox-shared-cache/examples/performance.rs | 113 +++++
 proxmox-shared-cache/src/lib.rs              | 485 +++++++++++++++++++
 proxmox-shared-memory/src/lib.rs             |   4 +-
 proxmox-sys/src/fs/dir.rs                    |  76 ++-
 proxmox-sys/src/fs/file.rs                   |   4 +-
 proxmox-sys/src/fs/mod.rs                    |  15 +-
 12 files changed, 780 insertions(+), 19 deletions(-)
 create mode 100644 proxmox-shared-cache/Cargo.toml
 create mode 100644 proxmox-shared-cache/debian/changelog
 create mode 100644 proxmox-shared-cache/debian/control
 create mode 100644 proxmox-shared-cache/debian/copyright
 create mode 100644 proxmox-shared-cache/debian/debcargo.toml
 create mode 100644 proxmox-shared-cache/examples/performance.rs
 create mode 100644 proxmox-shared-cache/src/lib.rs


proxmox-perl-rs:

Lukas Wagner (1):
  cache: add bindings for `SharedCache`

 common/pkg/Makefile                  |  1 +
 common/pkg/Proxmox/RS/SharedCache.pm | 46 ++++++++++++++
 common/src/mod.rs                    |  1 +
 common/src/shared_cache.rs           | 89 ++++++++++++++++++++++++++++
 pve-rs/Cargo.toml                    |  1 +
 5 files changed, 138 insertions(+)
 create mode 100644 common/pkg/Proxmox/RS/SharedCache.pm
 create mode 100644 common/src/shared_cache.rs


pve-storage:

Lukas Wagner (1):
  stats: api: cache storage plugin status

 src/PVE/API2/Storage/Config.pm  | 18 +++++++
 src/PVE/API2/Storage/Content.pm | 15 ++++++
 src/PVE/API2/Storage/Status.pm  | 38 ++++++++++++++-
 src/PVE/Storage.pm              | 84 ++++++++++++++++++++++++---------
 4 files changed, 131 insertions(+), 24 deletions(-)


Summary over all repositories:
  21 files changed, 1049 insertions(+), 43 deletions(-)

-- 
murpp v0.4.0

[pve-devel] [PATCH v2 proxmox 1/7] sys: fs: remove unnecessary clippy allow directive

[Lukas Wagner]

It seems like the mentioned clippy bug has since been fixed.

Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
---
 proxmox-sys/src/fs/dir.rs | 4 ----
 proxmox-sys/src/fs/mod.rs | 2 --
 2 files changed, 6 deletions(-)

diff --git a/proxmox-sys/src/fs/dir.rs b/proxmox-sys/src/fs/dir.rs
index 6aee316..0b409d7 100644
--- a/proxmox-sys/src/fs/dir.rs
+++ b/proxmox-sys/src/fs/dir.rs
@@ -14,8 +14,6 @@ use crate::fs::{fchown, CreateOptions};
 ///
 /// Errors if the directory already exists.
 pub fn create_dir<P: AsRef<Path>>(path: P, options: CreateOptions) -> Result<(), nix::Error> {
-    // clippy bug?: from_bits_truncate is actually a const fn...
-    #[allow(clippy::or_fun_call)]
     let mode: stat::Mode = options
         .perm
         .unwrap_or(stat::Mode::from_bits_truncate(0o770));
@@ -126,8 +124,6 @@ fn create_path_at_do(
                     final_opts.as_ref()
                 };
 
-                // clippy bug?: from_bits_truncate is actually a const fn...
-                #[allow(clippy::or_fun_call)]
                 let mode = opts
                     .and_then(|o| o.perm)
                     .unwrap_or(stat::Mode::from_bits_truncate(0o755));
diff --git a/proxmox-sys/src/fs/mod.rs b/proxmox-sys/src/fs/mod.rs
index 8fb677c..ae54d78 100644
--- a/proxmox-sys/src/fs/mod.rs
+++ b/proxmox-sys/src/fs/mod.rs
@@ -69,8 +69,6 @@ impl CreateOptions {
     }
 
     pub fn apply_to(&self, file: &mut File, path: &Path) -> Result<(), Error> {
-        // clippy bug?: from_bits_truncate is actually a const fn...
-        #[allow(clippy::or_fun_call)]
         let mode: stat::Mode = self.perm.unwrap_or(stat::Mode::from_bits_truncate(0o644));
 
         if let Err(err) = stat::fchmod(file.as_raw_fd(), mode) {
-- 
2.39.2

[pve-devel] [PATCH v2 proxmox 2/7] sys: fs: let CreateOptions::apply_to take RawFd instead of File

[Lukas Wagner]

Suggested-by: Wolfgang Bumiller <w.bumiller@proxmox.com>
Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
---
 proxmox-shared-memory/src/lib.rs | 4 ++--
 proxmox-sys/src/fs/file.rs       | 4 ++--
 proxmox-sys/src/fs/mod.rs        | 9 ++++-----
 3 files changed, 8 insertions(+), 9 deletions(-)

diff --git a/proxmox-shared-memory/src/lib.rs b/proxmox-shared-memory/src/lib.rs
index 4809bd0..3aa04cd 100644
--- a/proxmox-shared-memory/src/lib.rs
+++ b/proxmox-shared-memory/src/lib.rs
@@ -130,8 +130,8 @@ impl<T: Sized + Init> SharedMemory<T> {
         // create temporary file using O_TMPFILE
         let mut file = match nix::fcntl::open(&dir_name, oflag | OFlag::O_TMPFILE, Mode::empty()) {
             Ok(fd) => {
-                let mut file = unsafe { File::from_raw_fd(fd) };
-                options.apply_to(&mut file, &dir_name)?;
+                let file = unsafe { File::from_raw_fd(fd) };
+                options.apply_to(fd, &dir_name)?;
                 file
             }
             Err(err) => {
diff --git a/proxmox-sys/src/fs/file.rs b/proxmox-sys/src/fs/file.rs
index ac51389..5d74f31 100644
--- a/proxmox-sys/src/fs/file.rs
+++ b/proxmox-sys/src/fs/file.rs
@@ -135,12 +135,12 @@ pub fn make_tmp_file<P: AsRef<Path>>(
     // use mkstemp here, because it works with different processes, threads, even tokio tasks
     let mut template = path.to_owned();
     template.set_extension("tmp_XXXXXX");
-    let (mut file, tmp_path) = match unistd::mkstemp(&template) {
+    let (file, tmp_path) = match unistd::mkstemp(&template) {
         Ok((fd, path)) => (unsafe { File::from_raw_fd(fd) }, path),
         Err(err) => bail!("mkstemp {:?} failed: {}", template, err),
     };
 
-    match options.apply_to(&mut file, &tmp_path) {
+    match options.apply_to(file.as_raw_fd(), &tmp_path) {
         Ok(()) => Ok((file, tmp_path)),
         Err(err) => {
             let _ = unistd::unlink(&tmp_path);
diff --git a/proxmox-sys/src/fs/mod.rs b/proxmox-sys/src/fs/mod.rs
index ae54d78..8d790a4 100644
--- a/proxmox-sys/src/fs/mod.rs
+++ b/proxmox-sys/src/fs/mod.rs
@@ -1,12 +1,11 @@
 //! File system related utilities
-use std::fs::File;
 use std::path::Path;
 
 use anyhow::{bail, Error};
 
 use nix::sys::stat;
 use nix::unistd::{Gid, Uid};
-use std::os::unix::io::{AsRawFd, RawFd};
+use std::os::unix::io::RawFd;
 
 #[cfg(feature = "acl")]
 pub mod acl;
@@ -68,15 +67,15 @@ impl CreateOptions {
         self.owner(nix::unistd::ROOT)
     }
 
-    pub fn apply_to(&self, file: &mut File, path: &Path) -> Result<(), Error> {
+    pub fn apply_to(&self, fd: RawFd, path: &Path) -> Result<(), Error> {
         let mode: stat::Mode = self.perm.unwrap_or(stat::Mode::from_bits_truncate(0o644));
 
-        if let Err(err) = stat::fchmod(file.as_raw_fd(), mode) {
+        if let Err(err) = stat::fchmod(fd, mode) {
             bail!("fchmod {:?} failed: {}", path, err);
         }
 
         if self.owner.is_some() || self.group.is_some() {
-            if let Err(err) = fchown(file.as_raw_fd(), self.owner, self.group) {
+            if let Err(err) = fchown(fd, self.owner, self.group) {
                 bail!("fchown {:?} failed: {}", path, err);
             }
         }
-- 
2.39.2

[pve-devel] [PATCH v2 proxmox 3/7] sys: fs: use inline formatting for bail! macro

[Lukas Wagner]

Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
---
 proxmox-sys/src/fs/mod.rs | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/proxmox-sys/src/fs/mod.rs b/proxmox-sys/src/fs/mod.rs
index 8d790a4..f54aaf6 100644
--- a/proxmox-sys/src/fs/mod.rs
+++ b/proxmox-sys/src/fs/mod.rs
@@ -71,12 +71,12 @@ impl CreateOptions {
         let mode: stat::Mode = self.perm.unwrap_or(stat::Mode::from_bits_truncate(0o644));
 
         if let Err(err) = stat::fchmod(fd, mode) {
-            bail!("fchmod {:?} failed: {}", path, err);
+            bail!("fchmod {path:?} failed: {err}");
         }
 
         if self.owner.is_some() || self.group.is_some() {
             if let Err(err) = fchown(fd, self.owner, self.group) {
-                bail!("fchown {:?} failed: {}", path, err);
+                bail!("fchown {path:?} failed: {err}");
             }
         }
         Ok(())
-- 
2.39.2

[pve-devel] [PATCH v2 proxmox 4/7] sys: add make_tmp_dir

[Lukas Wagner]

Under the hood, this function calls `mkdtemp` from libc. Unfortunatly
the nix crate did not provide bindings for this function, so we have
to call into libc directly.

Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
---

Notes:
    Changes from v1 -> v2:
      - Use remove_dir instead of unlink
      - Log error if cleaning up dir did not work
      - Change how the tmp dir path is passed to mkdtemp, retaining
        ownership at all time.
      - Check for os_error immediately after calling mkdtemp

 proxmox-sys/src/fs/dir.rs | 72 +++++++++++++++++++++++++++++++++++++--
 1 file changed, 70 insertions(+), 2 deletions(-)

diff --git a/proxmox-sys/src/fs/dir.rs b/proxmox-sys/src/fs/dir.rs
index 0b409d7..2ef5e2e 100644
--- a/proxmox-sys/src/fs/dir.rs
+++ b/proxmox-sys/src/fs/dir.rs
@@ -1,6 +1,7 @@
-use std::ffi::CStr;
+use std::ffi::{CStr, OsStr};
+use std::os::unix::ffi::OsStrExt;
 use std::os::unix::io::{AsRawFd, OwnedFd};
-use std::path::Path;
+use std::path::{Path, PathBuf};
 
 use anyhow::{bail, Error};
 use nix::errno::Errno;
@@ -8,6 +9,8 @@ use nix::fcntl::OFlag;
 use nix::sys::stat;
 use nix::unistd;
 
+use proxmox_lang::try_block;
+
 use crate::fs::{fchown, CreateOptions};
 
 /// Creates directory at the provided path with specified ownership.
@@ -148,6 +151,54 @@ fn create_path_at_do(
     }
 }
 
+///  Create a temporary directory.
+///
+/// `prefix` determines where the temporary directory will be created. For instance, if
+/// `prefix` is `/tmp`, on success the function will return a path in the style of
+/// `/tmp/tmp_XXXXXX`, where X stands for a random string, ensuring that the path is unique.
+///
+/// By default, the created directory has `0o700` permissions. If this is not desired, custom
+/// [`CreateOptions`] can be passed via the `option` parameter.
+pub fn make_tmp_dir<P: AsRef<Path>>(
+    prefix: P,
+    options: Option<CreateOptions>,
+) -> Result<PathBuf, Error> {
+    let mut template = prefix.as_ref().to_owned();
+    template = template.join("tmp_XXXXXX");
+
+    let mut template = template.into_os_string().as_bytes().to_owned();
+    // Push NULL byte so that we have a proper NULL-terminated string
+    template.push(0);
+
+    let returned_buffer = unsafe {
+        let raw_buffer: *mut i8 = std::mem::transmute(template.as_mut_ptr());
+        libc::mkdtemp(raw_buffer)
+    };
+
+    // Check errno immediately, so that nothing else can overwrite it.
+    let err = std::io::Error::last_os_error();
+
+    if returned_buffer.is_null() {
+        return Err(err.into());
+    }
+    let path = PathBuf::from(OsStr::from_bytes(&template[..template.len() - 1]));
+
+    if let Some(options) = options {
+        if let Err(err) = try_block!({
+            let fd = crate::fd::open(&path, OFlag::O_DIRECTORY, stat::Mode::empty())?;
+            options.apply_to(fd.as_raw_fd(), &path)?;
+            Ok::<(), Error>(())
+        }) {
+            if let Err(err) = std::fs::remove_dir(&path) {
+                log::error!("could not clean up temporary directory at {path:?}: {err}")
+            }
+            bail!("could not apply create options to new temporary directory: {err}");
+        }
+    }
+
+    Ok(path)
+}
+
 #[cfg(test)]
 mod tests {
     use super::*;
@@ -165,4 +216,21 @@ mod tests {
         )
         .expect("expected create_path to work");
     }
+
+    #[test]
+    fn test_make_tmp_dir() -> Result<(), Error> {
+        let options = CreateOptions::new()
+            .owner(unistd::Uid::effective())
+            .group(unistd::Gid::effective())
+            .perm(stat::Mode::from_bits_truncate(0o755));
+
+        let path = make_tmp_dir("/tmp", Some(options))?;
+
+        assert!(path.exists());
+        assert!(path.is_dir());
+
+        std::fs::remove_dir_all(&path)?;
+
+        Ok(())
+    }
 }
-- 
2.39.2

[pve-devel] [PATCH v2 proxmox 5/7] cache: add new crate 'proxmox-shared-cache'

[Lukas Wagner]

This crate contains a file-backed cache with expiration logic.
The cache should be safe to be accessed from multiple processes at
once.

The cache stores values in a directory, based on the key.
E.g. key "foo" results in a file 'foo.json' in the given base
directory. If a new value is set, the file is atomically replaced.
The JSON file also contains some metadata, namely 'added_at' and
'expire_in' - they are used for cache expiration.

Note: This cache is not suited to applications that
 - Might want to cache huge amounts of data, and/or access the cache
   very frequently (due to the overhead of JSON de/serialization)
 - Require arbitrary keys - right now, keys are limited by
   SAFE_ID_REGEX

The cache was developed for the use in pvestatd, in order to cache
e.g. storage plugin status. There, these limitations do not really
play any role.

Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
---
 Cargo.toml                                   |   1 +
 proxmox-shared-cache/Cargo.toml              |  18 +
 proxmox-shared-cache/debian/changelog        |   5 +
 proxmox-shared-cache/debian/control          |  53 ++
 proxmox-shared-cache/debian/copyright        |  18 +
 proxmox-shared-cache/debian/debcargo.toml    |   7 +
 proxmox-shared-cache/examples/performance.rs | 113 +++++
 proxmox-shared-cache/src/lib.rs              | 485 +++++++++++++++++++
 8 files changed, 700 insertions(+)
 create mode 100644 proxmox-shared-cache/Cargo.toml
 create mode 100644 proxmox-shared-cache/debian/changelog
 create mode 100644 proxmox-shared-cache/debian/control
 create mode 100644 proxmox-shared-cache/debian/copyright
 create mode 100644 proxmox-shared-cache/debian/debcargo.toml
 create mode 100644 proxmox-shared-cache/examples/performance.rs
 create mode 100644 proxmox-shared-cache/src/lib.rs

diff --git a/Cargo.toml b/Cargo.toml
index e334ac1..7b1e8e3 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -22,6 +22,7 @@ members = [
     "proxmox-schema",
     "proxmox-section-config",
     "proxmox-serde",
+    "proxmox-shared-cache",
     "proxmox-shared-memory",
     "proxmox-sortable-macro",
     "proxmox-subscription",
diff --git a/proxmox-shared-cache/Cargo.toml b/proxmox-shared-cache/Cargo.toml
new file mode 100644
index 0000000..ada1a12
--- /dev/null
+++ b/proxmox-shared-cache/Cargo.toml
@@ -0,0 +1,18 @@
+[package]
+name = "proxmox-shared-cache"
+version = "0.1.0"
+authors.workspace = true
+edition.workspace = true
+license.workspace = true
+repository.workspace = true
+exclude.workspace = true
+description = "A cache that can be used from multiple processes simultaneously"
+
+[dependencies]
+anyhow.workspace = true
+proxmox-sys = { workspace = true, features = ["timer"] }
+proxmox-time.workspace = true
+proxmox-schema = { workspace = true, features = ["api-types"]}
+serde_json = { workspace = true, features = ["raw_value"] }
+serde = { workspace = true, features = ["derive"]}
+nix.workspace = true
diff --git a/proxmox-shared-cache/debian/changelog b/proxmox-shared-cache/debian/changelog
new file mode 100644
index 0000000..54d39f5
--- /dev/null
+++ b/proxmox-shared-cache/debian/changelog
@@ -0,0 +1,5 @@
+rust-proxmox-shared-cache (0.1.0-1) unstable; urgency=medium
+
+  * initial Debian package
+
+ -- Proxmox Support Team <support@proxmox.com>  Thu, 04 May 2023 08:40:38 +0200
diff --git a/proxmox-shared-cache/debian/control b/proxmox-shared-cache/debian/control
new file mode 100644
index 0000000..c8f0d8e
--- /dev/null
+++ b/proxmox-shared-cache/debian/control
@@ -0,0 +1,53 @@
+Source: rust-proxmox-shared-cache
+Section: rust
+Priority: optional
+Build-Depends: debhelper (>= 12),
+ dh-cargo (>= 25),
+ cargo:native <!nocheck>,
+ rustc:native <!nocheck>,
+ libstd-rust-dev <!nocheck>,
+ librust-anyhow-1+default-dev <!nocheck>,
+ librust-nix-0.26+default-dev (>= 0.26.1-~~) <!nocheck>,
+ librust-proxmox-schema-2+api-types-dev <!nocheck>,
+ librust-proxmox-schema-2+default-dev <!nocheck>,
+ librust-proxmox-sys-0.5+default-dev <!nocheck>,
+ librust-proxmox-sys-0.5+timer-dev <!nocheck>,
+ librust-proxmox-time-1+default-dev (>= 1.1.4-~~) <!nocheck>,
+ librust-serde-1+default-dev <!nocheck>,
+ librust-serde-1+derive-dev <!nocheck>,
+ librust-serde-json-1+default-dev <!nocheck>,
+ librust-serde-json-1+raw-value-dev <!nocheck>
+Maintainer: Proxmox Support Team <support@proxmox.com>
+Standards-Version: 4.6.1
+Vcs-Git: https://salsa.debian.org/rust-team/debcargo-conf.git [src/proxmox-shared-cache]
+Vcs-Browser: https://salsa.debian.org/rust-team/debcargo-conf/tree/master/src/proxmox-shared-cache
+X-Cargo-Crate: proxmox-shared-cache
+Rules-Requires-Root: no
+
+Package: librust-proxmox-shared-cache-dev
+Architecture: any
+Multi-Arch: same
+Depends:
+ ${misc:Depends},
+ librust-anyhow-1+default-dev,
+ librust-nix-0.26+default-dev (>= 0.26.1-~~),
+ librust-proxmox-schema-2+api-types-dev,
+ librust-proxmox-schema-2+default-dev,
+ librust-proxmox-sys-0.5+default-dev,
+ librust-proxmox-sys-0.5+timer-dev,
+ librust-proxmox-time-1+default-dev (>= 1.1.4-~~),
+ librust-serde-1+default-dev,
+ librust-serde-1+derive-dev,
+ librust-serde-json-1+default-dev,
+ librust-serde-json-1+raw-value-dev
+Provides:
+ librust-proxmox-shared-cache+default-dev (= ${binary:Version}),
+ librust-proxmox-shared-cache-0-dev (= ${binary:Version}),
+ librust-proxmox-shared-cache-0+default-dev (= ${binary:Version}),
+ librust-proxmox-shared-cache-0.1-dev (= ${binary:Version}),
+ librust-proxmox-shared-cache-0.1+default-dev (= ${binary:Version}),
+ librust-proxmox-shared-cache-0.1.0-dev (= ${binary:Version}),
+ librust-proxmox-shared-cache-0.1.0+default-dev (= ${binary:Version})
+Description: Cache implementations - Rust source code
+ This package contains the source for the Rust proxmox-shared-cache crate,
+ packaged by debcargo for use with cargo and dh-cargo.
diff --git a/proxmox-shared-cache/debian/copyright b/proxmox-shared-cache/debian/copyright
new file mode 100644
index 0000000..0d9eab3
--- /dev/null
+++ b/proxmox-shared-cache/debian/copyright
@@ -0,0 +1,18 @@
+Format: https://www.debian.org/doc/packaging-manuals/copyright-format/1.0/
+
+Files:
+ *
+Copyright: 2019 - 2023 Proxmox Server Solutions GmbH <support@proxmox.com>
+License: AGPL-3.0-or-later
+ This program is free software: you can redistribute it and/or modify it under
+ the terms of the GNU Affero General Public License as published by the Free
+ Software Foundation, either version 3 of the License, or (at your option) any
+ later version.
+ .
+ This program is distributed in the hope that it will be useful, but WITHOUT
+ ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS
+ FOR A PARTICULAR PURPOSE. See the GNU Affero General Public License for more
+ details.
+ .
+ You should have received a copy of the GNU Affero General Public License along
+ with this program. If not, see <https://www.gnu.org/licenses/>.
diff --git a/proxmox-shared-cache/debian/debcargo.toml b/proxmox-shared-cache/debian/debcargo.toml
new file mode 100644
index 0000000..14ad800
--- /dev/null
+++ b/proxmox-shared-cache/debian/debcargo.toml
@@ -0,0 +1,7 @@
+overlay = "."
+crate_src_path = ".."
+maintainer = "Proxmox Support Team <support@proxmox.com>"
+
+[source]
+#vcs_git = "git://git.proxmox.com/git/proxmox.git"
+#vcs_browser = "https://git.proxmox.com/?p=proxmox.git"
diff --git a/proxmox-shared-cache/examples/performance.rs b/proxmox-shared-cache/examples/performance.rs
new file mode 100644
index 0000000..54a9bf9
--- /dev/null
+++ b/proxmox-shared-cache/examples/performance.rs
@@ -0,0 +1,113 @@
+use proxmox_shared_cache::SharedCache;
+use proxmox_sys::fs::CreateOptions;
+use serde_json::Value;
+use std::time::{Duration, Instant};
+
+fn main() {
+    let options = CreateOptions::new()
+        .owner(nix::unistd::Uid::effective())
+        .group(nix::unistd::Gid::effective())
+        .perm(nix::sys::stat::Mode::from_bits_truncate(0o755));
+
+    let cache = SharedCache::new("/tmp/pmx-cache", options).unwrap();
+
+    let mut keys = Vec::new();
+
+    for i in 0..100000 {
+        keys.push(format!("key_{i}"));
+    }
+
+    let data = serde_json::json!({
+        "member1": "foo",
+        "member2": "foo",
+        "member3": "foo",
+        "member4": "foo",
+        "member5": "foo",
+        "member5": "foo",
+        "member6": "foo",
+        "member7": "foo",
+        "member8": "foo",
+        "array": [10, 20, 30, 40, 50],
+        "object": {
+            "member1": "foo",
+            "member2": "foo",
+            "member3": "foo",
+            "member4": "foo",
+            "member5": "foo",
+            "member5": "foo",
+            "member6": "foo",
+            "member7": "foo",
+            "member8": "foo",
+        }
+    });
+
+    // #####################################
+    let before = Instant::now();
+
+    for key in &keys {
+        cache.set(key, &data, None).expect("could not insert value");
+    }
+
+    let time = Instant::now() - before;
+    let time_per_op = time / keys.len() as u32;
+    println!(
+        "inserting {len} keys took {time:?} ({time_per_op:?} per key)",
+        len = keys.len(),
+    );
+
+    // #####################################
+    let before = Instant::now();
+    for key in &keys {
+        let _: Option<Value> = cache.get(key).expect("could not get value");
+    }
+
+    let time = Instant::now() - before;
+    let time_per_op = time / keys.len() as u32;
+    println!(
+        "getting {len} unexpired keys took {time:?} ({time_per_op:?} per key)",
+        len = keys.len(),
+    );
+
+    // #####################################
+    let before = Instant::now();
+    for key in &keys {
+        cache
+            .set(key, &data, Some(0))
+            .expect("could not insert value");
+    }
+
+    let time = Instant::now() - before;
+    let time_per_op = time / keys.len() as u32;
+    println!(
+        "updating {len} keys took {time:?} ({time_per_op:?} per key)",
+        len = keys.len(),
+    );
+
+    std::thread::sleep(Duration::from_secs(1));
+
+    // #####################################
+    let before = Instant::now();
+    for key in &keys {
+        let _: Option<Value> = cache.get(key).expect("could not get value");
+    }
+
+    let time = Instant::now() - before;
+    let time_per_op = time / keys.len() as u32;
+    println!(
+        "getting {len} expired keys took {time:?} ({time_per_op:?} per key)",
+        len = keys.len(),
+    );
+
+    // #####################################
+    let before = Instant::now();
+    for key in &keys {
+        cache.delete(key).expect("could not delete value");
+    }
+
+    let time = Instant::now() - before;
+    let time_per_op = time / keys.len() as u32;
+    println!(
+        "deleting {len} keys took {time:?} ({time_per_op:?} per key)",
+        len = keys.len(),
+    );
+}
diff --git a/proxmox-shared-cache/src/lib.rs b/proxmox-shared-cache/src/lib.rs
new file mode 100644
index 0000000..c63de91
--- /dev/null
+++ b/proxmox-shared-cache/src/lib.rs
@@ -0,0 +1,485 @@
+use std::fs::File;
+use std::os::fd::{FromRawFd, IntoRawFd, RawFd};
+use std::path::{Path, PathBuf};
+use std::time::Duration;
+
+use anyhow::{bail, Error};
+use serde::de::DeserializeOwned;
+use serde::{Deserialize, Serialize};
+use serde_json::value::RawValue;
+
+use proxmox_schema::api_types::SAFE_ID_FORMAT;
+use proxmox_sys::fs::CreateOptions;
+
+/// Lock guard for a locked cache entry.
+///
+/// The lock is dropped when the guard is dropped.
+pub struct CacheLockGuard(File);
+
+impl FromRawFd for CacheLockGuard {
+    unsafe fn from_raw_fd(fd: RawFd) -> Self {
+        CacheLockGuard(File::from_raw_fd(fd))
+    }
+}
+
+impl IntoRawFd for CacheLockGuard {
+    fn into_raw_fd(self) -> RawFd {
+        self.0.into_raw_fd()
+    }
+}
+
+/// A simple, file-backed cache that can be used from multiple processes concurrently.
+///
+/// Cache entries are stored as individual files inside a base directory. For instance,
+/// a cache entry with the key 'disk_stats' will result in a file 'disk_stats.json' inside
+/// the base directory. As the extension implies, the cached data will be stored as a JSON
+/// string.
+///
+/// For optimal performance, `SharedCache` should have its base directory in a `tmpfs`.
+///
+/// ## Key Space
+/// Due to the fact that cache keys are being directly used as filenames, they have to match the
+/// following regular expression: `[A-Za-z0-9_][A-Za-z0-9._\-]*`
+///
+/// ## Concurrency
+/// set/delete will use file locking to ensure that there are no race conditions.
+/// get does not require any locking, since all file operations used by set/delete should be
+/// atomic.
+///
+/// If multiple cache operations must be at the same locked context, `lock` can be used
+/// to manually lock a cache entry. The returned lock guard can then be passed to
+/// `{set,delete,get}_with_lock`.
+/// If multiple keys are locked at the same time, make sure that you always lock them
+/// in same order (e.g. by sorting the keys) - otherwise circular waits can occur.
+///
+/// ## Performance
+/// On a tmpfs:
+/// ```sh
+///   $ cargo run --release --example=performance
+///   inserting 100000 keys took 2.495008362s (24.95µs per key)
+///   getting 100000 unexpired keys took 1.557399535s (15.573µs per key)
+///   updating 100000 keys took 2.488894178s (24.888µs per key)
+///   getting 100000 expired keys took 1.324983239s (13.249µs per key)
+///   deleting 100000 keys took 1.533744028s (15.337µs per key)
+///
+/// Inserting/getting large objects might of course result in lower performance due to the cost
+/// of serialization.
+/// ```
+///
+/// # Limitations
+/// - At the moment, stale/expired keys are never cleaned - at the moment
+///   of creation this was simply not needed, since we only use the crate for
+///   caching data in pvestatd with a limited set of keys that are not changing
+///   - so there will not be any cache entries that need to be cleaned up.
+///
+pub struct SharedCache {
+    base_path: PathBuf,
+    create_options: CreateOptions,
+    #[cfg(test)]
+    time: std::cell::Cell<i64>,
+}
+
+impl SharedCache {
+    /// Instantiate a new cache instance for a given `base_path`.
+    ///
+    /// If `base_path` does not exist, it will be created - the access permissions
+    /// are determined by `options`.
+    /// If the base directory already contains cache entries, they will be available
+    /// via `get` for later retrieval. In other words, `SharedCache::new` never touches
+    /// existing cache entries.
+    pub fn new<P: AsRef<Path>>(base_path: P, options: CreateOptions) -> Result<Self, Error> {
+        proxmox_sys::fs::create_path(
+            base_path.as_ref(),
+            Some(options.clone()),
+            Some(options.clone()),
+        )?;
+
+        Ok(SharedCache {
+            base_path: base_path.as_ref().to_owned(),
+            create_options: options,
+            #[cfg(test)]
+            time: std::cell::Cell::new(0),
+        })
+    }
+
+    /// Set a cache entry, with optional value expiration.
+    ///
+    /// This method will attempt to lock the cache entry before deleting it.
+    ///
+    /// Keys have to match the following regular expression to be valid:
+    /// `[A-Za-z0-9_][A-Za-z0-9._\-]*`
+    ///
+    /// Returns an error if value serialization or storing the value failed.
+    pub fn set<S: AsRef<str>, V: Serialize>(
+        &self,
+        key: S,
+        value: &V,
+        expires_in: Option<i64>,
+    ) -> Result<(), Error> {
+        let lock = self.lock(key.as_ref(), true)?;
+        self.set_with_lock(key, value, expires_in, &lock)
+    }
+
+    /// Set a cache entry, with optional value expiration.
+    ///
+    /// This method assumes that the cache entry was locked before using `lock`.
+    ///
+    /// Keys have to match the following regular expression to be valid:
+    /// `[A-Za-z0-9_][A-Za-z0-9._\-]*`
+    ///
+    /// Returns an error if value serialization or storing the value failed.
+    pub fn set_with_lock<S: AsRef<str>, V: Serialize>(
+        &self,
+        key: S,
+        value: &V,
+        expires_in: Option<i64>,
+        _lock: &CacheLockGuard,
+    ) -> Result<(), Error> {
+        let path = self.get_entry_path(key.as_ref())?;
+        let added_at = self.get_time();
+
+        let item = CachedItem {
+            value,
+            added_at,
+            expires_in,
+        };
+
+        let serialized = serde_json::to_vec_pretty(&item)?;
+
+        // Atomically replace file
+        proxmox_sys::fs::replace_file(path, &serialized, self.create_options.clone(), true)?;
+        Ok(())
+    }
+
+    /// Delete a cache entry.
+    ///
+    /// This method will attempt to lock the cache entry before deleting it.
+    ///
+    /// Keys have to match the following regular expression to be valid:
+    /// `[A-Za-z0-9_][A-Za-z0-9._\-]*`
+    ///
+    /// Returns an error if the entry could not be deleted.
+    pub fn delete<S: AsRef<str>>(&self, key: S) -> Result<(), Error> {
+        let lock = self.lock(key.as_ref(), true)?;
+        self.delete_with_lock(key.as_ref(), &lock)?;
+
+        Ok(())
+    }
+
+    /// Delete a cache entry.
+    ///
+    /// This method assumes that the cache entry was locked before using `lock`.
+    ///
+    /// Keys have to match the following regular expression to be valid:
+    /// `[A-Za-z0-9_][A-Za-z0-9._\-]*`
+    ///
+    /// Returns an error if the entry could not be deleted.
+    pub fn delete_with_lock<S: AsRef<str>>(
+        &self,
+        key: S,
+        _lock: &CacheLockGuard,
+    ) -> Result<(), Error> {
+        let path = self.get_entry_path(key.as_ref())?;
+        std::fs::remove_file(path)?;
+
+        // Unlink the lock file's dir entry from the fs, but since we have
+        // an open file handle for the lock file, it continues to exist until
+        // the handle is closed
+        std::fs::remove_file(self.get_lockfile_path(key.as_ref())?)?;
+
+        Ok(())
+    }
+
+    /// Get a value from the cache.
+    ///
+    /// This method will attempt to lock the entry with a non-exclusive lock before reading it.
+    ///
+    /// Keys have to match the following regular expression to be valid:
+    /// `[A-Za-z0-9_][A-Za-z0-9._\-]*`
+    ///
+    /// Returns an error if the entry could not be retrieved.
+    pub fn get<S: AsRef<str>, V: DeserializeOwned>(&self, key: S) -> Result<Option<V>, Error> {
+        let lock = self.lock(key.as_ref(), false)?;
+        self.get_with_lock(key, &lock)
+    }
+
+    /// Get a value from the cache.
+    ///
+    /// If the key does not exist or the value has expired, `Ok(None)` is returned.
+    /// This method assumes that the cache entry was locked before using `lock`.
+    ///
+    /// Keys have to match the following regular expression to be valid:
+    /// `[A-Za-z0-9_][A-Za-z0-9._\-]*`
+    ///
+    /// Returns an error if the entry could not be retrieved.
+    pub fn get_with_lock<S: AsRef<str>, V: DeserializeOwned>(
+        &self,
+        key: S,
+        _lock: &CacheLockGuard,
+    ) -> Result<Option<V>, Error> {
+        let path = self.get_entry_path(key.as_ref())?;
+
+        if let Some(content) = proxmox_sys::fs::file_get_optional_contents(path)? {
+            // Use RawValue so that we can deserialize the actual payload after
+            // checking value expiry. This should improve performance for large payloads.
+            let value: CachedItem<&'_ RawValue> = serde_json::from_slice(&content)?;
+
+            let now = self.get_time();
+
+            if let Some(expires_in) = value.expires_in {
+                // Check if value is not expired yet. Also do not allow
+                // values from the future, in case we have clock jumps
+                if value.added_at + expires_in > now && value.added_at <= now {
+                    Ok(Some(serde_json::from_str(value.value.get())?))
+                } else {
+                    Ok(None)
+                }
+            } else {
+                Ok(Some(serde_json::from_str(value.value.get())?))
+            }
+        } else {
+            Ok(None)
+        }
+    }
+
+    /// Get value from the cache. If it does not exist/is expired, compute
+    /// the new value from a passed closure and insert it into the cache.
+    ///
+    /// Keys have to match the following regular expression to be valid:
+    /// `[A-Za-z0-9_][A-Za-z0-9._\-]*`
+    pub fn get_or_update<K, V, F>(
+        &self,
+        key: K,
+        value_func: &mut F,
+        expires_in: Option<i64>,
+    ) -> Result<V, Error>
+    where
+        K: AsRef<str>,
+        F: FnMut() -> Result<V, Error>,
+        V: Serialize + DeserializeOwned,
+    {
+        // Lookup value and return if it exists and has not expired yet
+        // get uses a non-exclusive lock, so we can have concurrent lookups
+        let val = self.get(key.as_ref())?;
+        if let Some(val) = val {
+            return Ok(val);
+        }
+
+        // If not, lock the entry ...
+        let lock = self.lock(key.as_ref(), true)?;
+
+        // ... and check again, maybe somebody else has set the value
+        // before we locked it.
+        let val = self.get_with_lock(key.as_ref(), &lock)?;
+        if let Some(val) = val {
+            return Ok(val);
+        }
+
+        // If the value is still not there, compute its new value and store it
+        let val = value_func()?;
+        self.set_with_lock(key.as_ref(), &val, expires_in, &lock)?;
+
+        Ok(val)
+    }
+
+    /// Locks a cache entry.
+    ///
+    /// Useful if you must perform multiple operations while being locked.
+    ///
+    /// This will create a lockfile `<base-path>/<key>.lck` which will be locked
+    /// with an advisory file lock via `flock`.
+    ///
+    /// On success, `CacheLockGuard` is returned. It serves as a handle to
+    /// the open lock file. If the handle is dropped, the lock is removed.
+    ///
+    /// Keys have to match the following regular expression to be valid:
+    /// `[A-Za-z0-9_][A-Za-z0-9._\-]*`
+    ///
+    /// Returns an error if the entry could not be locked.
+    pub fn lock<S: AsRef<str>>(&self, key: S, exclusive: bool) -> Result<CacheLockGuard, Error> {
+        let mut path = self.get_entry_path(key.as_ref())?;
+        path.set_extension("lck");
+
+        let options = proxmox_sys::fs::CreateOptions::new()
+            .perm(nix::sys::stat::Mode::from_bits_truncate(0o660));
+
+        let lockfile =
+            proxmox_sys::fs::open_file_locked(path, Duration::from_secs(5), exclusive, options)?;
+
+        Ok(CacheLockGuard(lockfile))
+    }
+
+    #[cfg(not(test))]
+    fn get_time(&self) -> i64 {
+        proxmox_time::epoch_i64()
+    }
+
+    #[cfg(test)]
+    fn get_time(&self) -> i64 {
+        self.time.get()
+    }
+
+    #[cfg(test)]
+    fn set_time(&self, time: i64) {
+        self.time.set(time);
+    }
+
+    fn enforce_safe_key(key: &str) -> Result<(), Error> {
+        let safe_id_regex = SAFE_ID_FORMAT.unwrap_pattern_format();
+        if safe_id_regex.is_match(key) {
+            Ok(())
+        } else {
+            bail!("invalid key format")
+        }
+    }
+
+    fn get_entry_path(&self, key: &str) -> Result<PathBuf, Error> {
+        Self::enforce_safe_key(key)?;
+        let mut path = self.base_path.join(key);
+        path.set_extension("json");
+        Ok(path)
+    }
+
+    fn get_lockfile_path(&self, key: &str) -> Result<PathBuf, Error> {
+        Self::enforce_safe_key(key)?;
+        let mut path = self.base_path.join(key);
+        path.set_extension("lck");
+        Ok(path)
+    }
+}
+
+#[derive(Debug, Clone, Serialize, Deserialize)]
+struct CachedItem<V> {
+    value: V,
+    added_at: i64,
+    expires_in: Option<i64>,
+}
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+    use serde_json::Value;
+
+    #[test]
+    fn test_basic_set_and_get() {
+        let cache = TestCache::new();
+        cache
+            .cache
+            .set("foo", &Value::String("bar".into()), None)
+            .unwrap();
+
+        assert_eq!(
+            cache.cache.get("foo").unwrap(),
+            Some(Value::String("bar".into()))
+        );
+        assert!(cache.cache.get::<_, Value>("notthere").unwrap().is_none());
+    }
+
+    struct TestCache {
+        cache: SharedCache,
+    }
+
+    impl TestCache {
+        fn new() -> Self {
+            let path = proxmox_sys::fs::make_tmp_dir("/tmp/", None).unwrap();
+
+            let options = CreateOptions::new()
+                .owner(nix::unistd::Uid::effective())
+                .group(nix::unistd::Gid::effective())
+                .perm(nix::sys::stat::Mode::from_bits_truncate(0o600));
+
+            let cache = SharedCache::new(&path, options).unwrap();
+            Self { cache }
+        }
+    }
+
+    impl Drop for TestCache {
+        fn drop(&mut self) {
+            let _ = std::fs::remove_dir_all(&self.cache.base_path);
+        }
+    }
+
+    #[test]
+    fn test_expiry() {
+        let wrapper = TestCache::new();
+
+        wrapper
+            .cache
+            .set("expiring", &Value::String("bar".into()), Some(10))
+            .unwrap();
+        assert!(wrapper.cache.get::<_, Value>("expiring").unwrap().is_some());
+
+        wrapper.cache.set_time(9);
+        assert!(wrapper.cache.get::<_, Value>("expiring").unwrap().is_some());
+        wrapper.cache.set_time(11);
+        assert!(wrapper.cache.get::<_, Value>("expiring").unwrap().is_none());
+    }
+
+    #[test]
+    fn test_backwards_time_jump() {
+        let wrapper = TestCache::new();
+
+        wrapper.cache.set_time(50);
+        wrapper
+            .cache
+            .set("future", &Value::String("bar".into()), Some(10))
+            .unwrap();
+        wrapper.cache.set_time(30);
+        assert!(wrapper.cache.get::<_, Value>("future").unwrap().is_none());
+    }
+
+    #[test]
+    fn test_invalid_keys() {
+        let wrapper = TestCache::new();
+
+        assert!(wrapper
+            .cache
+            .set("../escape_base", &Value::Null, None)
+            .is_err());
+        assert!(wrapper
+            .cache
+            .set("bjørnen drikker øl", &Value::Null, None)
+            .is_err());
+        assert!(wrapper.cache.set("test space", &Value::Null, None).is_err());
+        assert!(wrapper.cache.set("~/foo", &Value::Null, None).is_err());
+    }
+
+    #[test]
+    fn test_deletion() {
+        let wrapper = TestCache::new();
+
+        wrapper
+            .cache
+            .set("delete", &Value::String("bar".into()), Some(10))
+            .unwrap();
+
+        assert!(wrapper.cache.delete("delete").is_ok());
+        assert!(wrapper.cache.get::<_, Value>("delete").unwrap().is_none());
+    }
+
+    #[test]
+    fn test_get_or_update() {
+        let wrapper = TestCache::new();
+
+        let val = wrapper
+            .cache
+            .get_or_update("test", &mut || Ok(0), Some(5))
+            .unwrap();
+
+        assert_eq!(val, 0);
+
+        wrapper.cache.set_time(4);
+        let val = wrapper
+            .cache
+            .get_or_update("test", &mut || Ok(4), Some(5))
+            .unwrap();
+        assert_eq!(val, 0);
+
+        wrapper.cache.set_time(6);
+        let val = wrapper
+            .cache
+            .get_or_update("test", &mut || Ok(6), Some(5))
+            .unwrap();
+        assert_eq!(val, 6);
+    }
+}
-- 
2.39.2

[pve-devel] [PATCH v2 proxmox-perl-rs 6/7] cache: add bindings for `SharedCache`

[Lukas Wagner]

These bindings are contained in the `SharedCacheBase` class, which is
subclassed by `SharedCache` in Perl. The subclass was needed to
implement the `get_or_update` method since that requires to call a
closure as a passed parameter.

Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
---

Notes:
    Changes v1 -> v2:
      - Added lock/unlock
      - Added get_or_update in perl subclass
      - added *_with_lock methods

 common/pkg/Makefile                  |  1 +
 common/pkg/Proxmox/RS/SharedCache.pm | 46 ++++++++++++++
 common/src/mod.rs                    |  1 +
 common/src/shared_cache.rs           | 89 ++++++++++++++++++++++++++++
 pve-rs/Cargo.toml                    |  1 +
 5 files changed, 138 insertions(+)
 create mode 100644 common/pkg/Proxmox/RS/SharedCache.pm
 create mode 100644 common/src/shared_cache.rs

diff --git a/common/pkg/Makefile b/common/pkg/Makefile
index 7bf669f..a99c30d 100644
--- a/common/pkg/Makefile
+++ b/common/pkg/Makefile
@@ -26,6 +26,7 @@ Proxmox/RS/CalendarEvent.pm:
 	  Proxmox::RS::APT::Repositories \
 	  Proxmox::RS::CalendarEvent \
 	  Proxmox::RS::Notify \
+	  Proxmox::RS::SharedCacheBase \
 	  Proxmox::RS::Subscription
 
 all: Proxmox/RS/CalendarEvent.pm
diff --git a/common/pkg/Proxmox/RS/SharedCache.pm b/common/pkg/Proxmox/RS/SharedCache.pm
new file mode 100644
index 0000000..a35e0c5
--- /dev/null
+++ b/common/pkg/Proxmox/RS/SharedCache.pm
@@ -0,0 +1,46 @@
+package Proxmox::RS::SharedCache;
+
+use base 'Proxmox::RS::SharedCacheBase';
+
+use strict;
+use warnings;
+
+# This part has to be implemented in perl, since we calculate the new value on
+# demand from a passed closure.
+sub get_or_update {
+    my ($self, $key, $value_func, $timeout) = @_;
+
+    #Lookup value
+    my $val = $self->get($key);
+
+    if (!$val) {
+	my $lock = undef;
+	eval {
+	    # If expired, lock cache entry. This makes sure that other processes
+	    # cannot update it at the same time.
+	    $lock = $self->lock($key, 1);
+
+	    # Check again, may somebody else has already updated the value
+	    $val = $self->get_with_lock($key, $lock);
+
+	    # If still expired, update it
+	    if (!$val) {
+	        $val = $value_func->();
+	        $self->set_with_lock($key, $val, $timeout, $lock);
+	    }
+	};
+
+	my $err = $@;
+
+	# If the file has been locked, we *must* unlock it, no matter what
+	if (defined($lock)) {
+	    $self->unlock($lock)
+	}
+
+	die $err if $err;
+    }
+
+    return $val;
+}
+
+1;
diff --git a/common/src/mod.rs b/common/src/mod.rs
index c3574f4..badfc98 100644
--- a/common/src/mod.rs
+++ b/common/src/mod.rs
@@ -2,4 +2,5 @@ pub mod apt;
 mod calendar_event;
 pub mod logger;
 pub mod notify;
+pub mod shared_cache;
 mod subscription;
diff --git a/common/src/shared_cache.rs b/common/src/shared_cache.rs
new file mode 100644
index 0000000..0e2b561
--- /dev/null
+++ b/common/src/shared_cache.rs
@@ -0,0 +1,89 @@
+#[perlmod::package(name = "Proxmox::RS::SharedCacheBase")]
+mod export {
+    use std::os::fd::{FromRawFd, IntoRawFd, RawFd};
+
+    use anyhow::Error;
+    use nix::sys::stat::Mode;
+    use perlmod::Value;
+    use serde_json::Value as JSONValue;
+
+    use proxmox_shared_cache::{SharedCache, CacheLockGuard};
+    use proxmox_sys::fs::CreateOptions;
+
+    pub struct CacheWrapper(SharedCache);
+
+    perlmod::declare_magic!(Box<CacheWrapper> : &CacheWrapper as "Proxmox::RS::SharedCacheBase");
+
+    #[export(raw_return)]
+    fn new(#[raw] class: Value, base_dir: &str) -> Result<Value, Error> {
+        // TODO: Make this configurable once we need to cache values that should be
+        // accessible by other users.
+        let options = CreateOptions::new()
+            .root_only()
+            .perm(Mode::from_bits_truncate(0o700));
+
+        Ok(perlmod::instantiate_magic!(&class, MAGIC => Box::new(
+            CacheWrapper (
+                SharedCache::new(base_dir, options)?
+            )
+        )))
+    }
+
+    #[export]
+    fn set(
+        #[try_from_ref] this: &CacheWrapper,
+        key: &str,
+        value: JSONValue,
+        expires_in: Option<i64>,
+    ) -> Result<(), Error> {
+        this.0.set(key, &value, expires_in)
+    }
+
+    #[export]
+    fn set_with_lock(
+        #[try_from_ref] this: &CacheWrapper,
+        key: &str,
+        value: JSONValue,
+        expires_in: Option<i64>,
+        lock: RawFd,
+    ) -> Result<(), Error> {
+        let lock = unsafe { CacheLockGuard::from_raw_fd(lock) };
+        this.0.set_with_lock(key, &value, expires_in, &lock)
+    }
+
+    #[export]
+    fn get(#[try_from_ref] this: &CacheWrapper, key: &str) -> Result<Option<JSONValue>, Error> {
+        this.0.get(key)
+    }
+
+    #[export]
+    fn get_with_lock(#[try_from_ref] this: &CacheWrapper, key: &str, lock: RawFd) -> Result<Option<JSONValue>, Error> {
+        let lock = unsafe { CacheLockGuard::from_raw_fd(lock) };
+        this.0.get_with_lock(key, &lock)
+    }
+
+    #[export]
+    fn delete(#[try_from_ref] this: &CacheWrapper, key: &str) -> Result<(), Error> {
+        this.0.delete(key)
+    }
+
+    #[export]
+    fn delete_with_lock(#[try_from_ref] this: &CacheWrapper, key: &str, lock: RawFd) -> Result<(), Error> {
+        let lock = unsafe { CacheLockGuard::from_raw_fd(lock) };
+        this.0.delete_with_lock(key, &lock)
+    }
+
+    #[export]
+    fn lock(#[try_from_ref] this: &CacheWrapper, key: &str, exclusive: bool) -> Result<RawFd, Error> {
+        let file = this.0.lock(key, exclusive)?;
+        Ok(file.into_raw_fd())
+    }
+
+    #[export]
+    fn unlock(#[try_from_ref] _this: &CacheWrapper, lock: RawFd) -> Result<(), Error> {
+        // advisory file locks using flock are unlocked once the FD is closed
+        let _ = unsafe { CacheLockGuard::from_raw_fd(lock) };
+
+        Ok(())
+    }
+}
diff --git a/pve-rs/Cargo.toml b/pve-rs/Cargo.toml
index f9e3291..fa78dd6 100644
--- a/pve-rs/Cargo.toml
+++ b/pve-rs/Cargo.toml
@@ -39,6 +39,7 @@ proxmox-http-error = "0.1.0"
 proxmox-notify = "0.2"
 proxmox-openid = "0.10"
 proxmox-resource-scheduling = "0.3.0"
+proxmox-shared-cache = "0.1.0"
 proxmox-subscription = "0.4"
 proxmox-sys = "0.5"
 proxmox-tfa = { version = "4.0.4", features = ["api"] }
-- 
2.39.2

[pve-devel] [PATCH v2 pve-storage 7/7] stats: api: cache storage plugin status

[Lukas Wagner]

Cache storage plugin status so that pvestatd and API calls can use the
cached results, without having to query all storage plugins again.

Introduces the `ignore-cache` on some storage status API calls. By
default it is 0, but when set to 1 the values from the cache will be
ignored.

Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
---

Notes:
    Changes v1 -> v2:
    - Add `ignore-cache` paramter
    - use `get_or_update` method from Proxmox::RS::SharedCached
    - invalidated the cache in other cases (e.g. when allocating volumes,
      as this might change the amount of free space)

 src/PVE/API2/Storage/Config.pm  | 18 +++++++
 src/PVE/API2/Storage/Content.pm | 15 ++++++
 src/PVE/API2/Storage/Status.pm  | 38 ++++++++++++++-
 src/PVE/Storage.pm              | 84 ++++++++++++++++++++++++---------
 4 files changed, 131 insertions(+), 24 deletions(-)

diff --git a/src/PVE/API2/Storage/Config.pm b/src/PVE/API2/Storage/Config.pm
index e04b6ab..e0ee0d8 100755
--- a/src/PVE/API2/Storage/Config.pm
+++ b/src/PVE/API2/Storage/Config.pm
@@ -274,6 +274,12 @@ __PACKAGE__->register_method ({
 		die $err;
 	    }
 
+	    eval {
+		# Invalidate cached plugin status on configuration changes.
+		PVE::Storage::status_cache()->delete("storage_plugin_status");
+	    };
+	    warn "could not invalidate storage plugin status cache: $@\n" if $@;
+
 	    PVE::Storage::write_config($cfg);
 
 	}, "create storage failed");
@@ -373,6 +379,12 @@ __PACKAGE__->register_method ({
 		    ." in Proxmox VE 9. Use 'create-base-path' or 'create-subdirs' instead.\n"
 	    }
 
+	    eval {
+		# Invalidate cached plugin status on configuration changes.
+		PVE::Storage::status_cache()->delete("storage_plugin_status");
+	    };
+	    warn "could not invalidate storage plugin status cache: $@\n" if $@;
+
 	    PVE::Storage::write_config($cfg);
 
 	}, "update storage failed");
@@ -422,6 +434,12 @@ __PACKAGE__->register_method ({
 
 	    delete $cfg->{ids}->{$storeid};
 
+	    eval {
+		# Invalidate cached plugin status on configuration changes.
+		PVE::Storage::status_cache()->delete("storage_plugin_status");
+	    };
+	    warn "could not invalidate storage plugin status cache: $@\n" if $@;
+
 	    PVE::Storage::write_config($cfg);
 
 	}, "delete storage failed");
diff --git a/src/PVE/API2/Storage/Content.pm b/src/PVE/API2/Storage/Content.pm
index fe0ad4a..cc4f5cd 100644
--- a/src/PVE/API2/Storage/Content.pm
+++ b/src/PVE/API2/Storage/Content.pm
@@ -224,6 +224,11 @@ __PACKAGE__->register_method ({
 					       $param->{format},
 					       $name, $size);
 
+	eval {
+	    # Invalidate cached plugin status on configuration changes.
+	    PVE::Storage::status_cache()->delete("storage_plugin_status");
+	};
+	warn "could not invalidate storage plugin status cache: $@\n" if $@;
 	return $volid;
     }});
 
@@ -459,6 +464,11 @@ __PACKAGE__->register_method ({
 		# Remove log file #318 and notes file #3972 if they still exist
 		PVE::Storage::archive_auxiliaries_remove($path);
 	    }
+	    eval {
+		# Invalidate cached plugin status on configuration changes.
+		PVE::Storage::status_cache()->delete("storage_plugin_status");
+	    };
+	    print "could not invalidate storage plugin status cache: $@\n" if $@;
 	};
 
 	my $id = (defined $ownervm ? "$ownervm@" : '') . $storeid;
@@ -549,6 +559,11 @@ __PACKAGE__->register_method ({
 	    # ssh to connect to local host (which is not needed
 	    my $sshinfo = PVE::SSHInfo::get_ssh_info($target_node);
 	    PVE::Storage::storage_migrate($cfg, $src_volid, $sshinfo, $target_sid, {'target_volname' => $target_volname});
+	    eval {
+		# Invalidate cached plugin status on configuration changes.
+		PVE::Storage::status_cache()->delete("storage_plugin_status");
+	    };
+	    print "could not invalidate storage plugin status cache: $@\n" if $@;
 
 	    print "DEBUG: end worker $upid\n";
 
diff --git a/src/PVE/API2/Storage/Status.pm b/src/PVE/API2/Storage/Status.pm
index b2336e6..e048975 100644
--- a/src/PVE/API2/Storage/Status.pm
+++ b/src/PVE/API2/Storage/Status.pm
@@ -85,6 +85,12 @@ __PACKAGE__->register_method ({
 		optional => 1,
 		default => 0,
 	    },
+	    'ignore-cache' => {
+		description => "Ignore cached storage plugin status",
+		type => 'boolean',
+		optional => 1,
+		default => 0,
+	    },
 	},
     },
     returns => {
@@ -158,7 +164,12 @@ __PACKAGE__->register_method ({
 
 	my $cfg = PVE::Storage::config();
 
-	my $info = PVE::Storage::storage_info($cfg, $param->{content}, $param->{format});
+	my $info = PVE::Storage::storage_info(
+	    $cfg,
+	    $param->{content},
+	    $param->{format},
+	    $param->{'ignore-cache'}
+	);
 
 	raise_param_exc({ storage => "No such storage." })
 	    if $param->{storage} && !defined($info->{$param->{storage}});
@@ -252,6 +263,12 @@ __PACKAGE__->register_method ({
 	properties => {
 	    node => get_standard_option('pve-node'),
 	    storage => get_standard_option('pve-storage-id'),
+	    'ignore-cache' => {
+		description => "Ignore cached storage plugin status",
+		type => 'boolean',
+		optional => 1,
+		default => 0,
+	    },
 	},
     },
     returns => {
@@ -263,7 +280,12 @@ __PACKAGE__->register_method ({
 
 	my $cfg = PVE::Storage::config();
 
-	my $info = PVE::Storage::storage_info($cfg, $param->{content});
+	my $info = PVE::Storage::storage_info(
+	    $cfg,
+	    $param->{content},
+	    undef,
+	    $param->{'ignore-cache'}
+	);
 
 	my $data = $info->{$param->{storage}};
 
@@ -528,6 +550,12 @@ __PACKAGE__->register_method ({
 	    unlink $tmpfilename; # the temporary file got only uploaded locally, no need to rm remote
 	    warn "unable to clean up temporary file '$tmpfilename' - $!\n" if $! && $! != ENOENT;
 
+	    eval {
+		# Invalidate cached plugin status on configuration changes.
+		PVE::Storage::status_cache()->delete("storage_plugin_status");
+	    };
+	    print "could not invalidate storage plugin status cache: $@\n" if $@;
+
 	    if (my $err = $@) {
 		eval { $err_cleanup->() };
 		warn "$@" if $@;
@@ -663,6 +691,12 @@ __PACKAGE__->register_method({
 		$opts->{decompression_command} = $info->{decompressor};
 	    }
 	    PVE::Tools::download_file_from_url("$path/$filename", $url, $opts);
+
+	    eval {
+		# Invalidate cached plugin status on configuration changes.
+		PVE::Storage::status_cache()->delete("storage_plugin_status");
+	    };
+	    print "could not invalidate storage plugin status cache: $@\n" if $@;
 	};
 
 	my $worker_id = PVE::Tools::encode_text($filename); # must not pass : or the like as w-ID
diff --git a/src/PVE/Storage.pm b/src/PVE/Storage.pm
index 8ad493f..0c5c38f 100755
--- a/src/PVE/Storage.pm
+++ b/src/PVE/Storage.pm
@@ -23,6 +23,7 @@ use PVE::INotify;
 use PVE::RPCEnvironment;
 use PVE::SSHInfo;
 use PVE::RESTEnvironment qw(log_warn);
+use Proxmox::RS::SharedCache;
 
 use PVE::Storage::Plugin;
 use PVE::Storage::DirPlugin;
@@ -115,8 +116,13 @@ our $BACKUP_EXT_RE_2 = qr/\.(tgz|(?:tar|vma)(?:\.(${\PVE::Storage::Plugin::COMPR
 # FIXME remove with PVE 8.0, add versioned breaks for pve-manager
 our $vztmpl_extension_re = $VZTMPL_EXT_RE_1;
 
+
 #  PVE::Storage utility functions
 
+sub status_cache {
+    return Proxmox::RS::SharedCache->new("/run/pvestatd-cache");
+}
+
 sub config {
     return cfs_read_file("storage.cfg");
 }
@@ -1248,7 +1254,7 @@ sub deactivate_volumes {
 }
 
 sub storage_info {
-    my ($cfg, $content, $includeformat) = @_;
+    my ($cfg, $content, $includeformat, $ignore_cache) = @_;
 
     my $ids = $cfg->{ids};
 
@@ -1287,35 +1293,69 @@ sub storage_info {
 	push @$slist, $storeid;
     }
 
-    my $cache = {};
 
-    foreach my $storeid (keys %$ids) {
-	my $scfg = $ids->{$storeid};
+    my $get_plugin_status = sub {
+	my $cache = {};
+	my $status = {};
 
-	next if !$info->{$storeid};
-	next if !$info->{$storeid}->{enabled};
+	foreach my $storeid (keys %$ids) {
+	    my $scfg = $ids->{$storeid};
 
-	my $plugin = PVE::Storage::Plugin->lookup($scfg->{type});
-	if ($includeformat) {
-	    my $pd = $plugin->plugindata();
-	    $info->{$storeid}->{format} = $pd->{format}
-		if $pd->{format};
-	    $info->{$storeid}->{select_existing} = $pd->{select_existing}
-		if $pd->{select_existing};
+	    next if !$info->{$storeid};
+	    next if !$info->{$storeid}->{enabled};
+
+	    my $plugin = PVE::Storage::Plugin->lookup($scfg->{type});
+	    if ($includeformat) {
+		my $pd = $plugin->plugindata();
+		$info->{$storeid}->{format} = $pd->{format}
+		    if $pd->{format};
+		$info->{$storeid}->{select_existing} = $pd->{select_existing}
+		    if $pd->{select_existing};
+	    }
+
+	    eval { activate_storage($cfg, $storeid, $cache); };
+	    if (my $err = $@) {
+		warn $err;
+		next;
+	    }
+	    $status->{$storeid} = eval { $plugin->status($storeid, $scfg, $cache); };
+
+	    my ($total, $avail, $used, $active) = eval { $plugin->status($storeid, $scfg, $cache); };
+	    warn $@ if $@;
+	    $status->{$storeid} = {};
+
+	    $status->{$storeid}->{total} = int($total);
+	    $status->{$storeid}->{avail} = int($avail);
+	    $status->{$storeid}->{used} = int($used);
+	    $status->{$storeid}->{active} = $active;
 	}
 
-	eval { activate_storage($cfg, $storeid, $cache); };
-	if (my $err = $@) {
-	    warn $err;
-	    next;
+	return $status;
+    };
+
+    my $status;
+    if ($ignore_cache) {
+	$status = $get_plugin_status->();
+    } else {
+	eval {
+	    $status = status_cache()->get_or_update("storage_plugin_status", $get_plugin_status, 60)
+	};
+	if ($@) {
+	    warn "could not fetch cached storage plugin status: $@";
+	    $status = $get_plugin_status->();
 	}
+    }
 
-	my ($total, $avail, $used, $active) = eval { $plugin->status($storeid, $scfg, $cache); };
-	warn $@ if $@;
+
+    foreach my $storeid (keys %$ids) {
+	next if !$info->{$storeid};
+	next if !$info->{$storeid}->{enabled};
+	my $active = $status->{$storeid}->{active};
 	next if !$active;
-	$info->{$storeid}->{total} = int($total);
-	$info->{$storeid}->{avail} = int($avail);
-	$info->{$storeid}->{used} = int($used);
+
+	$info->{$storeid}->{total} = $status->{$storeid}->{total};
+	$info->{$storeid}->{avail} = $status->{$storeid}->{avail};
+	$info->{$storeid}->{used} = $status->{$storeid}->{used};
 	$info->{$storeid}->{active} = $active;
     }
 
-- 
2.39.2