From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gate001.proxmox.com (gate001.proxmox.com [IPv6:2a0f:8001:1:32::40]) by lore.proxmox.com (Postfix) with ESMTPS id 1A7521FF0A8 for ; Thu, 20 Aug 2026 16:53:17 +0200 (CEST) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id 4DD85215AA; Thu, 20 Aug 2026 16:52:56 +0200 (CEST) From: Lukas Wagner To: pdm-devel@lists.proxmox.com Subject: [PATCH datacenter-manager v2 09/20] context: establish PdmApplication object Date: Thu, 20 Aug 2026 16:52:09 +0200 Message-ID: <20260820145220.418032-10-l.wagner@proxmox.com> X-Mailer: git-send-email 2.47.3 In-Reply-To: <20260820145220.418032-1-l.wagner@proxmox.com> References: <20260820145220.418032-1-l.wagner@proxmox.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1787237518909 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.702 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 T_FILL_THIS_FORM_SHORT 0.01 Fill in a short form with personal information Message-ID-Hash: BDRNZMP3TH7J5KE6SDNM6AZ64356XEWL X-Message-ID-Hash: BDRNZMP3TH7J5KE6SDNM6AZ64356XEWL 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 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: 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 diff --git a/server/src/bin/proxmox-datacenter-api/main.rs b/server/src/bin/proxmox-datacenter-api/main.rs index 57aa6e22..8ae7192c 100644 --- a/server/src/bin/proxmox-datacenter-api/main.rs +++ b/server/src/bin/proxmox-datacenter-api/main.rs @@ -28,6 +28,7 @@ use proxmox_auth_api::api::assemble_csrf_prevention_token; use server::auth; use server::auth::csrf::csrf_secret; +use server::context::PdmApplication; use server::metric_collection; use server::resource_cache; use server::task_utils; @@ -67,9 +68,9 @@ fn main() -> Result<(), Error> { } proxmox_product_config::init(pdm_config::api_user()?, pdm_config::priv_user()?); - server::context::init()?; + let app = server::context::init()?; - proxmox_async::runtime::main(run(debug)) + proxmox_async::runtime::main(run(app, debug)) } async fn get_index_future(env: RestEnvironment, parts: Parts) -> Response { @@ -144,7 +145,7 @@ async fn get_index_future(env: RestEnvironment, parts: Parts) -> Response Result<(), Error> { +async fn run(app: PdmApplication, debug: bool) -> Result<(), Error> { auth::init(false); proxmox_acme_api::init(configdir!("/acme"), false)?; @@ -339,7 +340,7 @@ async fn run(debug: bool) -> Result<(), Error> { }); start_task_scheduler(); - metric_collection::start_task()?; + metric_collection::start_task(app)?; tasks::remote_node_mapping::start_task(); resource_cache::start_task(); tasks::remote_tasks::start_task()?; diff --git a/server/src/bin/proxmox-datacenter-privileged-api.rs b/server/src/bin/proxmox-datacenter-privileged-api.rs index 59d30513..e1de53c3 100644 --- a/server/src/bin/proxmox-datacenter-privileged-api.rs +++ b/server/src/bin/proxmox-datacenter-privileged-api.rs @@ -14,6 +14,7 @@ use proxmox_rest_server::{ApiConfig, RestServer}; use proxmox_router::RpcEnvironmentType; use proxmox_sys::fs::CreateOptions; +use server::context::PdmApplication; use server::{api_cache, auth}; use pdm_buildcfg::configdir; @@ -54,9 +55,9 @@ fn main() -> Result<(), Error> { } } - server::context::init()?; + let app = server::context::init()?; - proxmox_async::runtime::main(run()) + proxmox_async::runtime::main(run(app)) } fn create_directories() -> Result<(), Error> { @@ -114,7 +115,7 @@ fn create_directories() -> Result<(), Error> { Ok(()) } -async fn run() -> Result<(), Error> { +async fn run(app: PdmApplication) -> Result<(), Error> { auth::init(true); proxmox_acme_api::init(configdir!("/acme"), true)?; diff --git a/server/src/connection.rs b/server/src/connection.rs index a63ea7da..df122836 100644 --- a/server/src/connection.rs +++ b/server/src/connection.rs @@ -7,9 +7,9 @@ use std::collections::HashMap; use std::future::Future; use std::pin::{Pin, pin}; use std::sync::Arc; +use std::sync::LazyLock; use std::sync::Mutex as StdMutex; use std::sync::Once; -use std::sync::{LazyLock, OnceLock}; use std::time::{Duration, SystemTime}; use anyhow::{Error, bail, format_err}; @@ -25,11 +25,10 @@ use proxmox_time::epoch_i64; use pdm_api_types::remotes::{NodeUrl, Remote, RemoteType, TlsProbeOutcome}; use pve_api_types::client::PveClientImpl; +use crate::context; use crate::pbs_client::PbsClient; use crate::remote_cache::ConnectionState; -static INSTANCE: OnceLock> = OnceLock::new(); - /// Connection Info returned from [`prepare_connect_client`] struct ConnectInfo { prefix: String, @@ -404,19 +403,11 @@ impl ClientFactory for DefaultClientFactory { } } -fn instance() -> &'static (dyn ClientFactory + Send + Sync) { - // Not initializing the connection factory instance is - // entirely in our responsibility and not something we can recover from, - // so it should be okay to panic in this case. - INSTANCE - .get() - .expect("client factory instance not set") - .as_ref() -} - /// Create a new API client for PVE remotes pub fn make_pve_client(remote: &Remote) -> Result, Error> { - instance().make_pve_client(remote) + context::pdm_application() + .client_factory() + .make_pve_client(remote) } /// Create a new API client for PVE remotes, but for a specific endpoint @@ -424,21 +415,29 @@ pub fn make_pve_client_with_endpoint( remote: &Remote, target_endpoint: Option<&str>, ) -> Result, Error> { - instance().make_pve_client_with_endpoint(remote, target_endpoint) + context::pdm_application() + .client_factory() + .make_pve_client_with_endpoint(remote, target_endpoint) } /// Create a new API client for PVE remotes and try to make it connect to a specific *node*. pub fn make_pve_client_with_node(remote: &Remote, node: &str) -> Result, Error> { - instance().make_pve_client_with_node(remote, node) + context::pdm_application() + .client_factory() + .make_pve_client_with_node(remote, node) } /// Create a new API client for PBS remotes pub fn make_pbs_client(remote: &Remote) -> Result, Error> { - instance().make_pbs_client(remote) + context::pdm_application() + .client_factory() + .make_pbs_client(remote) } pub fn make_raw_client(remote: &Remote) -> Result, Error> { - instance().make_raw_client(remote) + context::pdm_application() + .client_factory() + .make_raw_client(remote) } /// Create a new API client for PVE remotes. @@ -451,7 +450,10 @@ pub fn make_raw_client(remote: &Remote) -> Result, Error> { /// /// Note: currently does not support two factor authentication. pub async fn make_pve_client_and_login(remote: &Remote) -> Result, Error> { - instance().make_pve_client_and_login(remote).await + context::pdm_application() + .client_factory() + .make_pve_client_and_login(remote) + .await } /// Create a new API client for PBS remotes. @@ -464,16 +466,10 @@ pub async fn make_pve_client_and_login(remote: &Remote) -> Result /// /// Note: currently does not support two factor authentication. pub async fn make_pbs_client_and_login(remote: &Remote) -> Result>, Error> { - instance().make_pbs_client_and_login(remote).await -} - -/// Initialize the [`ClientFactory`] instance. -/// -/// Will panic if the instance has already been set. -pub fn init(instance: Box) { - if INSTANCE.set(instance).is_err() { - panic!("connection factory instance already set"); - } + context::pdm_application() + .client_factory() + .make_pbs_client_and_login(remote) + .await } /// In order to allow the [`MultiClient`] to check the cached reachability state of a client, we 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 = OnceLock::new(); + /// Dependency-inject concrete implementations needed at runtime. -pub fn init() -> Result<(), Error> { +pub fn init() -> Result { 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() } pub trait ContextFactory { @@ -43,6 +65,30 @@ pub trait ContextFactory { pdm_config::subscriptions::DefaultSubscriptionKeyConfig, )) } + + fn make_product_config(&self) -> Result { + 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 { + 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 { @@ -55,3 +101,50 @@ fn context_factory() -> Result { 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, +} + +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/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 + } +} + +#[derive(Default)] +pub struct ProductConfigBuilder { + api_user: Option, + priv_user: Option, + config_dir: Option, + state_dir: Option, + run_dir: Option, + cache_dir: Option, +} + +impl ProductConfigBuilder { + pub fn api_user(mut self, api_user: User) -> Self { + self.api_user = Some(api_user); + self + } + + pub fn priv_user(mut self, priv_user: User) -> Self { + self.priv_user = Some(priv_user); + self + } + + pub fn config_dir(mut self, config_dir: impl Into) -> Self { + self.config_dir = Some(config_dir.into()); + self + } + + pub fn state_dir(mut self, state_dir: impl Into) -> Self { + self.state_dir = Some(state_dir.into()); + self + } + + pub fn run_dir(mut self, run_dir: impl Into) -> Self { + self.run_dir = Some(run_dir.into()); + self + } + + pub fn cache_dir(mut self, cache_dir: impl Into) -> Self { + self.cache_dir = Some(cache_dir.into()); + self + } + + pub fn build(self) -> Result { + Ok(ProductConfig { + api_user: self + .api_user + .ok_or_else(|| anyhow::format_err!("missing api_user"))?, + priv_user: self + .priv_user + .ok_or_else(|| anyhow::format_err!("missing priv_user"))?, + config_dir: self + .config_dir + .ok_or_else(|| anyhow::format_err!("missing config_dir"))?, + state_dir: self + .state_dir + .ok_or_else(|| anyhow::format_err!("missing state_dir"))?, + run_dir: self + .run_dir + .ok_or_else(|| anyhow::format_err!("missing run_dir"))?, + cache_dir: self + .cache_dir + .ok_or_else(|| anyhow::format_err!("missing cache_dir"))?, + }) + } +} diff --git a/server/src/metric_collection/mod.rs b/server/src/metric_collection/mod.rs index 3d7a477d..15b01b9a 100644 --- a/server/src/metric_collection/mod.rs +++ b/server/src/metric_collection/mod.rs @@ -20,6 +20,7 @@ pub mod top_entities; use remote_collection_task::{ControlMsg, RemoteMetricCollectionTask}; use rrd_cache::RrdCache; +use crate::context::PdmApplication; use crate::metric_collection::local_collection_task::LocalMetricCollectionTask; const RRD_CACHE_BASEDIR: &str = concat!(PDM_STATE_DIR_M!(), "/rrdb"); @@ -39,7 +40,7 @@ pub fn init() -> Result<(), Error> { } /// Start the metric collection task. -pub fn start_task() -> Result<(), Error> { +pub fn start_task(app: PdmApplication) -> Result<(), Error> { let (metric_data_tx, metric_data_rx) = mpsc::channel(128); let cache = rrd_cache::get_cache(); @@ -57,7 +58,8 @@ pub fn start_task() -> Result<(), Error> { let metric_data_tx_clone = metric_data_tx.clone(); tokio::spawn(async move { let metric_collection_task_future = pin!(async move { - match RemoteMetricCollectionTask::new(metric_data_tx_clone, trigger_collection_rx) { + match RemoteMetricCollectionTask::new(app, metric_data_tx_clone, trigger_collection_rx) + { Ok(mut task) => task.run().await, Err(err) => log::error!("could not start metric collection task: {err}"), } diff --git a/server/src/metric_collection/remote_collection_task.rs b/server/src/metric_collection/remote_collection_task.rs index d243dcf1..57910d46 100644 --- a/server/src/metric_collection/remote_collection_task.rs +++ b/server/src/metric_collection/remote_collection_task.rs @@ -19,8 +19,9 @@ use proxmox_sys::fs::CreateOptions; use pdm_api_types::remotes::{Remote, RemoteType}; +use crate::context::PdmApplication; use crate::metric_collection::rrd_task::CollectionStats; -use crate::{connection, task_utils}; +use crate::task_utils; use super::{ rrd_task::{RrdStoreRequest, RrdStoreResult}, @@ -48,6 +49,7 @@ pub(super) enum ControlMsg { /// Task which periodically collects metrics from all remotes and stores /// them in the local metrics database. pub(super) struct RemoteMetricCollectionTask { + app: PdmApplication, state: MetricCollectionState, metric_data_tx: Sender, control_message_rx: Receiver, @@ -56,12 +58,14 @@ pub(super) struct RemoteMetricCollectionTask { impl RemoteMetricCollectionTask { /// Create a new metric collection task. pub(super) fn new( + app: PdmApplication, metric_data_tx: Sender, control_message_rx: Receiver, ) -> Result { let state = load_state()?; Ok(Self { + app, state, metric_data_tx, control_message_rx, @@ -234,9 +238,12 @@ impl RemoteMetricCollectionTask { // called on the semaphore. let permit = Arc::clone(&semaphore).acquire_owned().await.unwrap(); + let app_clone = self.app.clone(); + if let Some(remote) = remote_config.get(remote_name).cloned() { log::debug!("fetching remote '{}'", remote.id); handles.spawn(Self::fetch_single_remote( + app_clone, remote, status, self.metric_data_tx.clone(), @@ -294,6 +301,7 @@ impl RemoteMetricCollectionTask { /// Fetch a single remote. #[tracing::instrument(skip_all, fields(remote = remote.id), name = "metric_collection_task")] async fn fetch_single_remote( + app: PdmApplication, remote: Remote, mut status: RemoteStatus, sender: Sender, @@ -307,7 +315,7 @@ impl RemoteMetricCollectionTask { let res: Result = async { match remote.ty { RemoteType::Pve => { - let client = connection::make_pve_client(&remote)?; + let client = app.client_factory().make_pve_client(&remote)?; let metrics = client .cluster_metrics_export( Some(true), @@ -331,7 +339,7 @@ impl RemoteMetricCollectionTask { .await?; } RemoteType::Pbs => { - let client = connection::make_pbs_client(&remote)?; + let client = app.client_factory().make_pbs_client(&remote)?; let metrics = client .metrics(Some(true), Some(status.most_recent_datapoint)) .await?; @@ -386,8 +394,6 @@ pub(super) fn load_state() -> Result { #[cfg(test)] pub(super) mod tests { - use std::sync::Once; - use anyhow::bail; use http::StatusCode; @@ -397,6 +403,7 @@ pub(super) mod tests { use crate::{ connection::{ClientFactory, PveClient}, + context::ContextFactory, metric_collection::rrd_task::RrdStoreResult, pbs_client::PbsClient, test_support::temp::NamedTempFile, @@ -550,25 +557,18 @@ pub(super) mod tests { number_of_requests } - static START: Once = Once::new(); + const NOW: i64 = 1000; - fn test_init() -> i64 { - let now = 10000; - START.call_once(|| { - // TODO: the client factory is currently stored in a OnceLock - - // we can only set it from one test... Ideally we'd like to have the - // option to set it in every single test if needed - task/thread local? - connection::init(Box::new(TestClientFactory { now })); - }); + struct TestContextFactory(); - now + impl ContextFactory for TestContextFactory { + fn make_client_factory(&self) -> Result, Error> { + Ok(Arc::new(TestClientFactory { now: NOW })) + } } #[tokio::test] async fn test_fetch_remotes_updates_state() { - // Arrange - let now = test_init(); - let (tx, rx) = tokio::sync::mpsc::channel(10); let handle = tokio::task::spawn(fake_rrd_task(rx)); @@ -579,7 +579,10 @@ pub(super) mod tests { let (_control_tx, control_rx) = tokio::sync::mpsc::channel(10); + let app = TestContextFactory().make_pdm_application().unwrap(); + let mut task = RemoteMetricCollectionTask { + app, state, metric_data_tx: tx, control_message_rx: control_rx, @@ -608,7 +611,7 @@ pub(super) mod tests { ); assert_eq!(status.last_collection, None); } else { - assert!(now - status.most_recent_datapoint <= 10); + assert!(NOW - status.most_recent_datapoint <= 10); assert!(status.error.is_none()); } } @@ -619,9 +622,6 @@ pub(super) mod tests { #[tokio::test] async fn test_fetch_overdue() { - // Arrange - test_init(); - let (tx, rx) = tokio::sync::mpsc::channel(10); let handle = tokio::task::spawn(fake_rrd_task(rx)); @@ -630,6 +630,8 @@ pub(super) mod tests { let state_file = NamedTempFile::new(get_create_options()).unwrap(); let mut state = MetricCollectionState::new(state_file.path().into(), get_create_options()); + let app = TestContextFactory().make_pdm_application().unwrap(); + let now = proxmox_time::epoch_i64(); // This one should be fetched @@ -652,6 +654,7 @@ pub(super) mod tests { let (_control_tx, control_rx) = tokio::sync::mpsc::channel(10); let mut task = RemoteMetricCollectionTask { + app, state, metric_data_tx: tx, control_message_rx: control_rx, -- 2.47.3