Re: [pdm-devel] [PATCH datacenter-manager v5 04/11] views: add implementation for view resource filtering

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

On Thu Nov 13, 2025 at 8:54 PM CET, Thomas Lamprecht wrote:
> Am 13.11.25 um 13:16 schrieb Lukas Wagner:
>> This commit adds the resource filter implementation for the previously
>> defined ViewConfig type.
>> 
>
> a bit late given that this was already applied, don't get me wrong, I'm
> thankful @Dominik for doing so, just a quick heads-up for coordination would
> have been welcome by my side. Only few higher level comments for potential
> future improvement below.
>

Just to be sure, what would be your preferred way to handle situations
similar to this one these? A quick "Hey, we are about to apply XYZ, any
objections?" ?

I'm still trying to find the right balance for "can *I*
apply this?" and "should this go through the project lead for final
signoff?". In the end, being to liberal when applying patches can lead
to more work for you (and all of us), which is of course something I
want to avoid. Any advice?

>> There are include/exclude rules for the following properties:
>>   - (global) resource-id
>
> (host)name would be good to have too from the start.
>

Good idea. Should be fairly trivial to add.

>>   - resource pool
>>   - resource type
>>   - remote
>>   - tags
>> 
>> The rules are interpreted as follows:
>> - no rules: everything matches
>
> No hard feelings and this definitively works, but it might be a bit clearer
> (and safer!) to require an explicit include "all" rule for that.
>
> Deleting all rules by accident is easier than setting some explicit value,
> and giving out access to all resources can have implications.
> And sure, that's not something frequent or the like, but if there's no
> disadvantage it can be still worth to go for the safer route.
>
> It could be also a specific "include-all" flag, to make matching for it
> easier and avoid that one has to iterate over all loops.
> FWIW, we could still introduce this later if users actually run into it,
> so mostly interested in what you think?
>

Yeah, I understand your point. Given that the matching also impacts
permissions, being a bit more cautious might be appropriate. The
dedicated 'include-all' flag sounds like a good idea. If we want to go
that route, we should probably add it right away.

I'll start a quick follow-up series with such a flag, then we can
continue the discussion there. Should not be much work, as far as I can
tell.

>> - only includes: included resources match
>> - only excluded: everything *but* the excluded resources match
>> - include and exclude: excludes are applied *after* includes, meaning if
>>   one has a `include-remote foo` and `exclude-remote foo` at the same
>>   time, the remote `foo` will never match
>> 
>> Some experiments were performed with a cache that works roughly as following:
>>   - HashMap<ViewId, HashMap<ResourceId, bool>> in a mutex
>>   - Cache invalidated if view config digest changed
>>   - Cache invalidated if certain resource fields such as tags or resource pools change
>>     from the last time (also with a digest-based implementation)
>> 
>> Experimented with the `fake-remote` feature and and 15000 guests showed
>> that caching was only faster than direct evaluation if the number of
>> rules in the ViewConfig is *huge* (e.g. >1000 `include-resource-id`
>> entries). But even for those, direct evaluation was always plenty fast,
>> with evaluation times ~20ms for *all* resources.
>> 
>> -> for any *realistic* view config, we should be good with direct
>> evaluation, as long as we don't add any filter rules which are very
>> expensive to evaluate.
>
> As already mentioned offlist a few weeks ago: I really would like to
> see some flexible matching support (glob, regex, ...?)
>
> I'm fine with that not being done here already, getting the core plumbing
> in place is definitively more important, but I'd like to know how this
> would look like, so doing at least a tiny/simple POC would be IMO good to
> try before bumping 1.0.
>

The off-list discussion didn't explicitly mention regex/globs, but
mentioned reusing the matching from the notification stack, which *does*
support regexes. Since I didn't end up reusing that part, I kind of lost
track of that aspect. Should be added now rather than later, otherwise
adding support for this in the config could become awkward.

Off-list we briefly discussed the issue; you mentioned the option to use
property strings to support the regex mode later on, e.g.

	include op=regex,value=tag:a[a-z]+

FWIW, we could just use the same convention that we already use for
'match-field' for notification matchers:

  include exact:tag=aaaz (where the "exact:" part is optional) [1]
  include regex:tag=a[a-z]+

Could make sense to just use the same syntax to avoid any confusion for
our users. This would require a minor change for the config format, so
we should do that *now*.

If we want to use this syntax, I'd quickly send a follow-up patch for
the config changes so that at least [1] is supported; we can then add
regex support at our convenience.

>> diff --git a/server/src/views/tests.rs b/server/src/views/tests.rs
>> new file mode 100644
>> index 00000000..030b7994
>> --- /dev/null
>> +++ b/server/src/views/tests.rs
>> @@ -0,0 +1,619 @@
>
>> +fn run_test(config: ViewConfig, tests: &[((&str, &Resource), bool)]) {
>> +    let filter = View::new(config);
>> +
>> +    for ((remote_name, resource), expected) in tests {
>> +        eprintln!("remote: {remote_name}, resource: {resource:?}");
>> +        assert_eq!(filter.resource_matches(remote_name, resource), *expected);
>> +    }
>> +}
>> +
>> +const NODE: &str = "somenode";
>> +const STORAGE: &str = "somestorage";
>> +const REMOTE: &str = "someremote";
>> +
>> +#[test]
>> +fn include_remotes() {
>> +    let config = ViewConfig {
>> +        id: "only-includes".into(),
>> +        include: vec![
>> +            FilterRule::Remote("remote-a".into()),
>> +            FilterRule::Remote("remote-b".into()),
>> +        ],
>> +        ..Default::default()
>> +    };
>
> might be nice to have some of the tests also deserialize the config from
> a raw string, as while one can argue that it's section config's job to get
> us from that to the deserialized type, one can still introduce regressions
> outside of the format it self (e.g. typos) and it's IMO also just nice to
> see how the configs look in their serialized form, especially when planning
> to extend this feature-wise.

Agreed.


_______________________________________________
pdm-devel mailing list
pdm-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pdm-devel