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

["Lukas Wagner" <l.wagner@proxmox.com>]

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 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.
>

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.

>>  
>>  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.

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!