[pbs-devel] [PATCH proxmox-backup 0/3] fix #4315: datastore: Exclude entries from sync

[pbs-devel] [PATCH proxmox-backup 0/3] fix #4315: datastore: Exclude entries from sync

[Philipp Hufnagl]

This allows to use Group Filter for sync jobs so matches can not just be
included but also excluded. For this the "group-filter" configuration
syntax has been extended with an optional "behaviour" parameter.
this can be "include" or "exclude". Filter will be applied in the order
of the config file. For example config entries could look like this:

Will continue include the match for compatiblity.
        group-filter regex:.*

Will exclude the match from the sync job.
        group-filter exclude:regex:.*10[1-3].*

Will include the match to the sync job.
        group-filter include:regex:.*10[2-3].*

So after this 3 lines, 100, 202 and 103 would be included but 101
would be excluded.

Since obmitting the behaviour parameter will continue to include the
match, this parameter is not intendet to be set to "include" by the GUI
or other tooling. Its just there to make manual configuration edits more
forgiving.

Signed-off-by: Philipp Hufnagl <p.hufnagl@proxmox.com>
---

Philipp Hufnagl (3):
  fix #4315: jobs: modify GroupFilter so include/exclude is tracked
  ui: Show if Filter includes or excludes
  docs: document new include/exclude paramenter

 docs/managing-remotes.rst      |  7 +++++
 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 ++++-
 www/form/GroupFilter.js        | 57 +++++++++++++++++++++++++++++++---
 6 files changed, 115 insertions(+), 25 deletions(-)

-- 
2.39.2

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

[Philipp Hufnagl]

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
+    }
+}
+
 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
-- 
2.39.2

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

[Lukas Wagner]

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

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

[Philipp Hufnagl]

On 10/24/23 11:18, Lukas Wagner wrote:
> 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

I am not 100% sure how I feel about including all as a starting point
for exclusion filter. While I understand the intuitive benefit, it
also may make the process more error prone, since removing 1 include
filter may change everything to include all. User might not think of that.

> including and excluding  -> first compute included groups, then subtract
>                             excluded ones
> 
I considered this. The reason why I decided for only one list is
because it enables user to make more sophisticated rules more easily.

Having 2 lists that get processed after each other can make it much
harder to filter on a complex setup.

> Ideally, this behavior should be
>   a.) obvious in the GUI
>   b.) documented in the docs, with a few concrete examples
> 
I see what I can do about explaining this behaviour better in the GUI
and how to extend the documentation.

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

[Lukas Wagner]

On 10/24/23 11:54, Philipp Hufnagl wrote:
>> 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
> 
> I am not 100% sure how I feel about including all as a starting point
> for exclusion filter. While I understand the intuitive benefit, it
> also may make the process more error prone, since removing 1 include
> filter may change everything to include all. User might not think of that.
>  >> including and excluding  -> first compute included groups, then subtract
>>                              excluded ones
>>
> I considered this. The reason why I decided for only one list is
> because it enables user to make more sophisticated rules more easily.
> 
> Having 2 lists that get processed after each other can make it much
> harder to filter on a complex setup.

Do you have any examples in mind that would be more difficult to
represent?

If you keep it as it is right now, there should be a way to reorder
match rules in the UI, maybe similar to the boot-order drag'n'drop
thing in PVE.

> 
>> Ideally, this behavior should be
>>    a.) obvious in the GUI
>>    b.) documented in the docs, with a few concrete examples
>>
> I see what I can do about explaining this behaviour better in the GUI
> and how to extend the documentation.
> 
> 

-- 
- Lukas

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

[Philipp Hufnagl]

On 10/24/23 12:43, Lukas Wagner wrote:

> Do you have any examples in mind that would be more difficult to
> represent?

I would like to include all vms from 10 to to 30, but not 17,18 and 20.

> 
> If you keep it as it is right now, there should be a way to reorder
> match rules in the UI, maybe similar to the boot-order drag'n'drop
> thing in PVE.
> 

Yes. I spoke with Dominik Csapak about it. A new UI element to easily
re-sort the filters is planned but since it will be quite a bit of
work it is planned as a new patch.

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

[Thomas Lamprecht]

