Filter pods by namespaces #338

ingvagabund · 2020-07-11T18:29:57Z

Fixes: #251

E2e for the exclude namespaces part are deleting all pods from all namespaces but default which are passing IsEvictable. It can be tricky when the test is run over a cluster with pods that are not supposed to be evicted during the test. Though, I don't see any other way how to do it now. On the other hand, running the descheduler is always destructive so it needs to be run under certain assumptions/conditions.

With introduction of included namespaces one can configure the same strategy with with different conditions for eviction. E.g. evict pods in namespace N1 that are older than 1h but in namespace N2 only pods older than 2h.

The following strategies are subject to namespace filtering now:

RemovePodsViolatingInterPodAntiAffinity
RemovePodsViolatingNodeAffinity
RemovePodsViolatingNodeTaints
RemovePodsHavingTooManyRestarts
PodLifeTime

Note: the policy config has backward incompatible changes for which we need to bump the version to v1alpha2

TODO:

add namespace filters
add validation of params
e2e for namespace filtering
docs

Built on top of #343

ingvagabund · 2020-07-13T14:38:20Z

/hold

ingvagabund · 2020-07-13T14:40:05Z

@seanmalloy I diverged a bit from the proposal renaming includeNamespaces to just include. The same for excludeNamespaces as the keyword Namespaces is already mentioned in the tree:

namespaces:
  includeNamespaces:
  - ns1
  - ns2
  excludeNamespaces:
  - ns3
  - ns4

ingvagabund · 2020-07-13T14:42:01Z

https://travis-ci.org/github/kubernetes-sigs/descheduler/builds/707654262 going green as well

ingvagabund · 2020-07-13T14:43:01Z

@seanmalloy @damemi @lixiang233 PTAL

ingvagabund · 2020-07-13T14:58:42Z

We need to have make test-e2e run through Prow before this gets merged: kubernetes/test-infra#18292

ingvagabund · 2020-07-13T16:58:01Z

/test pull-descheduler-test-e2e-k8s-master

k8s-ci-robot · 2020-07-13T16:58:02Z

@ingvagabund: The specified target(s) for /test were not found.
The following commands are available to trigger jobs:

/test pull-descheduler-verify-master
/test pull-descheduler-verify-build-master
/test pull-descheduler-unit-test-master-master

Use /test all to run all jobs.

In response to this:

/test pull-descheduler-test-e2e-k8s-master

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/test-infra repository.

damemi · 2020-07-13T17:02:06Z

Maybe a close/reopen will pull in the new test
/close

k8s-ci-robot · 2020-07-13T17:02:13Z

@damemi: Closed this PR.

In response to this:

Maybe a close/reopen will pull in the new test
/close

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/test-infra repository.

damemi · 2020-07-13T17:02:21Z

/reopen

k8s-ci-robot · 2020-07-13T17:02:28Z

@damemi: Reopened this PR.

In response to this:

/reopen

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/test-infra repository.

damemi · 2020-07-13T17:18:16Z

Opened #340 to try and fix the e2e

ingvagabund · 2020-07-13T20:25:58Z

/retest

lixiang233 · 2020-07-14T02:59:00Z

pkg/descheduler/pod/pods.go

 )

+type Options struct {
+	filter             func(pod *v1.Pod) bool


Maybe better to make filter a list of filter funcs, so that we can easily add more filters to descheduler without changing other funcs in pod util.

You can always create function which will call all filter functions:

func(pod *v1.Pod) bool { return filter1(pod) && ... filtern(pod) }

and pass into WithFilter.

pkg/api/types.go

damemi

My only real comment is that I think it should be clear that you can only set either Include or Exclude, rather than implicitly preferring one over the other

pkg/api/types.go

pkg/api/v1alpha1/types.go

damemi · 2020-07-14T15:20:07Z

pkg/descheduler/pod/pods_test.go

@@ -67,7 +67,7 @@ func TestListPodsOnANode(t *testing.T) {
 			}
 			return true, nil, fmt.Errorf("Failed to list: %v", list)
 		})
-		pods, _ := ListPodsOnANode(context.TODO(), fakeClient, testCase.node, nil)
+		pods, _ := ListPodsOnANode(context.TODO(), fakeClient, testCase.node)


+1 nice, we can get rid of these nils

