Skip to content

Commit

Permalink
Merge pull request #854 from Miciah/OCPBUGS-434-absorb-PodsScheduled-…
Browse files Browse the repository at this point in the history 
…condition-into-AllAvailable

OCPBUGS-434: Absorb PodsScheduled condition into MinAvailable
  • Loading branch information
@openshift-merge-robot
openshift-merge-robot committed Jan 13, 2023
2 parents fc69408 + 67b0e92 commit b81ee72
Show file tree
Hide file tree
Showing 4 changed files with 66 additions and 85 deletions.
1 change: 0 additions & 1 deletion pkg/operator/controller/ingress/controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,6 @@ const (
// TODO: consider moving these to openshift/api
const (
IngressControllerAdmittedConditionType = "Admitted"
IngressControllerPodsScheduledConditionType = "PodsScheduled"
IngressControllerDeploymentAvailableConditionType = "DeploymentAvailable"
IngressControllerDeploymentReplicasMinAvailableConditionType = "DeploymentReplicasMinAvailable"
IngressControllerDeploymentReplicasAllAvailableConditionType = "DeploymentReplicasAllAvailable"
Expand Down
1 change: 0 additions & 1 deletion pkg/operator/controller/ingress/metrics_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,6 @@ func TestDeleteIngressControllerConditionsMetric(t *testing.T) {
{Type: "Available", Status: operatorv1.ConditionFalse},
{Type: "Degraded", Status: operatorv1.ConditionTrue},
{Type: "Admitted", Status: operatorv1.ConditionTrue},
{Type: "PodsScheduled", Status: operatorv1.ConditionTrue},
}),
expectedMetricFormat: `
# HELP ingress_controller_conditions Report the conditions for ingress controllers. 0 is False and 1 is True.
Expand Down
53 changes: 21 additions & 32 deletions pkg/operator/controller/ingress/status.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -75,9 +75,8 @@ func (r *reconciler) syncIngressControllerStatus(ic *operatorv1.IngressControlle
updated.Status.EndpointPublishingStrategy.LoadBalancer.AllowedSourceRanges = computeAllowedSourceRanges(service)
}

updated.Status.Conditions = MergeConditions(updated.Status.Conditions, computeDeploymentPodsScheduledCondition(deployment, pods))
updated.Status.Conditions = MergeConditions(updated.Status.Conditions, computeDeploymentAvailableCondition(deployment))
updated.Status.Conditions = MergeConditions(updated.Status.Conditions, computeDeploymentReplicasMinAvailableCondition(deployment))
updated.Status.Conditions = MergeConditions(updated.Status.Conditions, computeDeploymentReplicasMinAvailableCondition(deployment, pods))
updated.Status.Conditions = MergeConditions(updated.Status.Conditions, computeDeploymentReplicasAllAvailableCondition(deployment))
updated.Status.Conditions = MergeConditions(updated.Status.Conditions, computeDeploymentRollingOutCondition(deployment))
updated.Status.Conditions = MergeConditions(updated.Status.Conditions, computeLoadBalancerStatus(ic, service, operandEvents)...)
Expand Down Expand Up @@ -153,9 +152,9 @@ func MergeConditions(conditions []operatorv1.OperatorCondition, updates ...opera
// Returns the updated condition array.
func PruneConditions(conditions []operatorv1.OperatorCondition) []operatorv1.OperatorCondition {
for i, condition := range conditions {
// TODO: Remove this fix-up logic in 4.8
if condition.Type == "DeploymentDegraded" {
// DeploymentDegraded was removed in 4.6.0
// PodsScheduled was removed in 4.13.0.
// TODO: Remove this fix-up logic in 4.14.
if condition.Type == "PodsScheduled" {
conditions = append(conditions[:i], conditions[i+1:]...)
}
}
Expand Down Expand Up @@ -204,18 +203,17 @@ func computeAllowedSourceRanges(service *corev1.Service) []operatorv1.CIDR {
return nil
}

// computeDeploymentPodsScheduledCondition computes the ingress controller's
// current PodsScheduled status condition state by inspecting the PodScheduled
// conditions of the pods associated with the deployment.
func computeDeploymentPodsScheduledCondition(deployment *appsv1.Deployment, pods []corev1.Pod) operatorv1.OperatorCondition {
// checkPodsScheduledForDeployment checks whether the given deployment has a
// valid label selector and whether all of the deployment's pods are reporting
// PodsScheduled=True. This function returns an error value indicating the
// 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")
}
selector, err := metav1.LabelSelectorAsSelector(deployment.Spec.Selector)
if err != nil || selector.Empty() {
return operatorv1.OperatorCondition{
Type: IngressControllerPodsScheduledConditionType,
Status: operatorv1.ConditionUnknown,
Reason: "InvalidLabelSelector",
Message: "Deployment has an invalid label selector.",
}
return errors.New("Deployment has an invalid label selector.")
}
unscheduled := make(map[*corev1.Pod]corev1.PodCondition)
for i, pod := range pods {
Expand Down Expand Up @@ -255,17 +253,9 @@ func computeDeploymentPodsScheduledCondition(deployment *appsv1.Deployment, pods
if haveUnschedulable {
message = message + " Make sure you have sufficient worker nodes."
}
return operatorv1.OperatorCondition{
Type: IngressControllerPodsScheduledConditionType,
Status: operatorv1.ConditionFalse,
Reason: "PodsNotScheduled",
Message: message,
}
}
return operatorv1.OperatorCondition{
Type: IngressControllerPodsScheduledConditionType,
Status: operatorv1.ConditionTrue,
return errors.New(message)
}
return nil
}

// computeIngressAvailableCondition computes the ingress controller's current Available status state
Expand Down Expand Up @@ -411,7 +401,7 @@ func computeDeploymentAvailableCondition(deployment *appsv1.Deployment) operator
// update parameters. The "DeploymentReplicasMinAvailable" condition is true if
// the number of available replicas is equal to or greater than the number of
// desired replicas less the number minimum available.
func computeDeploymentReplicasMinAvailableCondition(deployment *appsv1.Deployment) operatorv1.OperatorCondition {
func computeDeploymentReplicasMinAvailableCondition(deployment *appsv1.Deployment, pods []corev1.Pod) operatorv1.OperatorCondition {
replicas := int32(1)
if deployment.Spec.Replicas != nil {
replicas = *deployment.Spec.Replicas
Expand Down Expand Up @@ -450,11 +440,15 @@ func computeDeploymentReplicasMinAvailableCondition(deployment *appsv1.Deploymen
maxUnavailable = 1
}
if int(deployment.Status.AvailableReplicas) < int(replicas)-maxUnavailable {
message := fmt.Sprintf("%d/%d of replicas are available, max unavailable is %d", deployment.Status.AvailableReplicas, replicas, maxUnavailable)
if err := checkPodsScheduledForDeployment(deployment, pods); err != nil {
message = fmt.Sprintf("%s: %s", message, err.Error())
}
return operatorv1.OperatorCondition{
Type: IngressControllerDeploymentReplicasMinAvailableConditionType,
Status: operatorv1.ConditionFalse,
Reason: "DeploymentMinimumReplicasNotMet",
Message: fmt.Sprintf("%d/%d of replicas are available, max unavailable is %d", deployment.Status.AvailableReplicas, replicas, maxUnavailable),
Message: message,
}
}

Expand Down Expand Up @@ -556,11 +550,6 @@ func computeIngressDegradedCondition(conditions []operatorv1.OperatorCondition,
condition: IngressControllerAdmittedConditionType,
status: operatorv1.ConditionTrue,
},
{
condition: IngressControllerPodsScheduledConditionType,
status: operatorv1.ConditionTrue,
gracePeriod: time.Minute * 10,
},
{
condition: IngressControllerDeploymentAvailableConditionType,
status: operatorv1.ConditionTrue,
Expand Down
96 changes: 45 additions & 51 deletions pkg/operator/controller/ingress/status_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,7 @@ func cond(t string, status operatorv1.ConditionStatus, reason string, lt time.Ti
}
}

func TestComputePodsScheduledCondition(t *testing.T) {
func Test_checkPodsScheduledForDeployment(t *testing.T) {
deployment := &appsv1.Deployment{
Spec: appsv1.DeploymentSpec{
Selector: &metav1.LabelSelector{
Expand Down Expand Up @@ -203,46 +203,56 @@ func TestComputePodsScheduledCondition(t *testing.T) {
},
}
tests := []struct {
name string
deployment *appsv1.Deployment
pods []corev1.Pod
expect operatorv1.ConditionStatus
name string
deployment *appsv1.Deployment
pods []corev1.Pod
expectError bool
}{
{
name: "no pods",
deployment: deployment,
pods: []corev1.Pod{unrelatedPod},
expect: operatorv1.ConditionTrue,
name: "nil deployment",
deployment: nil,
pods: []corev1.Pod{scheduledPod},
expectError: true,
},
{
name: "no pods",
deployment: deployment,
pods: []corev1.Pod{unrelatedPod},
expectError: false,
},
{
name: "all pods scheduled",
deployment: deployment,
pods: []corev1.Pod{scheduledPod},
expect: operatorv1.ConditionTrue,
name: "all pods scheduled",
deployment: deployment,
pods: []corev1.Pod{scheduledPod},
expectError: false,
},
{
name: "some pod unscheduled",
deployment: deployment,
pods: []corev1.Pod{scheduledPod, unscheduledPod},
expect: operatorv1.ConditionFalse,
name: "some pod unscheduled",
deployment: deployment,
pods: []corev1.Pod{scheduledPod, unscheduledPod},
expectError: true,
},
{
name: "some pod unschedulable",
deployment: deployment,
pods: []corev1.Pod{scheduledPod, unschedulablePod},
expect: operatorv1.ConditionFalse,
name: "some pod unschedulable",
deployment: deployment,
pods: []corev1.Pod{scheduledPod, unschedulablePod},
expectError: true,
},
{
name: "deployment with empty label selector",
deployment: invalidDeployment,
expect: operatorv1.ConditionUnknown,
name: "deployment with empty label selector",
deployment: invalidDeployment,
expectError: true,
},
}
for _, test := range tests {
actual := computeDeploymentPodsScheduledCondition(test.deployment, test.pods)
if actual.Status != test.expect {
t.Errorf("%q: expected %v, got %v", test.name, test.expect, actual.Status)
}
t.Run(test.name, func(t *testing.T) {
switch err := checkPodsScheduledForDeployment(test.deployment, test.pods); {
case err == nil && test.expectError:
t.Error("expected error, got nil")
case err != nil && !test.expectError:
t.Errorf("got unexpected error: %v", err)
}
})
}
}

Expand Down Expand Up @@ -279,26 +289,6 @@ func TestComputeIngressDegradedCondition(t *testing.T) {
// Just use the one minute retry duration for this degraded condition
expectAfter: time.Minute,
},
{
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,
},
{
name: "deployment unavailable for <30s",
conditions: []operatorv1.OperatorCondition{
Expand Down Expand Up @@ -426,7 +416,6 @@ func TestComputeIngressDegradedCondition(t *testing.T) {
name: "DNS not ready and deployment unavailable",
conditions: []operatorv1.OperatorCondition{
cond(IngressControllerAdmittedConditionType, operatorv1.ConditionTrue, "", clock.Now().Add(time.Hour*-1)),
cond(IngressControllerPodsScheduledConditionType, operatorv1.ConditionFalse, "", clock.Now().Add(time.Hour*-1)),
cond(IngressControllerDeploymentAvailableConditionType, operatorv1.ConditionFalse, "", clock.Now().Add(time.Hour*-1)),
cond(IngressControllerDeploymentReplicasMinAvailableConditionType, operatorv1.ConditionFalse, "", clock.Now().Add(time.Hour*-1)),
cond(IngressControllerDeploymentReplicasAllAvailableConditionType, operatorv1.ConditionFalse, "", clock.Now().Add(time.Hour*-1)),
Expand All @@ -444,7 +433,6 @@ func TestComputeIngressDegradedCondition(t *testing.T) {
name: "admitted, DNS, LB, and deployment OK",
conditions: []operatorv1.OperatorCondition{
cond(IngressControllerAdmittedConditionType, operatorv1.ConditionTrue, "", clock.Now().Add(time.Hour*-1)),
cond(IngressControllerPodsScheduledConditionType, operatorv1.ConditionTrue, "", clock.Now().Add(time.Hour*-1)),
cond(IngressControllerDeploymentAvailableConditionType, operatorv1.ConditionTrue, "", clock.Now().Add(time.Hour*-1)),
cond(IngressControllerDeploymentReplicasMinAvailableConditionType, operatorv1.ConditionTrue, "", clock.Now().Add(time.Hour*-1)),
cond(IngressControllerDeploymentReplicasAllAvailableConditionType, operatorv1.ConditionTrue, "", clock.Now().Add(time.Hour*-1)),
Expand Down Expand Up @@ -1060,6 +1048,12 @@ func TestComputeDeploymentReplicasMinAvailableCondition(t *testing.T) {
deploy := &appsv1.Deployment{
Spec: appsv1.DeploymentSpec{
Replicas: test.replicas,
Selector: &metav1.LabelSelector{
MatchLabels: map[string]string{
"ingresscontroller.operator.openshift.io/deployment-ingresscontroller": "default",
"ingresscontroller.operator.openshift.io/hash": "75678b564c",
},
},
Strategy: appsv1.DeploymentStrategy{
Type: appsv1.RollingUpdateDeploymentStrategyType,
RollingUpdate: test.rollingUpdate,
Expand All @@ -1070,7 +1064,7 @@ func TestComputeDeploymentReplicasMinAvailableCondition(t *testing.T) {
},
}

actual := computeDeploymentReplicasMinAvailableCondition(deploy)
actual := computeDeploymentReplicasMinAvailableCondition(deploy, []corev1.Pod{})
if actual.Status != test.expectDeploymentReplicasMinAvailableStatus {
t.Errorf("%q: expected %v, got %v", test.name, test.expectDeploymentReplicasMinAvailableStatus, actual.Status)
}
Expand Down

0 comments on commit b81ee72

Please sign in to comment.