[pbs-devel] [PATCH proxmox-backup v4 1/3] fix #4315: jobs: modify GroupFilter so include/exclude is tracked
Lukas Wagner
l.wagner at proxmox.com
Thu Dec 14 17:22:47 CET 2023
Hi Philipp,
some comments inline :)
On 12/4/23 11:04, Philipp Hufnagl wrote:
> After some discussion I canged the include/exclude behavior to first run
> all include filter and after that all exclude filter (rather then
> allowing to alternate inbetween). This is simply done by sorting the
> list (include first) before executing it.
>
> Since a GroupFilter now also features an behavior, 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 engeneered.
>
> Matching a filter will now iterate with a forech loop in order to also
> exclude matches.
>
> Signed-off-by: Philipp Hufnagl <p.hufnagl at proxmox.com>
> ---
> pbs-api-types/src/datastore.rs | 11 +++---
> pbs-api-types/src/jobs.rs | 64 +++++++++++++++++++++++++++-------
> src/api2/pull.rs | 11 +++++-
> src/api2/tape/backup.rs | 17 +++++++--
> src/server/pull.rs | 23 +++++++++---
> 5 files changed, 99 insertions(+), 27 deletions(-)
>
> diff --git a/pbs-api-types/src/datastore.rs b/pbs-api-types/src/datastore.rs
> index d4ead1d1..c8f26b57 100644
> --- a/pbs-api-types/src/datastore.rs
> +++ b/pbs-api-types/src/datastore.rs
> @@ -843,17 +843,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 1f5b3cf1..dff02395 100644
> --- a/pbs-api-types/src/jobs.rs
> +++ b/pbs-api-types/src/jobs.rs
> @@ -3,6 +3,7 @@ use std::str::FromStr;
>
> use regex::Regex;
> use serde::{Deserialize, Serialize};
> +use std::cmp::Ordering;
>
> use proxmox_schema::*;
>
> @@ -388,7 +389,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
> @@ -397,7 +398,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,
> @@ -408,27 +409,64 @@ 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 Eq for GroupFilter {}
> +
> +impl PartialOrd for GroupFilter {
> + fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
> + self.is_exclude.partial_cmp(&other.is_exclude)
> + }
> + > +
> +impl Ord for GroupFilter {
> + fn cmp(&self, other: &Self) -> Ordering {
> + self.is_exclude.cmp(&other.is_exclude)
> + }
> +}
Having Ord/ParitalOrd based on the exclude flag is extremely confusing,
please don't do this.
See later comments for a less confusing way.
> +
> 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))
> + }?;
Is there a reason why you change the error format here?
> + 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()),
> }
> }
> }
> @@ -441,9 +479,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.")
'adding ... before' sounds a bit odd - maybe "Can be inverted by
prepending 'exclude:'" would be better here?
Also 'include' is not documented here.
> .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/pull.rs b/src/api2/pull.rs
> index eb9a2199..f174926c 100644
> --- a/src/api2/pull.rs
> +++ b/src/api2/pull.rs
> @@ -72,6 +72,15 @@ impl TryFrom<&SyncJobConfig> for PullParameters {
> type Error = Error;
>
> fn try_from(sync_job: &SyncJobConfig) -> Result<Self, Self::Error> {
> + let filters = match &sync_job.group_filter {
> + Some(v) => {
> + let mut f = v.clone();
> + f.sort();
> + Some(f)
> + }
> + None => None,
> + };
> +
I don't think that .sort()'ing is a good way to separate include/exclude
groups. PartialEq/PartialOrd/Ord being only based on the exclude flag is
extremely confusing.
Rather split the GroupFilter into two groups manually via a helper
(since you need to do it in multiple places), based on the exclude flag.
Then, first process the includes and subtract the excludes afterwards.
I'd do that at [1].
> PullParameters::new(
> &sync_job.store,
> sync_job.ns.clone().unwrap_or_default(),
> @@ -85,7 +94,7 @@ impl TryFrom<&SyncJobConfig> for PullParameters {
> .clone(),
> sync_job.remove_vanished,
> sync_job.max_depth,
> - sync_job.group_filter.clone(),
> + filters,
> sync_job.limit.clone(),
> sync_job.transfer_last,
> )
> diff --git a/src/api2/tape/backup.rs b/src/api2/tape/backup.rs
> index 2f9385a7..80dcdd1d 100644
> --- a/src/api2/tape/backup.rs
> +++ b/src/api2/tape/backup.rs
> @@ -412,14 +412,25 @@ fn backup_worker(
> group_list.sort_unstable_by(|a, b| a.group().cmp(b.group()));
>
> 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 filter_fn = |group: &BackupGroup, group_filters: &[GroupFilter], start_with: bool| {
> + let mut is_match = start_with;
> + for filter in group_filters.iter() {
I think calling .iter() is not necessary here.
> + if group.matches(filter) {
> + is_match = !filter.is_exclude;
> + }
> + }
> + is_match
> };
>
> let group_count_full = group_list.len();
> + // if there are only exclude filter, inculude everything
Typo in 'include'
> + let mut include_all = false;
> + if !group_filters.is_empty() || group_filters.first().unwrap().is_exclude {
> + include_all = true;
> + }
I think the logic is off here.
If group_filters only includes INCLUDE filters, we only want to include
those groups.
So .is_empty() returns false and we invert that, we set include_all to
true... which is not what we want.
Just to illustrate the different cases:
- no filters: All groups
- only include filters: ONLY the included ones
- only exclude filters: ALL BUT the excluded ones
- both: ONLY the included ones, minus the excluded ones
----
[1]: I would split the GroupFilters into includes/excludes here.
> let list: Vec<BackupGroup> = group_list
> .into_iter()
> - .filter(|group| filter_fn(group, group_filters))
> + .filter(|group| filter_fn(group, group_filters, include_all))
> .collect();
> let group_count = list.len();
> task_log!(
> diff --git a/src/server/pull.rs b/src/server/pull.rs
> index 3b71c156..027194a1 100644
> --- a/src/server/pull.rs
> +++ b/src/server/pull.rs
> @@ -1368,15 +1368,26 @@ pub(crate) async fn pull_ns(
> }
> });
>
> - let apply_filters = |group: &BackupGroup, filters: &[GroupFilter]| -> bool {
> - filters.iter().any(|filter| group.matches(filter))
> + let apply_filters = |group: &BackupGroup, filters: &[GroupFilter], start_with: bool| -> bool {
> + let mut is_match = start_with;
> + for filter in filters.iter() {
> + if group.matches(filter) {
> + is_match = !filter.is_exclude;
> + }
> + }
> + is_match
> };
>
> let list = if let Some(ref group_filter) = ¶ms.group_filter {
> + // if there are only exclude filter, inculude everything
> + let mut include_all = false;
> + if !group_filter.is_empty() || group_filter.first().unwrap().is_exclude {
> + include_all = true;
> + }
Same logic error here.
> let unfiltered_count = list.len();
> let list: Vec<BackupGroup> = list
> .into_iter()
> - .filter(|group| apply_filters(group, group_filter))
> + .filter(|group| apply_filters(group, group_filter, include_all))
> .collect();
> task_log!(
> worker,
> @@ -1458,7 +1469,11 @@ pub(crate) async fn pull_ns(
> continue;
> }
> if let Some(ref group_filter) = ¶ms.group_filter {
> - if !apply_filters(local_group, group_filter) {
> + let mut include_all = false;
> + if !group_filter.is_empty() || group_filter.first().unwrap().is_exclude {
> + include_all = true;
> + }
Same logic error here.
> + if !apply_filters(local_group, group_filter, include_all) {
> continue;
> }
> }
--
- Lukas
More information about the pbs-devel
mailing list