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

Namespaces included by labelSelector act as IncludedNamespaces for Backup #8342

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

kaovilai
Copy link
Member

@kaovilai kaovilai commented Oct 23, 2024

Thank you for contributing to Velero!

Please add a summary of your change

Namespaces included by backup's selector behave the same as specifying backup.IncludedNamespaces. All resources under the labeled namespace is included.

Prior to this change, labeled namespaces are included, but anything else under the namespace if not labeled are not.

Does your change fix a particular issue?

Fixes #7492

Please indicate you've done the following:

pkg/backup/backup_test.go Outdated Show resolved Hide resolved
@kaovilai kaovilai changed the title Unit tests to check #7492 requirements Namespaces included by labelSelector act as IncludedNamespaces Oct 23, 2024
@kaovilai kaovilai force-pushed the issue7492 branch 11 times, most recently from 25e190c to d311933 Compare October 24, 2024 04:25
@kaovilai kaovilai force-pushed the issue7492 branch 3 times, most recently from 5e2355e to f2f044b Compare October 24, 2024 04:31
Copy link

codecov bot commented Oct 24, 2024

Codecov Report

Attention: Patch coverage is 92.30769% with 3 lines in your changes missing coverage. Please review.

Project coverage is 58.99%. Comparing base (ebbeb7a) to head (7f5c57e).
Report is 38 commits behind head on main.

Files with missing lines Patch % Lines
pkg/backup/item_collector.go 92.30% 2 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #8342      +/-   ##
==========================================
- Coverage   58.99%   58.99%   -0.01%     
==========================================
  Files         367      367              
  Lines       38847    38870      +23     
==========================================
+ Hits        22918    22931      +13     
- Misses      14467    14475       +8     
- Partials     1462     1464       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@kaovilai kaovilai marked this pull request as ready for review October 24, 2024 04:54
@kaovilai kaovilai changed the title Namespaces included by labelSelector act as IncludedNamespaces Namespaces included by labelSelector act as IncludedNamespaces for Backup Oct 24, 2024
every item under NS is included.

Signed-off-by: Tiger Kaovilai <[email protected]>
@blackpiglet
Copy link
Contributor

I understand there is a requirement from the community to back up resources under the labelled namespace, but we can also do that by using the namespace filter, do we need a new way to do the same function?

@kaovilai
Copy link
Member Author

kaovilai commented Oct 24, 2024

Some admins may find it easier to automate ns backup by label.
Perhaps individual small teams label their resources.
In some orgs, not everyone will have RBAC to create modify backups.velero.io custom resources.
And backing up by including the label may be a preferred way if available for some.

Apart from velero issue this pr fixes, it has also been requested to our team in https://issues.redhat.com/browse/OADP-4572

I think the question is, when is the current behavior of backing up namespace but not namespaced resources within it desirable?

The previous fix already decided that any namespace not included by namespace filters like label should not be in the backup.

Now we're just saying every namespace included via namespace filters (including label) will have namespaced resources of those namespaces in the backup.

@reasonerjt
Copy link
Contributor

reasonerjt commented Oct 28, 2024

@kaovilai Thanks for this PR, but have we reached agreement in terms of whether we should implement it?
cc @blackpiglet

@kaovilai
Copy link
Member Author

I don't think we have reached agreements yet.

@blackpiglet
Copy link
Contributor

blackpiglet commented Oct 29, 2024

@kaovilai
We can discuss scenarios requiring labels as the namespace filters, but please note this is a break change.

I agree that only backing up the namespace with selected labels without any resources in it is not a common use case, but at least, we have that option.

After introducing this change, we lost that option.

If the introduced method is the preferable option, we can go forward that way, but before applying it, I think we need to make sure that.

@kaovilai
Copy link
Member Author

Another thing with this implementation is that namespaces included via label selector would not show up in backup describe under IncludedNamespaces, but would likely still show in Resource List. I think that should be good enough.

@weshayutin
Copy link
Contributor

@kaovilai v1.16 candidate?

@kaovilai
Copy link
Member Author

Please add this PR to v1.16 milestone

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.

Select namespace to include by its labelSelector
5 participants