Am 24/10/2023 um 16:32 schrieb Philipp Hufnagl:
> 
> 
> On 10/24/23 12:43, Lukas Wagner wrote:
> 
>> Do you have any examples in mind that would be more difficult to
>> represent?
> 
> I would like to include all vms from 10 to to 30, but not 17,18 and 20.
> 

How is that more difficult?

IMO Lukas proposal seems reasonable, a deterministic remove matches from
excludes fromm all matches from includes seems easier to understand,
from top of my head.

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

[Philipp Hufnagl]

On 10/25/23 15:33, Thomas Lamprecht wrote:
> Am 24/10/2023 um 16:32 schrieb Philipp Hufnagl:
>>
>>
>> On 10/24/23 12:43, Lukas Wagner wrote:
>>
>>> Do you have any examples in mind that would be more difficult to
>>> represent?
>>
>> I would like to include all vms from 10 to to 30, but not 17,18 and 20.
>>
> 
> How is that more difficult?
> 
> IMO Lukas proposal seems reasonable, a deterministic remove matches from
> excludes fromm all matches from includes seems easier to understand,
> from top of my head.

You would have to do something like

include 10-30
exclude 17
exclude 18
exclude 20

instead of

include 10-30
exclude 17-20
include 19

While the first is easier to understand, the 2nd one allows (in my
opinion) to build a cleaner solution for complex filtering.

However both are in my opinion fine solutions. I you think having 2
lists is a better solution I can implement that as well.

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

[Thomas Lamprecht]

Am 25/10/2023 um 17:07 schrieb Philipp Hufnagl:
> 
> 
> On 10/25/23 15:33, Thomas Lamprecht wrote:
>> Am 24/10/2023 um 16:32 schrieb Philipp Hufnagl:
>>>
>>>
>>> On 10/24/23 12:43, Lukas Wagner wrote:
>>>
>>>> Do you have any examples in mind that would be more difficult to
>>>> represent?
>>>
>>> I would like to include all vms from 10 to to 30, but not 17,18 and 20.
>>>
>>
>> How is that more difficult?
>>
>> IMO Lukas proposal seems reasonable, a deterministic remove matches from
>> excludes fromm all matches from includes seems easier to understand,
>> from top of my head.
> 
> You would have to do something like
> 
> include 10-30
> exclude 17
> exclude 18
> exclude 20
> 
> instead of
> 
> include 10-30
> exclude 17-20
> include 19
> 
> While the first is easier to understand, the 2nd one allows (in my
> opinion) to build a cleaner solution for complex filtering.

Easier to understand is *much* cleaner though, any admin that changes
a simple include/exclude filter manually, e.g., reversing the order,
has no idea that the end result is completely different.

So if one would want to have such complex filtering, the order would
need to be explicitly encoded, and we always could extend a simpler
solution to a more complex one, while vice versa would be a breaking
change. But here I doubt I want to ever do that, most of the time one
has simple filter needs, servicing complex ones is error-prone not
only for the implementation but for the admin too, while only help
theoretical use cases.

Note that setups can already use namespaces for basic grouping,
and then simple exclude a few, or include a few, filters are enough
most of the time – having both, an include- and exclude- filter at the
same time is IMO already borderline overkill..

> 
> However both are in my opinion fine solutions. I you think having 2
> lists is a better solution I can implement that as well.

As mentioned off-list, the actual implementation wouldn't be really
that different in either implementation.

Grouping or splitting exclusions and inclusions in the UI would
naturally be good then to better convey the basic principles.

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

[Wolfgang Bumiller]

