feat: Add KubeDeploymentRolloutStuck

[VannTen]

This add an alert when a deployment rollout hits its
spec.progressDeadlineSeconds.
Those could have a number of causes, which can originates from the
cluster or the deployment:

• pods taking too much time to start

• cluster at full capacity and deployment surging during upgrade
  (maxSurge > 0).

  Relevant doc : https://kubernetes.io/docs/concepts/workloads/controllers/deployment/#failed-deployment

[povilasv]

Question how do we make sure that deployment is stuck longer than spec.progressDeadlineSeconds. ?

Is it this condition="Progressing", status="false" label combination?

Would be great to add tests

[VannTen]

Yes, that condition is set to false by the controller when exceeding the deadline ( https://kubernetes.io/docs/concepts/workloads/controllers/deployment/#failed-deployment ). I just noticed it's not the only possible reason though, it can also come from ReplicaSetCreateError. Reason aren't included in metrics labels (AFAICT), so we can distinguish. Would that be a problem ? We care about the symptom rather than the cause right ? I'll look at the test syntax and add something shortly.

[povilasv]

I think in both cases alert KubeDeploymentRolloutStuck firing makes sense

[VannTen]

I've added some tests

[povilasv]

Just noticed that we also have KubeDeploymentReplicasMismatch wouldn't both alerts fire at the same time when deployment is stuck?

[VannTen]

              (
                kube_deployment_spec_replicas{%(prefixedNamespaceSelector)s%(kubeStateMetricsSelector)s}
                  >
                kube_deployment_status_replicas_available{%(prefixedNamespaceSelector)s%(kubeStateMetricsSelector)s}
              ) and (
                changes(kube_deployment_status_replicas_updated{%(prefixedNamespaceSelector)s%(kubeStateMetricsSelector)s}[10m])
                  ==
                0
              )

(this is from the KubeDeploymentReplicasMismatch def)
My initial thought was :
I think the changes part (which I read as "the status.updated field has not changed in the last 10 minutes) means that KubeDeploymentReplicasMismatch won't fire in the same case, since a rollout implies a updated replicas, right ?

However, the default spec.progressDeadlineSeconds 600 seconds, aka 10 minutes, so they would indeed fire at the same time, I think. Expect in the case of non-default spec.progressDeadlineSeconds, I guess.

Third thought:
A stuck rollout can perfectly happen without available replicas being less than spec.replicas : you just need to have maxSurge > 0, and the controller could try to launch a new pod, and won't remove the old one until it succeed.

In conclusion (sorry for that brain dump) I don't think they would fire for the same thing in all cases. but there should some overlap.

[VannTen]

Is there any other questions to address regarding this ? Do you think it's mergeable in it's current state or does it need more work ?

[povilasv]

So I am currently worried about this part:

However, the default spec.progressDeadlineSeconds 600 seconds, aka 10 minutes, so they would indeed fire at the same time, I think. Expect in the case of non-default spec.progressDeadlineSeconds, I guess.

Anyway we can make this alert to not fire together?

[VannTen]

You mean we should have an exclusive rather than inclusive OR between these two right ?

[povilasv]

I mean two alerts shouldnt fire for same issue.

Err on the side of removing noisy alerts – over-monitoring is a harder problem to solve than under-monitoring.

From: https://docs.google.com/document/d/199PqyG3UsyXlwieHaqbGiWVa8eMWi8zzAn0YfcApr8Q/edit

[VannTen]

I think the case where this can happen (both alerts firing) is :

Deployment is rolling out, with maxUnavailable != 0 => controller can update by removing a pod first. => if the deployment can't progress, spec.replicas > status.availableReplicas will become true both alerts will fire.

So what do you think of adding a "spec.replicas <= status.availableReplicas" condition to KubeDeploymentRolloutStuck ? This should narrow the alerts to the case where the rollout tries to surge but can't (cluster full, scheduler problems, etc).

I'm a bit wary of adding that though because it seems like it would be designing the alert not to stand on its own.

wdyt ?

[povilasv]

Maybe let's leave it as it is.We can add the condition later. I think I'm a bit nit picky here :D