* [pve-devel] [PATCH v2 proxmox 1/7] sys: fs: remove unnecessary clippy allow directive
2023-09-28 11:50 [pve-devel] [PATCH v2 storage/proxmox{, -perl-rs} 0/7] cache storage plugin status for pvestatd/API status update calls Lukas Wagner
@ 2023-09-28 11:50 ` Lukas Wagner
2023-09-28 11:50 ` [pve-devel] [PATCH v2 proxmox 2/7] sys: fs: let CreateOptions::apply_to take RawFd instead of File Lukas Wagner
` (5 subsequent siblings)
6 siblings, 0 replies; 8+ messages in thread
From: Lukas Wagner @ 2023-09-28 11:50 UTC (permalink / raw)
To: pve-devel
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
^ permalink raw reply [flat|nested] 8+ messages in thread* [pve-devel] [PATCH v2 proxmox 2/7] sys: fs: let CreateOptions::apply_to take RawFd instead of File
2023-09-28 11:50 [pve-devel] [PATCH v2 storage/proxmox{, -perl-rs} 0/7] cache storage plugin status for pvestatd/API status update calls Lukas Wagner
2023-09-28 11:50 ` [pve-devel] [PATCH v2 proxmox 1/7] sys: fs: remove unnecessary clippy allow directive Lukas Wagner
@ 2023-09-28 11:50 ` Lukas Wagner
2023-09-28 11:50 ` [pve-devel] [PATCH v2 proxmox 3/7] sys: fs: use inline formatting for bail! macro Lukas Wagner
` (4 subsequent siblings)
6 siblings, 0 replies; 8+ messages in thread
From: Lukas Wagner @ 2023-09-28 11:50 UTC (permalink / raw)
To: pve-devel; +Cc: Lukas Wagner, Wolfgang Bumiller
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
^ permalink raw reply [flat|nested] 8+ messages in thread* [pve-devel] [PATCH v2 proxmox 3/7] sys: fs: use inline formatting for bail! macro
2023-09-28 11:50 [pve-devel] [PATCH v2 storage/proxmox{, -perl-rs} 0/7] cache storage plugin status for pvestatd/API status update calls Lukas Wagner
2023-09-28 11:50 ` [pve-devel] [PATCH v2 proxmox 1/7] sys: fs: remove unnecessary clippy allow directive Lukas Wagner
2023-09-28 11:50 ` [pve-devel] [PATCH v2 proxmox 2/7] sys: fs: let CreateOptions::apply_to take RawFd instead of File Lukas Wagner
@ 2023-09-28 11:50 ` Lukas Wagner
2023-09-28 11:50 ` [pve-devel] [PATCH v2 proxmox 4/7] sys: add make_tmp_dir Lukas Wagner
` (3 subsequent siblings)
6 siblings, 0 replies; 8+ messages in thread
From: Lukas Wagner @ 2023-09-28 11:50 UTC (permalink / raw)
To: pve-devel
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
^ permalink raw reply [flat|nested] 8+ messages in thread* [pve-devel] [PATCH v2 proxmox 4/7] sys: add make_tmp_dir
2023-09-28 11:50 [pve-devel] [PATCH v2 storage/proxmox{, -perl-rs} 0/7] cache storage plugin status for pvestatd/API status update calls Lukas Wagner
` (2 preceding siblings ...)
2023-09-28 11:50 ` [pve-devel] [PATCH v2 proxmox 3/7] sys: fs: use inline formatting for bail! macro Lukas Wagner
@ 2023-09-28 11:50 ` Lukas Wagner
2023-09-28 11:50 ` [pve-devel] [PATCH v2 proxmox 5/7] cache: add new crate 'proxmox-shared-cache' Lukas Wagner
` (2 subsequent siblings)
6 siblings, 0 replies; 8+ messages in thread
From: Lukas Wagner @ 2023-09-28 11:50 UTC (permalink / raw)
To: pve-devel
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
^ permalink raw reply [flat|nested] 8+ messages in thread* [pve-devel] [PATCH v2 proxmox 5/7] cache: add new crate 'proxmox-shared-cache'
2023-09-28 11:50 [pve-devel] [PATCH v2 storage/proxmox{, -perl-rs} 0/7] cache storage plugin status for pvestatd/API status update calls Lukas Wagner
` (3 preceding siblings ...)
2023-09-28 11:50 ` [pve-devel] [PATCH v2 proxmox 4/7] sys: add make_tmp_dir Lukas Wagner
@ 2023-09-28 11:50 ` Lukas Wagner
2023-09-28 11:50 ` [pve-devel] [PATCH v2 proxmox-perl-rs 6/7] cache: add bindings for `SharedCache` Lukas Wagner
2023-09-28 11:50 ` [pve-devel] [PATCH v2 pve-storage 7/7] stats: api: cache storage plugin status Lukas Wagner
6 siblings, 0 replies; 8+ messages in thread
From: Lukas Wagner @ 2023-09-28 11:50 UTC (permalink / raw)
To: pve-devel
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
^ permalink raw reply [flat|nested] 8+ messages in thread* [pve-devel] [PATCH v2 proxmox-perl-rs 6/7] cache: add bindings for `SharedCache`
2023-09-28 11:50 [pve-devel] [PATCH v2 storage/proxmox{, -perl-rs} 0/7] cache storage plugin status for pvestatd/API status update calls Lukas Wagner
` (4 preceding siblings ...)
2023-09-28 11:50 ` [pve-devel] [PATCH v2 proxmox 5/7] cache: add new crate 'proxmox-shared-cache' Lukas Wagner
@ 2023-09-28 11:50 ` Lukas Wagner
2023-09-28 11:50 ` [pve-devel] [PATCH v2 pve-storage 7/7] stats: api: cache storage plugin status Lukas Wagner
6 siblings, 0 replies; 8+ messages in thread
From: Lukas Wagner @ 2023-09-28 11:50 UTC (permalink / raw)
To: pve-devel
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
^ permalink raw reply [flat|nested] 8+ messages in thread* [pve-devel] [PATCH v2 pve-storage 7/7] stats: api: cache storage plugin status
2023-09-28 11:50 [pve-devel] [PATCH v2 storage/proxmox{, -perl-rs} 0/7] cache storage plugin status for pvestatd/API status update calls Lukas Wagner
` (5 preceding siblings ...)
2023-09-28 11:50 ` [pve-devel] [PATCH v2 proxmox-perl-rs 6/7] cache: add bindings for `SharedCache` Lukas Wagner
@ 2023-09-28 11:50 ` Lukas Wagner
6 siblings, 0 replies; 8+ messages in thread
From: Lukas Wagner @ 2023-09-28 11:50 UTC (permalink / raw)
To: pve-devel
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
^ permalink raw reply [flat|nested] 8+ messages in thread