use pod informers for listing pods in removepodsviolatingtopologyspreadconstraint and removepodsviolatinginterpodantiaffinity

[a7i]

follow up to #1033 and apply the same in removepodsviolatingtopologyspreadconstraint

[a7i]

With informers, the returned pods are not in order so the assertion is failing. Any suggestions @knelasevero @ingvagabund ?

e.g. one of the tests is expecting this order:

expectedEvictedPods:  []string{"pod-5", "pod-6", "pod-7", "pod-8"},

[damemi]

With informers, the returned pods are not in order so the assertion is failing. Any suggestions @knelasevero @ingvagabund ?

e.g. one of the tests is expecting this order:

expectedEvictedPods:  []string{"pod-5", "pod-6", "pod-7", "pod-8"},

I haven't looked at the test, but is there anything semantically different about evicting those different pods? If it's just an ordering issue then you can sort or update the output

[ingvagabund]

/test all

[ingvagabund]

/retest-required

[ingvagabund]

topologyspreadconstraint_test.go:1220: Expected pods [pod-5 pod-6 pod-7 pod-8] to be evicted but [pod-8 pod-5 pod-6 pod-7] were not evicted. Actual pods evicted: [pod-1 pod-2 pod-3 pod-10 pod-11]

The test is probably broken. expectedEvictedCount is 5 but the list claims it needs only 4 items. We should probably randomize the order in which the pods are appended into objs var that is passed into fake.NewSimpleClientset in cases where the order does not matter. At the same time I recall we needed some tests to actually evict a specific list of pods so we can validate the code picks the right pods for eviction.

[a7i]

topologyspreadconstraint_test.go:1220: Expected pods [pod-5 pod-6 pod-7 pod-8] to be evicted but [pod-8 pod-5 pod-6 pod-7] were not evicted. Actual pods evicted: [pod-1 pod-2 pod-3 pod-10 pod-11]

The test is probably broken. expectedEvictedCount is 5 but the list claims it needs only 4 items. We should probably randomize the order in which the pods are appended into objs var that is passed into fake.NewSimpleClientset in cases where the order does not matter. At the same time I recall we needed some tests to actually evict a specific list of pods so we can validate the code picks the right pods for eviction.

Thanks @ingvagabund . The test only checks that expected is included in the evicted list, so having only 4 was passing before.

Yes, there are ~3 tests that are expecting a particular order. I can perhaps change them to evicted count per node/topology.

[a7i]

fyi - it used to be sorted, but then it changed: https://github.com/kubernetes/kubernetes/pull/75730

"noone should assume the fact that keys are sorted."

[a7i]

/label tide/merge-method-squash

[ingvagabund]

As long as the strategy works deterministically this is fine. /approve

[k8s-ci-robot]

[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](https://github.com/kubernetes-sigs/descheduler/blob/master/OWNERS)~~ [ingvagabund] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment

[JaneLiuL]

/lgtm