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 2487F1FF15C for ; Fri, 14 Nov 2025 14:24:41 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id AA5A6138EF; Fri, 14 Nov 2025 14:25:36 +0100 (CET) Date: Fri, 14 Nov 2025 14:25:32 +0100 Message-Id: From: "Lukas Wagner" To: "Thomas Lamprecht" , "Proxmox Datacenter Manager development discussion" , "Lukas Wagner" Mime-Version: 1.0 X-Mailer: aerc 0.21.0-0-g5549850facc2-dirty References: <20251113121644.236005-1-l.wagner@proxmox.com> <20251113121644.236005-5-l.wagner@proxmox.com> <7af68c3c-f71d-4304-be2a-e47f7e6e63fb@proxmox.com> In-Reply-To: <7af68c3c-f71d-4304-be2a-e47f7e6e63fb@proxmox.com> X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1763126705923 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 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 Subject: Re: [pdm-devel] [PATCH datacenter-manager v5 04/11] views: add implementation for view resource filtering 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-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: pdm-devel-bounces@lists.proxmox.com Sender: "pdm-devel" On Thu Nov 13, 2025 at 8:54 PM CET, Thomas Lamprecht wrote: > Am 13.11.25 um 13:16 schrieb Lukas Wagner: >> This commit adds the resource filter implementation for the previously >> defined ViewConfig type. >> > > a bit late given that this was already applied, don't get me wrong, I'm > thankful @Dominik for doing so, just a quick heads-up for coordination would > have been welcome by my side. Only few higher level comments for potential > future improvement below. > Just to be sure, what would be your preferred way to handle situations similar to this one these? A quick "Hey, we are about to apply XYZ, any objections?" ? I'm still trying to find the right balance for "can *I* apply this?" and "should this go through the project lead for final signoff?". In the end, being to liberal when applying patches can lead to more work for you (and all of us), which is of course something I want to avoid. Any advice? >> There are include/exclude rules for the following properties: >> - (global) resource-id > > (host)name would be good to have too from the start. > Good idea. Should be fairly trivial to add. >> - resource pool >> - resource type >> - remote >> - tags >> >> The rules are interpreted as follows: >> - no rules: everything matches > > No hard feelings and this definitively works, but it might be a bit clearer > (and safer!) to require an explicit include "all" rule for that. > > Deleting all rules by accident is easier than setting some explicit value, > and giving out access to all resources can have implications. > And sure, that's not something frequent or the like, but if there's no > disadvantage it can be still worth to go for the safer route. > > It could be also a specific "include-all" flag, to make matching for it > easier and avoid that one has to iterate over all loops. > FWIW, we could still introduce this later if users actually run into it, > so mostly interested in what you think? > Yeah, I understand your point. Given that the matching also impacts permissions, being a bit more cautious might be appropriate. The dedicated 'include-all' flag sounds like a good idea. If we want to go that route, we should probably add it right away. I'll start a quick follow-up series with such a flag, then we can continue the discussion there. Should not be much work, as far as I can tell. >> - 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 >> >> Some experiments were performed with a cache that works roughly as following: >> - HashMap> in a mutex >> - Cache invalidated if view 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 ViewConfig 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* view config, we should be good with direct >> evaluation, as long as we don't add any filter rules which are very >> expensive to evaluate. > > As already mentioned offlist a few weeks ago: I really would like to > see some flexible matching support (glob, regex, ...?) > > I'm fine with that not being done here already, getting the core plumbing > in place is definitively more important, but I'd like to know how this > would look like, so doing at least a tiny/simple POC would be IMO good to > try before bumping 1.0. > The off-list discussion didn't explicitly mention regex/globs, but mentioned reusing the matching from the notification stack, which *does* support regexes. Since I didn't end up reusing that part, I kind of lost track of that aspect. Should be added now rather than later, otherwise adding support for this in the config could become awkward. Off-list we briefly discussed the issue; you mentioned the option to use property strings to support the regex mode later on, e.g. include op=regex,value=tag:a[a-z]+ FWIW, we could just use the same convention that we already use for 'match-field' for notification matchers: include exact:tag=aaaz (where the "exact:" part is optional) [1] include regex:tag=a[a-z]+ Could make sense to just use the same syntax to avoid any confusion for our users. This would require a minor change for the config format, so we should do that *now*. If we want to use this syntax, I'd quickly send a follow-up patch for the config changes so that at least [1] is supported; we can then add regex support at our convenience. >> diff --git a/server/src/views/tests.rs b/server/src/views/tests.rs >> new file mode 100644 >> index 00000000..030b7994 >> --- /dev/null >> +++ b/server/src/views/tests.rs >> @@ -0,0 +1,619 @@ > >> +fn run_test(config: ViewConfig, tests: &[((&str, &Resource), bool)]) { >> + let filter = View::new(config); >> + >> + for ((remote_name, resource), expected) in tests { >> + eprintln!("remote: {remote_name}, resource: {resource:?}"); >> + assert_eq!(filter.resource_matches(remote_name, resource), *expected); >> + } >> +} >> + >> +const NODE: &str = "somenode"; >> +const STORAGE: &str = "somestorage"; >> +const REMOTE: &str = "someremote"; >> + >> +#[test] >> +fn include_remotes() { >> + let config = ViewConfig { >> + id: "only-includes".into(), >> + include: vec![ >> + FilterRule::Remote("remote-a".into()), >> + FilterRule::Remote("remote-b".into()), >> + ], >> + ..Default::default() >> + }; > > might be nice to have some of the tests also deserialize the config from > a raw string, as while one can argue that it's section config's job to get > us from that to the deserialized type, one can still introduce regressions > outside of the format it self (e.g. typos) and it's IMO also just nice to > see how the configs look in their serialized form, especially when planning > to extend this feature-wise. Agreed. _______________________________________________ pdm-devel mailing list pdm-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pdm-devel