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

Filter by label #195

Closed
killianmuldoon opened this issue Nov 11, 2019 · 19 comments · Fixed by #510
Closed

Filter by label #195

killianmuldoon opened this issue Nov 11, 2019 · 19 comments · Fixed by #510
Assignees
Labels
kind/feature Categorizes issue or PR as related to a new feature.

Comments

@killianmuldoon
Copy link

I'm looking to filter pods by label before applying a descheduling strategy to them. This allows us to slice our descheduling operations helping with testing and with only descheduling workloads that are actively identified as deschedulable based on developer/operator knowledge. This should be opt in - i.e. with no label selector in the policy descheduler will work as now.

I'm happy to write a PR for this - but I wanted to ask for advice on where this logic should be implemented.

One option is in the ListEvictablePodsOnNode method, or one of its sub methods, - but it would need a change to the function signature which would break current uses of this package.

Another option is to implement the label selector with a call in every strategy. There's duplication of work here that may be tough to maintain in future.

A third option is to wrap the Kubernetes Client in an LabelledClient Interface that embeds the kubernetes.Interface but also holds a labelSelector. This could then be called in the ListEvictablePodsOnNode, but this requires a type assertion in that method.

I'd love to get some feedback on this - or any implementation ideas I might be missing.

@killianmuldoon
Copy link
Author

PR to resolve this at #202

@seanmalloy
Copy link
Member

/kind feature

@k8s-ci-robot k8s-ci-robot added the kind/feature Categorizes issue or PR as related to a new feature. label Feb 6, 2020
@fejta-bot
Copy link

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label May 6, 2020
@seanmalloy
Copy link
Member

/remove-lifecycle stale

@k8s-ci-robot k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label May 6, 2020
@seanmalloy
Copy link
Member

The proposal has been reviewed by SIG scheduling and this feature can now be implemented. Descheduling by pod label will be implemented using a strategy parameter, not a CLI option. The labelSelector field in the YAML should be a standard k8s labelSelector.

Here is an example of what the YAML might look like:

---
apiVersion: "descheduler/v1alpha1"
kind: "DeschedulerPolicy"
strategies:
  "PodLifeTime":
     enabled: true
     params:
        maxPodLifeTimeSeconds: 86400
	labelSelector:
          matchLabels:
            component: redis
          matchExpressions:
            - {key: tier, operator: In, values: [cache]}
            - {key: environment, operator: NotIn, values: [dev]}

The proposal document can be found here: https://bit.ly/k8s-descheduler-labels-namespaces

@hanumanthan you had mentioned that you were interested in implementing this feature. Feel free to start working on it. Let us know if you have any questions. Let us know if you are able to work on this or not. Thanks!

@bytetwin
Copy link
Contributor

/assign @hanumanthan

@bytetwin
Copy link
Contributor

@seanmalloy I couldnt find the standard implementation for labels parsing. Can you please provide any doc or code for parsing labels.
Found this https://godoc.org/k8s.io/apimachinery/pkg/labels#Selector but want to know the standard way to parse this from the input config

matchLabels:
	component: redis
matchExpressions:
	- {key: tier, operator: In, values: [cache]}
	- {key: environment, operator: NotIn, values: [dev]}

@damemi
Copy link
Contributor

damemi commented Jul 15, 2020

@hanumanthan these links might be helpful, the example config is from the k8s docs on label selectors: https://kubernetes.io/docs/concepts/overview/working-with-objects/labels/#resources-that-support-set-based-requirements

So really the type that needs to be added to the config is just a metav1.LabelSelector, which can be parsed to a labels.Selector with metav1.LabelSelectorAsSelector(). Then you have all of the matching helpers you linked that come with a labels.Selector.

@ingvagabund
Copy link
Contributor

@hanumanthan do you still want to work on this issue? There's a Options struct which can be easily extended with WithLabels method. ListPodsOnANode already accepts the struct so it's easy to wire the labels down to it.

https://github.com/kubernetes-sigs/descheduler/blob/master/pkg/descheduler/pod/pods.go#L30-L34

@bytetwin
Copy link
Contributor

Yeah I will work on this weekend

@fejta-bot
Copy link

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Oct 27, 2020
@seanmalloy
Copy link
Member

/remove-lifecycle stale

@k8s-ci-robot k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Oct 27, 2020
@fejta-bot
Copy link

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jan 25, 2021
@ingvagabund
Copy link
Contributor

/remove-lifecycle stale

@k8s-ci-robot k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jan 25, 2021
@lindhe
Copy link

lindhe commented Feb 22, 2021

Would be nice to have label filtering for namespaces too!

@lixiang233
Copy link
Contributor

Hi @hanumanthan, do you still plan to work on this? If not, I'm glad to pick this up.

@seanmalloy
Copy link
Member

@lixiang233 I say go for it if you have the time.

@lixiang233
Copy link
Contributor

/unassign @hanumanthan
/assign

@seanmalloy
Copy link
Member

This feature will be available in descheduler v0.21.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature Categorizes issue or PR as related to a new feature.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants