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

[Dominik Csapak <d.csapak@proxmox.com>]

On 11/5/25 11:57 AM, Lukas Wagner wrote:
> 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.

make sense, so just keep it as is

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


mhmm i don't think creating a fake `PveNode` resource is any more wrong
that creating a 'fake' `ResourceData` in place, but again no hard
feelings. I probably would have to see both approaches side-by-side
to see which is better, but it's not super important to change for now



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