diff --git a/pkg/operator/controller/ingress/status.go b/pkg/operator/controller/ingress/status.go index d0df3d874e..9cdcfe0e39 100644 --- a/pkg/operator/controller/ingress/status.go +++ b/pkg/operator/controller/ingress/status.go @@ -31,6 +31,17 @@ import ( // clock is to enable unit testing var clock utilclock.Clock = utilclock.RealClock{} +// expectedCondition contains a condition that is expected to be checked when +// determining Available or Degraded status of the ingress controller +type expectedCondition struct { + condition string + status operatorv1.ConditionStatus + // ifConditionsTrue is a list of prerequisite conditions that should be true + // or else the condition is not checked. + ifConditionsTrue []string + gracePeriod time.Duration +} + // syncIngressControllerStatus computes the current status of ic and // updates status upon any changes since last sync. func (r *reconciler) syncIngressControllerStatus(ic *operatorv1.IngressController, deployment *appsv1.Deployment, pods []corev1.Pod, service *corev1.Service, operandEvents []corev1.Event, wildcardRecord *iov1.DNSRecord, dnsConfig *configv1.DNS) error { @@ -46,12 +57,12 @@ func (r *reconciler) syncIngressControllerStatus(ic *operatorv1.IngressControlle updated.Status.Selector = selector.String() updated.Status.TLSProfile = computeIngressTLSProfile(ic.Status.TLSProfile, deployment) updated.Status.Conditions = MergeConditions(updated.Status.Conditions, computeDeploymentPodsScheduledCondition(deployment, pods)) - updated.Status.Conditions = MergeConditions(updated.Status.Conditions, computeIngressAvailableCondition(deployment)) 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, computeDeploymentReplicasAllAvailableCondition(deployment)) updated.Status.Conditions = MergeConditions(updated.Status.Conditions, computeLoadBalancerStatus(ic, service, operandEvents)...) updated.Status.Conditions = MergeConditions(updated.Status.Conditions, computeDNSStatus(ic, wildcardRecord, dnsConfig)...) + updated.Status.Conditions = MergeConditions(updated.Status.Conditions, computeIngressAvailableCondition(updated.Status.Conditions)) degradedCondition, err := computeIngressDegradedCondition(updated.Status.Conditions, updated.Name) errs = append(errs, err) updated.Status.Conditions = MergeConditions(updated.Status.Conditions, degradedCondition) @@ -172,35 +183,101 @@ func computeDeploymentPodsScheduledCondition(deployment *appsv1.Deployment, pods } // computeIngressAvailableCondition computes the ingress controller's current Available status state -// by inspecting the Available condition of deployment. The ingresscontroller is only available if -// the deployment is also available. -func computeIngressAvailableCondition(deployment *appsv1.Deployment) operatorv1.OperatorCondition { - for _, cond := range deployment.Status.Conditions { - if cond.Type != appsv1.DeploymentAvailable { +// by inspecting the following: +// 1) the Available condition of Deployment, +// 2) the DNSReady condition of the IngressController, and +// 3) the LoadBalancerReady condition of the IngressController. +// The ingresscontroller is judged Available only if all 3 conditions are true +func computeIngressAvailableCondition(conditions []operatorv1.OperatorCondition) operatorv1.OperatorCondition { + expected := []expectedCondition{ + { + condition: IngressControllerDeploymentAvailableConditionType, + status: operatorv1.ConditionTrue, + }, + { + condition: operatorv1.DNSReadyIngressConditionType, + status: operatorv1.ConditionTrue, + ifConditionsTrue: []string{ + operatorv1.LoadBalancerManagedIngressConditionType, + operatorv1.LoadBalancerReadyIngressConditionType, + operatorv1.DNSManagedIngressConditionType, + }, + }, + { + condition: operatorv1.LoadBalancerReadyIngressConditionType, + status: operatorv1.ConditionTrue, + ifConditionsTrue: []string{operatorv1.LoadBalancerManagedIngressConditionType}, + }, + } + + // Cover the rare case of no conditions + if len(conditions) == 0 { + return operatorv1.OperatorCondition{Type: operatorv1.OperatorStatusTypeAvailable, Status: operatorv1.ConditionFalse} + } + _, unavailableConditions, _ := checkConditions(expected, conditions) + if len(unavailableConditions) != 0 { + degraded := formatConditions(unavailableConditions) + return operatorv1.OperatorCondition{ + Type: operatorv1.IngressControllerAvailableConditionType, + Status: operatorv1.ConditionFalse, + Reason: "IngressControllerUnavailable", + Message: "One or more status conditions indicate unavailable: " + degraded, + } + } + return operatorv1.OperatorCondition{ + Type: operatorv1.IngressControllerAvailableConditionType, + Status: operatorv1.ConditionTrue, + } +} + +// checkConditions compares expected operator conditions to existing operator +// conditions and returns a list of graceConditions, degradedconditions, and a +// requeueing wait time. +func checkConditions(expectedConds []expectedCondition, conditions []operatorv1.OperatorCondition) ([]*operatorv1.OperatorCondition, []*operatorv1.OperatorCondition, time.Duration) { + var graceConditions, degradedConditions []*operatorv1.OperatorCondition + var requeueAfter time.Duration + conditionsMap := make(map[string]*operatorv1.OperatorCondition) + + for i := range conditions { + conditionsMap[conditions[i].Type] = &conditions[i] + } + now := clock.Now() + for _, expected := range expectedConds { + condition, haveCondition := conditionsMap[expected.condition] + if !haveCondition { + continue + } + if condition.Status == expected.status { continue } - switch cond.Status { - case corev1.ConditionTrue: - return operatorv1.OperatorCondition{ - Type: operatorv1.IngressControllerAvailableConditionType, - Status: operatorv1.ConditionTrue, + failedPredicates := false + for _, ifCond := range expected.ifConditionsTrue { + predicate, havePredicate := conditionsMap[ifCond] + if !havePredicate || predicate.Status != operatorv1.ConditionTrue { + failedPredicates = true + break } - case corev1.ConditionFalse: - return operatorv1.OperatorCondition{ - Type: operatorv1.IngressControllerAvailableConditionType, - Status: operatorv1.ConditionFalse, - Reason: cond.Reason, - Message: "The deployment is unavailable: " + cond.Message, + } + if failedPredicates { + continue + } + if expected.gracePeriod != 0 { + t1 := now.Add(-expected.gracePeriod) + t2 := condition.LastTransitionTime + if t2.After(t1) { + d := t2.Sub(t1) + if len(graceConditions) == 0 || d < requeueAfter { + // Recompute status conditions again + // after the grace period has elapsed. + requeueAfter = d + } + graceConditions = append(graceConditions, condition) + continue } } + degradedConditions = append(degradedConditions, condition) } - - return operatorv1.OperatorCondition{ - Type: operatorv1.IngressControllerAvailableConditionType, - Status: operatorv1.ConditionFalse, - Reason: "DeploymentAvailabilityUnknown", - Message: "The deployment's Available condition couldn't be interpreted", - } + return graceConditions, degradedConditions, requeueAfter } // computeDeploymentAvailableCondition computes the ingresscontroller's @@ -339,18 +416,7 @@ func computeDeploymentReplicasAllAvailableCondition(deployment *appsv1.Deploymen // reconcile the ingresscontroller again after that period to update its status // conditions. func computeIngressDegradedCondition(conditions []operatorv1.OperatorCondition, icName string) (operatorv1.OperatorCondition, error) { - var requeueAfter time.Duration - conditionsMap := make(map[string]*operatorv1.OperatorCondition) - for i := range conditions { - conditionsMap[conditions[i].Type] = &conditions[i] - } - - expectedConditions := []struct { - condition string - status operatorv1.ConditionStatus - ifConditionsTrue []string - gracePeriod time.Duration - }{ + expectedConditions := []expectedCondition{ { condition: IngressControllerAdmittedConditionType, status: operatorv1.ConditionTrue, @@ -410,54 +476,16 @@ func computeIngressDegradedCondition(conditions []operatorv1.OperatorCondition, expectedConditions = append(expectedConditions, canaryCond) } - var graceConditions, degradedConditions []*operatorv1.OperatorCondition - now := clock.Now() - for _, expected := range expectedConditions { - condition, haveCondition := conditionsMap[expected.condition] - if !haveCondition { - continue - } - if condition.Status == expected.status { - continue - } - failedPredicates := false - for _, ifCond := range expected.ifConditionsTrue { - predicate, havePredicate := conditionsMap[ifCond] - if !havePredicate || predicate.Status != operatorv1.ConditionTrue { - failedPredicates = true - break - } - } - if failedPredicates { - continue - } - if expected.gracePeriod != 0 { - t1 := now.Add(-expected.gracePeriod) - t2 := condition.LastTransitionTime - if t2.After(t1) { - d := t2.Sub(t1) - if len(graceConditions) == 0 || d < requeueAfter { - // Recompute status conditions again - // after the grace period has elapsed. - requeueAfter = d - } - graceConditions = append(graceConditions, condition) - continue - } - } - degradedConditions = append(degradedConditions, condition) + // Cover the rare case of no conditions + if len(conditions) == 0 { + return operatorv1.OperatorCondition{Type: operatorv1.OperatorStatusTypeDegraded, Status: operatorv1.ConditionFalse}, nil } - + graceConditions, degradedConditions, requeueAfter := checkConditions(expectedConditions, conditions) if len(degradedConditions) != 0 { // Keep checking conditions every minute while degraded. - requeueAfter = time.Minute - - var degraded string - for _, cond := range degradedConditions { - degraded = degraded + fmt.Sprintf(", %s=%s (%s: %s)", cond.Type, cond.Status, cond.Reason, cond.Message) - } - degraded = degraded[2:] + retryAfter := time.Minute + degraded := formatConditions(degradedConditions) condition := operatorv1.OperatorCondition{ Type: operatorv1.OperatorStatusTypeDegraded, Status: operatorv1.ConditionTrue, @@ -465,14 +493,12 @@ func computeIngressDegradedCondition(conditions []operatorv1.OperatorCondition, Message: "One or more other status conditions indicate a degraded state: " + degraded, } - return condition, retryableerror.New(errors.New("IngressController is degraded: "+degraded), requeueAfter) + return condition, retryableerror.New(errors.New("IngressController is degraded: "+degraded), retryAfter) } - condition := operatorv1.OperatorCondition{ Type: operatorv1.OperatorStatusTypeDegraded, Status: operatorv1.ConditionFalse, } - var err error if len(graceConditions) != 0 { var grace string @@ -483,10 +509,21 @@ func computeIngressDegradedCondition(conditions []operatorv1.OperatorCondition, err = retryableerror.New(errors.New("IngressController may become degraded soon: "+grace), requeueAfter) } - return condition, err } +func formatConditions(conditions []*operatorv1.OperatorCondition) string { + var formatted string + if len(conditions) == 0 { + return "" + } + for _, cond := range conditions { + formatted = formatted + fmt.Sprintf(", %s=%s (%s: %s)", cond.Type, cond.Status, cond.Reason, cond.Message) + } + formatted = formatted[2:] + return formatted +} + // IngressStatusesEqual compares two IngressControllerStatus values. Returns true // if the provided values should be considered equal for the purpose of determining // whether an update is necessary, false otherwise. diff --git a/pkg/operator/controller/ingress/status_test.go b/pkg/operator/controller/ingress/status_test.go index 326de73b12..75ec88fc49 100644 --- a/pkg/operator/controller/ingress/status_test.go +++ b/pkg/operator/controller/ingress/status_test.go @@ -1,7 +1,6 @@ package ingress import ( - "fmt" "testing" "time" @@ -236,12 +235,22 @@ func TestComputePodsScheduledCondition(t *testing.T) { } func TestComputeIngressDegradedCondition(t *testing.T) { + // Inject a fake clock and don't forget to reset it + fakeClock := utilclock.NewFakeClock(time.Time{}) + clock = fakeClock + defer func() { + clock = utilclock.RealClock{} + }() + tests := []struct { name string icName string conditions []operatorv1.OperatorCondition expectIngressDegradedStatus operatorv1.ConditionStatus expectRequeue bool + // A degraded condition will give a 1 minute retry duration + // unless there is a grace period expected + expectAfter time.Duration }{ { name: "no conditions set", @@ -255,15 +264,18 @@ func TestComputeIngressDegradedCondition(t *testing.T) { }, expectIngressDegradedStatus: operatorv1.ConditionTrue, expectRequeue: true, + // 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)), + 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", @@ -272,6 +284,8 @@ func TestComputeIngressDegradedCondition(t *testing.T) { }, expectIngressDegradedStatus: operatorv1.ConditionTrue, expectRequeue: true, + // Just use the one minute retry duration for this degraded condition + expectAfter: time.Minute, }, { name: "deployment unavailable for <30s", @@ -280,6 +294,8 @@ func TestComputeIngressDegradedCondition(t *testing.T) { }, expectIngressDegradedStatus: operatorv1.ConditionFalse, expectRequeue: true, + // Grace period is 30 seconds, subtract the 20 second spoofed last transition time + expectAfter: time.Second * 10, }, { name: "deployment unavailable for >30s", @@ -288,6 +304,8 @@ func TestComputeIngressDegradedCondition(t *testing.T) { }, expectIngressDegradedStatus: operatorv1.ConditionTrue, expectRequeue: true, + // Exceeded grace period, just use the one minute for this degraded condition + expectAfter: time.Minute, }, { name: "deployment minimum replicas unavailable for <60s", @@ -296,6 +314,8 @@ func TestComputeIngressDegradedCondition(t *testing.T) { }, expectIngressDegradedStatus: operatorv1.ConditionFalse, expectRequeue: true, + // Grace period is 60 seconds, subtract the 20 second spoofed last transition time + expectAfter: time.Second * 40, }, { name: "deployment minimum replicas unavailable for >60s", @@ -304,6 +324,8 @@ func TestComputeIngressDegradedCondition(t *testing.T) { }, expectIngressDegradedStatus: operatorv1.ConditionTrue, expectRequeue: true, + // Exceeded grace period, just use the one minute for this degraded condition + expectAfter: time.Minute, }, { name: "deployment not all replicas available for <60m", @@ -312,6 +334,8 @@ func TestComputeIngressDegradedCondition(t *testing.T) { }, expectIngressDegradedStatus: operatorv1.ConditionFalse, expectRequeue: true, + // Grace period is 60 minutes, subtract the 20 minute spoofed last transition time + expectAfter: time.Minute * 40, }, { name: "deployment not all replicas available for >60m", @@ -320,6 +344,8 @@ func TestComputeIngressDegradedCondition(t *testing.T) { }, expectIngressDegradedStatus: operatorv1.ConditionTrue, expectRequeue: true, + // Exceeded grace period, just use the one minute for this degraded condition + expectAfter: time.Minute, }, { name: "DNS and LB not managed", @@ -342,6 +368,8 @@ func TestComputeIngressDegradedCondition(t *testing.T) { }, expectIngressDegradedStatus: operatorv1.ConditionFalse, expectRequeue: true, + // Minimum grace period of combined conditions (DNSReadyIngressConditionType) is 30 seconds + expectAfter: time.Second * 30, }, { name: "LB provisioning failing >90s", @@ -353,6 +381,8 @@ func TestComputeIngressDegradedCondition(t *testing.T) { }, expectIngressDegradedStatus: operatorv1.ConditionTrue, expectRequeue: true, + // Exceeded grace period, just use the one minute for this degraded condition + expectAfter: time.Minute, }, { name: "DNS failing <30s", @@ -364,6 +394,8 @@ func TestComputeIngressDegradedCondition(t *testing.T) { }, expectIngressDegradedStatus: operatorv1.ConditionFalse, expectRequeue: true, + // Minimum grace period of combined conditions (DNSReadyIngressConditionType) is 30 seconds, subtract the 15 second spoofed last transition time + expectAfter: time.Second * 15, }, { name: "DNS failing >30s", @@ -375,6 +407,8 @@ func TestComputeIngressDegradedCondition(t *testing.T) { }, expectIngressDegradedStatus: operatorv1.ConditionTrue, expectRequeue: true, + // Exceeded grace period, just use the one minute for this degraded condition + expectAfter: time.Minute, }, { name: "DNS not ready and deployment unavailable", @@ -391,6 +425,8 @@ func TestComputeIngressDegradedCondition(t *testing.T) { }, expectIngressDegradedStatus: operatorv1.ConditionTrue, expectRequeue: true, + // Exceeded grace period, just use the one minute for these degraded conditions + expectAfter: time.Minute, }, { name: "admitted, DNS, LB, and deployment OK", @@ -416,6 +452,8 @@ func TestComputeIngressDegradedCondition(t *testing.T) { expectIngressDegradedStatus: operatorv1.ConditionTrue, expectRequeue: true, icName: "default", + // Exceeded grace period, just use the one minute for these degraded conditions + expectAfter: time.Minute, }, { name: "default ingress controller, canary check passing", @@ -427,14 +465,16 @@ func TestComputeIngressDegradedCondition(t *testing.T) { icName: "default", }, } - for _, test := range tests { actual, err := computeIngressDegradedCondition(test.conditions, test.icName) - switch err.(type) { + switch e := err.(type) { case retryable.Error: if !test.expectRequeue { t.Errorf("%q: expected not to be told to requeue", test.name) } + if test.expectAfter.Seconds() != e.After().Seconds() { + t.Errorf("%q: expected requeue after %s, got %s", test.name, test.expectAfter.String(), e.After().String()) + } case nil: if test.expectRequeue { t.Errorf("%q: expected to be told to requeue", test.name) @@ -840,51 +880,74 @@ func TestComputeLoadBalancerStatus(t *testing.T) { func TestComputeIngressAvailableCondition(t *testing.T) { testCases := []struct { - description string - deploymentConditions []appsv1.DeploymentCondition - expect operatorv1.OperatorCondition + description string + conditions []operatorv1.OperatorCondition + expect operatorv1.OperatorCondition }{ { - description: "deployment available", - deploymentConditions: []appsv1.DeploymentCondition{ - {Type: appsv1.DeploymentAvailable, Status: corev1.ConditionTrue}, + description: "deployment, dns, and lb available", + conditions: []operatorv1.OperatorCondition{ + {Type: IngressControllerDeploymentAvailableConditionType, Status: operatorv1.ConditionTrue}, + {Type: operatorv1.DNSManagedIngressConditionType, Status: operatorv1.ConditionTrue}, + {Type: operatorv1.DNSReadyIngressConditionType, Status: operatorv1.ConditionTrue}, + {Type: operatorv1.LoadBalancerManagedIngressConditionType, Status: operatorv1.ConditionTrue}, + {Type: operatorv1.LoadBalancerReadyIngressConditionType, Status: operatorv1.ConditionTrue}, }, expect: operatorv1.OperatorCondition{Type: operatorv1.OperatorStatusTypeAvailable, Status: operatorv1.ConditionTrue}, }, { - description: "deployment not available", - deploymentConditions: []appsv1.DeploymentCondition{ - {Type: appsv1.DeploymentAvailable, Status: corev1.ConditionFalse}, + description: "deployment not available, but dns and lb available", + conditions: []operatorv1.OperatorCondition{ + {Type: IngressControllerDeploymentAvailableConditionType, Status: operatorv1.ConditionFalse}, + {Type: operatorv1.DNSManagedIngressConditionType, Status: operatorv1.ConditionTrue}, + {Type: operatorv1.DNSReadyIngressConditionType, Status: operatorv1.ConditionTrue}, + {Type: operatorv1.LoadBalancerManagedIngressConditionType, Status: operatorv1.ConditionTrue}, + {Type: operatorv1.LoadBalancerReadyIngressConditionType, Status: operatorv1.ConditionTrue}, }, expect: operatorv1.OperatorCondition{Type: operatorv1.OperatorStatusTypeAvailable, Status: operatorv1.ConditionFalse}, }, { - description: "deployment availability unknown", - deploymentConditions: []appsv1.DeploymentCondition{ - {Type: appsv1.DeploymentAvailable, Status: corev1.ConditionUnknown}, + description: "dns not available, but deployment and lb available", + conditions: []operatorv1.OperatorCondition{ + {Type: IngressControllerDeploymentAvailableConditionType, Status: operatorv1.ConditionTrue}, + {Type: operatorv1.LoadBalancerManagedIngressConditionType, Status: operatorv1.ConditionTrue}, + {Type: operatorv1.LoadBalancerReadyIngressConditionType, Status: operatorv1.ConditionTrue}, + {Type: operatorv1.DNSManagedIngressConditionType, Status: operatorv1.ConditionTrue}, + {Type: operatorv1.DNSReadyIngressConditionType, Status: operatorv1.ConditionFalse}, }, expect: operatorv1.OperatorCondition{Type: operatorv1.OperatorStatusTypeAvailable, Status: operatorv1.ConditionFalse}, }, { - description: "deployment availability not present", - deploymentConditions: []appsv1.DeploymentCondition{ - {Type: appsv1.DeploymentProgressing, Status: corev1.ConditionUnknown}, + description: "lb not available, but dns and deployment available", + conditions: []operatorv1.OperatorCondition{ + {Type: IngressControllerDeploymentAvailableConditionType, Status: operatorv1.ConditionTrue}, + {Type: operatorv1.DNSManagedIngressConditionType, Status: operatorv1.ConditionTrue}, + {Type: operatorv1.DNSReadyIngressConditionType, Status: operatorv1.ConditionTrue}, + {Type: operatorv1.LoadBalancerManagedIngressConditionType, Status: operatorv1.ConditionTrue}, + {Type: operatorv1.LoadBalancerReadyIngressConditionType, Status: operatorv1.ConditionFalse}, }, expect: operatorv1.OperatorCondition{Type: operatorv1.OperatorStatusTypeAvailable, Status: operatorv1.ConditionFalse}, }, - } - - for i, tc := range testCases { - deploy := &appsv1.Deployment{ - ObjectMeta: metav1.ObjectMeta{ - Name: fmt.Sprintf("ingress-controller-%d", i+1), - }, - Status: appsv1.DeploymentStatus{ - Conditions: tc.deploymentConditions, + { + description: "all availability unknown", + conditions: []operatorv1.OperatorCondition{ + {Type: IngressControllerDeploymentAvailableConditionType, Status: operatorv1.ConditionUnknown}, + {Type: operatorv1.DNSManagedIngressConditionType, Status: operatorv1.ConditionTrue}, + {Type: operatorv1.DNSReadyIngressConditionType, Status: operatorv1.ConditionUnknown}, + {Type: operatorv1.LoadBalancerManagedIngressConditionType, Status: operatorv1.ConditionTrue}, + {Type: operatorv1.LoadBalancerReadyIngressConditionType, Status: operatorv1.ConditionUnknown}, }, - } + expect: operatorv1.OperatorCondition{Type: operatorv1.OperatorStatusTypeAvailable, Status: operatorv1.ConditionFalse}, + }, + { + description: "all availability not present", + conditions: []operatorv1.OperatorCondition{}, + expect: operatorv1.OperatorCondition{Type: operatorv1.OperatorStatusTypeAvailable, Status: operatorv1.ConditionFalse}, + }, + } - actual := computeIngressAvailableCondition(deploy) + for _, tc := range testCases { + actual := computeIngressAvailableCondition(tc.conditions) conditionsCmpOpts := []cmp.Option{ cmpopts.IgnoreFields(operatorv1.OperatorCondition{}, "LastTransitionTime", "Reason", "Message"), cmpopts.EquateEmpty(),