From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [IPv6:2a01:7e0:0:424::9]) by lore.proxmox.com (Postfix) with ESMTPS id 55F3F1FF183 for ; Wed, 5 Nov 2025 11:07:25 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 47ADA1FD92; Wed, 5 Nov 2025 11:08:05 +0100 (CET) Message-ID: Date: Wed, 5 Nov 2025 11:08:00 +0100 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Beta To: Proxmox Datacenter Manager development discussion , Lukas Wagner References: <20251103123521.266258-1-l.wagner@proxmox.com> <20251103123521.266258-5-l.wagner@proxmox.com> Content-Language: en-US From: Dominik Csapak In-Reply-To: <20251103123521.266258-5-l.wagner@proxmox.com> X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1762337262356 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.029 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Subject: Re: [pdm-devel] [PATCH datacenter-manager v2 04/12] views: add implementation for view filters X-BeenThere: pdm-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox Datacenter Manager development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: Proxmox Datacenter Manager development discussion Content-Transfer-Encoding: 7bit Content-Type: text/plain; charset="us-ascii"; Format="flowed" Errors-To: pdm-devel-bounces@lists.proxmox.com Sender: "pdm-devel" 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 > --- > 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 { > + 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 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> 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