Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add --or-selector for backup and restore command #6475

Merged
merged 1 commit into from
Sep 27, 2023
Merged

Add --or-selector for backup and restore command #6475

merged 1 commit into from
Sep 27, 2023

Conversation

nilesh-akhade
Copy link
Contributor

@nilesh-akhade nilesh-akhade commented Jul 7, 2023

Thank you for contributing to Velero!

Please add a summary of your change

Add 'orLabelSelector' that allows selecting resources matching any one of the label selectors.

With the current implementation of the --selector, it is not possible to select resources that have any one of the labels, app=nginx or foo=bar. This is because the selector, app=nginx,foo=bar will expect both the labels on the resource.

Selecting resources that have any one of the labels is already supported by the Restore and Backup CRDs, through orLabelSelectors: https://velero.io/docs/v1.11/api-types/restore/. This PR adds --or-selector to achieve the same thing using velero cli.

# syntax
velero backup create <backup-name> --or-selector "<label-selector1> or <label-selector2> or ..."

# example 1
velero restore create restore-prod --from-backup=prod-backup --or-selector "env in (prod,production) or environment in (prod,production)"

# example 2
velero backup create backup1 --or-selector "foo=bar or baz=qux"

# example 3
app in (web,nginx) or foo in (bar,baz)
# selects apps that have any one of the labels app=web, app=nginx, foo=bar, foo=baz

Does your change fix a particular issue?

Fixes #5679

Please indicate you've done the following:

  • Accepted the DCO. Commits without the DCO will delay acceptance.
  • Created a changelog file or added /kind changelog-not-required as a comment on this pull request.
  • Updated the corresponding documentation in site/content/docs/main.

@@ -101,6 +101,14 @@ Includes cluster-scoped resources. Cannot work with `--include-cluster-scoped-re

For more information read the [Kubernetes label selector documentation](https://kubernetes.io/docs/concepts/overview/working-with-objects/labels/#label-selectors)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking this docs, couldn't we achieve the same result using the --selector flag with in?

Ex.: Do these commands produce the same result?

  • velero backup create nginx-backup --selector "app in (nginx,my-nginx,nginx-example)"
  • velero backup create nginx-backup --or-selector "app=nginx|app=my-nginx|app=nginx-example"

Did not try every possibility, but maybe this PR can be achieve only adding examples to the docs with different scenarios (and updating help description of --selector flag in CLI)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But I think the usage from the docs is limited. For example, if I have 2 (or more) different labels (KEYs), I can't do an OR with them ("Similarly the comma separator acts as an AND operator.")

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mateusoliveira43 yes -- that's the sort of scenario the or selector was added for:
velero backup create nginx-backup --or-selector "app=nginx|foo=bar"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If resources to be backed up are labeled either env=prod or environment=production, then we cannot use --selector env=prod,environment=production. But we can use --or-selector "env=prod|environment=production"

I have updated the PR to include examples and explanations.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have modified the syntax a little bit. The or keyword can be used in place of | pipe symbol.

@nilesh-akhade nilesh-akhade changed the title Add 'orLabelSelectors' for backup, restore commands Add --or-selector for backup and restore command Jul 14, 2023
sseago
sseago previously approved these changes Jul 14, 2023
@codecov-commenter
Copy link

Codecov Report

Merging #6475 (d9a7e2b) into main (f234dd6) will decrease coverage by 0.03%.
The diff coverage is 48.00%.

@@            Coverage Diff             @@
##             main    #6475      +/-   ##
==========================================
- Coverage   60.28%   60.26%   -0.03%     
==========================================
  Files         238      239       +1     
  Lines       25256    25306      +50     
==========================================
+ Hits        15226    15250      +24     
- Misses       8976     8998      +22     
- Partials     1054     1058       +4     
Impacted Files Coverage Δ
pkg/cmd/util/output/restore_describer.go 31.52% <0.00%> (-1.23%) ⬇️
pkg/cmd/cli/backup/create.go 66.41% <40.00%> (-0.51%) ⬇️
pkg/cmd/cli/restore/create.go 61.05% <40.00%> (-0.38%) ⬇️
pkg/cmd/util/output/backup_describer.go 55.49% <45.45%> (-0.21%) ⬇️
pkg/cmd/util/flag/orlabelselector.go 83.33% <83.33%> (ø)

@reasonerjt
Copy link
Contributor

Since it's passed the v1.12 FC milestone, and this does not introduce big changes to the API, we'll target this one to the patch v1.12.1

@draghuram
Copy link
Contributor

Now that 1.12 is released, can this PR be merged?

@ywk253100 ywk253100 merged commit 563f1cc into vmware-tanzu:main Sep 27, 2023
22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

On restore, when multiple selector with same key are declared, only the last is applied
7 participants