Re: [pdm-devel] [PATCH datacenter-manager v2 09/12] api: subscription status: add support for view-filter parameter

[Dominik Csapak <d.csapak@proxmox.com>]

On 11/5/25 3:11 PM, Lukas Wagner wrote:
> On Wed Nov 5, 2025 at 11:08 AM CET, Dominik Csapak wrote:
>>> @@ -590,35 +605,62 @@ pub async fn get_subscription_status(
>>>        };
>>>    
>>>        for (remote_name, remote) in remotes_config {
>>> -        if !allow_all && !check_priv(&remote_name) {
>>> +        if let Some(filter) = &view_filter {
>>> +            if filter.can_skip_remote(&remote_name) {
>>> +                continue;
>>> +            }
>>> +        } else if !allow_all && !check_priv(&remote_name) {
>>>                continue;
>>>            }
>>>    
>>> +        let view_filter_clone = view_filter.clone();
>>
>> this could just be named 'view_filter' too, no need to postfix it with
>> '_clone'
> 
> Ack - will do for v3.
> 
>>
>>> +
>>>            let future = async move {
>>>                let (node_status, error) =
>>>                    match get_subscription_info_for_remote(&remote, max_age).await {
>>> -                    Ok(node_status) => (Some(node_status), None),
>>> +                    Ok(mut node_status) => {
>>> +                        node_status.retain(|node, _| {
>>> +                            if let Some(filter) = &view_filter_clone {
>>> +                                filter.is_node_included(&remote.id, node)
>>> +                            } else {
>>> +                                true
>>> +                            }
>>> +                        });
>>> +                        (Some(node_status), None)
>>> +                    }
>>>                        Err(error) => (None, Some(error.to_string())),
>>>                    };
>>>    
>>> -            let mut state = RemoteSubscriptionState::Unknown;
>>> +            let state = if let Some(node_status) = &node_status {
>>> +                if error.is_some() && view_filter_clone.is_some() {
>>> +                    // Don't leak the existence of failed remotes, since we cannot apply
>>> +                    // view-filters here.
>>
>> why not? we can check if the remote should be included, that does not
>> requires any more info?
>>
>>
> 
> The problem is that we cannot always reliably tell if a remote is
> included or not without checking the remote's resources. There is
> ViewFilter::can_skip_remote - but since that one only checks
> `include/exclude remote:...` rules, it might return false (meaning,
> the remote must be considered) even if at the end the data from this
> remote might be filtered out due to the other rules (and if no resources
> remain, the remote as a whole would then be filtered out from the
> response - similar to what I did in patch 6 to which you replied).
> 
> Example:
> 
> Consider only having `include tag:sometag` in the config. From this
> alone, we cannot exclude any remotes from being queried a priori and
> must do *all* filtering when we already have the data.
> 
> Similar thing here. If we cannot reach the remote, then we don't have a
> list of nodes for this remote. Without the list of nodes, we cannot do
> any checks with the node resource. This means if we do not filter out
> the failed remote, we would leak its existence in a view which
> potentially would not match on any of the node's resources.
> 
> I hope you could follow this explanation. Do you maybe have an idea on
> how to fix this?
> 
> Only thing that comes to mind is making the include/exclude
> remote:{remote} rules required, so that a view *always* needs to specify
> which remotes to (not) consider. If we make these required, we probably
> should make these a separate key in the config file again.
> 

ok i understand the issue, but instead of excluding all remotes that
have errors, we could at least include those that we know should be
included by the 'remote' filter?

IMHO a remote is already a resource, so if i include some remote with
it's name (or by type, etc.), even if it does not contain any sub
resources, i should get back an (empty) remote

does that make sense?

i get we can't include/exclude based on other filters though, that
must remain hidden as you mentioned, but for explicitly included
remotes we should include them also with errors.

>>> +                    return None;
>>> +                }
>>>    
>>> -            if let Some(node_status) = &node_status {
>>> -                state = map_node_subscription_list_to_state(node_status);
>>> -            }
>>> +                if node_status.is_empty() {
>>> +                    return None;
>>> +                }
>>>    
>>> -            RemoteSubscriptions {
>>> +                map_node_subscription_list_to_state(node_status)
>>> +            } else {
>>> +                RemoteSubscriptionState::Unknown
>>> +            };
>>> +
>>> +            Some(RemoteSubscriptions {
>>>                    remote: remote_name,
>>>                    error,
>>>                    state,
>>>                    node_status: if verbose { node_status } else { None },
>>> -            }
>>> +            })
>>>            };
>>>    
>>>            futures.push(future);
>>>        }
>>>    
>>> -    Ok(join_all(futures).await)
>>> +    let status = join_all(futures).await.into_iter().flatten().collect();
>>> +
>>> +    Ok(status)
>>>    }
>>>    
>>>    // FIXME: make timeframe and count parameters?
> 



_______________________________________________
pdm-devel mailing list
pdm-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pdm-devel