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

[Thomas Lamprecht <t.lamprecht@proxmox.com>]

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.

> There are include/exclude rules for the following properties:
>   - (global) resource-id

(host)name would be good to have too from the start.

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

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

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




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