On Wed, Oct 25, 2023 at 05:45:11PM +0200, Thomas Lamprecht wrote:
> Am 25/10/2023 um 17:07 schrieb Philipp Hufnagl:
> > 
> > 
> > On 10/25/23 15:33, Thomas Lamprecht wrote:
> >> Am 24/10/2023 um 16:32 schrieb Philipp Hufnagl:
> >>>
> >>>
> >>> On 10/24/23 12:43, Lukas Wagner wrote:
> >>>
> >>>> Do you have any examples in mind that would be more difficult to
> >>>> represent?
> >>>
> >>> I would like to include all vms from 10 to to 30, but not 17,18 and 20.
> >>>
> >>
> >> How is that more difficult?
> >>
> >> IMO Lukas proposal seems reasonable, a deterministic remove matches from
> >> excludes fromm all matches from includes seems easier to understand,
> >> from top of my head.
> > 
> > You would have to do something like
> > 
> > include 10-30
> > exclude 17
> > exclude 18
> > exclude 20
> > 
> > instead of
> > 
> > include 10-30
> > exclude 17-20
> > include 19
> > 
> > While the first is easier to understand, the 2nd one allows (in my
> > opinion) to build a cleaner solution for complex filtering.
> 
> Easier to understand is *much* cleaner though, any admin that changes
> a simple include/exclude filter manually, e.g., reversing the order,
> has no idea that the end result is completely different.

I disagree that either of them is actually easier, and we already have
order dependent include/exclude behavior in `.pxarexcludes` and on the
CLI via `--exclude` when creating a backup with proxmox-backup-client.

Also, `.gitignore` also works like this, you have exclude and includes
in order, the last match wins.
This makes it much easier to say things like "exclude 1-100 except 50",
which, to me, seems like the most complex case you would want anyway?

Which you find easier probably depends on what you see more often, but
for consistency's sake it may make more sense to stick to what we
already have?

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

[Thomas Lamprecht]

Am 07/11/2023 um 08:43 schrieb Wolfgang Bumiller:
> On Wed, Oct 25, 2023 at 05:45:11PM +0200, Thomas Lamprecht wrote:
>> Easier to understand is *much* cleaner though, any admin that changes
>> a simple include/exclude filter manually, e.g., reversing the order,
>> has no idea that the end result is completely different.
> 
> I disagree that either of them is actually easier, and we already have
> order dependent include/exclude behavior in `.pxarexcludes` and on the

