Consistent update for ObservedGeneration for ingress: update observed…

… generation -> reconcile -> update status
knative · Aug 26, 2019 · b8dd98c · b8dd98c
1 parent e82e5a1
commit b8dd98c
Show file tree

Hide file tree

Showing 5 changed files with 104 additions and 9 deletions.
diff --git a/pkg/apis/networking/v1alpha1/ingress_lifecycle.go b/pkg/apis/networking/v1alpha1/ingress_lifecycle.go
@@ -78,6 +78,11 @@ func (is *IngressStatus) MarkLoadBalancerPending() {
 		"Waiting for VirtualService to be ready")
 }
 
+// MarkIngressNotReady marks the "IngressConditionReady" condition to unknown.
+func (is *IngressStatus) MarkIngressNotReady(reason, message string) {
+	ingressCondSet.Manage(is).MarkUnknown(IngressConditionReady, reason, message)
+}
+
 // IsReady looks at the conditions and if the Status has a condition
 // IngressConditionReady returns true if ConditionStatus is True
 func (is *IngressStatus) IsReady() bool {

diff --git a/pkg/apis/networking/v1alpha1/ingress_lifecycle_test.go b/pkg/apis/networking/v1alpha1/ingress_lifecycle_test.go
@@ -100,4 +100,15 @@ func TestIngressTypicalFlow(t *testing.T) {
 	// Mark not owned.
 	r.MarkResourceNotOwned("i own", "you")
 	apitest.CheckConditionFailed(r.duck(), IngressConditionReady, t)
+
+	// Mark network configured, and check that ingress is ready again
+	r.MarkNetworkConfigured()
+	apitest.CheckConditionSucceeded(r.duck(), IngressConditionReady, t)
+	if !r.IsReady() {
+		t.Fatal("IsReady()=false, wanted true")
+	}
+
+	// Mark ingress not ready
+	r.MarkIngressNotReady("", "")
+	apitest.CheckConditionOngoing(r.duck(), IngressConditionReady, t)
 }
diff --git a/pkg/reconciler/clusteringress/clusteringress_test.go b/pkg/reconciler/clusteringress/clusteringress_test.go
@@ -232,6 +232,78 @@ func TestReconcile(t *testing.T) {
 			Eventf(corev1.EventTypeNormal, "Updated", "Updated status for Ingress %q", "no-virtualservice-yet"),
 		},
 		Key: "no-virtualservice-yet",
+	}, {
+		Name:                    "observed generation is updated when error is encountered in reconciling, and ingress ready status is unknown",
+		SkipNamespaceValidation: true,
+		WantErr:                 true,
+		WithReactors: []clientgotesting.ReactionFunc{
+			InduceFailure("update", "virtualservices"),
+		},
+		Objects: []runtime.Object{
+			ingressWithStatus("reconcile-failed", 1234,
+				v1alpha1.IngressStatus{
+					Status: duckv1beta1.Status{
+						Conditions: duckv1beta1.Conditions{{
+							Type:     v1alpha1.IngressConditionLoadBalancerReady,
+							Status:   corev1.ConditionTrue,
+						}, {
+							Type:     v1alpha1.IngressConditionNetworkConfigured,
+							Status:   corev1.ConditionTrue,
+						}, {
+							Type:     v1alpha1.IngressConditionReady,
+							Status:   corev1.ConditionTrue,
+						}},
+					},
+				},
+			),
+			&v1alpha3.VirtualService{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "reconcile-failed",
+					Namespace: system.Namespace(),
+					Labels: map[string]string{
+						networking.ClusterIngressLabelKey: "reconcile-failed",
+						serving.RouteLabelKey:             "test-route",
+						serving.RouteNamespaceLabelKey:    "test-ns",
+					},
+					OwnerReferences: []metav1.OwnerReference{*kmeta.NewControllerRef(ingress("reconcile-failed", 1234))},
+				},
+				Spec: v1alpha3.VirtualServiceSpec{},
+			},
+		},
+		WantCreates: []runtime.Object{
+			insertProbe(t, resources.MakeMeshVirtualService(ingress("reconcile-failed", 1234))),
+		},
+		WantUpdates: []clientgotesting.UpdateActionImpl{{
+			Object: insertProbe(t, resources.MakeIngressVirtualService(ingress("reconcile-failed", 1234),
+				makeGatewayMap([]string{"knative-testing/knative-test-gateway", "knative-testing/knative-ingress-gateway"}, nil))),
+		}},
+		WantStatusUpdates: []clientgotesting.UpdateActionImpl{{
+			Object: ingressWithStatus("reconcile-failed", 1234,
+				v1alpha1.IngressStatus{
+					Status: duckv1beta1.Status{
+						Conditions: duckv1beta1.Conditions{{
+							Type:     v1alpha1.IngressConditionLoadBalancerReady,
+							Status:   corev1.ConditionTrue,
+						}, {
+							Type:     v1alpha1.IngressConditionNetworkConfigured,
+							Status:   corev1.ConditionTrue,
+						}, {
+							Type:     v1alpha1.IngressConditionReady,
+							Status:   corev1.ConditionUnknown,
+							Severity: apis.ConditionSeverityError,
+							Reason:   ing.NotReconciledReason,
+							Message:  ing.NotReconciledMessage,
+						}},
+					},
+				},
+			),
+		}},
+		WantEvents: []string{
+			Eventf(corev1.EventTypeNormal, "Created", "Created VirtualService %q", "reconcile-failed-mesh"),
+			Eventf(corev1.EventTypeWarning, "InternalError", "inducing failure for update virtualservices"),
+			Eventf(corev1.EventTypeNormal, "Updated", "Updated status for Ingress %q", "reconcile-failed"),
+		},
+		Key: "reconcile-failed",
 	}, {
 		Name:                    "reconcile VirtualService to match desired one",
 		SkipNamespaceValidation: true,
@@ -446,6 +518,8 @@ func TestReconcile_EnableAutoTLS(t *testing.T) {
 							Type:     v1alpha1.IngressConditionReady,
 							Status:   corev1.ConditionUnknown,
 							Severity: apis.ConditionSeverityError,
+							Reason:   ing.NotReconciledReason,
+							Message:  ing.NotReconciledMessage,
 						}},
 					},
 				},
