From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) by lore.proxmox.com (Postfix) with ESMTPS id 272151FF13A for ; Wed, 13 May 2026 10:45:49 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 17FB648E7; Wed, 13 May 2026 10:45:48 +0200 (CEST) Mime-Version: 1.0 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=UTF-8 Date: Wed, 13 May 2026 10:45:38 +0200 Message-Id: Subject: Re: [PATCH datacenter-manager 1/4] add persistent, generic, namespaced key-value cache implementation From: "Lukas Wagner" To: "Wolfgang Bumiller" , "Lukas Wagner" X-Mailer: aerc 0.21.0-0-g5549850facc2-dirty References: <20260508150330.363622-1-l.wagner@proxmox.com> <20260508150330.363622-2-l.wagner@proxmox.com> <3b7fegzovd2vvukjsctqba73qtdhize7s2xchoko7dq5xngzp6@2x7dunsotkik> In-Reply-To: <3b7fegzovd2vvukjsctqba73qtdhize7s2xchoko7dq5xngzp6@2x7dunsotkik> X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1778661825453 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.054 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more information. [rust-lang.org] Message-ID-Hash: U4AMAO6ZXTIBH2GMCDAV2FPIOZP7RQDV X-Message-ID-Hash: U4AMAO6ZXTIBH2GMCDAV2FPIOZP7RQDV X-MailFrom: l.wagner@proxmox.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; loop; banned-address; emergency; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header CC: pdm-devel@lists.proxmox.com X-Mailman-Version: 3.3.10 Precedence: list List-Id: Proxmox Datacenter Manager development discussion List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: On Tue May 12, 2026 at 2:29 PM CEST, Wolfgang Bumiller wrote: >> + >> +impl CacheEntry { >> + fn is_expired(&self, now: i64, max_age: i64) -> bool { >> + if max_age =3D=3D 0 { >> + return true; >> + } >> + >> + let diff =3D now - self.timestamp; >> + diff >=3D 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>( > > Consider `Into` - 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( >> + base: &Path, >> + namespace: &str, >> + key: &str, >> + max_age: Option, >> +) -> Result, CacheError> { >> + let path =3D get_path(base, namespace, key); >> + let content =3D proxmox_sys::fs::file_read_optional_string(path)?; >> + >> + if let Some(content) =3D content { >> + let val =3D serde_json::from_str::>(&content)?; >> + >> + if let Some(max_age) =3D 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 =3D 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=E2=80=A6), at least curren= tly > it's all `const`s=E2=80=A6) 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 =3D base.join(format!(".{namespace}.lock")); >> + path >> +} >> + >> +#[cfg(test)] >> +mod tests { >> + use tempfile::TempDir; >> + >> + use super::*; >> + >> + fn make_cache() -> (TempDir, NamespacedCache) { >> + let dir =3D 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