order is an extra dimension that users need to take care when writing
these, so by that alone it's easy to state that not having that is
simpler (naturally one can make it way worse by doing a bad UI, but well,
just don't do that).

And, we can always go for an order adhering model later

> CLI via `--exclude` when creating a backup with proxmox-backup-client.

Which is not really good UX, the "find" core util, and similar consorts,
are exactly deemed as hard to understand as that implicit order matters
is making it harder for users, especially those without a programming
background (i.e., the majority of our users).

> Also, `.gitignore` also works like this, you have exclude and includes
> in order, the last match wins.

Not sure if git should be used a pillar of good UX example, some would
even say that doing the opposite might be warranted in that case ;-P

> This makes it much easier to say things like "exclude 1-100 except 50",

Not really, you just use "include 50" and if the remaining set is bigger
do "exclude 1-49; exclude 51-100;" and especially in bigger examples this
is easier to reason about for the standard users.

> which, to me, seems like the most complex case you would want anyway?
> 
> Which you find easier probably depends on what you see more often, but
> for consistency's sake it may make more sense to stick to what we
> already have?

pxarexclude has nothing to do with this though? Would not mix walking
a file-system with filtering backup groups, two very different things.

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

[Wolfgang Bumiller]

On Tue, Nov 07, 2023 at 08:55:01AM +0100, Thomas Lamprecht wrote:
> Am 07/11/2023 um 08:43 schrieb Wolfgang Bumiller:
> > On Wed, Oct 25, 2023 at 05:45:11PM +0200, Thomas Lamprecht wrote:
> >> Easier to understand is *much* cleaner though, any admin that changes
> >> a simple include/exclude filter manually, e.g., reversing the order,
> >> has no idea that the end result is completely different.
> > 
> > I disagree that either of them is actually easier, and we already have
> > order dependent include/exclude behavior in `.pxarexcludes` and on the
> 
> order is an extra dimension that users need to take care when writing
> these, so by that alone it's easy to state that not having that is
> simpler (naturally one can make it way worse by doing a bad UI, but well,
> just don't do that).
> 
> And, we can always go for an order adhering model later

I'm fine with either way in the end and think changing it later is
worse. I'm just bringing this up thinking about consistency with other
options following a similar mechanic.
But sure, the pxar exudes aren't directly related as you stated later
on. (Otherwise another similar mechanic would be in the pve firewall
(and no, the way groups are translated to iptables rules is in no way a
*fair* argument against ordering :-P))

(And yes, I know that one is not actually any easier either...)

> 
> > CLI via `--exclude` when creating a backup with proxmox-backup-client.
> 
> Which is not really good UX, the "find" core util, and similar consorts,
> are exactly deemed as hard to understand as that implicit order matters
> is making it harder for users, especially those without a programming
> background (i.e., the majority of our users).

Contrary to most tools, options come after the paths, you use single
dashes for long options, use exclamation marks for inversion which used
to get butchered by most shells with history expansion unless they
recognize `find` themselves and parenthesis which don't work if you
don't have spaces around them as they need to be actual separate
parameters so you also need to watch for quotes...

But sure, let's say the ordering is the issue... 🙄

The thing is, includes and excludes are conflicting statements, they
must be resolved somehow, and in regular speech it's often by order.
Which is not to say that people wouldn't get confused *regardless*, even
if you use precise language.

Then again, we also have a whole group of people insisting on
always saying "including but not limited to...".
I guess there's a point to that if we change the default depending on
whether only includes or only excludes are present.
Ultimately, *that* is the part that would confuse the hell out of me,
personally, but sure, you'll end up dealing with both types of users
anyway, so as long as that behavior is documented... 🤷

> 
> > Also, `.gitignore` also works like this, you have exclude and includes
> > in order, the last match wins.
> 
> Not sure if git should be used a pillar of good UX example, some would
> even say that doing the opposite might be warranted in that case ;-P

Well I mostly brought it up because it's what we used as a base to
decide how the file based tools should work.

> 
> > This makes it much easier to say things like "exclude 1-100 except 50",
> 
> Not really, you just use "include 50" and if the remaining set is bigger
> do "exclude 1-49; exclude 51-100;" and especially in bigger examples this
> is easier to reason about for the standard users.

Maybe we have different interpretations of the word "easier" :-P

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

[Dominik Csapak]

just to chime in here to the discussion order/not order:
(writing mostly so that it is public record ;) )

if we want it to keep it *really* simple, we could
ditch the whole mixing of include/exclude completely
and make it *either* , so add a flag 'groups are excluding'

and if people want to do more complex things, they have
to setup multiple sync jobs

(e.g. the 'exclude 1-100 except 50' example would be one job
excluding 1-100 and another only including 50)

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

[Thomas Lamprecht]

Am 07/11/2023 um 10:01 schrieb Dominik Csapak:
> just to chime in here to the discussion order/not order:
> (writing mostly so that it is public record ;) )
> 
> if we want it to keep it *really* simple, we could
> ditch the whole mixing of include/exclude completely
> and make it *either* , so add a flag 'groups are excluding'
> 
> and if people want to do more complex things, they have
> to setup multiple sync jobs

requiring multiple jobs does not make things really easier for users either,
but sure, that would be the easier option for "lets decide what to actually
support for more flexible exclude/include in the future", as adding that would
be an extension, not a change in existing behavior.

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

[Thomas Lamprecht]

Am 07/11/2023 um 09:26 schrieb Wolfgang Bumiller:
> On Tue, Nov 07, 2023 at 08:55:01AM +0100, Thomas Lamprecht wrote:
> I'm fine with either way in the end and think changing it later is
> worse. I'm just bringing this up thinking about consistency with other

