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 5C5251FF0AA for ; Fri, 21 Aug 2026 14:26:37 +0200 (CEST) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id 36B0E21592; Fri, 21 Aug 2026 14:26:37 +0200 (CEST) Content-Type: text/plain; charset=UTF-8 Date: Fri, 21 Aug 2026 14:26:30 +0200 Message-Id: To: "Thomas Ellmenreich" , "Lukas Wagner" , Subject: Re: [PATCH datacenter-manager v2 09/20] context: establish PdmApplication object From: "Lukas Wagner" Mime-Version: 1.0 Content-Transfer-Encoding: quoted-printable X-Mailer: aerc 0.21.0-0-g5549850facc2-dirty References: <20260820145220.418032-1-l.wagner@proxmox.com> <20260820145220.418032-10-l.wagner@proxmox.com> In-Reply-To: X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1787315163691 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.683 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: OLOMEEICZWS3TSERDTGMIBFATCDXG3LQ X-Message-ID-Hash: OLOMEEICZWS3TSERDTGMIBFATCDXG3LQ 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: On Fri Aug 21, 2026 at 11:52 AM CEST, Thomas Ellmenreich wrote: > 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? Yes, that would be the general idea. For obvious reasons I did not change everything at once, since the entire idea is still in the 'proposal' stage. I'm convinced that it is a good idea, but we need some kind of consensus on this being the direction we want to move towards, at least in PDM, since having *two* different approaches side-by-side would be quite harmful in the long run. In PDM, the effort for converting the remaining code to this approach should be manageable; more challenging is obviously everything that is shared between PDM and other products. At least for *new* shared crates we should consider avoiding having static globals, and rather pass any context along with calls to the library. For proxmox_notify, where I'm the main maintainer, I'm actually preparing a patch series for doing just that, which actually led me to pursue the ideas presented in this patch series again. > > Two comments below > | > v > > On Thu Aug 20, 2026 at 4:52 PM CEST, Lukas Wagner wrote: >> + >> +/// 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 bette= r > testing, maybe this function could be marked as `#[deprecated]` just to m= ake > 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. > Yeah, I considered marking this as deprecated, however whether this would make sense kind of depends on how long this 'transition' period is, where we need the accessor in the first place. What I don't want is spamming the build logs with deprecation warnings for the next foreseeable future. >> =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_con= fig()?, >> + 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 g= eneral product >> +/// configuration. >> +/// >> +/// This implements [`Clone`] and can be cheaply copied (it contains a = single `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 tel= l > what is actually needed to test a specific API route, instead of always > having to provide a whole `TestApplication`. ( even if most of the contai= ned > 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. Good thinking! This is actually something I pondered about for quite some time as well. I guess this is definitely something to explore; the good thing is that we don't have to settle for anything at this stage, we can start with a more monolithic application handle and split later (or the other way round). I guess it also depends on how this pattern will be picked up by shared crates; the `ProductConfig` type is probably 100% product-agnostic and could be useful in shared implementations, then it could make sense to register this type on its own, so that other crates defining API handlers can request it. On the other hand, maybe the better approach is for each shared crate to define its own context type, and *that* one is then registered in the API... We'll see. Happy to hear other opinions on this as well!