Re: [pbs-devel] [PATCH proxmox-backup 1/3] fix #4315: jobs: modify GroupFilter so include/exclude is tracked

[Lukas Wagner <l.wagner@proxmox.com>]

Hi,

Also, some higher-level comments:

The include/exclude behavior seems not super intuitive to me, as it 
appears that the behavior depends on the order of the group-filter
config entries - e.g. "include all VMs" followed by "exclude VM 190"
has a different behavior than the other way round. In the GUI this is
not super obvious, and also there is no way to reorder the matching
rules.

I would probably do it this way:

no group-filters defined -> everything is included
only including filter    -> only groups matching the filter(s) are
                             included
only excluding filter    -> all groups but the matching ones are
                             included
including and excluding  -> first compute included groups, then subtract
                             excluded ones

Ideally, this behavior should be
   a.) obvious in the GUI
   b.) documented in the docs, with a few concrete examples

Also, from a UX perspective, it would be cool to see a 'preview' in the 
GUI, to see which of the currently available groups will be included in
the sync job.
But that's a thing for a follow-up, I think that might be a bit more
work.

One further comment inline.

On 10/23/23 17:43, Philipp Hufnagl wrote:
> I wanted to implement the feature in a way that a user can alternate
> between includes and excludes. That way a way more complex rule set can
> be achieved then when a include list is run first and an exclude list
> afterwards. Therefor if a filter includes or excludes is considered a
> property of the individual filters. I called this property 'behaviour'.
> 
> Since a GroupFilter now also features an behaviour, the Struct has been
> renamed To GroupType (since simply type is a keyword). The new
> GroupFilter now has a behaviour as a flag 'is_exclude'.
> 
> I considered calling it 'is_include' but a reader later then might not
> know what the opposite of 'include' is (do not include?  deactivate?). I
> also considered making a new enum 'behaviour' but since there are only 2
> values I considered it over engineered.
> 
> Matching a filter will now iterate with a forech loop in order to also
> exclude matches.
> 
> Signed-off-by: Philipp Hufnagl <p.hufnagl@proxmox.com>
> ---
>   pbs-api-types/src/datastore.rs | 11 ++++----
>   pbs-api-types/src/jobs.rs      | 49 +++++++++++++++++++++++++---------
>   src/api2/tape/backup.rs        |  8 +++++-
>   src/server/pull.rs             |  8 +++++-
>   4 files changed, 55 insertions(+), 21 deletions(-)
> 
> diff --git a/pbs-api-types/src/datastore.rs b/pbs-api-types/src/datastore.rs
> index 73c4890e..c3beedaf 100644
> --- a/pbs-api-types/src/datastore.rs
> +++ b/pbs-api-types/src/datastore.rs
> @@ -841,17 +841,16 @@ impl BackupGroup {
>       }
>   
>       pub fn matches(&self, filter: &crate::GroupFilter) -> bool {
> -        use crate::GroupFilter;
> -
> -        match filter {
> -            GroupFilter::Group(backup_group) => {
> +        use crate::FilterType;
> +        match &filter.filter_type {
> +            FilterType::Group(backup_group) => {
>                   match backup_group.parse::<BackupGroup>() {
>                       Ok(group) => *self == group,
>                       Err(_) => false, // shouldn't happen if value is schema-checked
>                   }
>               }
> -            GroupFilter::BackupType(ty) => self.ty == *ty,
> -            GroupFilter::Regex(regex) => regex.is_match(&self.to_string()),
> +            FilterType::BackupType(ty) => self.ty == *ty,
> +            FilterType::Regex(regex) => regex.is_match(&self.to_string()),
>           }
>       }
>   }
> diff --git a/pbs-api-types/src/jobs.rs b/pbs-api-types/src/jobs.rs
> index 23e19b7b..b9c8c981 100644
> --- a/pbs-api-types/src/jobs.rs
> +++ b/pbs-api-types/src/jobs.rs
> @@ -383,7 +383,7 @@ pub struct TapeBackupJobStatus {
>   
>   #[derive(Clone, Debug)]
>   /// Filter for matching `BackupGroup`s, for use with `BackupGroup::filter`.
> -pub enum GroupFilter {
> +pub enum FilterType {
>       /// BackupGroup type - either `vm`, `ct`, or `host`.
>       BackupType(BackupType),
>       /// Full identifier of BackupGroup, including type
> @@ -392,7 +392,7 @@ pub enum GroupFilter {
>       Regex(Regex),
>   }
>   
> -impl PartialEq for GroupFilter {
> +impl PartialEq for FilterType {
>       fn eq(&self, other: &Self) -> bool {
>           match (self, other) {
>               (Self::BackupType(a), Self::BackupType(b)) => a == b,
> @@ -403,27 +403,50 @@ impl PartialEq for GroupFilter {
>       }
>   }
>   
> +#[derive(Clone, Debug)]
> +pub struct GroupFilter {
> +    pub is_exclude: bool,
> +    pub filter_type: FilterType,
> +}
> +
> +impl PartialEq for GroupFilter {
> +    fn eq(&self, other: &Self) -> bool {
> +        self.filter_type == other.filter_type && self.is_exclude == other.is_exclude
> +    }
> +}
> +

^ I think PartialEq can be derived for GroupFilter, no need to implement
it manually.

>   impl std::str::FromStr for GroupFilter {
>       type Err = anyhow::Error;
>   
>       fn from_str(s: &str) -> Result<Self, Self::Err> {
> -        match s.split_once(':') {
> -            Some(("group", value)) => BACKUP_GROUP_SCHEMA.parse_simple_value(value).map(|_| GroupFilter::Group(value.to_string())),
> -            Some(("type", value)) => Ok(GroupFilter::BackupType(value.parse()?)),
> -            Some(("regex", value)) => Ok(GroupFilter::Regex(Regex::new(value)?)),
> +        let (is_exclude, type_str) = match s.split_once(':') {
> +            Some(("include", value)) => (false, value),
> +            Some(("exclude", value)) => (true, value),
> +            _ => (false, s),
> +        };
> +
> +        let filter_type = match type_str.split_once(':') {
> +            Some(("group", value)) => BACKUP_GROUP_SCHEMA.parse_simple_value(value).map(|_| FilterType::Group(value.to_string())),
> +            Some(("type", value)) => Ok(FilterType::BackupType(value.parse()?)),
> +            Some(("regex", value)) => Ok(FilterType::Regex(Regex::new(value)?)),
>               Some((ty, _value)) => Err(format_err!("expected 'group', 'type' or 'regex' prefix, got '{}'", ty)),
>               None => Err(format_err!("input doesn't match expected format '<group:GROUP||type:<vm|ct|host>|regex:REGEX>'")),
> -        }.map_err(|err| format_err!("'{}' - {}", s, err))
> +        }?;
> +        Ok(GroupFilter {
> +            is_exclude,
> +            filter_type,
> +        })
>       }
>   }
>   
>   // used for serializing below, caution!
>   impl std::fmt::Display for GroupFilter {
>       fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
> -        match self {
> -            GroupFilter::BackupType(backup_type) => write!(f, "type:{}", backup_type),
> -            GroupFilter::Group(backup_group) => write!(f, "group:{}", backup_group),
> -            GroupFilter::Regex(regex) => write!(f, "regex:{}", regex.as_str()),
> +        let exclude = if self.is_exclude { "exclude:" } else { "" };
> +        match &self.filter_type {
> +            FilterType::BackupType(backup_type) => write!(f, "{}type:{}", exclude, backup_type),
> +            FilterType::Group(backup_group) => write!(f, "{}group:{}", exclude, backup_group),
> +            FilterType::Regex(regex) => write!(f, "{}regex:{}", exclude, regex.as_str()),
>           }
>       }
>   }
> @@ -436,9 +459,9 @@ fn verify_group_filter(input: &str) -> Result<(), anyhow::Error> {
>   }
>   
>   pub const GROUP_FILTER_SCHEMA: Schema = StringSchema::new(
> -    "Group filter based on group identifier ('group:GROUP'), group type ('type:<vm|ct|host>'), or regex ('regex:RE').")
> +    "Group filter based on group identifier ('group:GROUP'), group type ('type:<vm|ct|host>'), or regex ('regex:RE'). Can be inverted by adding 'exclude:' before.")
>       .format(&ApiStringFormat::VerifyFn(verify_group_filter))
> -    .type_text("<type:<vm|ct|host>|group:GROUP|regex:RE>")
> +    .type_text("[<exclude:|include:>]<type:<vm|ct|host>|group:GROUP|regex:RE>")
>       .schema();
>   
>   pub const GROUP_FILTER_LIST_SCHEMA: Schema =
> diff --git a/src/api2/tape/backup.rs b/src/api2/tape/backup.rs
> index 2f9385a7..04abdb71 100644
> --- a/src/api2/tape/backup.rs
> +++ b/src/api2/tape/backup.rs
> @@ -413,7 +413,13 @@ fn backup_worker(
>   
>       let (group_list, group_count) = if let Some(group_filters) = &setup.group_filter {
>           let filter_fn = |group: &BackupGroup, group_filters: &[GroupFilter]| {
> -            group_filters.iter().any(|filter| group.matches(filter))
> +            let mut is_match = false;
> +            for filter in group_filters.iter() {
> +                if group.matches(filter) {
> +                    is_match = !filter.is_exclude;
> +                }
> +            }
> +            is_match
>           };
>   
>           let group_count_full = group_list.len();
> diff --git a/src/server/pull.rs b/src/server/pull.rs
> index a973a10e..0124bd3f 100644
> --- a/src/server/pull.rs
> +++ b/src/server/pull.rs
> @@ -1084,7 +1084,13 @@ pub(crate) async fn pull_ns(
>       });
>   
>       let apply_filters = |group: &pbs_api_types::BackupGroup, filters: &[GroupFilter]| -> bool {
> -        filters.iter().any(|filter| group.matches(filter))
> +        let mut is_match = false;
> +        for filter in filters.iter() {
> +            if group.matches(filter) {
> +                is_match = !filter.is_exclude;
> +            }
> +        }
> +        is_match
>       };
>   
>       // Get groups with target NS set

-- 
- Lukas