TooManyRestart: state filter for CrashLoopBackOff

[a7i]

• Revert PodLifeTime: support CrashLoopBackOff container state #1164
• Address feedback from @damemi

Also as a follow up it might be useful to add a restartThreshold parameter for this specifically to evict pods that have crashlooped X times instead of crashlooping for X seconds

cc: @ingvagabund @damemi
Please let me know if you prefer the revert in a separate PR

[a7i]

/retest

[ingvagabund]

/retest-required

[damemi]

Oh I completely forgot about TooManyRestarts, agree that is a better place for this.

So CrashLoop didn't count toward the restart threshold before with this strategy?

[a7i]

@damemi it does, but I am wanting to evict only pods in CrashLoopBackOff. For my use-case, if a pod was previously restarted 20 times but now it has been running without issues, I don't see any value in evicting it.

[ingvagabund]

Wrt. #1164 (comment):

  containerStatuses:
  - containerID: containerd://274b89ca9b2d7a815cf6550453af29663c114c51917358bb02330613d39fbb73
    ...
    restartCount: 3
    ...
    state:
      waiting:
        message: back-off 40s restarting failed container=rdescheduler-podlifetime pod=descheduler-podlifetime
        reason: CrashLoopBackOff
  phase: Running

A pod can be restarted many times but the reason for the restart can vary in time. There's currently no way of knowing what cause the previous restarts. We can only assume if a pod gets into CrashLoopBackOff state it is a repetitive event.

Both PodLifeTime and TooManyRestarts strategies have a lot in common. Either the reason for evicting is "too many restarts" or "running for too long". Too many restarts can be in a way converted to "running for too long" (e.g. depending on how the pod probes are configured). So both strategies are suitable for the new state/reason for eviction. So both evict pods in CrashLoopBackOff state for "some time" and "getting restarted too many times" are valid.

What about to merge both plugins into a single one? Making sure only one of MaxPodLifeTimeSeconds and PodRestartThreshold can be used at the same time. The valid states can be limited based on which criteria gets set. With the new v1alpha2 configuration one can configure the same strategy multiple times to get benefit of both.

[a7i]

Ok, I can work on that but I feel that these are substantial request for changes with API changes that also impact users.

If we can do this in multiple steps, that'd be ideal.

• merge small change
• come back refactor
• upgrade API later to get rid of redundant plugins.

[ingvagabund]

We can make the change as part of v1alpha3.
/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 [ingvagabund]

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

[damemi]

+1 to @ingvagabund's comments, these 2 plugins are converging in what they accomplish... I also think it's easier for people to understand that 1 plugin handles crashing/restarting/failing pods rather than know that a CrashLooping pod is technically Running and not "failed"

this lgtm, we can refactor later. I'd propose opening an issue to track and justify the reasoning
/lgtm