Re: [PATCH datacenter-manager 1/4] add persistent, generic, namespaced key-value cache implementation

["Lukas Wagner" <l.wagner@proxmox.com>]

On Tue May 12, 2026 at 2:29 PM CEST, Wolfgang Bumiller wrote:
>> +
>> +impl<T> CacheEntry<T> {
>> +    fn is_expired(&self, now: i64, max_age: i64) -> bool {
>> +        if max_age == 0 {
>> +            return true;
>> +        }
>> +
>> +        let diff = now - self.timestamp;
>> +        diff >= max_age || diff < 0
>> +    }
>> +}
>> +
>> +pub struct NamespacedCache {
>> +    base_path: PathBuf,
>> +    file_options: CreateOptions,
>> +    dir_options: CreateOptions,
>> +}
>> +
>> +impl NamespacedCache {
>> +    /// Create a new cache instance.
>> +    pub fn new<P: AsRef<Path>>(
>
> Consider `Into<PathBuf>` - since you force a clone on `PathBuf` now.
>

Done, thanks!

>> +        base_path: P,
>> +        dir_options: CreateOptions,
>> +        file_options: CreateOptions,
>> +    ) -> Self {
>> +        Self {
>> +            base_path: base_path.as_ref().into(),
>> +            dir_options,
>> +            file_options,
>> +        }
>> +    }
>> +

[...]

>> +
>> +fn get_impl<T: Serialize + DeserializeOwned>(
>> +    base: &Path,
>> +    namespace: &str,
>> +    key: &str,
>> +    max_age: Option<i64>,
>> +) -> Result<Option<T>, CacheError> {
>> +    let path = get_path(base, namespace, key);
>> +    let content = proxmox_sys::fs::file_read_optional_string(path)?;
>> +
>> +    if let Some(content) = content {
>> +        let val = serde_json::from_str::<CacheEntry<T>>(&content)?;
>> +
>> +        if let Some(max_age) = max_age {
>> +            if val.is_expired(proxmox_time::epoch_i64(), max_age) {
>> +                return Ok(None);
>> +            }
>> +        }
>> +        return Ok(Some(val.value));
>> +    }
>> +
>> +    return Ok(None);
>> +}
>> +
>> +fn get_path(base: &Path, namespace: &str, key: &str) -> PathBuf {
>> +    let mut path = base.join(namespace).join(key);
>> +    path.set_extension("json");
>
> ^ I'd rather join base with `format!("{key}.json")`, otherwise if a
> `key` contains dots, it'll lose its final component.

Did that for the next revision, thanks!

>
> Alternatively we could just skip the extension altogether.
>
> Ah and I think we might want to `panic` (or fail) if
> `namespace.starts_with('/')`, that would cause discard `base` to be
> discarded! (I'm leaning towards panicking since this isn't something
> that should ever come from the outside (I hope…), at least currently
> it's all `const`s…)

Added, thanks!

I went with a failure, since I added some basic checks on keys and
namespaces anyway (e.g. prohibiting '../'). I added new error variants
InvalidNamespace and InvalidKey for this.

>
>> +    path
>> +}
>> +
>> +fn get_lockfile(base: &Path, namespace: &str) -> PathBuf {
>> +    let path = base.join(format!(".{namespace}.lock"));
>> +    path
>> +}
>> +
>> +#[cfg(test)]
>> +mod tests {
>> +    use tempfile::TempDir;
>> +
>> +    use super::*;
>> +
>> +    fn make_cache() -> (TempDir, NamespacedCache) {
>> +        let dir = tempfile::tempdir().unwrap();
>
> (I wonder if we should use `tempdir_in` and pass `$OUT_DIR` or some
> other dir cargo provides via env vars for these things?)

Unfortunately, cargo does not seem to set OUT_DIR when `cargo test` is
used. According to [1], there is CARGO_TARGET_TMPDIR, but that one is
only set for *integration* tests, not *unit* tests.

Once we put this cache impl into a shared crate, I'd likely promote the
test cases to be integration tests anyway, then we can consider
switching the directory. For now, I'll just keep on using `tmpdir`.

[1] https://doc.rust-lang.org/cargo/reference/environment-variables.html