From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) by lore.proxmox.com (Postfix) with ESMTPS id AF5051FF183 for ; Wed, 5 Nov 2025 12:47:47 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id B5BC221C6A; Wed, 5 Nov 2025 12:48:27 +0100 (CET) Message-ID: Date: Wed, 5 Nov 2025 12:48:23 +0100 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Beta To: Lukas Wagner , Proxmox Datacenter Manager development discussion 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: X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1762343285063 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.027 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 RCVD_IN_VALIDITY_CERTIFIED_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. RCVD_IN_VALIDITY_RPBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. RCVD_IN_VALIDITY_SAFE_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more information. [config.id] 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" 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 { >>> + 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 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) >> > > 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