[pdm-devel] [PATCH datacenter-manager v2 04/12] views: add implementation for view filters
Dominik Csapak
d.csapak at proxmox.com
Wed Nov 5 12:48:23 CET 2025
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<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)
>
> 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<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)
>>
>
> 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
More information about the pdm-devel
mailing list