Bug 1761506: status: prevent degraded status flapping on rollout #134
Conversation
openshift-ci-robot
added
size/S
|
/cc @wking |
| @@ -70,14 +70,14 @@ func computeDNSDegradedCondition(oldCondition *operatorv1.OperatorCondition, clu | |||
| degradedCondition.Status = operatorv1.ConditionTrue | |||
| degradedCondition.Reason = "NoClusterIP" | |||
| degradedCondition.Message = "No ClusterIP assigned to DNS Service" | |||
| case ds.Status.NumberAvailable == 0: | |||
| case ds.Status.DesiredNumberScheduled == 0: | |||
There was a problem hiding this comment.
This should stay NumberAvailable == 0, right?
There was a problem hiding this comment.
i.e. wanting more than one pod is not enough, you actually have to have more than one pod to get out of NoPodsScheduled
There was a problem hiding this comment.
NoPodsScheduled is actually the misnomer, the check is intentional... if none are even desired to be scheduled, then available seems irrelevant. I'm not even sure how we can get here in the wild...
There was a problem hiding this comment.
Maybe DesiredNumberScheduled=0 if none of the nodes match the scheduling criteria due to taints or something.
| case ds.Status.NumberAvailable != ds.Status.DesiredNumberScheduled: | ||
| degradedCondition.Reason = "NoPodsScheduled" | ||
| degradedCondition.Message = "No CoreDNS pods are desired to be scheduled" | ||
| case ds.Status.DesiredNumberScheduled > 0 && ds.Status.NumberAvailable <= 1: |
There was a problem hiding this comment.
@Miciah suggested ds.Status.DesiredNumberScheduled - ds.Status.NumberAvailable > ds.Spec.UpdateStrategy.RollingUpdate.MaxUnavailable for this case, and I like that better than NumberAvailable <= 1, because it means that Kubernetes is violating our DaemonSet rollout condition, and presumably you could grow a cluster to be large enough that a single CoreDNS could not serve the whole cluster.
ironcladlou
force-pushed
the
dns-status-flapping
branch
from
47143c1
to
0a173c0
Compare
openshift-ci-robot
added
size/M
The current DNS degraded status calculation doesn't incorporate time. If at any time of observation the number of desired daemonset available replicas diverges from the desired scheduled replicas, the CoreDNS daemonset is instantly considered degraded. This results in degraded true/false flapping when the operator aggregates the DNS status during a daemonset rollout. This seems like a poor meausure of degraded, because most of the time this condition arises during a rollout when QoS is not seriously affected. A better way might be to incorprate progressing time into the calculation, and that is something we should probably do, but the solution is a little more complex. This patch should provide a good improvement in the meantime by only considering DNS degraded if the number of unavailable replicas exceeds the max unavailable count on the daemonset's rolling parameters, which seems like an objectively poor state to be in. And, as mentioned, we can further improve the situation later by trying to incorporate progressing time. That later refinement could replace the logic introduced in this commit.
ironcladlou
force-pushed
the
dns-status-flapping
branch
from
0a173c0
to
f8d3aab
Compare
|
/lgtm |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ironcladlou, Miciah, wking 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:
Approvers can indicate their approval by writing |
ironcladlou
changed the title
|
@ironcladlou: All pull requests linked via external trackers have merged. Bugzilla bug 1761506 has been moved to the MODIFIED state. In response to this:
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. |
|
/cherrypick release-4.2 |
|
@ironcladlou: new pull request created: #135 In response to this:
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. |
|
@ironcladlou trying to understand @wking 's comment here openshift/machine-api-operator#417 (comment) Does this mean this change introduces a regression, or did it just uncover another thing that should be cleaned up. |
|
Not sure that I understand the question, can you try another way? |
status: prevent degraded status flapping on rollout
The current DNS degraded status calculation doesn't incorporate time. If at any
time of observation the number of desired daemonset available replicas diverges
from the desired scheduled replicas, the CoreDNS daemonset is instantly
considered degraded. This results in degraded true/false flapping when the
operator aggregates the DNS status during a daemonset rollout.
This seems like a poor meausure of degraded, because most of the time this
condition arises during a rollout when QoS is not seriously affected. A better
way might be to incorprate progressing time into the calculation, and that is
something we should probably do, but the solution is a little more complex.
This patch should provide a good improvement in the meantime by only considering
DNS degraded if the number of unavailable replicas exceeds the max unavailable
count on the daemonset's rolling parameters, which seems like an objectively
poor state to be in.
And, as mentioned, we can further improve the situation later by trying to
incorporate progressing time. That later refinement could replace the logic
introduced in this commit.