From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gate001.proxmox.com (gate001.proxmox.com [45.144.208.40]) by lore.proxmox.com (Postfix) with ESMTPS id A33ED1FF0AD for ; Fri, 21 Aug 2026 11:52:22 +0200 (CEST) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id 5A1C821587; Fri, 21 Aug 2026 11:52:22 +0200 (CEST) Mime-Version: 1.0 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=UTF-8 Date: Fri, 21 Aug 2026 11:52:17 +0200 Message-Id: Subject: Re: [PATCH datacenter-manager v2 09/20] context: establish PdmApplication object From: "Thomas Ellmenreich" To: "Lukas Wagner" , X-Mailer: aerc 0.20.0 References: <20260820145220.418032-1-l.wagner@proxmox.com> <20260820145220.418032-10-l.wagner@proxmox.com> In-Reply-To: <20260820145220.418032-10-l.wagner@proxmox.com> X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1787305910973 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.723 Adjusted score from AWL reputation of From: address DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment (newer systems) RCVD_IN_DNSWL_MED -2.3 Sender listed at https://www.dnswl.org/, medium trust SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Message-ID-Hash: ICWH6A2MT542PKM5E6K4HDY7WVPPE6NC X-Message-ID-Hash: ICWH6A2MT542PKM5E6K4HDY7WVPPE6NC X-MailFrom: t.ellmenreich@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 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: Big fan of this change, but I am wondering what the future plans are with a= ll the remaining static values? Just slowly switch them out for state as we go= ? Two comments below | v On Thu Aug 20, 2026 at 4:52 PM CEST, Lukas Wagner wrote: > PdmApplication bundles the client factory, remote config, subscription > key config, and product config behind a single cloneable handle. This > replaces the growing set of independent global statics with one object > that can be assembled differently for production, the fake-remote > feature, and integration tests. > > It is reachable through a new context::pdm_application() accessor, > built by the same context::init() call that already seeds the > existing remote/subscription config globals. Later commits in this > series thread it through the API handlers via the router's shared > state instead of this accessor. > > Signed-off-by: Lukas Wagner > --- > server/src/bin/proxmox-datacenter-api/main.rs | 9 +- > .../bin/proxmox-datacenter-privileged-api.rs | 7 +- > server/src/connection.rs | 54 ++++----- > server/src/context/mod.rs | 107 +++++++++++++++-- > server/src/context/product_config.rs | 111 ++++++++++++++++++ > server/src/metric_collection/mod.rs | 6 +- > .../remote_collection_task.rs | 47 ++++---- > 7 files changed, 274 insertions(+), 67 deletions(-) > create mode 100644 server/src/context/product_config.rs > [snip] > diff --git a/server/src/context/mod.rs b/server/src/context/mod.rs > index 24d653c3..bbf5f53e 100644 > --- a/server/src/context/mod.rs > +++ b/server/src/context/mod.rs > @@ -2,9 +2,10 @@ > //! > //! Make sure to call `init` *once* when starting up the API server. > =20 > -use std::sync::Arc; > +use std::sync::{Arc, OnceLock}; > =20 > use anyhow::Error; > + > use pdm_config::{remotes::RemoteConfig, subscriptions::SubscriptionKeyCo= nfig}; > =20 > use crate::connection::{self, ClientFactory}; > @@ -15,16 +16,37 @@ mod faked_remotes; > #[cfg(not(remote_config =3D "faked"))] > mod default; > =20 > +pub mod product_config; > + > +use product_config::ProductConfig; > + > +static APP: OnceLock =3D OnceLock::new(); > + > /// Dependency-inject concrete implementations needed at runtime. > -pub fn init() -> Result<(), Error> { > +pub fn init() -> Result { > let factory =3D context_factory()?; > =20 > - pdm_config::subscriptions::init(factory.make_subscription_key_config= ()?); > - pdm_config::remotes::init(factory.make_remote_config()?); > - // FIXME: Rather let connection use an Application context object fr= om here > - connection::init(Box::new(connection::DefaultClientFactory)); > + let app =3D factory.make_pdm_application()?; > + APP.set(app.clone()) > + .map_err(|_| anyhow::format_err!("context::init was already call= ed"))?; > =20 > - Ok(()) > + // NOTE: This is technically a second, independent instance of the r= emote config. > + // Long-term, we'd like to get rid of the global instance handle in = pdm_config::remotes > + // anyway, and the implementation is stateless, so having this secon= d > + // instance is not an issue *currently*. > + pdm_config::remotes::init(factory.make_remote_config()?); > + pdm_config::subscriptions::init(factory.make_subscription_key_config= ()?); > + > + Ok(app) > +} > + > +/// Retrieve a handle to [`PdmApplication`] for this server. > +/// > +/// Prefer to retrieve this via the API handler using [`proxmox_router::= State`]. > +pub fn pdm_application() -> PdmApplication { > + APP.get() > + .expect("context::init was not called to set up the application = context object") > + .clone() > } Considering that reducing the number of statics is advantageous for better testing, maybe this function could be marked as `#[deprecated]` just to mak= e sure that it is not used unless completely necessary? Although this would lead to you also having to add a bunch of `#[allow(deprecated)]` to the current uses. > =20 > pub trait ContextFactory { > @@ -43,6 +65,30 @@ pub trait ContextFactory { > pdm_config::subscriptions::DefaultSubscriptionKeyConfig, > )) > } > + > + fn make_product_config(&self) -> Result { > + let product_config =3D ProductConfig::builder() > + .api_user(pdm_config::api_user()?) > + .priv_user(pdm_config::priv_user()?) > + .config_dir(pdm_buildcfg::configdir!("/")) > + .state_dir(pdm_buildcfg::statedir!("/")) > + .run_dir(pdm_buildcfg::rundir!("/")) > + .cache_dir(pdm_buildcfg::PDM_CACHE_DIR) > + .build()?; > + > + Ok(product_config) > + } > + > + fn make_pdm_application(&self) -> Result { > + Ok(PdmApplication { > + inner: Arc::new(PdmApplicationInner { > + client_factory: self.make_client_factory()?, > + remote_config: self.make_remote_config()?, > + subscription_key_config: self.make_subscription_key_conf= ig()?, > + product_config: self.make_product_config()?, > + }), > + }) > + } > } > =20 > fn context_factory() -> Result { > @@ -55,3 +101,50 @@ fn context_factory() -> Result { > Ok(default::DefaultContextFactory) > } > } > + > +/// Application context handle. > +/// > +/// This type gives access to dependency-injected implementations and ge= neral product > +/// configuration. > +/// > +/// This implements [`Clone`] and can be cheaply copied (it contains a s= ingle `Arc`). > +#[derive(Clone)] > +pub struct PdmApplication { > + inner: Arc, > +} I might be misunderstanding something, but if I have understood the implementation correctly, we do not have to have a single big object that contains all dependency injected implementation, right? My first impression is that by registering all of the contained objects as their own states in the `SharedStateRegistry`, one could define API functions that explicitly define the exact state they are interested in? That might also have advantages for testing, as one could very easily tell what is actually needed to test a specific API route, instead of always having to provide a whole `TestApplication`. ( even if most of the containe= d values are just dummy values ;) ). That said, doing so would also mean a lot more boilerplate, so I'm not completely sold on my own idea, just wanted to ask about it. > + > +impl PdmApplication { > + /// Get a handle to the [`ClientFactory`] as an [`Arc`]. > + /// > + /// Prefer to use [`Self::client_factory`] if possible. > + pub fn client_factory_shared(&self) -> Arc { > + Arc::clone(&self.inner.client_factory) > + } > + > + /// Get a reference to the [`ClientFactory`]. > + pub fn client_factory(&self) -> &(dyn ClientFactory + Send + Sync) { > + self.inner.client_factory.as_ref() > + } > + > + /// Get a reference to the [`RemoteConfig`]. > + pub fn remote_config(&self) -> &(dyn RemoteConfig + Send + Sync) { > + self.inner.remote_config.as_ref() > + } > + > + /// Get a reference to the [`SubscriptionKeyConfig`]. > + pub fn subscription_key_config(&self) -> &(dyn SubscriptionKeyConfig= + Send + Sync) { > + self.inner.subscription_key_config.as_ref() > + } > + > + /// Get a reference to the [`ProductConfig`]. > + pub fn product_config(&self) -> &ProductConfig { > + &self.inner.product_config > + } > +} > + > +struct PdmApplicationInner { > + client_factory: Arc, > + remote_config: Box, > + subscription_key_config: Box, > + product_config: ProductConfig, > +} > diff --git a/server/src/context/product_config.rs b/server/src/context/pr= oduct_config.rs > new file mode 100644 > index 00000000..3c1f44ab > --- /dev/null > +++ b/server/src/context/product_config.rs > @@ -0,0 +1,111 @@ > +// NOTE: This is probably generic enough to be moved to proxmox-product-= config. > + > +use std::path::{Path, PathBuf}; > + > +use anyhow::Error; > +use nix::unistd::User; > + > +#[derive(Clone, Debug)] > +pub struct ProductConfig { > + api_user: User, > + priv_user: User, > + config_dir: PathBuf, > + state_dir: PathBuf, > + run_dir: PathBuf, > + cache_dir: PathBuf, > +} > + > +impl ProductConfig { > + pub fn builder() -> ProductConfigBuilder { > + ProductConfigBuilder::default() > + } > + > + pub fn api_user(&self) -> &User { > + &self.api_user > + } > + > + pub fn priv_user(&self) -> &User { > + &self.priv_user > + } > + > + pub fn config_dir(&self) -> &Path { > + &self.config_dir > + } > + > + pub fn state_dir(&self) -> &Path { > + &self.state_dir > + } > + > + pub fn run_dir(&self) -> &Path { > + &self.run_dir > + } > + > + pub fn cache_dir(&self) -> &Path { > + &self.cache_dir > + } > +} [snip]