From 593867c996662979be0bb1f6015881fb09e2061d Mon Sep 17 00:00:00 2001 From: Tara Gu Date: Thu, 22 Aug 2019 12:27:22 -0400 Subject: [PATCH] Consistent update for ObservedGeneration for ingress: update observed generation -> reconcile -> update status --- .../networking/v1alpha1/ingress_lifecycle.go | 5 ++ .../clusteringress/clusteringress_test.go | 76 ++++++++++++++++++- pkg/reconciler/ingress/ingress.go | 15 ++-- pkg/reconciler/route/route.go | 2 +- 4 files changed, 89 insertions(+), 9 deletions(-) diff --git a/pkg/apis/networking/v1alpha1/ingress_lifecycle.go b/pkg/apis/networking/v1alpha1/ingress_lifecycle.go index 22a2255c2f29..c5b5f3511154 100644 --- 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/reconciler/clusteringress/clusteringress_test.go b/pkg/reconciler/clusteringress/clusteringress_test.go index 63a20acd423b..85cdf4d1b1cb 100644 --- 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+": inducing failure for update virtualservices", + }}, + }, + }, + ), + }}, + 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+": gateway.networking.istio.io \"knative-ingress-gateway\" not found", }}, }, }, @@ -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 index e81bb34d1def..fcf2cd53096d 100644 --- a/pkg/reconciler/ingress/ingress.go +++ b/pkg/reconciler/ingress/ingress.go @@ -62,7 +62,9 @@ import ( ) const ( - controllerAgentName = "ingress-controller" + controllerAgentName = "ingress-controller" + NotReconciledReason = "ReconcileIngressFailed" + NotReconciledMessage = "Ingress reconciliation failed" ) type Reconciler struct { @@ -244,6 +246,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, fmt.Sprintf(NotReconciledMessage+": %v", reconcileErr)) + } 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 +267,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 +293,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 +334,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 index 9bb44bfb0dd6..5fe8426f792c 100644 --- 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())