RemoveFailedPods: guard against nil descheduler strategy (e.g. in case of default that loads all strategies)

[a7i]

Closes #630

Background: Descheduler by default loads all strategies and as a result, the strategy params can be nil. The strategy needs to guard against that.

[k8s-ci-robot]

Hi @a7i. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

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

[damemi]

The error reported came from line 44: /go/src/sigs.k8s.io/descheduler/pkg/descheduler/strategies/failedpods.go:44 +0x70

So, we also need to add a guard for the priority threshold to be non-nil https://github.com/kubernetes-sigs/descheduler/pull/632/files#diff-413def6475d16ce340793b62bb4c179595cc28669ad68d474763006464d8ee65R44

[damemi]

Also, would be good to add a test for this. It should probably be an e2e so that the whole config gets tested against (rather than just the eviction logic in a unit test)

[a7i]

The error reported came from line 44: /go/src/sigs.k8s.io/descheduler/pkg/descheduler/strategies/failedpods.go:44 +0x70

So, we also need to add a guard for the priority threshold to be non-nil https://github.com/kubernetes-sigs/descheduler/pull/632/files#diff-413def6475d16ce340793b62bb4c179595cc28669ad68d474763006464d8ee65R44

Hey @damemi thresholdPriority blongs to ValidatedStrategyParams so this is the actual fix:

-	*validation.ValidatedStrategyParams
+	validation.ValidatedStrategyParams

I have added a unit test which will catch the error if reverted back to reference.

[damemi]

@a7i Ah, I see. The test looks good too, thanks.
/approve
/lgtm

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: a7i, damemi

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

[damemi]

/ok-to-test

[a7i]

Also, would be good to add a test for this. It should probably be an e2e so that the whole config gets tested against (rather than just the eviction logic in a unit test)

Yes, good idea. I will work on that in parallel but may take me a day or two.

[a7i]

/retest
Will create an issue if flaky test

[ingvagabund]

Yes, good idea. I will work on that in parallel but may take me a day or two.

Let's do it in this PR as well. Unless it's critical to fix this right away.

[damemi]

@ingvagabund I would consider this critical since the strategy crashes in the default use case which causes the entire program to crash. We should merge this asap and release a patch to the v0.22 image.

@a7i will you be able to add the e2e in the next day or so? If not, we can merge this and work on the e2e

[a7i]

@ingvagabund / @damemi Added a comprehensive set of tests for FailedPods strategy. Kept the commits separate for better organization/tracking.

[damemi]

I have no more comments, @ingvagabund do you?

@a7i could you also open a PR to the release-1.22 branch picking these commits there? Will also need to update the version in the Helm chart to v0.22.1

[ingvagabund]

/lgtm

[ingvagabund]

@a7i thank you!!!