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 3A5DB1FF17E for ; Thu, 13 Nov 2025 20:54:09 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 62AAE26FAC; Thu, 13 Nov 2025 20:55:04 +0100 (CET) Message-ID: <7af68c3c-f71d-4304-be2a-e47f7e6e63fb@proxmox.com> Date: Thu, 13 Nov 2025 20:54:30 +0100 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Beta To: Proxmox Datacenter Manager development discussion , Lukas Wagner References: <20251113121644.236005-1-l.wagner@proxmox.com> <20251113121644.236005-5-l.wagner@proxmox.com> Content-Language: en-US From: Thomas Lamprecht In-Reply-To: <20251113121644.236005-5-l.wagner@proxmox.com> X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1763063644813 X-SPAM-LEVEL: Spam detection results: 0 AWL -0.024 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 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" 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. > There are include/exclude rules for the following properties: > - (global) resource-id (host)name would be good to have too from the start. > - 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? > - 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. > 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. _______________________________________________ pdm-devel mailing list pdm-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pdm-devel