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

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

some comments inline

On 11/3/25 1:36 PM, Lukas Wagner wrote:
> This commit adds the filter implementation for the previously defined
> ViewFilterConfig type.
> 
> There are include/exclude rules for the following properties:
>    - (global) resource-id
>    - resource pool
>    - resource type
>    - remote
>    - tags
> 
> The rules are interpreted as follows:
> - no rules: everything matches
> - 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
> 
> Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
> ---
>   server/src/lib.rs               |   1 +
>   server/src/views/mod.rs         |   1 +
>   server/src/views/view_filter.rs | 182 ++++++++++++++++++++++++++++++++
>   3 files changed, 184 insertions(+)
>   create mode 100644 server/src/views/mod.rs
>   create mode 100644 server/src/views/view_filter.rs
> 
> diff --git a/server/src/lib.rs b/server/src/lib.rs
> index 964807eb..0f25aa71 100644
> --- a/server/src/lib.rs
> +++ b/server/src/lib.rs
> @@ -12,6 +12,7 @@ pub mod remote_tasks;
>   pub mod remote_updates;
>   pub mod resource_cache;
>   pub mod task_utils;
> +pub mod views;
>   
>   pub mod connection;
>   pub mod pbs_client;
> diff --git a/server/src/views/mod.rs b/server/src/views/mod.rs
> new file mode 100644
> index 00000000..9a2856a4
> --- /dev/null
> +++ b/server/src/views/mod.rs
> @@ -0,0 +1 @@
> +pub mod view_filter;
> diff --git a/server/src/views/view_filter.rs b/server/src/views/view_filter.rs
> new file mode 100644
> index 00000000..4f77e7bf
> --- /dev/null
> +++ b/server/src/views/view_filter.rs
> @@ -0,0 +1,182 @@
> +use anyhow::Error;
> +
> +use pdm_api_types::{
> +    resource::{Resource, ResourceType},
> +    views::{FilterRule, ViewFilterConfig},
> +};
> +
> +/// 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)

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

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)

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

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



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