my sentence wasn't finished, sorry, what I wanted to say it's easier to
add ordering later, if we really must due to overwhelming user feedback,
than vice versa (albeit simulating the removal of it via UI could always
be done, would have it's own confusion potential though).

> options following a similar mechanic.

same options should follow same logic, and we have such filters for
tasks and (in development) guests for bulk-actions, and also
notifications matches (also in development) and to those we
should align to, as they are (partially already) present in the UI,
that's why I also value Lukas' opinion here, he spent more time with
such higher-level matching/grouping/filter stuff already.

If ordering, then it should be at least encoded explicit, the order
of sub-properties in a config isn't mattering for anything IIRC.

>>
>>> CLI via `--exclude` when creating a backup with proxmox-backup-client.
>>
>> Which is not really good UX, the "find" core util, and similar consorts,
>> are exactly deemed as hard to understand as that implicit order matters
>> is making it harder for users, especially those without a programming
>> background (i.e., the majority of our users).
> 
> Contrary to most tools, options come after the paths, you use single
> dashes for long options, use exclamation marks for inversion which used
> to get butchered by most shells with history expansion unless they
> recognize `find` themselves and parenthesis which don't work if you
> don't have spaces around them as they need to be actual separate
> parameters so you also need to watch for quotes...
> 
> But sure, let's say the ordering is the issue... 🙄

you know that there can be more than one issue at the same time..
Again, ordering is *another* dimension, just like inversions and grouping
via parenthesis is, adding more of them is never going to make things
easier, but sure, more expressive and often turing complete, depending
on the use case that can be great or not – here it's not.

> The thing is, includes and excludes are conflicting statements, they
> must be resolved somehow, and in regular speech it's often by order.
> Which is not to say that people wouldn't get confused *regardless*, even
> if you use precise language.

Yeah sure, it's a given that one cannot make all users content at the
same time

> Then again, we also have a whole group of people insisting on
> always saying "including but not limited to...".
> I guess there's a point to that if we change the default depending on
> whether only includes or only excludes are present.

The defaults should IMO be unrelated to the existence of entries for
the other thing, and be:

Include: All
Exclude: None

> Ultimately, *that* is the part that would confuse the hell out of me,
> personally, but sure, you'll end up dealing with both types of users
> anyway, so as long as that behavior is documented... 🤷
> 

What part do you mean exactly here?

>>> This makes it much easier to say things like "exclude 1-100 except 50",
>>
>> Not really, you just use "include 50" and if the remaining set is bigger
>> do "exclude 1-49; exclude 51-100;" and especially in bigger examples this
>> is easier to reason about for the standard users.
> 
> Maybe we have different interpretations of the word "easier" :-P

definitively about the word "much" ;-)

[pbs-devel] [PATCH proxmox-backup 2/3] ui: Show if Filter includes or excludes

[Philipp Hufnagl]

To make the UI compatible, the Group Filter dialogue has been extended
by a "behaviour" column which will show if the filter includes or
excludes the matches.

While it would be a nice feature to also re-sort the filter in the GUI,
it is not strictly needed for it to work and should (in my option) be
done in an other patch.

Signed-off-by: Philipp Hufnagl <p.hufnagl@proxmox.com>
---
 www/form/GroupFilter.js | 57 ++++++++++++++++++++++++++++++++++++++---
 1 file changed, 53 insertions(+), 4 deletions(-)

