OCPBUGS-434: Absorb PodsScheduled condition into MinAvailable #854
Conversation
openshift-ci-robot
added
the
jira/invalid-bug
|
@Miciah: This pull request references Jira Issue OCPBUGS-434, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. 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. |
|
/jira refresh |
openshift-ci-robot
added
jira/valid-bug
|
@Miciah: This pull request references Jira Issue OCPBUGS-434, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
Requesting review from QA contact: 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. |
Miciah
force-pushed
the
OCPBUGS-434-absorb-PodsScheduled-condition-into-AllAvailable
branch
from
6947f72
to
509baf5
Compare
|
@Miciah: This pull request references Jira Issue OCPBUGS-434, which is valid. 3 validation(s) were run on this bug
Requesting review from QA contact: 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. |
Miciah
changed the title
|
Latest push changes from reporting the information about unscheduled/unschedulable pods from |
|
e2e-aws-ovn-upgrade failed because deprovisioning failed. e2e-gcp-ovn-serial because etcd had an unhealthy member. e2e-azure-ovn failed because terraform failed to configure the vnet. |
|
/assign @gcs278 |
| pods []corev1.Pod | ||
| expect operatorv1.ConditionStatus | ||
| name string | ||
| deployment *appsv1.Deployment |
There was a problem hiding this comment.
I always test to ensure functions can handle nil pointers. Do you agree with testing this edge case?
There was a problem hiding this comment.
I'm ambivalent in this case and in the general case. Adding nil checks everywhere can make code safer (less likely to panic), but it can also obscure bugs (maybe it's a bug in the caller if the argument is nil). In this case, the caller of checkPodsScheduledForDeployment itself dereferences deployment, so it cannot be nil. For short helper functions like this that are used in only one or two places, I would be fine with saying something like, "The caller must ensure that the deployment argument is not nil." Would a comment in the godoc for checkPodsScheduledForDeployment suffice, or do you still prefer an explicit nil check in checkPodsScheduledForDeployment?
There was a problem hiding this comment.
Actually, checkPodsScheduledForDeployment returns an error value, so I'll go ahead and add a check that returns an error if deployment is nil.
There was a problem hiding this comment.
So I was a bit of a NIL Pointer Check absolutist until you mentioned it can also obscure bugs. So I agree, it's not that cut-and-dry whether every function needs a NIL pointer check. The GO community seems torn on it too based on my web searches.
So I think I don't mind either way.
|
@Miciah I have a small concern with this. What if SD or customers have set up their own monitoring for specific conditions, and we are changing things here. Will there be a good release note to explain the changes to the condition set? |
We don't document the status condition, and we don't declare it in openshift/api, so we have made no promises to anyone that they can rely on it. We could make a release note if you think that's appropriate, though I would want to state clearly in any release note that no one should be relying on undocumented status conditions anyway. Your question does spark a thought—we should prune the removed status condition on upgrade to make sure we don't leave a stale status condition around. |
Remove the PodsScheduled status condition on IngressController objects, and report the information that PodsScheduled reported instead in the DeploymentReplicasMinAvailable status condition's message when its status is "False". Before this commit, the PodsScheduled status condition could erroneously signal that there was a problem when some misscheduled pods existed even if there were enough correctly scheduled and available pods. The PodsScheduled status condition was added in order to provide diagnostic information when there was a problem. That is, PodsScheduled was intended to indicate *why* there was a problem, not *whether* there was a problem. The DeploymentReplicasMinAvailable status condition definitively indicates *whether* there is a problem, and its status message should indicate *why* there is a problem. Having a separate status condition for the "why" is superfluous and unnecessarily complicated in this instance. Follow-up to commit c5e0f4a. This bug fixes OCPBUGS-434. https://issues.redhat.com/browse/OCPBUGS-434 * pkg/operator/controller/ingress/controller.go (IngressControllerPodsScheduledConditionType): Delete const. * pkg/operator/controller/ingress/metrics_test.go (TestDeleteIngressControllerConditionsMetric): Delete a reference to the "PodsScheduled" status condition in a test case. * pkg/operator/controller/ingress/status.go (syncIngressControllerStatus): Delete the call to computeDeploymentPodsScheduledCondition. Pass the list of pods to computeDeploymentReplicasMinAvailableCondition. (PruneConditions): Delete pruning of the "DeploymentDegraded" status condition (which was removed in 4.6.0) and instead prune the "PodsScheduled" status condition. (computeDeploymentPodsScheduledCondition): Rename... (checkPodsScheduledForDeployment): ...to this. Change the return value from an operator status condition to an error value. (computeDeploymentReplicasMinAvailableCondition): Call checkPodsScheduledForDeployment and include the error string in the status condition's message if the function returns an error. (computeIngressDegradedCondition): Remove IngressControllerPodsScheduledConditionType from the list of conditions to check. * pkg/operator/controller/ingress/status_test.go (TestComputePodsScheduledCondition): Rename... (Test_checkPodsScheduledForDeployment): ...to this. Update for the function's new name and result value. (TestComputeIngressDegradedCondition): Delete test cases for PodsScheduled. (TestComputeDeploymentReplicasMinAvailableCondition): Update to set the deployment's label selector and pass in a list of pods (which is empty).
Miciah
force-pushed
the
OCPBUGS-434-absorb-PodsScheduled-condition-into-AllAvailable
branch
from
f4da6f3
to
436fd31
Compare
|
@Miciah: This pull request references Jira Issue OCPBUGS-434, which is valid. 3 validation(s) were run on this bug
Requesting review from QA contact: The bug has been updated to refer to the pull request using the external bug tracker. 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. |
| // result of that check. | ||
| func checkPodsScheduledForDeployment(deployment *appsv1.Deployment, pods []corev1.Pod) error { | ||
| if deployment == nil { | ||
| return errors.New("Deployment was nil. This is a bug!") |
There was a problem hiding this comment.
| return errors.New("Deployment was nil. This is a bug!") | |
| return errors.New("System or configuration error detected - deployment was nil.") |
There was a problem hiding this comment.
Is that better than just saying, "This is a bug"? We don't want to mislead the cluster admin into thinking that maybe something is wrong with configuration that the cluster admin can fix when really the problem is that the operator has a bug that needs to be fixed.
There was a problem hiding this comment.
Then maybe just "System error detected." and put a link to the bug reporting tool!
There was a problem hiding this comment.
I updated the error message as you suggested. The Jira link is a bit unwieldy though.
| { | ||
| name: "pods not scheduled for <10m", | ||
| conditions: []operatorv1.OperatorCondition{ | ||
| cond(IngressControllerPodsScheduledConditionType, operatorv1.ConditionFalse, "", clock.Now().Add(time.Minute*-9)), | ||
| }, | ||
| expectIngressDegradedStatus: operatorv1.ConditionFalse, | ||
| expectRequeue: true, | ||
| // Grace period is 10 minutes, subtract the 9 minute spoofed last transition time | ||
| expectAfter: time.Minute, | ||
| }, | ||
| { | ||
| name: "pods not scheduled for >10m", | ||
| conditions: []operatorv1.OperatorCondition{ | ||
| cond(IngressControllerPodsScheduledConditionType, operatorv1.ConditionFalse, "", clock.Now().Add(time.Minute*-31)), | ||
| }, | ||
| expectIngressDegradedStatus: operatorv1.ConditionTrue, | ||
| expectRequeue: true, | ||
| // Just use the one minute retry duration for this degraded condition | ||
| expectAfter: time.Minute, | ||
| }, |
There was a problem hiding this comment.
Do we no longer have a grace period for which pods can remain unscheduled and not yet considered unavailable?
There was a problem hiding this comment.
There is no longer a grace period for the PodsScheduled status condition, because the PodsScheduled status condition no longer even exists after this PR.
The point of this PR is to remove the PodsScheduled status condition because it was spuriously indicating a condition (some pods weren't scheduled) as a problem when really it wasn't necessarily a problem at all (the not-scheduled pods might actually be pods that are waiting to be garbage-collected). However, the operator still reports the DeploymentReplicasMinAvailable status condition (which has a 60-second grace period, before and after this PR), which conveys the information that really does matter (whether there are enough pods that are scheduled, running, and ready).
There was a problem hiding this comment.
So will this automatically continue to reconcile while there are not enough pods scheduled?
There was a problem hiding this comment.
Yeah, if there are not enough available pods, then DeploymentReplicasMinAvailable will have status False, and the grace logic will queue reconciliation for the end of the condition's grace period. This is another way in which PodsScheduled is superfluous: DeploymentReplicasMinAvailable has a shorter grace period than PodsScheduled had, so if DeploymentReplicasMinAvailable is False, its grace period already would have triggered reconcilation (and still does trigger it) before PodsScheduled's grace period would have triggered reconciliation.
|
I forgot about status pruning, good catch. /lgtm |
checkPodsScheduledForDeployment takes a pointer to a deployment, which should never be nil, so return an error indicating that there is a bug if the deployment is somehow nil. * pkg/operator/controller/ingress/status.go (checkPodsScheduledForDeployment): Check whether deployment is nil and return an error if it is. * pkg/operator/controller/ingress/status_test.go (Test_checkPodsScheduledForDeployment): Add a "nil deployment" test case.
Miciah
force-pushed
the
OCPBUGS-434-absorb-PodsScheduled-condition-into-AllAvailable
branch
from
436fd31
to
67b0e92
Compare
| // result of that check. | ||
| func checkPodsScheduledForDeployment(deployment *appsv1.Deployment, pods []corev1.Pod) error { | ||
| if deployment == nil { | ||
| return errors.New("System error detected: deployment was nil. Please report this issue to Red Hat: https://issues.redhat.com/secure/CreateIssueDetails!init.jspa?pid=12332330&issuetype=1&components=12367900&priority=10300&customfield_12316142=26752") |
There was a problem hiding this comment.
I have some concern this link may break in the future. Would it be better to just do https://issues.redhat.com/?
There was a problem hiding this comment.
I think that that would lead to more misrouted bug reports.
I could say "open a support case", but I don't even know the link for that, and neither would OKD users.
There was a problem hiding this comment.
🤷♂️ this is fine, hopefully we never hit this logic
|
/lgtm |
|
/retest-required |
|
This test output for e2e-gcp-ovn-serial is unusual in that it mentions two bugs:
|
|
/retest-required |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: candita 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 |
openshift-ci
bot
added
the
approved
|
@Miciah: The following tests failed, say
Full PR test history. Your PR dashboard. 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. |
|
/jira refresh The requirements for Jira bugs have changed (Jira issues linked to PRs on main branch need to target OCP 4.12), recalculating validity. |
|
/bugzilla refresh The requirements for Bugzilla bugs have changed (BZs linked to PRs on main branch need to target OCP 4.12), recalculating validity. |
|
@openshift-bot: This pull request references Jira Issue OCPBUGS-434, which is valid. 3 validation(s) were run on this bug
Requesting review from QA contact: 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. |
|
@openshift-bot: No Bugzilla bug is referenced in the title of this pull request. Retaining the bugzilla/valid-bug label as it was manually added. 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. |
|
@Miciah There's a new frequent (but not permanent) failure in |
|
@Miciah: All pull requests linked via external trackers have merged: Jira Issue OCPBUGS-434 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. |
Absorb PodsScheduled condition into MinAvailable
Remove the
PodsScheduledstatus condition on IngressController objects, and report the information thatPodsScheduledreported instead in theDeploymentReplicasMinAvailablestatus condition's message when its status is "False".Before this change, the
PodsScheduledstatus condition could erroneously signal that there was a problem when some misscheduled pods existed even if there were enough correctly scheduled and available pods.The
PodsScheduledstatus condition was added in order to provide diagnostic information when there was a problem. That is,PodsScheduledwas intended to indicate why there was a problem, not whether there was a problem. TheDeploymentReplicasMinAvailablestatus condition definitively indicates whether there is a problem, and its status message should indicate why there is a problem. Having a separate status condition for the "why" is superfluous and unnecessarily complicated in this instance.Follow-up to #465.
pkg/operator/controller/ingress/controller.go(IngressControllerPodsScheduledConditionType): Delete const.pkg/operator/controller/ingress/metrics_test.go(TestDeleteIngressControllerConditionsMetric): Delete a reference to the "PodsScheduled" status condition in a test case.pkg/operator/controller/ingress/status.go(syncIngressControllerStatus): Delete the call tocomputeDeploymentPodsScheduledCondition. Pass the list of pods tocomputeDeploymentReplicasMinAvailableCondition.(
PruneConditions): Delete pruning of the "DeploymentDegraded" status condition (which was removed in 4.6.0) and instead prune the "PodsScheduled" status condition.(
computeDeploymentPodsScheduledCondition): Rename...(
checkPodsScheduledForDeployment): ...to this. Change the return value from an operator status condition to an error value.(
computeDeploymentReplicasMinAvailableCondition): CallcheckPodsScheduledForDeploymentand include the error string in the status condition's message if the function returns an error.(
computeIngressDegradedCondition): RemoveIngressControllerPodsScheduledConditionTypefrom the list of conditions to check.pkg/operator/controller/ingress/status_test.go(TestComputePodsScheduledCondition): Rename...(
Test_checkPodsScheduledForDeployment): ...to this. Update for the function's new name and result value.(
TestComputeIngressDegradedCondition): Delete test cases forPodsScheduled.(
TestComputeDeploymentReplicasMinAvailableCondition): Update to set the deployment's label selector and pass in a list of pods (which is empty).checkPodsScheduledForDeployment: Add nil checkcheckPodsScheduledForDeploymenttakes a pointer to a deployment, which should never be nil, so return an error indicating that there is a bug if the deployment is somehow nil.pkg/operator/controller/ingress/status.go(checkPodsScheduledForDeployment): Check whether deployment is nil and return an error if it is.pkg/operator/controller/ingress/status_test.go(Test_checkPodsScheduledForDeployment): Add a "nil deployment" test case.