[pdm-devel] superseded: [PATCH proxmox-datacenter-manager v2 00/28] metric collection improvements (concurrency, config, API, CLI)

[Lukas Wagner]

Sorry for the late reply.

On  2025-03-16 22:45, Thomas Lamprecht wrote:
> On 14/03/2025 15:10, Lukas Wagner wrote:
>> On  2025-02-14 14:06, Lukas Wagner wrote:
>>> ## To reviewers / open questions:
>>> - Please review the defaults I've chosen for the settings, especially
>>>   the ones for the default metric collection interval (10 minutes) as
>>>   well as max-concurrency (10).
>>>   I also kindly ask to double-check the naming of the properties.
>>>   See "pdm-api-types: add CollectionSettings type" for details
>>>
>>> - Please review path and params for new API endpoints (anything public
>>>   facing that is hard to change later)
>>>
>>> - I've chosen a section-config config now, even though we only have a
>>>   single section for now. This was done for future-proofing reasons,
>>>   maybe we want to add support for different setting 'groups' or
>>>   something, e.g. to have different settings for distinct sets of
>>>   remotes. Does this make sense?
>>>   Or should I just stick to a simple config for now? (At moments like
>>>   these I wish for TOML configs where we could be a bit more flexible...)
>>>
>>> 	collection-settings: default
>>> 	    max-concurrency 10
>>> 	    collection-interval 180
>>> 	    min-interval-offset 0
>>> 	    max-interval-offset 20
>>> 	    min-connection-delay 10
>>> 	    max-connection-delay 100
>>>
>>
>> Currently thinking about generalizing the `max-concurrency` setting into something global
>> that affects all 'background' polling operations (resource cache/task cache refreshes/
>> metric fetching).
> 
> The usefulness of such a thing depends a bit on what we want to
> limit (amount of total requests to a target remote?), and why (reducing
> network traffic, load on target node, load on PDM, ...?)
> 
I think it makes sense to think about this from the following perspective:
It makes sense to limit concurrency (we already do), and if one limits
concurrency, I think that should be done globally, not per task.
Otherwise the total number of connections at a time might depended
on how polling tasks are scheduled (e.g. if they happen to run at the same
time or not, which might lead to hard to predict load spikes)

Later we could have separate settings for 'groups of remotes', e.g. to limit
the number of concurrent connections via some slow VPN tunnel to some data center that
houses a couple, but not all remotes.

>>
>> For actually controlling the concurrency we could maybe have a globally available
>> semaphore (potential deadlock potential in some cases).
>> Alternatively, we could think about having a 'background request queue' and
>> a 'background request scheduler', which does the actual requests on behalf of
>> the other tasks.
> 
> semaphores sound nice but IMO often aren't, especially as they lack
> introspection, I'm sure that's better in rust than in C, but  a dedicated
> queueing mechanism _might_ be nicer, especially as one can then also
> use more useful approaches to queue/schedule things. And, e.g., once our
> target APIs support something nice as QUIC we could even batch requests
> to a single target (remote) together.

After playing around with some ideas, I'll probably go with a semaphore at first, but
abstracted in a way that it should be quite easy to change to something more complex later.


> 
>>
>> I'll give this a bit more thoughts in the next days/weeks, so maybe don't merge
>> this in the meanwhile.
>>
> 
> ack.

I've sent a [v3] with some of the settings which would potentially be changed again
by a request queue/scheduler removed, namely:
  - max-concurrency
  - *-interval-offset
  - *-connection-delay

I'll be on vacation soon and I don't think I can whip out the request queue thingy
before that, and I didn't want to keep this patch series in a limbo state until I return :)
Hopefully v3 is ready to be applied and I can then build on that later.

[v3]: https://lore.proxmox.com/pdm-devel/20250416125642.291552-1-l.wagner@proxmox.com/T/#t

-- 
- Lukas