add namespace filter to nodeutilization #967

knelasevero · 2022-09-28T14:07:42Z

Closes #966

knelasevero · 2022-09-28T14:15:19Z

pkg/framework/plugins/nodeutilization/nodeutilization.go

+				WithFilter(podEvictor.PreEvictionFilter).
+				WithNamespaces(includedNamespaces).
+				WithoutNamespaces(excludedNamespaces).
+				BuildFilterFunc()


Do we want to use these, or do we want to throw this functionality into the PreEvictionFilter directly?

The namespace is a parameter of the strategies so it needs to stay outside of PreEvictionFilter extension point

damemi · 2022-09-28T14:15:48Z

We chose not to allow namespace filtering for NodeUtilization because it ultimately only hinders the strategy (see #391 for a longer discussion on it)

tl;dr is that NodeUtilization is designed to balance resource consumption for an entire node. The idea of balanced resources across specific namespaces doesn't have a clear use case, and we can't guarantee it with this strategy anyway. Most of the use cases that seem like they fit are actually better served by a different strategy (like topology spreading).

Just curious is there more detail on where this is coming from for these strategies?

knelasevero · 2022-09-28T14:24:41Z

We chose not to allow namespace filtering for NodeUtilization because it ultimately only hinders the strategy (see #391 for a longer discussion on it)

tl;dr is that NodeUtilization is designed to balance resource consumption for an entire node. The idea of balanced resources across specific namespaces doesn't have a clear use case, and we can't guarantee it with this strategy anyway. Most of the use cases that seem like they fit are actually better served by a different strategy (like topology spreading).

Just curious is there more detail on where this is coming from for these strategies?

In the discussion #391 (comment)

I saw this message:

+1 for allowing to set a list of excluded namespaces only yet clearly expressing risks of misusing it. Also stressing the setting applies in eviction phase only.

So, for NodeUtilization all namespaces need to be considered for balancing, and here in this PR we are only deciding not to evict a pod from an excluded namespace right before the eviction call (still considering all namespaces while processing the balancing).

Internally we have a client requesting this because they have some pods in kube-system namespace that not necessarily have system critical priority class, and because of that, they want to enforce exclusion explicitly in the descheduler config. (and would also want to exclude other critical namespaces similar to kube-system)

damemi · 2022-09-28T15:17:27Z

So, for NodeUtilization all namespaces need to be considered for balancing, and here in this PR we are only deciding not to evict a pod from an excluded namespace right before the eviction call (still considering all namespaces while processing the balancing).

I think this will still create some confusing situations where the descheduler is reporting over/underutilized nodes and not doing anything about it because the namespaces are excluded.

@ingvagabund's point of clearly explaining the risks helps with that, but it won't stop people from misusing it. My concern is that the downsides to this outweigh the potential benefits

ingvagabund · 2022-09-28T19:13:57Z

I think this will still create some confusing situations where the descheduler is reporting over/underutilized nodes and not doing anything about it because the namespaces are excluded.

The same situation can already occur when the over/underutilized nodes are full of pods with system critical priority class set. Or pods with local storage or pvc.

but it won't stop people from misusing it. My concern is that the downsides to this outweigh the potential benefits

This is one of those design decisions which are not easy to make. We need a mechanism to protect namespaces but to also guarantee sufficient degree of balancing nodes. Two goals going against each other under certain condition.

We might hardcode a protection mechanism calculating whether pods from excluded namespaces occupy "too many" nodes. In which case the strategy would refuse any eviction until the pods are re-distributed.

knelasevero · 2022-10-07T10:38:58Z

We might hardcode a protection mechanism calculating whether pods from excluded namespaces occupy "too many" nodes. In which case the strategy would refuse any eviction until the pods are re-distributed.

Hmm 🤔 I personally think this would be even more confusing.

I think that if you explicitly choose a namespace to be ignored, and you are using a strategy that re-balances pods, you would expect the behavior that we described (where the descheduler is reporting over/underutilized nodes and not doing anything about it because the namespaces are excluded). It is something that if well documented would also be easy to respond to if users get confused (maybe not only docs, but also reporting things in lower log levels of these re-balancing strategies, maybe something like "we found pods that should be evicted to proceed with rebalancing, but they are system-critical or part of a filtered namespace").

ingvagabund · 2022-10-10T11:56:11Z

@damemi I would like to find an agreement on this. Either get this moving or decide to look for a different approach for namespace inclusion/exclusion. Alternative is to allow to specify only a list of excluded protected namespaces. Users will need to explicitly and willingly enumerate a list of excluded namespaces.

damemi · 2022-10-10T12:56:55Z

Overall, I'm personally against this because it doesn't have a clear use case and is designed to detriment an existing plugin. I think doing this will encourage users to use lownodeutilization when their use case is actually better fit for something like pod topology spread. Adding a big warning not to use this just adds confusion (why add a feature that we're warning most people not to use?)

Having voiced my concerns, I do see that I'm in the minority on this so it's unfair of me to block it. So, thank you for hearing me out

Alternative is to allow to specify only a list of excluded protected namespaces. Users will need to explicitly and willingly enumerate a list of excluded namespaces.

I would prefer this. I'm thinking most clusters will have a large number of overall namespaces with a smaller fraction of high-priority namespaces which they want to exclude. The alternative, where there is only a small fraction of namespaces to include, is much worse for the strategy's effectiveness. So discouraging that is probably best

knelasevero · 2022-10-10T13:16:25Z

@damemi @ingvagabund do we mean disallowing the Include field of namespaces? Or using another struct for this?

ingvagabund · 2022-10-10T13:21:20Z

yes, making use of included invalid/forbidden in the preEvictionFilter EP

damemi · 2022-10-10T13:33:30Z

yes, making use of included invalid/forbidden in the preEvictionFilter EP

to clarify, we are only talking about NodeUtilization strategies right? Others can still use include

If making include available to NodeUtilization is a technical requirement for it to be available to all strategies, then we don't have much choice there besides adding it to this strategy's validation

ingvagabund · 2022-10-10T14:33:04Z

Any plugin using preEvictionFilter will be affected. Currently only adopted by the node utilization strategies. Unless we accept a new plugin, it will stay this way forever. Also, we have validation to make sure preEvictionFilter will not get used in plugins where it is not expected to be used. When it comes to custom/third party plugins the validation will be relaxed by the descheduling framework unless the custom/third party plugins provide additional validation.

damemi · 2022-10-10T14:51:40Z

Any plugin using preEvictionFilter will be affected

That's not good, I didn't realize the scope of this. Any plugin should have the option of using either/or Include/Exclude, but they shouldn't be restricted by the framework.

The NodeUtilization plugins themselves should support or disallow namespace inclusion filters. If they're not able to do that, we may need to refactor how preEvictionFilter works

knelasevero · 2022-10-11T12:17:21Z

Can't we use nodeutilization validation (https://github.com/kubernetes-sigs/descheduler/blob/master/pkg/framework/plugins/nodeutilization/validation.go) to disallow this only for this plugin like @damemi suggested? We don't have to change anything on other plugins.

ingvagabund · 2022-10-11T13:28:47Z

I mean the expectation is to combine the preEvictionFilter with the namespace inclusion/exclusion. Namespace inclusion/exclusion is set by each strategy explicitly. Though, there's no way of detecting whether a custom plugin implementing the preEvictionFilter extension point is performing its own namespace inclusion/exclusion check. As providers of the upstream plugins with default setting and validation for all "approved" plugins we can make sure the namespace inclusion/exclusion is limited for specific strategies. However, we can not guarantee the same quality of protection will be enforced globally for custom plugins.

damemi · 2022-10-11T13:45:02Z

As providers of the upstream plugins with default setting and validation for all "approved" plugins we can make sure the namespace inclusion/exclusion is limited for specific strategies. However, we can not guarantee the same quality of protection will be enforced globally for custom plugins.

This is totally fine for me, we're the code owners for this plugin, so it's up to us what options to allow. Having Include/Exclude be available for every plugin makes sense (the framework is providing the mechanism to use this feature), but plugins like this one should choose to disallow it.

Do you think that's a reasonable flow for developers?

knelasevero · 2022-10-11T13:45:07Z

I mean the expectation is to combine the preEvictionFilter with the namespace inclusion/exclusion. Namespace inclusion/exclusion is set by each strategy explicitly. Though, there's no way of detecting whether a custom plugin implementing the preEvictionFilter extension point is performing its own namespace inclusion/exclusion check. As providers of the upstream plugins with default setting and validation for all "approved" plugins we can make sure the namespace inclusion/exclusion is limited for specific strategies. However, we can not guarantee the same quality of protection will be enforced globally for custom plugins.

I think that's fine, we can have guidelines for people creating custom plugins, and also we should be able to review/accept new plugins at a "registry" repo like it is done for the kube-scheduler. We just need to make sure this is part of the review process.

and validation for all "approved" plugins we can make sure the namespace inclusion/exclusion is limited for specific strategies

same quality of protection

So, about making sure, and quality of protection, are we talking about the validation.go of those plugins still?

ingvagabund · 2022-10-11T13:52:35Z

but plugins like this one should choose to disallow it.

A plugin only sees the preEvictionFilter extension point. It does not see what's invoked inside. So the only explicit mechanism for disallowing the node utilization plugins to get the node inclusion is to enforce the DefaultEvictor plugin for the preEvictionFilter extension point during the profile validation. Which is also acceptable.

ingvagabund · 2022-10-11T13:54:47Z

So, about making sure, and quality of protection, are we talking about the validation.go of those plugins still?

There's no way to validate the behavior which we do not control/own unless we enforce use of specific extension points for specific plugins. Which might be too constraining for users. In which point we can introduce a new descheduler option saying --disable-strict-validation or similar.

damemi · 2022-10-11T13:55:04Z

Maybe it would be good to see a poc change with that kind of enforcement applied? I'm getting a little confused so seeing the alternative implementation could help inform a better decision. @ingvagabund would you mind throwing together a rough approach to show what you mean?

knelasevero · 2022-10-11T14:01:13Z

I think I got it now. I can provide the implementation that Jan mentions here

is to enforce the DefaultEvictor plugin for the preEvictionFilter extension point during the profile validation

And indeed it can end up being too restricting 🤔. But I will push a commit so we can have a look

knelasevero · 2022-10-11T14:51:53Z

I just noticed that having v1alpha2 and the changes to policyconfig.go of #974 would be kind of necessary here to work on that 😓. Since this is validation at the profile level and not at the plugin level.

ingvagabund · 2022-10-11T16:29:55Z

@damemi locking a profile for the node utilization strategies. E.g. whenever a profile configures LowNodeUtilization plugin, the validation would make sure preEvictionFilter is always set to enabled = [DefaultEvictor]. To avoid enabling another preEvictionFilter extension point of a different plugin which would include/exclude namespaces.

apiVersion: descheduler/v1alpha2
kind: DeschedulerPolicy
profiles:
- name: ProfileName
  pluginConfig:
  - name: DefaultEvictor
    args:
      ...
  - name: LowNodeUtilization
    args:
      ...
  plugins:
    balance:
      enabled:
      - LowNodeUtilization
    preEvictionFilter:
      enabled:
      - DefaultEvictor

damemi · 2022-10-12T17:41:59Z

@ingvagabund ah, I see. That seems like a reasonable approach for plugin developers to take imo

Instead of enforcing it against a specific evictor, could it enforce that the evictor's arguments don't use namespace filtering? So you don't get stuck with DefaultEvictor

knelasevero · 2022-10-21T11:41:12Z

So, as per latest discussions of the last community meeting, should we merge this one? I added docs and that simple validation.

I wanted to land #974 before merging this one 😢 , but since I am kind of stuck with decoding/conversions there, I wanted to push this one forward already.

knelasevero · 2022-10-21T12:11:22Z

/retest

ingvagabund · 2022-10-24T09:41:14Z

pkg/framework/plugins/nodeutilization/nodeutilization.go

@@ -280,14 +283,30 @@ func evictPods(
 	continueEviction continueEvictionCond,
 ) {

+	var includedNamespaces, excludedNamespaces sets.String


includedNamespaces can be dropped

ingvagabund · 2022-10-24T10:55:49Z

README.md

@@ -243,6 +243,7 @@ actual usage metrics. Implementing metrics-based descheduling is currently TODO
 |`thresholdPriority`|int (see [priority filtering](#priority-filtering))|
 |`thresholdPriorityClassName`|string (see [priority filtering](#priority-filtering))|
 |`nodeFit`|bool (see [node fit filtering](#node-fit-filtering))|
+|`evictionNamespaces`|(see [namespace filtering](#namespace-filtering))|


evictableNamespaces

ingvagabund · 2022-10-24T10:56:16Z

pkg/framework/plugins/nodeutilization/types.go

+	// Naming this one differently since namespaces are still
+	// considered while considering resoures used by pods
+	// but then filtered out before eviction
+	EvictionNamespaces *api.Namespaces


EvictableNamespaces

ingvagabund · 2022-10-24T10:56:22Z

pkg/framework/plugins/nodeutilization/types.go

+	// Naming this one differently since namespaces are still
+	// considered while considering resoures used by pods
+	// but then filtered out before eviction
+	EvictionNamespaces *api.Namespaces


EvictableNamespaces

ingvagabund · 2022-10-24T10:57:12Z

pkg/framework/plugins/nodeutilization/nodeutilization.go

+				WithFilter(podEvictor.PreEvictionFilter).
+				WithNamespaces(includedNamespaces).
+				WithoutNamespaces(excludedNamespaces).
+				BuildFilterFunc()


The namespace is a parameter of the strategies so it needs to stay outside of PreEvictionFilter extension point

ingvagabund · 2022-10-24T10:57:40Z

Few nits. Looks good in overall.

knelasevero · 2022-10-25T19:57:04Z

The namespace is a parameter of the strategies so it needs to stay outside of PreEvictionFilter extension point

I couldn't understand this one.

ingvagabund · 2022-10-27T11:49:57Z

Related to

Do we want to use these, or do we want to throw this functionality into the PreEvictionFilter directly?

The former.

ingvagabund · 2022-10-27T11:50:42Z

@knelasevero there are still few mentions of the evictionNamespaces and #967 (comment). Otherwise lgtm.

ingvagabund · 2022-10-30T14:48:06Z

/approve
/lgtm

k8s-ci-robot · 2022-10-30T14:48:28Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ingvagabund

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [ingvagabund]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Sep 28, 2022

k8s-ci-robot requested review from a7i and ingvagabund September 28, 2022 14:08

knelasevero commented Sep 28, 2022

View reviewed changes

JaneLiuL mentioned this pull request Oct 18, 2022

I hope the descheduler can support some functions such as namespaces filtering and pods filtering in the LowNodeUtilization strategy #992

Closed

knelasevero force-pushed the pre-ev-namespace-filter branch from 955fc23 to cced01d Compare October 21, 2022 11:39

knelasevero force-pushed the pre-ev-namespace-filter branch 3 times, most recently from 75783c6 to 4c4663a Compare October 21, 2022 11:52

ingvagabund requested changes Oct 24, 2022

View reviewed changes

knelasevero force-pushed the pre-ev-namespace-filter branch from 4c4663a to a646d62 Compare October 25, 2022 19:55

knelasevero force-pushed the pre-ev-namespace-filter branch from a646d62 to 63f7a93 Compare October 29, 2022 22:50

add namespace filter to nodeutilization

398ffa7

knelasevero force-pushed the pre-ev-namespace-filter branch from 63f7a93 to 398ffa7 Compare October 29, 2022 22:52

k8s-ci-robot assigned ingvagabund Oct 30, 2022

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 30, 2022

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 30, 2022

k8s-ci-robot merged commit 5641fa3 into kubernetes-sigs:master Oct 30, 2022

jenciso mentioned this pull request Dec 2, 2022

Node Filtering Not available in LowNodeUtilization and HighNodeUtilization #1005

Closed

damemi mentioned this pull request Dec 21, 2022

LowNodeUtilization doesn't obey namespaces.exclude and doesn't check nodeFit (or does it incorrectly) #1026

Closed

poornima-krishnasamy mentioned this pull request Mar 23, 2023

Concourse erroring '"ProcessNotFoundError" ministryofjustice/cloud-platform#4433

Closed

6 tasks

add namespace filter to nodeutilization #967

add namespace filter to nodeutilization #967

Conversation

knelasevero commented Sep 28, 2022

knelasevero Sep 28, 2022

Choose a reason for hiding this comment

ingvagabund Oct 24, 2022

Choose a reason for hiding this comment

damemi commented Sep 28, 2022 • edited Loading

knelasevero commented Sep 28, 2022 • edited Loading

damemi commented Sep 28, 2022

ingvagabund commented Sep 28, 2022 • edited Loading

knelasevero commented Oct 7, 2022

ingvagabund commented Oct 10, 2022 • edited Loading

damemi commented Oct 10, 2022

knelasevero commented Oct 10, 2022

ingvagabund commented Oct 10, 2022

damemi commented Oct 10, 2022

ingvagabund commented Oct 10, 2022 • edited Loading

damemi commented Oct 10, 2022

knelasevero commented Oct 11, 2022

ingvagabund commented Oct 11, 2022 • edited Loading

damemi commented Oct 11, 2022

knelasevero commented Oct 11, 2022

ingvagabund commented Oct 11, 2022

ingvagabund commented Oct 11, 2022

damemi commented Oct 11, 2022

knelasevero commented Oct 11, 2022

knelasevero commented Oct 11, 2022

ingvagabund commented Oct 11, 2022

damemi commented Oct 12, 2022

knelasevero commented Oct 21, 2022 • edited Loading

knelasevero commented Oct 21, 2022

ingvagabund Oct 24, 2022

Choose a reason for hiding this comment

ingvagabund Oct 24, 2022

Choose a reason for hiding this comment

ingvagabund Oct 24, 2022

Choose a reason for hiding this comment

ingvagabund Oct 24, 2022

Choose a reason for hiding this comment

ingvagabund Oct 24, 2022

Choose a reason for hiding this comment

ingvagabund commented Oct 24, 2022

knelasevero commented Oct 25, 2022

ingvagabund commented Oct 27, 2022

ingvagabund commented Oct 27, 2022

ingvagabund commented Oct 30, 2022

k8s-ci-robot commented Oct 30, 2022

damemi commented Sep 28, 2022 •

edited

Loading

knelasevero commented Sep 28, 2022 •

edited

Loading

ingvagabund commented Sep 28, 2022 •

edited

Loading

ingvagabund commented Oct 10, 2022 •

edited

Loading

ingvagabund commented Oct 10, 2022 •

edited

Loading

ingvagabund commented Oct 11, 2022 •

edited

Loading

knelasevero commented Oct 21, 2022 •

edited

Loading