Re: [pdm-devel] [PATCH datacenter-manager v2 04/12] views: add implementation for view filters

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

On Wed Nov 5, 2025 at 11:08 AM CET, Dominik Csapak wrote:
>> +
>> +/// Get view filter with a given ID.
>> +///
>> +/// Returns an error if the view filter configuration file could not be read, or
>> +/// if the view filter with the provided ID does not exist.
>> +pub fn get_view_filter(filter_id: &str) -> Result<ViewFilter, Error> {
>> +    pdm_config::views::get_view_filter_config(filter_id).map(ViewFilter::new)
>> +}
>> +
>> +/// View filter implementation.
>> +///
>> +/// Given a [`ViewFilterConfig`], this struct can be used to check if a resource/remote/node
>> +/// matches the filter rules.
>> +#[derive(Clone)]
>> +pub struct ViewFilter {
>> +    config: ViewFilterConfig,
>> +}
>
> wouldn't a newtype suffice here too?
>
> pub struct ViewFilter(ViewFilterConfig)

Personally I'm not the biggest fan of newtypes unless I'm 100%
convinced that the type is only ever going to contain this single
member (simple example: pub struct UnixTimestamp(i64)), just to avoid
having to change any code when I have to introduce another member (e.g.
changing self.0 to self.config). Since it's only a very minor syntactic
thing, I'd be tempted to leave it as is - unless you insist I change it.

>
> ? alternatively, what about having freestanding functions that
> take a `&ViewFilterConfig` as parameter ?
>

I want to shield callers from the ViewFilterConfig itself (also the
reason why the `get_view_filter` fn exist, instead of having the caller
call directly into pdm_config) - so I'd prefer to keep this struct.

> If we're doing it this way though, I'd rather implement a
> From<ViewFilterConfig> for ViewFilter than a `new` method
> (or maybe both)
>
>> +
>> +impl ViewFilter {
>> +    /// Create a new [`ViewFiler`].
>> +    pub fn new(config: ViewFilterConfig) -> Self {
>> +        Self { config }
>> +    }
>> +
>> +    /// Check if a [`Resource`] matches the filter rules.
>> +    pub fn resource_matches(&self, remote: &str, resource: &Resource) -> bool {
>> +        // NOTE: Establishing a cache here is not worth the effort at the moment, evaluation of
>> +        // rules is *very* fast.
>> +        //
>> +        // Some experiments were performed with a cache that works roughly as following:
>> +        //   - HashMap<ViewId, HashMap<ResourceId, bool>> in a mutex
>> +        //   - Cache invalidated if view-filter 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
>> +        // ViewFilterConfig 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* filter config, we should be good with direct evaluation, as long
>> +        // as we don't add any filter rules which are very expensive to evaluate.
>
> isn't that (full) info more suited for the commit message than a  comment?
>
> e.g. a single line comment with 'caching here is currently not worth it'
> and the full text in the commit message should also be ok?
>
> (no hard feelings though)
>

I'll move it to the commit message and leave a short summary in the
comment - thanks!

>> +
>> +        let resource_data = resource.into();
>> +
>> +        self.check_if_included(remote, &resource_data)
>> +            && !self.check_if_excluded(remote, &resource_data)
>> +    }
>> +
>> +    /// Check if a remote can be safely skipped based on the filter rule definition.
>> +    ///
>> +    /// When there are `include remote:<...>` or `exclude remote:<...>` rules, we can use these to
>> +    /// check if a remote needs to be considered at all.
>> +    pub fn can_skip_remote(&self, remote: &str) -> bool {
>> +        let mut has_any_include_remote = false;
>> +        let mut matches_any_include_remote = false;
>> +
>> +        let mut any_other = false;
>> +
>> +        for include in &self.config.include {
>> +            if let FilterRule::Remote(r) = include {
>> +                has_any_include_remote = true;
>> +                if r == remote {
>> +                    matches_any_include_remote = true;
>> +                    break;
>> +                }
>> +            } else {
>> +                any_other = true;
>> +            }
>> +        }
>> +
>> +        let matches_any_exclude_remote = self.config.exclude.iter().any(|e| {
>> +            if let FilterRule::Remote(r) = e {
>> +                r == remote
>> +            } else {
>> +                false
>> +            }
>> +        });
>> +
>> +        (has_any_include_remote && !matches_any_include_remote && !any_other)
>> +            || matches_any_exclude_remote
>> +    }
>> +
>> +    /// Check if a node is matched by the filter rules.
>> +    ///
>> +    /// This is equivalent to checking an actual node resource.
>> +    pub fn is_node_included(&self, remote: &str, node: &str) -> bool {
>> +        let resource_data = ResourceData {
>> +            resource_type: ResourceType::Node,
>> +            tags: None,
>> +            resource_pool: None,
>> +            resource_id: &format!("remote/{remote}/node/{node}"),
>> +        };
>> +
>> +        self.check_if_included(remote, &resource_data)
>> +            && !self.check_if_excluded(remote, &resource_data)
>> +    }
>> +
>> +    /// Returns the name of the view filter.
>> +    pub fn name(&self) -> &str {
>> +        &self.config.id
>> +    }
>> +
>> +    fn check_if_included(&self, remote: &str, resource: &ResourceData) -> bool {
>> +        if self.config.include.is_empty() {
>> +            // If there are no include rules, any resource is included (unless excluded)
>> +            return true;
>> +        }
>> +
>> +        check_rules(&self.config.include, remote, resource)
>> +    }
>> +
>> +    fn check_if_excluded(&self, remote: &str, resource: &ResourceData) -> bool {
>> +        check_rules(&self.config.exclude, remote, resource)
>> +    }
>> +}
>> +
>> +fn check_rules(rules: &[FilterRule], remote: &str, resource: &ResourceData) -> bool {
>> +    for rule in rules {
>> +        let verdict = match rule {
>> +            FilterRule::ResourceType(resource_type) => resource.resource_type == *resource_type,
>> +            FilterRule::ResourcePool(pool) => resource.resource_pool == Some(pool),
>> +            FilterRule::ResourceId(resource_id) => resource.resource_id == resource_id,
>> +            FilterRule::Tag(tag) => {
>> +                if let Some(resource_tags) = resource.tags {
>> +                    resource_tags.contains(tag)
>> +                } else {
>> +                    false
>> +                }
>> +            }
>> +            FilterRule::Remote(included_remote) => included_remote == remote,
>> +        };
>> +
>> +        if verdict {
>> +            return true;
>> +        }
>> +    }
>> +
>> +    false
>
> wouldn't this boil down to:
>
> return rules.any(|rule| match rule { ... } ) ?
>
> instead of looping and doing an early return manually?
>

You are absolutely right - I'll change it as suggested.

>> +}
>> +
>> +struct ResourceData<'a> {
>> +    resource_type: ResourceType,
>> +    tags: Option<&'a [String]>,
>> +    resource_pool: Option<&'a String>,
>> +    resource_id: &'a str,
>> +}
>> +
>> +impl<'a> From<&'a Resource> for ResourceData<'a> {
>> +    fn from(value: &'a Resource) -> Self {
>> +        match value {
>> +            Resource::PveQemu(resource) => ResourceData {
>> +                resource_type: value.resource_type(),
>> +                tags: Some(&resource.tags),
>> +                resource_pool: Some(&resource.pool),
>> +                resource_id: value.global_id(),
>> +            },
>> +            Resource::PveLxc(resource) => ResourceData {
>> +                resource_type: value.resource_type(),
>> +                tags: Some(&resource.tags),
>> +                resource_pool: Some(&resource.pool),
>> +                resource_id: value.global_id(),
>> +            },
>> +            Resource::PveNode(_)
>> +            | Resource::PveSdn(_)
>> +            | Resource::PbsNode(_)
>> +            | Resource::PbsDatastore(_)
>> +            | Resource::PveStorage(_) => ResourceData {
>> +                resource_type: value.resource_type(),
>> +                tags: None,
>> +                resource_pool: None,
>> +                resource_id: value.global_id(),
>> +            },
>> +        }
>> +    }
>> +}
>
> Is it really worht it, to define a seperate type that you only use 
> internally?
> couldn't you simple use the &Resource type directly?
> or maybe just having 2 helper methods to extract the relevant info?
> (the type and global_id is already abstracted, so it's only relevant
> for the tags and the resource_pool ?)
>
> but i guess i'd have to see it to determine what is better...

The sole reason why there is this 'itermediary' type is the
'is_node_included' function. It is used when we don't handle resources,
but only really care about the node (e.g. task API). I didn't want to
'synthesize' some fake PveNode resource in the 'is_node_included'
function; it felt kind of wrong.


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