-
Notifications
You must be signed in to change notification settings - Fork 762
Add namespace label selector #1786
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
base: master
Are you sure you want to change the base?
Add namespace label selector #1786
Conversation
Signed-off-by: Danila Bobkov <[email protected]>
|
Welcome @W1seKappa! |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
Hi @W1seKappa. Thanks for your PR. I'm waiting for a github.com member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
Signed-off-by: Danila Bobkov <[email protected]>
|
@ingvagabund Hi, I'm happy to continue the discussion based on the initiative from this pr1501. |
| return indexer, nil | ||
| } | ||
|
|
||
| func getNamespacesListByLabelSelector(indexName string, labelSelector *metav1.LabelSelector, handle frameworktypes.Handle) (cache.Indexer, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand the idea here. If a namespace is matched by the label selector it's returned. If it is not an empty list is returned. I.e.:
- indexer.ByIndex(indexName, NSmatched) -> {NSmatched} // list with a single ns
- indexer.ByIndex(indexName, NSNotmatched) -> {} // empty list
On the other hand getNamespacesListByLabelSelector gives the belief all namespaces matching a label selector are returned. I.e. a label selector is the key, not a namespace. I.e.
- indexer.ByIndex(indexName, LS1) -> list of namespaces matching LS1
- ...
- indexer.ByIndex(indexName, LSN) -> list of namespaces matching LSN
Given each profile can have its own DefaultEvictor configuration with a different label selector the current global "namespaceWithLabelSelector" indexer will not work correctly as once the first indexer gets registered, any second attempt returns the first indexer. So the second label selector will get ignored and all DefaultEvictors will share the first label selector.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@W1seKappa thank you for taking over. As the next step I suggest to create another unit test that have two profiles configured. Each with a different label selector and to make sure both evictors evict distinct set of namespaces to correctly validate the functionality. There's no need to create actual profiles. Just mimicking it through creating two default evictors over the same namespace informer.
Description
Added namespace label selector for filtering pods in the DefaultEvictor plugin.
This pull request was created to finalize the initiative from that pr1501
Checklist
Please ensure your pull request meets the following criteria before submitting
for review, these items will be used by reviewers to assess the quality and
completeness of your changes: