Mark operator as degraded if there are any pods in CrashLoopBackOff State

[tssurya]

This patch checks for the statuses of pods of those deployments and daemonsets
which are in a "hung" state. If any of the pods are in CrashLoopBackOff state
the operator will be marked as degraded.

Signed-off-by: Surya Seetharaman suryaseetharaman.9@gmail.com

[pecameron]

@tssurya This looks like network pods only? Also, I have encountered some non-network pods in CrashLoopBackoff during bringup. They are fine when cluster is fully up. Not sure why we try to start pods before cluster is ready.

[tssurya]

@tssurya This looks like network pods only?

This patch actually includes all the pods in the cluster - as in any pod in any namespace which is stuck in the crash looping phase will be taken into consideration. Also this is a really simple patch to update the status of the operator accordingly.

Also, I have encountered some non-network pods in CrashLoopBackoff during bringup. They are fine when cluster is fully up. Not sure why we try to start pods before cluster is ready.

I am not sure about this, haven't seen this before (but I'm also relatively new to CNO and operators in general)

[juanluisvaladas]

If pods are in crashloop backoff the daemonset should report as having less ready replicas than desired (if it's crashloopbackoff it's not ready). It looks me like duplicating the effort of the kube controller manager, is this resolving an observed problem or an improvement?

Also, some components aren't critical and therefore shouldn't be enough to mark the CO as degraded, those daemonsets created with the annotation networkoperator.openshift.io/non-critical: "" should be ignored here.

[squeed]

If pods are in crashloop backoff the daemonset should report as having less ready replicas than desired (if it's crashloopbackoff it's not ready). It looks me like duplicating the effort of the kube controller manager, is this resolving an observed problem or an improvement?

@juanluisvaladas yeah, this is somewhat of a cosmetic improvement: if a pod is in CrashLoopBackoff, we will eventually report degraded because NumberUnavailable > 0. However, we can do better: if we know that a pod is CrashLoopBackoff, we should go Degraded immediately.

[tssurya]

If pods are in crashloop backoff the daemonset should report as having less ready replicas than desired (if it's crashloopbackoff it's not ready).

yea, this is true, the daemonset reports having less ready replicas :

cluster-network-operator/pkg/controller/statusmanager/pod_status.go

Line 92 in a10490e

} else if ds.Status.NumberUnavailable > 0 {

and that's when I go in to check specifically for the crashloopbackoff case.

It looks me like duplicating the effort of the kube controller manager, is this resolving an observed problem or an improvement?

It's trying to do both I guess, firstly fix an observed problem - say we have a pod which is alternating between crashloopbackoff and running or completed states, the existing logic which checks for the daemonset's status won't catch this among the hung ones and secondly its also an improvement where we are saying its better to let the user/op know that the network is degraded because something is crashing.

Also, some components aren't critical and therefore shouldn't be enough to mark the CO as degraded, those daemonsets created with the annotation networkoperator.openshift.io/non-critical: "" should be ignored here.

I didn't know about this, I'll include the !isNonCritical check before doing this. Thanks for pointing this out.

[juanluisvaladas]

we are saying its better to let the user/op know that the network is degraded because something is crashing.

That's a pretty solid reason. Sounds like a good change.

[juanluisvaladas]

Just a couple nitpicks

[juanluisvaladas]

/lgtm

[tssurya]

/retest

[tssurya]

/retest

[squeed]

A few minor cleanups:

• rename lb to selector, name to kind.
• Write a quick docblock, something like

CheckCrashLoopBackOffPods checks for pods in the label selector with with 
any containers in state CrashLoopBackoff. It returns a human-readable string 
for any pod in such a state.

dName should be the name of a DaemonSet or Deployment.

[tssurya]

ack, I'll fix these up

[squeed]

One minor cleanup, then this looks good.
/approve

I'll lgtm when it's good to go. We might have to skip some tests.

[squeed]

/lgtm
/retest

[openshift-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: juanluisvaladas, squeed, tssurya

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 [squeed]

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

[openshift-bot]

/retest

Please review the full test history for this PR and help us cut down flakes.

[openshift-bot]

/retest

Please review the full test history for this PR and help us cut down flakes.

[openshift-bot]

/retest

Please review the full test history for this PR and help us cut down flakes.

[openshift-ci-robot]

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

Test name	Commit	Details	Rerun command
ci/prow/e2e-vsphere	840e67e	link	/test e2e-vsphere

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.

[openshift-bot]

/retest

Please review the full test history for this PR and help us cut down flakes.