Re: [PATCH datacenter-manager v2 09/20] context: establish PdmApplication object

["Thomas Ellmenreich" <t.ellmenreich@proxmox.com>]

Big fan of this change, but I am wondering what the future plans are with all
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 <l.wagner@proxmox.com>
> ---
>  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.
>  
> -use std::sync::Arc;
> +use std::sync::{Arc, OnceLock};
>  
>  use anyhow::Error;
> +
>  use pdm_config::{remotes::RemoteConfig, subscriptions::SubscriptionKeyConfig};
>  
>  use crate::connection::{self, ClientFactory};
> @@ -15,16 +16,37 @@ mod faked_remotes;
>  #[cfg(not(remote_config = "faked"))]
>  mod default;
>  
> +pub mod product_config;
> +
> +use product_config::ProductConfig;
> +
> +static APP: OnceLock<PdmApplication> = OnceLock::new();
> +
>  /// Dependency-inject concrete implementations needed at runtime.
> -pub fn init() -> Result<(), Error> {
> +pub fn init() -> Result<PdmApplication, Error> {
>      let factory = context_factory()?;
>  
> -    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 from here
> -    connection::init(Box::new(connection::DefaultClientFactory));
> +    let app = factory.make_pdm_application()?;
> +    APP.set(app.clone())
> +        .map_err(|_| anyhow::format_err!("context::init was already called"))?;
>  
> -    Ok(())
> +    // NOTE: This is technically a second, independent instance of the remote 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 second
> +    // 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 make
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.

>  
>  pub trait ContextFactory {
> @@ -43,6 +65,30 @@ pub trait ContextFactory {
>              pdm_config::subscriptions::DefaultSubscriptionKeyConfig,
>          ))
>      }
> +
> +    fn make_product_config(&self) -> Result<ProductConfig, Error> {
> +        let product_config = 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<PdmApplication, Error> {
> +        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_config()?,
> +                product_config: self.make_product_config()?,
> +            }),
> +        })
> +    }
>  }
>  
>  fn context_factory() -> Result<impl ContextFactory, Error> {
> @@ -55,3 +101,50 @@ fn context_factory() -> Result<impl ContextFactory, Error> {
>          Ok(default::DefaultContextFactory)
>      }
>  }
> +
> +/// Application context handle.
> +///
> +/// This type gives access to dependency-injected implementations and general product
> +/// configuration.
> +///
> +/// This implements [`Clone`] and can be cheaply copied (it contains a single `Arc`).
> +#[derive(Clone)]
> +pub struct PdmApplication {
> +    inner: Arc<PdmApplicationInner>,
> +}

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 contained
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<dyn ClientFactory + Send + Sync> {
> +        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<dyn ClientFactory + Send + Sync>,
> +    remote_config: Box<dyn RemoteConfig + Send + Sync>,
> +    subscription_key_config: Box<dyn SubscriptionKeyConfig + Send + Sync>,
> +    product_config: ProductConfig,
> +}
> diff --git a/server/src/context/product_config.rs b/server/src/context/product_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]