diff --git a/www/form/GroupFilter.js b/www/form/GroupFilter.js
index dee37b0b..10ac02c4 100644
--- a/www/form/GroupFilter.js
+++ b/www/form/GroupFilter.js
@@ -45,6 +45,22 @@ Ext.define('PBS.form.GroupFilter', {
 	    me.updateRealField();
 	},
 
+
+	onBehaviourChange: function(field, value) {
+	    let me = this;
+	    let record = field.getWidgetRecord();
+	    if (record === undefined) {
+		return;
+	    }
+
+	    record.set('behaviour', value);
+	    record.commit();
+	    if (record.widgets) {
+		me.setInputValue(record.widgets, record);
+	    }
+	    me.updateRealField();
+	},
+
 	onTypeChange: function(field, value) {
 	    let me = this;
 	    let record = field.getWidgetRecord();
@@ -77,8 +93,12 @@ Ext.define('PBS.form.GroupFilter', {
 	},
 
 	parseGroupFilter: function(filter) {
-	    let [, type, input] = filter.match(/^(type|group|regex):(.*)$/);
+	    let [, behaviour, type, input] = filter.match(/^(?:(exclude|include):)?(type|group|regex):(.*)$/);
+	    if (behaviour === undefined) {
+		behaviour = "include";
+	    }
 	    return {
+		behaviour,
 		type,
 		input,
 	    };
@@ -163,8 +183,12 @@ Ext.define('PBS.form.GroupFilter', {
 
 	    let filter = [];
 	    me.lookup('grid').getStore().each((rec) => {
-		if (rec.data.type && rec.data.input) {
-		    filter.push(`${rec.data.type}:${rec.data.input}`);
+		if (rec.data.type && rec.data.input && rec.data.behaviour) {
+		    let behaviour_string = '';
+		    if (rec.data.behaviour === 'exclude') {
+			behaviour_string = 'exclude:';
+		    }
+		    filter.push(`${behaviour_string}${rec.data.type}:${rec.data.input}`);
 		}
 	    });
 
@@ -175,6 +199,9 @@ Ext.define('PBS.form.GroupFilter', {
 	},
 
 	control: {
+	    'grid pbsGroupBehaviourSelector': {
+		change: 'onBehaviourChange',
+	    },
 	    'grid pbsGroupFilterTypeSelector': {
 		change: 'onTypeChange',
 	    },
@@ -277,6 +304,16 @@ Ext.define('PBS.form.GroupFilter', {
 		deferEmptyText: false,
 	    },
 	    columns: [
+		{
+		    text: gettext('Behaviour'),
+		    xtype: 'widgetcolumn',
+		    dataIndex: 'behaviour',
+		    flex: 1,
+		    widget: {
+			xtype: 'pbsGroupBehaviourSelector',
+			isFormField: false,
+		    },
+		},
 		{
 		    text: gettext('Filter Type'),
 		    xtype: 'widgetcolumn',
@@ -368,7 +405,7 @@ Ext.define('PBS.form.GroupFilter', {
 		    xtype: 'box',
 		    style: 'margin: 3px 0px;',
 		    html: `<span class="pmx-hint">${gettext('Note')}</span>: `
-			+ gettext('Filters are additive (OR-like)'),
+			+ gettext('Filters are additive or subtractive'),
 		},
 	    ],
 	},
@@ -384,6 +421,18 @@ Ext.define('PBS.form.GroupFilter', {
     },
 });
 
+Ext.define('PBS.form.pbsGroupBehaviourSelector', {
+    extend: 'Proxmox.form.KVComboBox',
+    alias: 'widget.pbsGroupBehaviourSelector',
+
+    allowBlank: false,
+
+    comboItems: [
+	['include', gettext('Include')],
+	['exclude', gettext('Exclude')],
+    ],
+});
+
 Ext.define('PBS.form.GroupFilterTypeSelector', {
     extend: 'Proxmox.form.KVComboBox',
     alias: 'widget.pbsGroupFilterTypeSelector',
-- 
2.39.2

Re: [pbs-devel] [PATCH proxmox-backup 2/3] ui: Show if Filter includes or excludes

[Lukas Wagner]

On 10/23/23 17:43, Philipp Hufnagl wrote:
> @@ -163,8 +183,12 @@ Ext.define('PBS.form.GroupFilter', {
>   
>   	    let filter = [];
>   	    me.lookup('grid').getStore().each((rec) => {
> -		if (rec.data.type && rec.data.input) {
> -		    filter.push(`${rec.data.type}:${rec.data.input}`);
> +		if (rec.data.type && rec.data.input && rec.data.behaviour) {
> +		    let behaviour_string = '';
> +		    if (rec.data.behaviour === 'exclude') {
> +			behaviour_string = 'exclude:';
                                 ^
should be in camelCase, according to our JS Style guide

> +		    }
> +		    filter.push(`${behaviour_string}${rec.data.type}:${rec.data.input}`);
>   		}
>   	    });
>   


-- 
- Lukas

Re: [pbs-devel] [PATCH proxmox-backup 2/3] ui: Show if Filter includes or excludes

[Lukas Wagner]

On 10/23/23 17:43, Philipp Hufnagl wrote:
> To make the UI compatible, the Group Filter dialogue has been extended
> by a "behaviour" column which will show if the filter includes or
> excludes the matches.
> 

Oh, and maybe do a s/behaviour/behavior/g - we generally prefer
US English over UK English.

-- 
- Lukas

Re: [pbs-devel] [PATCH proxmox-backup 2/3] ui: Show if Filter includes or excludes

[Philipp Hufnagl]

On 10/24/23 14:27, Lukas Wagner wrote:
> 
> 
> On 10/23/23 17:43, Philipp Hufnagl wrote:
>> To make the UI compatible, the Group Filter dialogue has been extended
>> by a "behaviour" column which will show if the filter includes or
>> excludes the matches.
>>
> 
> Oh, and maybe do a s/behaviour/behavior/g - we generally prefer
> US English over UK English.
> 

Actually I wondered about that too. If no one finds a good reason why
we should leave 'behaviour', I will s/behaviour/behavior/g.

Re: [pbs-devel] [PATCH proxmox-backup 2/3] ui: Show if Filter includes or excludes

[Philipp Hufnagl]

On 10/24/23 14:36, Philipp Hufnagl wrote:
> 
> 
> On 10/24/23 14:27, Lukas Wagner wrote:
>>
>>
>> On 10/23/23 17:43, Philipp Hufnagl wrote:
>>> To make the UI compatible, the Group Filter dialogue has been extended
>>> by a "behaviour" column which will show if the filter includes or
>>> excludes the matches.
>>>
>>
>> Oh, and maybe do a s/behaviour/behavior/g - we generally prefer
>> US English over UK English.
>>
> 
> Actually I wondered about that too. If no one finds a good reason why
> we should leave 'behaviour', I will s/behaviour/behavior/g.
> 
> 
> _______________________________________________
> pbs-devel mailing list
> pbs-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
> 
> 

I found out that our convention in this cases is to use US English. I
will change to behavior. [1]


[1]
https://wiki.intra.proxmox.com/index.php/Technical_Writing_Style_Guide#US_or_UK_English.3F

Re: [pbs-devel] [PATCH proxmox-backup 2/3] ui: Show if Filter includes or excludes

[Lukas Wagner]

On 10/24/23 16:09, Philipp Hufnagl wrote:
> 
> I found out that our convention in this cases is to use US English. I
> will change to behavior. [1]
> 
> 
> [1]
> https://wiki.intra.proxmox.com/index.php/Technical_Writing_Style_Guide#US_or_UK_English.3F
> 

That's what I meant - sorry, should've included the link with my
reply :)

-- 
- Lukas

Re: [pbs-devel] [PATCH proxmox-backup 2/3] ui: Show if Filter includes or excludes

[Thomas Lamprecht]

Am 24/10/2023 um 16:12 schrieb Lukas Wagner:
> 
> 
> On 10/24/23 16:09, Philipp Hufnagl wrote:
>>
>> I found out that our convention in this cases is to use US English. I
>> will change to behavior. [1]
>>
>>
>> [1]
>> https://wiki.intra.proxmox.com/index.php/Technical_Writing_Style_Guide#US_or_UK_English.3F
>>
> 
> That's what I meant - sorry, should've included the link with my
> reply :)
> 

Well, internal links on public lists are semi-helpful, but the real
issue is that such a wiki is still internal, if we expect all
contributors to follow it (at the beginning it was mostly for press
releases and articles, so there was no hard need for it to be public).

So, I plan to make that guide public soon. Daniela will give it a
deeper look sometime in the next days/weeks to fix some dead links
and references and ensure it's content is OK to publish in general.

[pbs-devel] [PATCH proxmox-backup 3/3] docs: document new include/exclude paramenter

[Philipp Hufnagl]

Adding the newly introduced optional include/exclude parameter to the
PBS documentation.

Signed-off-by: Philipp Hufnagl <p.hufnagl@proxmox.com>
---
 docs/managing-remotes.rst | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/docs/managing-remotes.rst b/docs/managing-remotes.rst
index 1c52e120..0597786b 100644
--- a/docs/managing-remotes.rst
+++ b/docs/managing-remotes.rst
@@ -116,6 +116,13 @@ of the specified criteria are synced. The available criteria are:
 The same filter is applied to local groups, for handling of the
 ``remove-vanished`` option.
 
+A ``group-filter`` can be inverted by adding ``exclude:`` to its beginning.
+
+* Regular expression example, excluding the match:
+    .. code-block:: console
+
+     # proxmox-backup-manager sync-job update ID --group-filter exclude:regex:'^vm/1\d{2,3}$'
+
 .. note:: The ``protected`` flag of remote backup snapshots will not be synced.
 
 Namespace Support
-- 
2.39.2