@@ -454,8 +528,8 @@ func TestReconcile_EnableAutoTLS(t *testing.T) {
 		WantEvents: []string{
 			Eventf(corev1.EventTypeNormal, "Created", "Created VirtualService %q", "reconciling-clusteringress-mesh"),
 			Eventf(corev1.EventTypeNormal, "Created", "Created VirtualService %q", "reconciling-clusteringress"),
-			Eventf(corev1.EventTypeNormal, "Updated", "Updated status for Ingress %q", "reconciling-clusteringress"),
 			Eventf(corev1.EventTypeWarning, "InternalError", `gateway.networking.istio.io "knative-ingress-gateway" not found`),
+			Eventf(corev1.EventTypeNormal, "Updated", "Updated status for Ingress %q", "reconciling-clusteringress"),
 		},
 		// Error should be returned when there is no preinstalled gateways.
 		WantErr: true,

diff --git a/pkg/reconciler/ingress/ingress.go b/pkg/reconciler/ingress/ingress.go
@@ -62,7 +62,13 @@ import (
 )
 
 const (
-	controllerAgentName = "ingress-controller"
+	controllerAgentName  = "ingress-controller"
+
+	// NotReconciledReason specifies the reason that ingress reconciliation has failed
+	NotReconciledReason  = "ReconcileIngressFailed"
+
+	// NotReconciledMessage indicates the message that ingress reconciliation has failed
+	NotReconciledMessage = "Ingress reconciliation failed"
 )
 
 type Reconciler struct {
@@ -244,6 +250,10 @@ func (r *BaseIngressReconciler) ReconcileIngress(ctx context.Context, ra Reconci
 	// Reconcile this copy of the Ingress and then write back any status
 	// updates regardless of whether the reconciliation errored out.
 	reconcileErr := r.reconcileIngress(ctx, ra, ingress)
+	if reconcileErr != nil {
+		r.Recorder.Event(ingress, corev1.EventTypeWarning, "InternalError", reconcileErr.Error())
+		ingress.GetStatus().MarkIngressNotReady(NotReconciledReason, NotReconciledMessage)
+	}
 	if equality.Semantic.DeepEqual(original.GetStatus(), ingress.GetStatus()) {
 		// If we didn't change anything then don't call updateStatus.
 		// This is important because the copy we loaded from the informer's
@@ -261,9 +271,6 @@ func (r *BaseIngressReconciler) ReconcileIngress(ctx context.Context, ra Reconci
 		r.Recorder.Eventf(ingress, corev1.EventTypeNormal, "Updated",
 			"Updated status for Ingress %q", ingress.GetName())
 	}
-	if reconcileErr != nil {
-		r.Recorder.Event(ingress, corev1.EventTypeWarning, "InternalError", reconcileErr.Error())
-	}
 	return reconcileErr
 }
 
@@ -290,9 +297,8 @@ func (r *BaseIngressReconciler) reconcileIngress(ctx context.Context, ra Reconci
 
 	// First, create the VirtualServices.
 	logger.Infof("Creating/Updating VirtualServices")
+	ia.GetStatus().ObservedGeneration = ia.GetGeneration()
 	if err := r.reconcileVirtualServices(ctx, ia, vses); err != nil {
-		// TODO(lichuqiang): should we explicitly mark the ingress as unready
-		// when error reconciling VirtualService?
 		return err
 	}
 
@@ -332,7 +338,6 @@ func (r *BaseIngressReconciler) reconcileIngress(ctx context.Context, ra Reconci
 
 	// Update status
 	ia.GetStatus().MarkNetworkConfigured()
-	ia.GetStatus().ObservedGeneration = ia.GetGeneration()
 
 	lbReady := true
 	for _, vs := range vses {

diff --git a/pkg/reconciler/route/route.go b/pkg/reconciler/route/route.go
@@ -264,7 +264,7 @@ func (c *Reconciler) reconcile(ctx context.Context, r *v1alpha1.Route) error {
 		return err
 	}
 
-	if ingress.GetObjectMeta().GetGeneration() != ingress.GetStatus().ObservedGeneration {
+	if ingress.GetObjectMeta().GetGeneration() != ingress.GetStatus().ObservedGeneration || !ingress.GetStatus().IsReady() {
 		r.Status.MarkIngressNotConfigured()
 	} else {
 		r.Status.PropagateIngressStatus(*ingress.GetStatus())