Add multiple label selector support to Velero Backup and Restore APIs #4650

shubham-pampattiwar · 2022-02-15T14:53:29Z

Signed-off-by: Shubham Pampattiwar [email protected]

Thank you for contributing to Velero!

Please add a summary of your change

The PR follows the approach specified in the multiple label support design and does the following:

adds a new spec to the Velero backup and Velero restore APIs called labelSelectors (plural)
updates the respective Velero backup and Velero restore CRDs with the addition of labelSelectors (plural) spec option
updates the logic in velero backup and restore workflow to respect the labelSelectors (plural) option when specified via API

Does your change fix a particular issue?

Fixes #1508

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.

pkg/restore/restore.go

pkg/backup/item_collector.go

reasonerjt · 2022-04-19T14:57:15Z

In addition to my inline comments, I wish to suggest finding a better naming for the fields in the CRD and variable names in the code.

Currently, after the PR is merged, we'll have co-existence of labelSelector and labelSelectors in the API, selectorand selectors in the same struct and they all have different meanings and usage, which is not quite maintainable.

pkg/backup/item_collector.go

codecov-commenter · 2022-05-06T07:42:16Z

Codecov Report

Merging #4650 (59b1f8f) into main (aa71427) will decrease coverage by 0.18%.
The diff coverage is 54.21%.

❗ Current head 59b1f8f differs from pull request most recent head e4bf5fb. Consider uploading reports for the commit e4bf5fb to get more accurate results

@@            Coverage Diff             @@
##             main    #4650      +/-   ##
==========================================
- Coverage   41.37%   41.19%   -0.19%     
==========================================
  Files         204      204              
  Lines       17943    18113     +170     
==========================================
+ Hits         7424     7461      +37     
- Misses       9972    10103     +131     
- Partials      547      549       +2

Impacted Files	Coverage Δ
pkg/builder/backup_builder.go	`0.00% <0.00%> (ø)`
pkg/builder/restore_builder.go	`0.00% <0.00%> (ø)`
pkg/backup/item_collector.go	`62.25% <38.29%> (+0.77%)`	⬆️
pkg/restore/restore.go	`67.06% <86.95%> (+0.36%)`	⬆️
pkg/controller/backup_controller.go	`48.29% <100.00%> (-9.71%)`	⬇️
pkg/controller/restore_controller.go	`66.96% <100.00%> (+0.22%)`	⬆️
pkg/cmd/server/server.go	`6.89% <0.00%> (-0.02%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update aa71427...e4bf5fb. Read the comment docs.

config/crd/v1/bases/velero.io_backups.yaml

config/crd/v1/bases/velero.io_restores.yaml

pkg/apis/velero/v1/backup.go

pkg/apis/velero/v1/restore.go

pkg/restore/restore.go

pkg/controller/backup_controller.go

pkg/controller/restore_controller.go

pkg/apis/velero/v1/backup.go

reasonerjt · 2022-05-11T11:57:56Z

pkg/backup/item_collector.go

+		}
+
+		// Listing items for labelSelector (singular)
+		if len(orLabelSelectors) == 0 {


it seems when len(orLabelSelectors) == 0 and you choose to use labelSelector, the logic is almost identical to the case when there's only one element in the orLabelSelectors?

Why not combine them into one func, that takes the slice of label selectors as a param and returns the unstructuredItems

Thanks @reasonerjt I have updated the PR, please take another look.

Signed-off-by: Shubham Pampattiwar <[email protected]> remove backup CLI bits Signed-off-by: Shubham Pampattiwar <[email protected]> labelselectors spec option for velero restore Signed-off-by: Shubham Pampattiwar <[email protected]> add changelog file Signed-off-by: Shubham Pampattiwar <[email protected]> update spec name to OrLabelSelectors Signed-off-by: Shubham Pampattiwar <[email protected]> minor fixes Signed-off-by: Shubham Pampattiwar <[email protected]> add validations for labelSelector and orLabelSelectors Signed-off-by: Shubham Pampattiwar <[email protected]> update crds.gp after fixing conflicts Signed-off-by: Shubham Pampattiwar <[email protected]> fix CI and add unit tests Signed-off-by: Shubham Pampattiwar <[email protected]> updated OrLabelSelector spec description and added validation failure unit tests Signed-off-by: Shubham Pampattiwar <[email protected]> add comments and change log level Signed-off-by: Shubham Pampattiwar <[email protected]> update site docs Signed-off-by: Shubham Pampattiwar <[email protected]> wrap pager client calls in a function Signed-off-by: Shubham Pampattiwar <[email protected]> resolve confilcts and update crds.go Signed-off-by: Shubham Pampattiwar <[email protected]> rebase and update crds.go Signed-off-by: Shubham Pampattiwar <[email protected]> combine listing items for a given label Signed-off-by: Shubham Pampattiwar <[email protected]>

reasonerjt

lgtm, thanks @shubham-pampattiwar

Shubham Pampattiwar has made several contributions to Velero, most recently in designing and implementing two v1.9 features, including the following: - [Add design for enabling multiple label support[(vmware-tanzu#4619) - [Add multiple label selector support to Velero Backup and Restore APIs](vmware-tanzu#4650) - [Add design for enabling support for ExistingResourcePolicy to restore API](vmware-tanzu#4613) - [Add existingResourcePolicy to Restore API](vmware-tanzu#4628) Signed-off-by: Scott Seago <[email protected]>

Shubham Pampattiwar has made several contributions to Velero, most recently in designing and implementing two v1.9 features, including the following: - [Add design for enabling multiple label support](vmware-tanzu#4619) - [Add multiple label selector support to Velero Backup and Restore APIs](vmware-tanzu#4650) - [Add design for enabling support for ExistingResourcePolicy to restore API](vmware-tanzu#4613) - [Add existingResourcePolicy to Restore API](vmware-tanzu#4628) Signed-off-by: Scott Seago <[email protected]>

Shubham Pampattiwar has made several contributions to Velero, most recently in designing and implementing two v1.9 features, including the following: - [Add design for enabling multiple label support](vmware-tanzu#4619) - [Add multiple label selector support to Velero Backup and Restore APIs](vmware-tanzu#4650) - [Add design for enabling support for ExistingResourcePolicy to restore API](vmware-tanzu#4613) - [Add existingResourcePolicy to Restore API](vmware-tanzu#4628) Shubham has also been driving forward the data mover requirements and design discussions for velero 1.10: - [Add datamover design](vmware-tanzu#4768) Signed-off-by: Scott Seago <[email protected]>

shubham-pampattiwar force-pushed the add-label-selectors branch 2 times, most recently from 51f367b to 939175a Compare March 30, 2022 07:02

shubham-pampattiwar changed the title ~~Add multiple label selector support to backup API~~ Add multiple label selector support to Velero Backup and Restore APIs Mar 30, 2022

shubham-pampattiwar force-pushed the add-label-selectors branch from 939175a to a9bc0c3 Compare March 30, 2022 07:14

github-actions bot added the has-changelog label Mar 30, 2022

shubham-pampattiwar marked this pull request as ready for review March 30, 2022 07:16

github-actions bot requested review from jenting and reasonerjt March 30, 2022 07:16

github-actions bot assigned shubham-pampattiwar Mar 30, 2022

shubham-pampattiwar mentioned this pull request Apr 1, 2022

Add design for enabling multiple label support #4619

Merged

3 tasks