damemi · 2020-07-14T15:21:18Z

pkg/descheduler/strategies/node_affinity.go

+							!nodeutil.PodFitsCurrentNode(pod, node) &&
+							nodeutil.PodFitsAnyNode(pod, nodes)
+					}),
+					podutil.WithNamespaces(strategy.Params.Namespaces.Include),


Ah, I see now how the strategies can easily control whether they implement the include/exclude. Cool!

test/e2e/e2e_test.go

damemi · 2020-07-21T17:27:38Z

/retest

ingvagabund · 2020-07-22T13:38:17Z

/retest

damemi · 2020-07-22T14:55:13Z

test/e2e/e2e_test.go

+
+		includePodNames := getPodNames(podList.Items)
+		// validate all pod were deleted
+		if len(intersectStrings(initialPodNames, includePodNames)) > 0 {


you could consider using sets.String

As long as the slices are not too long, the naive implementation is fine.

damemi · 2020-07-22T14:57:07Z

test/e2e/e2e_test.go

+				}
+			}
+			t.Logf("All %v pods are terminating", intersectStrings(initialPodNames, includePodNames))
+			return true, nil


don't think you need this, you have a return at the end of the function

damemi · 2020-07-22T15:06:27Z

test/e2e/e2e_test.go

+			t.Logf("Waiting until %v pods get deleted", intersectStrings(initialPodNames, includePodNames))
+			// check if there's at least one pod not in Terminating state
+			for _, pod := range podList.Items {
+				if pod.DeletionTimestamp == nil {


This is including the newly created pods, so I think you just want to iterate over initialPods? The errors in CI are failing against a pod that's not in your intersect list: https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/kubernetes-sigs_descheduler/338/pull-descheduler-test-e2e-k8s-master-1-16/1285935763120197633#1:build-log.txt%3A272

e2e_test.go:301: Waiting until [test-rc-podlifetime-9wqm7 test-rc-podlifetime-fvqt2 test-rc-podlifetime-hprj9 test-rc-podlifetime-q59m7 test-rc-podlifetime-vrdmk] pods get deleted e2e_test.go:305: Pod test-rc-podlifetime-8lh7l not in terminating state

I split it into two tests, each one running the strategy over separate namespace.

ah good idea

k8s-ci-robot · 2020-07-27T08:38:42Z

@ingvagabund: The following test failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
pull-descheduler-test-e2e-k8s-master	`49636c4`	link	`/test pull-descheduler-test-e2e-k8s-master`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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/test-infra repository. I understand the commands that are listed here.

…ategy

damemi

/approve
thanks @ingvagabund this looks good to me
@seanmalloy wdyt?

k8s-ci-robot · 2020-07-27T14:16:34Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: damemi, 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~~ [damemi]

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

seanmalloy · 2020-07-28T14:03:35Z

/kind feature

seanmalloy · 2020-07-28T15:56:58Z

/lgtm

…ds-by-namespaces Filter pods by namespaces

k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. 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 Jul 11, 2020

k8s-ci-robot requested review from aveshagarwal and k82cn July 11, 2020 18:30

ingvagabund changed the title ~~WIP: Filter out pods by namespaces~~ WIP: Filter pods by namespaces Jul 11, 2020

ingvagabund changed the title ~~WIP: Filter pods by namespaces~~ Filter pods by namespaces Jul 13, 2020

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 13, 2020

ingvagabund force-pushed the filter-out-pods-by-namespaces branch from 1875a45 to c5c8e76 Compare July 13, 2020 14:30

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 13, 2020

k8s-ci-robot closed this Jul 13, 2020

k8s-ci-robot reopened this Jul 13, 2020

lixiang233 reviewed Jul 14, 2020

View reviewed changes

ingvagabund force-pushed the filter-out-pods-by-namespaces branch from c5c8e76 to 238a2ec Compare July 14, 2020 14:27

damemi requested changes Jul 14, 2020

View reviewed changes

ingvagabund force-pushed the filter-out-pods-by-namespaces branch from 238a2ec to 6628814 Compare July 15, 2020 12:17

k8s-ci-robot removed the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jul 15, 2020

damemi reviewed Jul 21, 2020

View reviewed changes

test/e2e/e2e_test.go Outdated Show resolved Hide resolved

ingvagabund force-pushed the filter-out-pods-by-namespaces branch 6 times, most recently from 57aaf97 to 2306f0c Compare July 22, 2020 13:22

damemi reviewed Jul 22, 2020

View reviewed changes

ingvagabund force-pushed the filter-out-pods-by-namespaces branch from 6ec235f to 69cf370 Compare July 27, 2020 08:32

ingvagabund force-pushed the filter-out-pods-by-namespaces branch from 69cf370 to 9cb7a95 Compare July 27, 2020 08:45

ingvagabund added 2 commits July 27, 2020 10:54

e2e: add test for included/excluded namespace through PodLifeTime str…

c40a9c3

…ategy

README: describe usage of the namespace filtering

42db316

ingvagabund force-pushed the filter-out-pods-by-namespaces branch from 2fd939a to 42db316 Compare July 27, 2020 09:00

damemi approved these changes Jul 27, 2020

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 27, 2020

k8s-ci-robot added the kind/feature Categorizes issue or PR as related to a new feature. label Jul 28, 2020

k8s-ci-robot assigned seanmalloy Jul 28, 2020

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 28, 2020

k8s-ci-robot merged commit eee41ee into kubernetes-sigs:master Jul 28, 2020

ingvagabund deleted the filter-out-pods-by-namespaces branch July 28, 2020 16:03

lixiang233 mentioned this pull request Jul 29, 2020

Allow Ignoring Pods For Eviction #329

Closed

briend pushed a commit to briend/descheduler that referenced this pull request Feb 11, 2022

Merge pull request kubernetes-sigs#338 from ingvagabund/filter-out-po…

7c95a96

…ds-by-namespaces Filter pods by namespaces

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Filter pods by namespaces #338

Filter pods by namespaces #338

ingvagabund commented Jul 11, 2020 •

edited

ingvagabund commented Jul 13, 2020

ingvagabund commented Jul 13, 2020

ingvagabund commented Jul 13, 2020

ingvagabund commented Jul 13, 2020

ingvagabund commented Jul 13, 2020

ingvagabund commented Jul 13, 2020

k8s-ci-robot commented Jul 13, 2020

damemi commented Jul 13, 2020

k8s-ci-robot commented Jul 13, 2020

damemi commented Jul 13, 2020

k8s-ci-robot commented Jul 13, 2020

damemi commented Jul 13, 2020

ingvagabund commented Jul 13, 2020

lixiang233 Jul 14, 2020

ingvagabund Jul 14, 2020

damemi left a comment

damemi Jul 14, 2020

damemi Jul 14, 2020

damemi commented Jul 21, 2020

ingvagabund commented Jul 22, 2020

damemi Jul 22, 2020

ingvagabund Jul 27, 2020

damemi Jul 22, 2020

ingvagabund Jul 27, 2020

damemi Jul 22, 2020

ingvagabund Jul 27, 2020

damemi Jul 27, 2020

k8s-ci-robot commented Jul 27, 2020 •

edited

damemi left a comment

k8s-ci-robot commented Jul 27, 2020

seanmalloy commented Jul 28, 2020

seanmalloy commented Jul 28, 2020

Filter pods by namespaces #338

Filter pods by namespaces #338

Conversation

ingvagabund commented Jul 11, 2020 • edited

ingvagabund commented Jul 13, 2020

ingvagabund commented Jul 13, 2020

ingvagabund commented Jul 13, 2020

ingvagabund commented Jul 13, 2020

ingvagabund commented Jul 13, 2020

ingvagabund commented Jul 13, 2020

k8s-ci-robot commented Jul 13, 2020

damemi commented Jul 13, 2020

k8s-ci-robot commented Jul 13, 2020

damemi commented Jul 13, 2020

k8s-ci-robot commented Jul 13, 2020

damemi commented Jul 13, 2020

ingvagabund commented Jul 13, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

damemi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

damemi commented Jul 21, 2020

ingvagabund commented Jul 22, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k8s-ci-robot commented Jul 27, 2020 • edited

damemi left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Jul 27, 2020

seanmalloy commented Jul 28, 2020

seanmalloy commented Jul 28, 2020

ingvagabund commented Jul 11, 2020 •

edited

k8s-ci-robot commented Jul 27, 2020 •

edited