From d2ff02e5c43ef610589fb275d01aefaac52be502 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Markus=20Th=C3=B6mmes?= Date: Thu, 5 Dec 2019 19:34:25 +0100 Subject: [PATCH 1/8] Fetch objects via API before updating status to reduce conflicts. --- pkg/reconciler/autoscaling/reconciler.go | 2 +- pkg/reconciler/certificate/certificate.go | 2 +- pkg/reconciler/configuration/configuration.go | 2 +- pkg/reconciler/ingress/ingress.go | 2 +- pkg/reconciler/metric/metric.go | 3 ++- pkg/reconciler/revision/revision.go | 3 ++- pkg/reconciler/route/reconcile_resources.go | 2 +- pkg/reconciler/serverlessservice/serverlessservice.go | 2 +- pkg/reconciler/service/service.go | 2 +- 9 files changed, 11 insertions(+), 9 deletions(-) diff --git a/pkg/reconciler/autoscaling/reconciler.go b/pkg/reconciler/autoscaling/reconciler.go index fb5ea3798bdb..92791e6cee85 100644 --- a/pkg/reconciler/autoscaling/reconciler.go +++ b/pkg/reconciler/autoscaling/reconciler.go @@ -139,7 +139,7 @@ func (c *Base) ReconcileMetric(ctx context.Context, pa *pav1alpha1.PodAutoscaler // UpdateStatus updates the status of the given PodAutoscaler. func (c *Base) UpdateStatus(desired *pav1alpha1.PodAutoscaler) (*pav1alpha1.PodAutoscaler, error) { - pa, err := c.PALister.PodAutoscalers(desired.Namespace).Get(desired.Name) + pa, err := c.ServingClientSet.AutoscalingV1alpha1().PodAutoscalers(desired.Namespace).Get(desired.Name, metav1.GetOptions{}) if err != nil { return nil, err } diff --git a/pkg/reconciler/certificate/certificate.go b/pkg/reconciler/certificate/certificate.go index 1dbbf6323724..f36d7122fd37 100644 --- a/pkg/reconciler/certificate/certificate.go +++ b/pkg/reconciler/certificate/certificate.go @@ -174,7 +174,7 @@ func (c *Reconciler) reconcileCMCertificate(ctx context.Context, knCert *v1alpha } func (c *Reconciler) updateStatus(desired *v1alpha1.Certificate) (*v1alpha1.Certificate, error) { - cert, err := c.knCertificateLister.Certificates(desired.Namespace).Get(desired.Name) + cert, err := c.ServingClientSet.NetworkingV1alpha1().Certificates(desired.Namespace).Get(desired.Name, metav1.GetOptions{}) if err != nil { return nil, err } diff --git a/pkg/reconciler/configuration/configuration.go b/pkg/reconciler/configuration/configuration.go index 3e5a8f64aa3c..ab3f37d016a9 100644 --- a/pkg/reconciler/configuration/configuration.go +++ b/pkg/reconciler/configuration/configuration.go @@ -342,7 +342,7 @@ func (c *Reconciler) createRevision(ctx context.Context, config *v1alpha1.Config } func (c *Reconciler) updateStatus(desired *v1alpha1.Configuration) (*v1alpha1.Configuration, error) { - config, err := c.configurationLister.Configurations(desired.Namespace).Get(desired.Name) + config, err := c.ServingClientSet.ServingV1alpha1().Configurations(desired.Namespace).Get(desired.Name, metav1.GetOptions{}) if err != nil { return nil, err } diff --git a/pkg/reconciler/ingress/ingress.go b/pkg/reconciler/ingress/ingress.go index 1bef17039834..1eefce2eb03e 100644 --- a/pkg/reconciler/ingress/ingress.go +++ b/pkg/reconciler/ingress/ingress.go @@ -307,7 +307,7 @@ func (r *Reconciler) reconcileDeletion(ctx context.Context, ia *v1alpha1.Ingress // Update the Status of the Ingress. Caller is responsible for checking // for semantic differences before calling. func (r *Reconciler) updateStatus(desired *v1alpha1.Ingress) (*v1alpha1.Ingress, error) { - ingress, err := r.ingressLister.Ingresses(desired.GetNamespace()).Get(desired.GetName()) + ingress, err := r.ServingClientSet.NetworkingV1alpha1().Ingresses(desired.GetNamespace()).Get(desired.GetName(), metav1.GetOptions{}) if err != nil { return nil, err } diff --git a/pkg/reconciler/metric/metric.go b/pkg/reconciler/metric/metric.go index 6cd8f2a90944..a87655c1ea2e 100644 --- a/pkg/reconciler/metric/metric.go +++ b/pkg/reconciler/metric/metric.go @@ -32,6 +32,7 @@ import ( corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/equality" apierrs "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/client-go/tools/cache" ) @@ -103,7 +104,7 @@ func (r *reconciler) reconcileCollection(ctx context.Context, metric *v1alpha1.M } func (r *reconciler) updateStatus(m *v1alpha1.Metric) error { - ex, err := r.metricLister.Metrics(m.Namespace).Get(m.Name) + ex, err := r.ServingClientSet.AutoscalingV1alpha1().Metrics(m.Namespace).Get(m.Name, metav1.GetOptions{}) if err != nil { // If something deleted metric while we were reconciling ¯\(°_o)/¯. return err diff --git a/pkg/reconciler/revision/revision.go b/pkg/reconciler/revision/revision.go index 6eed164a37ae..a22aa0aba2bd 100644 --- a/pkg/reconciler/revision/revision.go +++ b/pkg/reconciler/revision/revision.go @@ -28,6 +28,7 @@ import ( corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/equality" apierrs "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/util/sets" appsv1listers "k8s.io/client-go/listers/apps/v1" corev1listers "k8s.io/client-go/listers/core/v1" @@ -232,7 +233,7 @@ func (c *Reconciler) updateRevisionLoggingURL( } func (c *Reconciler) updateStatus(desired *v1alpha1.Revision) (*v1alpha1.Revision, error) { - rev, err := c.revisionLister.Revisions(desired.Namespace).Get(desired.Name) + rev, err := c.ServingClientSet.ServingV1alpha1().Revisions(desired.Namespace).Get(desired.Name, metav1.GetOptions{}) if err != nil { return nil, err } diff --git a/pkg/reconciler/route/reconcile_resources.go b/pkg/reconciler/route/reconcile_resources.go index 57b29cd2f8ed..00716b736923 100644 --- a/pkg/reconciler/route/reconcile_resources.go +++ b/pkg/reconciler/route/reconcile_resources.go @@ -190,7 +190,7 @@ func (c *Reconciler) updatePlaceholderServices(ctx context.Context, route *v1alp // Update the Status of the route. Caller is responsible for checking // for semantic differences before calling. func (c *Reconciler) updateStatus(desired *v1alpha1.Route) (*v1alpha1.Route, error) { - route, err := c.routeLister.Routes(desired.Namespace).Get(desired.Name) + route, err := c.ServingClientSet.ServingV1alpha1().Routes(desired.Namespace).Get(desired.Name, metav1.GetOptions{}) if err != nil { return nil, err } diff --git a/pkg/reconciler/serverlessservice/serverlessservice.go b/pkg/reconciler/serverlessservice/serverlessservice.go index bfd28340f4de..515c4dae9100 100644 --- a/pkg/reconciler/serverlessservice/serverlessservice.go +++ b/pkg/reconciler/serverlessservice/serverlessservice.go @@ -128,7 +128,7 @@ func (r *reconciler) reconcile(ctx context.Context, sks *netv1alpha1.ServerlessS } func (r *reconciler) updateStatus(sks *netv1alpha1.ServerlessService, logger *zap.SugaredLogger) (*netv1alpha1.ServerlessService, error) { - original, err := r.sksLister.ServerlessServices(sks.Namespace).Get(sks.Name) + original, err := r.ServingClientSet.NetworkingV1alpha1().ServerlessServices(sks.Namespace).Get(sks.Name, metav1.GetOptions{}) if err != nil { return nil, err } diff --git a/pkg/reconciler/service/service.go b/pkg/reconciler/service/service.go index cb4f236bcff9..dcf831cf438b 100644 --- a/pkg/reconciler/service/service.go +++ b/pkg/reconciler/service/service.go @@ -274,7 +274,7 @@ func (c *Reconciler) checkRoutesNotReady(config *v1alpha1.Configuration, logger } func (c *Reconciler) updateStatus(desired *v1alpha1.Service, logger *zap.SugaredLogger) (*v1alpha1.Service, error) { - service, err := c.serviceLister.Services(desired.Namespace).Get(desired.Name) + service, err := c.ServingClientSet.ServingV1alpha1().Services(desired.Namespace).Get(desired.Name, metav1.GetOptions{}) if err != nil { return nil, err } From fdf85190b4426ddaa067ff6e37de37989223ef7a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Markus=20Th=C3=B6mmes?= Date: Mon, 9 Dec 2019 11:29:39 +0100 Subject: [PATCH 2/8] Implement retries on status updates and fallback to client. --- pkg/reconciler/autoscaling/reconciler.go | 37 +++++++++----- pkg/reconciler/certificate/certificate.go | 37 +++++++++----- pkg/reconciler/configuration/configuration.go | 36 +++++++++---- pkg/reconciler/ingress/ingress.go | 35 +++++++++---- pkg/reconciler/metric/metric.go | 42 ++++++++++----- pkg/reconciler/revision/revision.go | 36 +++++++++---- pkg/reconciler/route/reconcile_resources.go | 38 +++++++++----- .../serverlessservice/serverlessservice.go | 39 +++++++++----- pkg/reconciler/service/service.go | 51 +++++++++++-------- 9 files changed, 235 insertions(+), 116 deletions(-) diff --git a/pkg/reconciler/autoscaling/reconciler.go b/pkg/reconciler/autoscaling/reconciler.go index 92791e6cee85..813ec468b216 100644 --- a/pkg/reconciler/autoscaling/reconciler.go +++ b/pkg/reconciler/autoscaling/reconciler.go @@ -139,17 +139,30 @@ func (c *Base) ReconcileMetric(ctx context.Context, pa *pav1alpha1.PodAutoscaler // UpdateStatus updates the status of the given PodAutoscaler. func (c *Base) UpdateStatus(desired *pav1alpha1.PodAutoscaler) (*pav1alpha1.PodAutoscaler, error) { - pa, err := c.ServingClientSet.AutoscalingV1alpha1().PodAutoscalers(desired.Namespace).Get(desired.Name, metav1.GetOptions{}) - if err != nil { - return nil, err - } - // If there's nothing to update, just return. - if reflect.DeepEqual(pa.Status, desired.Status) { - return pa, nil - } - // Don't modify the informers copy - existing := pa.DeepCopy() - existing.Status = desired.Status + var err error + for i := 0; i < 4; i++ { + // The first iteration tries to use the informer's state. + var existing *pav1alpha1.PodAutoscaler + if i == 0 { + existing, err = c.PALister.PodAutoscalers(desired.Namespace).Get(desired.Name) + existing = existing.DeepCopy() + } else { + existing, err = c.ServingClientSet.AutoscalingV1alpha1().PodAutoscalers(desired.Namespace).Get(desired.Name, metav1.GetOptions{}) + } + if err != nil { + return nil, err + } + + // If there's nothing to update, just return. + if reflect.DeepEqual(existing.Status, desired.Status) { + return existing, nil + } - return c.ServingClientSet.AutoscalingV1alpha1().PodAutoscalers(pa.Namespace).UpdateStatus(existing) + existing.Status = desired.Status + existing, err = c.ServingClientSet.AutoscalingV1alpha1().PodAutoscalers(existing.Namespace).UpdateStatus(existing) + if !errors.IsConflict(err) { + return existing, err + } + } + return nil, err } diff --git a/pkg/reconciler/certificate/certificate.go b/pkg/reconciler/certificate/certificate.go index f36d7122fd37..51c6da4df5f2 100644 --- a/pkg/reconciler/certificate/certificate.go +++ b/pkg/reconciler/certificate/certificate.go @@ -174,17 +174,30 @@ func (c *Reconciler) reconcileCMCertificate(ctx context.Context, knCert *v1alpha } func (c *Reconciler) updateStatus(desired *v1alpha1.Certificate) (*v1alpha1.Certificate, error) { - cert, err := c.ServingClientSet.NetworkingV1alpha1().Certificates(desired.Namespace).Get(desired.Name, metav1.GetOptions{}) - if err != nil { - return nil, err - } - // If there's nothing to update, just return. - if reflect.DeepEqual(cert.Status, desired.Status) { - return cert, nil - } - // Don't modify the informers copy - existing := cert.DeepCopy() - existing.Status = desired.Status + var err error + for i := 0; i < 4; i++ { + // The first iteration tries to use the informer's state. + var existing *v1alpha1.Certificate + if i == 0 { + existing, err = c.knCertificateLister.Certificates(desired.Namespace).Get(desired.Name) + existing = existing.DeepCopy() + } else { + existing, err = c.ServingClientSet.NetworkingV1alpha1().Certificates(desired.Namespace).Get(desired.Name, metav1.GetOptions{}) + } + if err != nil { + return nil, err + } + + // If there's nothing to update, just return. + if reflect.DeepEqual(existing.Status, desired.Status) { + return existing, nil + } - return c.ServingClientSet.NetworkingV1alpha1().Certificates(existing.Namespace).UpdateStatus(existing) + existing.Status = desired.Status + existing, err = c.ServingClientSet.NetworkingV1alpha1().Certificates(existing.Namespace).UpdateStatus(existing) + if !apierrs.IsConflict(err) { + return existing, err + } + } + return nil, err } diff --git a/pkg/reconciler/configuration/configuration.go b/pkg/reconciler/configuration/configuration.go index ab3f37d016a9..0729395c3ab5 100644 --- a/pkg/reconciler/configuration/configuration.go +++ b/pkg/reconciler/configuration/configuration.go @@ -342,16 +342,30 @@ func (c *Reconciler) createRevision(ctx context.Context, config *v1alpha1.Config } func (c *Reconciler) updateStatus(desired *v1alpha1.Configuration) (*v1alpha1.Configuration, error) { - config, err := c.ServingClientSet.ServingV1alpha1().Configurations(desired.Namespace).Get(desired.Name, metav1.GetOptions{}) - if err != nil { - return nil, err - } - // If there's nothing to update, just return. - if reflect.DeepEqual(config.Status, desired.Status) { - return config, nil + var err error + for i := 0; i < 4; i++ { + // The first iteration tries to use the informer's state. + var existing *v1alpha1.Configuration + if i == 0 { + existing, err = c.configurationLister.Configurations(desired.Namespace).Get(desired.Name) + existing = existing.DeepCopy() + } else { + existing, err = c.ServingClientSet.ServingV1alpha1().Configurations(desired.Namespace).Get(desired.Name, metav1.GetOptions{}) + } + if err != nil { + return nil, err + } + + // If there's nothing to update, just return. + if reflect.DeepEqual(existing.Status, desired.Status) { + return existing, nil + } + + existing.Status = desired.Status + existing, err = c.ServingClientSet.ServingV1alpha1().Configurations(desired.Namespace).UpdateStatus(existing) + if !errors.IsConflict(err) { + return existing, err + } } - // Don't modify the informers copy - existing := config.DeepCopy() - existing.Status = desired.Status - return c.ServingClientSet.ServingV1alpha1().Configurations(desired.Namespace).UpdateStatus(existing) + return nil, err } diff --git a/pkg/reconciler/ingress/ingress.go b/pkg/reconciler/ingress/ingress.go index 1eefce2eb03e..96e752874399 100644 --- a/pkg/reconciler/ingress/ingress.go +++ b/pkg/reconciler/ingress/ingress.go @@ -307,19 +307,32 @@ func (r *Reconciler) reconcileDeletion(ctx context.Context, ia *v1alpha1.Ingress // Update the Status of the Ingress. Caller is responsible for checking // for semantic differences before calling. func (r *Reconciler) updateStatus(desired *v1alpha1.Ingress) (*v1alpha1.Ingress, error) { - ingress, err := r.ServingClientSet.NetworkingV1alpha1().Ingresses(desired.GetNamespace()).Get(desired.GetName(), metav1.GetOptions{}) - if err != nil { - return nil, err - } + var err error + for i := 0; i < 4; i++ { + // The first iteration tries to use the informer's state. + var existing *v1alpha1.Ingress + if i == 0 { + existing, err = r.ingressLister.Ingresses(desired.Namespace).Get(desired.Name) + existing = existing.DeepCopy() + } else { + existing, err = r.ServingClientSet.NetworkingV1alpha1().Ingresses(desired.GetNamespace()).Get(desired.GetName(), metav1.GetOptions{}) + } + if err != nil { + return nil, err + } - // If there's nothing to update, just return. - if reflect.DeepEqual(ingress.Status, desired.Status) { - return ingress, nil + // If there's nothing to update, just return. + if reflect.DeepEqual(existing.Status, desired.Status) { + return existing, nil + } + + existing.Status = desired.Status + existing, err = r.ServingClientSet.NetworkingV1alpha1().Ingresses(existing.GetNamespace()).UpdateStatus(existing) + if !apierrs.IsConflict(err) { + return existing, err + } } - // Don't modify the informers copy - existing := ingress.DeepCopy() - existing.Status = desired.Status - return r.ServingClientSet.NetworkingV1alpha1().Ingresses(existing.GetNamespace()).UpdateStatus(existing) + return nil, err } func (r *Reconciler) ensureFinalizer(ia *v1alpha1.Ingress) error { diff --git a/pkg/reconciler/metric/metric.go b/pkg/reconciler/metric/metric.go index a87655c1ea2e..028aa5345b5f 100644 --- a/pkg/reconciler/metric/metric.go +++ b/pkg/reconciler/metric/metric.go @@ -19,6 +19,7 @@ package metric import ( "context" "fmt" + "reflect" "go.uber.org/zap" "knative.dev/serving/pkg/apis/autoscaling/v1alpha1" @@ -82,7 +83,7 @@ func (r *reconciler) Reconcile(ctx context.Context, key string) error { if !equality.Semantic.DeepEqual(original.Status, metric.Status) { // Change of status, need to update the object. - if uErr := r.updateStatus(metric); uErr != nil { + if _, uErr := r.updateStatus(metric); uErr != nil { logger.Warnw("Failed to update metric status", zap.Error(uErr)) r.Recorder.Eventf(metric, corev1.EventTypeWarning, "UpdateFailed", "Failed to update metric status: %v", uErr) @@ -103,18 +104,31 @@ func (r *reconciler) reconcileCollection(ctx context.Context, metric *v1alpha1.M return nil } -func (r *reconciler) updateStatus(m *v1alpha1.Metric) error { - ex, err := r.ServingClientSet.AutoscalingV1alpha1().Metrics(m.Namespace).Get(m.Name, metav1.GetOptions{}) - if err != nil { - // If something deleted metric while we were reconciling ¯\(°_o)/¯. - return err - } - if equality.Semantic.DeepEqual(ex.Status, m.Status) { - // no-op - return nil +func (r *reconciler) updateStatus(desired *v1alpha1.Metric) (*v1alpha1.Metric, error) { + var err error + for i := 0; i < 4; i++ { + // The first iteration tries to use the informer's state. + var existing *v1alpha1.Metric + if i == 0 { + existing, err = r.metricLister.Metrics(desired.Namespace).Get(desired.Name) + existing = existing.DeepCopy() + } else { + existing, err = r.ServingClientSet.AutoscalingV1alpha1().Metrics(desired.Namespace).Get(desired.Name, metav1.GetOptions{}) + } + if err != nil { + return nil, err + } + + // If there's nothing to update, just return. + if reflect.DeepEqual(existing.Status, desired.Status) { + return existing, nil + } + + existing.Status = desired.Status + existing, err = r.ServingClientSet.AutoscalingV1alpha1().Metrics(existing.Namespace).UpdateStatus(existing) + if !apierrs.IsConflict(err) { + return existing, err + } } - ex = ex.DeepCopy() - ex.Status = m.Status - _, err = r.ServingClientSet.AutoscalingV1alpha1().Metrics(ex.Namespace).UpdateStatus(ex) - return err + return nil, err } diff --git a/pkg/reconciler/revision/revision.go b/pkg/reconciler/revision/revision.go index a22aa0aba2bd..65d604764b81 100644 --- a/pkg/reconciler/revision/revision.go +++ b/pkg/reconciler/revision/revision.go @@ -233,16 +233,30 @@ func (c *Reconciler) updateRevisionLoggingURL( } func (c *Reconciler) updateStatus(desired *v1alpha1.Revision) (*v1alpha1.Revision, error) { - rev, err := c.ServingClientSet.ServingV1alpha1().Revisions(desired.Namespace).Get(desired.Name, metav1.GetOptions{}) - if err != nil { - return nil, err - } - // If there's nothing to update, just return. - if reflect.DeepEqual(rev.Status, desired.Status) { - return rev, nil + var err error + for i := 0; i < 4; i++ { + // The first iteration tries to use the informer's state. + var existing *v1alpha1.Revision + if i == 0 { + existing, err = c.revisionLister.Revisions(desired.Namespace).Get(desired.Name) + existing = existing.DeepCopy() + } else { + existing, err = c.ServingClientSet.ServingV1alpha1().Revisions(desired.Namespace).Get(desired.Name, metav1.GetOptions{}) + } + if err != nil { + return nil, err + } + + // If there's nothing to update, just return. + if reflect.DeepEqual(existing.Status, desired.Status) { + return existing, nil + } + + existing.Status = desired.Status + existing, err = c.ServingClientSet.ServingV1alpha1().Revisions(desired.Namespace).UpdateStatus(existing) + if !apierrs.IsConflict(err) { + return existing, err + } } - // Don't modify the informers copy - existing := rev.DeepCopy() - existing.Status = desired.Status - return c.ServingClientSet.ServingV1alpha1().Revisions(desired.Namespace).UpdateStatus(existing) + return nil, err } diff --git a/pkg/reconciler/route/reconcile_resources.go b/pkg/reconciler/route/reconcile_resources.go index 00716b736923..8515a6803da3 100644 --- a/pkg/reconciler/route/reconcile_resources.go +++ b/pkg/reconciler/route/reconcile_resources.go @@ -187,21 +187,33 @@ func (c *Reconciler) updatePlaceholderServices(ctx context.Context, route *v1alp return eg.Wait() } -// Update the Status of the route. Caller is responsible for checking -// for semantic differences before calling. func (c *Reconciler) updateStatus(desired *v1alpha1.Route) (*v1alpha1.Route, error) { - route, err := c.ServingClientSet.ServingV1alpha1().Routes(desired.Namespace).Get(desired.Name, metav1.GetOptions{}) - if err != nil { - return nil, err - } - // If there's nothing to update, just return. - if reflect.DeepEqual(route.Status, desired.Status) { - return route, nil + var err error + for i := 0; i < 4; i++ { + // The first iteration tries to use the informer's state. + var existing *v1alpha1.Route + if i == 0 { + existing, err = c.routeLister.Routes(desired.Namespace).Get(desired.Name) + existing = existing.DeepCopy() + } else { + existing, err = c.ServingClientSet.ServingV1alpha1().Routes(desired.Namespace).Get(desired.Name, metav1.GetOptions{}) + } + if err != nil { + return nil, err + } + + // If there's nothing to update, just return. + if reflect.DeepEqual(existing.Status, desired.Status) { + return existing, nil + } + + existing.Status = desired.Status + existing, err = c.ServingClientSet.ServingV1alpha1().Routes(desired.Namespace).UpdateStatus(existing) + if !apierrs.IsConflict(err) { + return existing, err + } } - // Don't modify the informers copy - existing := route.DeepCopy() - existing.Status = desired.Status - return c.ServingClientSet.ServingV1alpha1().Routes(desired.Namespace).UpdateStatus(existing) + return nil, err } // Update the lastPinned annotation on revisions we target so they don't get GC'd. diff --git a/pkg/reconciler/serverlessservice/serverlessservice.go b/pkg/reconciler/serverlessservice/serverlessservice.go index 515c4dae9100..f3129691b984 100644 --- a/pkg/reconciler/serverlessservice/serverlessservice.go +++ b/pkg/reconciler/serverlessservice/serverlessservice.go @@ -94,7 +94,7 @@ func (r *reconciler) Reconcile(ctx context.Context, key string) error { r.Recorder.Eventf(sks, corev1.EventTypeWarning, "UpdateFailed", "InternalError: %v", reconcileErr.Error()) } if !equality.Semantic.DeepEqual(sks.Status, original.Status) { - if _, err := r.updateStatus(sks, logger); err != nil { + if _, err := r.updateStatus(sks); err != nil { r.Recorder.Eventf(sks, corev1.EventTypeWarning, "UpdateFailed", "Failed to update status: %v", err) return err } @@ -127,18 +127,33 @@ func (r *reconciler) reconcile(ctx context.Context, sks *netv1alpha1.ServerlessS return nil } -func (r *reconciler) updateStatus(sks *netv1alpha1.ServerlessService, logger *zap.SugaredLogger) (*netv1alpha1.ServerlessService, error) { - original, err := r.ServingClientSet.NetworkingV1alpha1().ServerlessServices(sks.Namespace).Get(sks.Name, metav1.GetOptions{}) - if err != nil { - return nil, err - } - if reflect.DeepEqual(original.Status, sks.Status) { - return original, nil +func (r *reconciler) updateStatus(desired *netv1alpha1.ServerlessService) (*netv1alpha1.ServerlessService, error) { + var err error + for i := 0; i < 4; i++ { + // The first iteration tries to use the informer's state. + var existing *netv1alpha1.ServerlessService + if i == 0 { + existing, err = r.sksLister.ServerlessServices(desired.Namespace).Get(desired.Name) + existing = existing.DeepCopy() + } else { + existing, err = r.ServingClientSet.NetworkingV1alpha1().ServerlessServices(desired.Namespace).Get(desired.Name, metav1.GetOptions{}) + } + if err != nil { + return nil, err + } + + // If there's nothing to update, just return. + if reflect.DeepEqual(existing.Status, desired.Status) { + return existing, nil + } + + existing.Status = desired.Status + existing, err = r.ServingClientSet.NetworkingV1alpha1().ServerlessServices(existing.Namespace).UpdateStatus(existing) + if !apierrs.IsConflict(err) { + return existing, err + } } - logger.Debugf("StatusDiff: %s", cmp.Diff(original.Status, sks.Status)) - original = original.DeepCopy() - original.Status = sks.Status - return r.ServingClientSet.NetworkingV1alpha1().ServerlessServices(sks.Namespace).UpdateStatus(original) + return nil, err } func (r *reconciler) reconcilePublicService(ctx context.Context, sks *netv1alpha1.ServerlessService) error { diff --git a/pkg/reconciler/service/service.go b/pkg/reconciler/service/service.go index dcf831cf438b..deb60d6a731f 100644 --- a/pkg/reconciler/service/service.go +++ b/pkg/reconciler/service/service.go @@ -274,27 +274,38 @@ func (c *Reconciler) checkRoutesNotReady(config *v1alpha1.Configuration, logger } func (c *Reconciler) updateStatus(desired *v1alpha1.Service, logger *zap.SugaredLogger) (*v1alpha1.Service, error) { - service, err := c.ServingClientSet.ServingV1alpha1().Services(desired.Namespace).Get(desired.Name, metav1.GetOptions{}) - if err != nil { - return nil, err - } - // If there's nothing to update, just return. - if reflect.DeepEqual(service.Status, desired.Status) { - return service, nil - } - becomesReady := desired.Status.IsReady() && !service.Status.IsReady() - // Don't modify the informers copy. - existing := service.DeepCopy() - existing.Status = desired.Status - - svc, err := c.ServingClientSet.ServingV1alpha1().Services(desired.Namespace).UpdateStatus(existing) - if err == nil && becomesReady { - duration := time.Since(svc.ObjectMeta.CreationTimestamp.Time) - logger.Infof("Service became ready after %v", duration) - c.StatsReporter.ReportServiceReady(service.Namespace, service.Name, duration) - } + var err error + for i := 0; i < 4; i++ { + // The first iteration tries to use the informer's state. + var existing *v1alpha1.Service + if i == 0 { + existing, err = c.serviceLister.Services(desired.Namespace).Get(desired.Name) + existing = existing.DeepCopy() + } else { + existing, err = c.ServingClientSet.ServingV1alpha1().Services(desired.Namespace).Get(desired.Name, metav1.GetOptions{}) + } + if err != nil { + return nil, err + } + + // If there's nothing to update, just return. + if reflect.DeepEqual(existing.Status, desired.Status) { + return existing, nil + } - return svc, err + becomesReady := desired.Status.IsReady() && !existing.Status.IsReady() + existing.Status = desired.Status + existing, err = c.ServingClientSet.ServingV1alpha1().Services(desired.Namespace).UpdateStatus(existing) + if err == nil && becomesReady { + duration := time.Since(existing.ObjectMeta.CreationTimestamp.Time) + logger.Infof("Service became ready after %v", duration) + c.StatsReporter.ReportServiceReady(existing.Namespace, existing.Name, duration) + } + if !apierrs.IsConflict(err) { + return existing, err + } + } + return nil, err } func (c *Reconciler) createConfiguration(service *v1alpha1.Service) (*v1alpha1.Configuration, error) { From 81e13ab3cf094b66910746ae564cfebb90097127 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Markus=20Th=C3=B6mmes?= Date: Mon, 9 Dec 2019 12:04:27 +0100 Subject: [PATCH 3/8] Use common retry function. --- pkg/reconciler/autoscaling/hpa/hpa.go | 2 +- pkg/reconciler/autoscaling/kpa/kpa.go | 2 +- pkg/reconciler/autoscaling/reconciler.go | 23 +++++++------ pkg/reconciler/certificate/certificate.go | 25 +++++++-------- pkg/reconciler/configuration/configuration.go | 25 +++++++-------- pkg/reconciler/ingress/ingress.go | 25 +++++++-------- pkg/reconciler/metric/metric.go | 25 +++++++-------- pkg/reconciler/retry.go | 32 +++++++++++++++++++ pkg/reconciler/revision/revision.go | 25 +++++++-------- pkg/reconciler/route/reconcile_resources.go | 24 +++++++------- pkg/reconciler/route/route.go | 2 +- .../serverlessservice/serverlessservice.go | 25 +++++++-------- pkg/reconciler/service/service.go | 25 +++++++-------- 13 files changed, 142 insertions(+), 118 deletions(-) create mode 100644 pkg/reconciler/retry.go diff --git a/pkg/reconciler/autoscaling/hpa/hpa.go b/pkg/reconciler/autoscaling/hpa/hpa.go index cd8724a871bf..c4cc209176fd 100644 --- a/pkg/reconciler/autoscaling/hpa/hpa.go +++ b/pkg/reconciler/autoscaling/hpa/hpa.go @@ -78,7 +78,7 @@ func (c *Reconciler) Reconcile(ctx context.Context, key string) error { // This is important because the copy we loaded from the informer's // cache may be stale and we don't want to overwrite a prior update // to status with this stale state. - } else if _, err = c.UpdateStatus(pa); err != nil { + } else if err = c.UpdateStatus(pa); err != nil { logger.Warnw("Failed to update pa status", zap.Error(err)) c.Recorder.Eventf(pa, corev1.EventTypeWarning, "UpdateFailed", "Failed to update status for PA %q: %v", pa.Name, err) diff --git a/pkg/reconciler/autoscaling/kpa/kpa.go b/pkg/reconciler/autoscaling/kpa/kpa.go index e57f3cda267c..ab4ebbb69e22 100644 --- a/pkg/reconciler/autoscaling/kpa/kpa.go +++ b/pkg/reconciler/autoscaling/kpa/kpa.go @@ -89,7 +89,7 @@ func (c *Reconciler) Reconcile(ctx context.Context, key string) error { // This is important because the copy we loaded from the informer's // cache may be stale and we don't want to overwrite a prior update // to status with this stale state. - } else if _, err = c.UpdateStatus(pa); err != nil { + } else if err = c.UpdateStatus(pa); err != nil { logger.Warnw("Failed to update pa status", zap.Error(err)) c.Recorder.Eventf(pa, corev1.EventTypeWarning, "UpdateFailed", "Failed to update status for PA %q: %v", pa.Name, err) diff --git a/pkg/reconciler/autoscaling/reconciler.go b/pkg/reconciler/autoscaling/reconciler.go index 813ec468b216..86f492416eb3 100644 --- a/pkg/reconciler/autoscaling/reconciler.go +++ b/pkg/reconciler/autoscaling/reconciler.go @@ -138,11 +138,13 @@ func (c *Base) ReconcileMetric(ctx context.Context, pa *pav1alpha1.PodAutoscaler } // UpdateStatus updates the status of the given PodAutoscaler. -func (c *Base) UpdateStatus(desired *pav1alpha1.PodAutoscaler) (*pav1alpha1.PodAutoscaler, error) { - var err error - for i := 0; i < 4; i++ { +func (c *Base) UpdateStatus(desired *pav1alpha1.PodAutoscaler) error { + return reconciler.RetryUpdateConflicts(func(i int) error { + var ( + existing *pav1alpha1.PodAutoscaler + err error + ) // The first iteration tries to use the informer's state. - var existing *pav1alpha1.PodAutoscaler if i == 0 { existing, err = c.PALister.PodAutoscalers(desired.Namespace).Get(desired.Name) existing = existing.DeepCopy() @@ -150,19 +152,16 @@ func (c *Base) UpdateStatus(desired *pav1alpha1.PodAutoscaler) (*pav1alpha1.PodA existing, err = c.ServingClientSet.AutoscalingV1alpha1().PodAutoscalers(desired.Namespace).Get(desired.Name, metav1.GetOptions{}) } if err != nil { - return nil, err + return err } // If there's nothing to update, just return. if reflect.DeepEqual(existing.Status, desired.Status) { - return existing, nil + return nil } existing.Status = desired.Status - existing, err = c.ServingClientSet.AutoscalingV1alpha1().PodAutoscalers(existing.Namespace).UpdateStatus(existing) - if !errors.IsConflict(err) { - return existing, err - } - } - return nil, err + _, err = c.ServingClientSet.AutoscalingV1alpha1().PodAutoscalers(existing.Namespace).UpdateStatus(existing) + return err + }) } diff --git a/pkg/reconciler/certificate/certificate.go b/pkg/reconciler/certificate/certificate.go index 51c6da4df5f2..868049034134 100644 --- a/pkg/reconciler/certificate/certificate.go +++ b/pkg/reconciler/certificate/certificate.go @@ -100,7 +100,7 @@ func (c *Reconciler) Reconcile(ctx context.Context, key string) error { // This is important because the copy we loaded from the informer's // cache may be stale and we don't want to overwrite a prior update // to status with this stale state. - } else if _, err := c.updateStatus(knCert); err != nil { + } else if err := c.updateStatus(knCert); err != nil { logger.Warnw("Failed to update certificate status", zap.Error(err)) c.Recorder.Eventf(knCert, corev1.EventTypeWarning, "UpdateFailed", "Failed to update status for Certificate %s: %v", key, err) @@ -173,11 +173,13 @@ func (c *Reconciler) reconcileCMCertificate(ctx context.Context, knCert *v1alpha return cmCert, nil } -func (c *Reconciler) updateStatus(desired *v1alpha1.Certificate) (*v1alpha1.Certificate, error) { - var err error - for i := 0; i < 4; i++ { +func (c *Reconciler) updateStatus(desired *v1alpha1.Certificate) error { + return reconciler.RetryUpdateConflicts(func(i int) error { + var ( + existing *v1alpha1.Certificate + err error + ) // The first iteration tries to use the informer's state. - var existing *v1alpha1.Certificate if i == 0 { existing, err = c.knCertificateLister.Certificates(desired.Namespace).Get(desired.Name) existing = existing.DeepCopy() @@ -185,19 +187,16 @@ func (c *Reconciler) updateStatus(desired *v1alpha1.Certificate) (*v1alpha1.Cert existing, err = c.ServingClientSet.NetworkingV1alpha1().Certificates(desired.Namespace).Get(desired.Name, metav1.GetOptions{}) } if err != nil { - return nil, err + return err } // If there's nothing to update, just return. if reflect.DeepEqual(existing.Status, desired.Status) { - return existing, nil + return nil } existing.Status = desired.Status - existing, err = c.ServingClientSet.NetworkingV1alpha1().Certificates(existing.Namespace).UpdateStatus(existing) - if !apierrs.IsConflict(err) { - return existing, err - } - } - return nil, err + _, err = c.ServingClientSet.NetworkingV1alpha1().Certificates(existing.Namespace).UpdateStatus(existing) + return err + }) } diff --git a/pkg/reconciler/configuration/configuration.go b/pkg/reconciler/configuration/configuration.go index 0729395c3ab5..f58ac7d13dde 100644 --- a/pkg/reconciler/configuration/configuration.go +++ b/pkg/reconciler/configuration/configuration.go @@ -87,7 +87,7 @@ func (c *Reconciler) Reconcile(ctx context.Context, key string) error { // This is important because the copy we loaded from the informer's // cache may be stale and we don't want to overwrite a prior update // to status with this stale state. - } else if _, err = c.updateStatus(config); err != nil { + } else if err = c.updateStatus(config); err != nil { logger.Warnw("Failed to update configuration status", zap.Error(err)) c.Recorder.Eventf(config, corev1.EventTypeWarning, "UpdateFailed", "Failed to update status: %v", err) return err @@ -341,11 +341,13 @@ func (c *Reconciler) createRevision(ctx context.Context, config *v1alpha1.Config return created, nil } -func (c *Reconciler) updateStatus(desired *v1alpha1.Configuration) (*v1alpha1.Configuration, error) { - var err error - for i := 0; i < 4; i++ { +func (c *Reconciler) updateStatus(desired *v1alpha1.Configuration) error { + return reconciler.RetryUpdateConflicts(func(i int) error { + var ( + existing *v1alpha1.Configuration + err error + ) // The first iteration tries to use the informer's state. - var existing *v1alpha1.Configuration if i == 0 { existing, err = c.configurationLister.Configurations(desired.Namespace).Get(desired.Name) existing = existing.DeepCopy() @@ -353,19 +355,16 @@ func (c *Reconciler) updateStatus(desired *v1alpha1.Configuration) (*v1alpha1.Co existing, err = c.ServingClientSet.ServingV1alpha1().Configurations(desired.Namespace).Get(desired.Name, metav1.GetOptions{}) } if err != nil { - return nil, err + return err } // If there's nothing to update, just return. if reflect.DeepEqual(existing.Status, desired.Status) { - return existing, nil + return nil } existing.Status = desired.Status - existing, err = c.ServingClientSet.ServingV1alpha1().Configurations(desired.Namespace).UpdateStatus(existing) - if !errors.IsConflict(err) { - return existing, err - } - } - return nil, err + _, err = c.ServingClientSet.ServingV1alpha1().Configurations(desired.Namespace).UpdateStatus(existing) + return err + }) } diff --git a/pkg/reconciler/ingress/ingress.go b/pkg/reconciler/ingress/ingress.go index 96e752874399..fd96b30373e7 100644 --- a/pkg/reconciler/ingress/ingress.go +++ b/pkg/reconciler/ingress/ingress.go @@ -130,7 +130,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, key string) error { // cache may be stale and we don't want to overwrite a prior update // to status with this stale state. } else { - if _, err = r.updateStatus(ingress); err != nil { + if err = r.updateStatus(ingress); err != nil { logger.Warnw("Failed to update Ingress status", zap.Error(err)) r.Recorder.Eventf(ingress, corev1.EventTypeWarning, "UpdateFailed", "Failed to update status for Ingress %q: %v", ingress.GetName(), err) @@ -306,11 +306,13 @@ func (r *Reconciler) reconcileDeletion(ctx context.Context, ia *v1alpha1.Ingress // Update the Status of the Ingress. Caller is responsible for checking // for semantic differences before calling. -func (r *Reconciler) updateStatus(desired *v1alpha1.Ingress) (*v1alpha1.Ingress, error) { - var err error - for i := 0; i < 4; i++ { +func (r *Reconciler) updateStatus(desired *v1alpha1.Ingress) error { + return reconciler.RetryUpdateConflicts(func(i int) error { + var ( + existing *v1alpha1.Ingress + err error + ) // The first iteration tries to use the informer's state. - var existing *v1alpha1.Ingress if i == 0 { existing, err = r.ingressLister.Ingresses(desired.Namespace).Get(desired.Name) existing = existing.DeepCopy() @@ -318,21 +320,18 @@ func (r *Reconciler) updateStatus(desired *v1alpha1.Ingress) (*v1alpha1.Ingress, existing, err = r.ServingClientSet.NetworkingV1alpha1().Ingresses(desired.GetNamespace()).Get(desired.GetName(), metav1.GetOptions{}) } if err != nil { - return nil, err + return err } // If there's nothing to update, just return. if reflect.DeepEqual(existing.Status, desired.Status) { - return existing, nil + return nil } existing.Status = desired.Status - existing, err = r.ServingClientSet.NetworkingV1alpha1().Ingresses(existing.GetNamespace()).UpdateStatus(existing) - if !apierrs.IsConflict(err) { - return existing, err - } - } - return nil, err + _, err = r.ServingClientSet.NetworkingV1alpha1().Ingresses(existing.GetNamespace()).UpdateStatus(existing) + return err + }) } func (r *Reconciler) ensureFinalizer(ia *v1alpha1.Ingress) error { diff --git a/pkg/reconciler/metric/metric.go b/pkg/reconciler/metric/metric.go index 028aa5345b5f..734cd33203df 100644 --- a/pkg/reconciler/metric/metric.go +++ b/pkg/reconciler/metric/metric.go @@ -83,7 +83,7 @@ func (r *reconciler) Reconcile(ctx context.Context, key string) error { if !equality.Semantic.DeepEqual(original.Status, metric.Status) { // Change of status, need to update the object. - if _, uErr := r.updateStatus(metric); uErr != nil { + if uErr := r.updateStatus(metric); uErr != nil { logger.Warnw("Failed to update metric status", zap.Error(uErr)) r.Recorder.Eventf(metric, corev1.EventTypeWarning, "UpdateFailed", "Failed to update metric status: %v", uErr) @@ -104,11 +104,13 @@ func (r *reconciler) reconcileCollection(ctx context.Context, metric *v1alpha1.M return nil } -func (r *reconciler) updateStatus(desired *v1alpha1.Metric) (*v1alpha1.Metric, error) { - var err error - for i := 0; i < 4; i++ { +func (r *reconciler) updateStatus(desired *v1alpha1.Metric) error { + return rbase.RetryUpdateConflicts(func(i int) error { + var ( + existing *v1alpha1.Metric + err error + ) // The first iteration tries to use the informer's state. - var existing *v1alpha1.Metric if i == 0 { existing, err = r.metricLister.Metrics(desired.Namespace).Get(desired.Name) existing = existing.DeepCopy() @@ -116,19 +118,16 @@ func (r *reconciler) updateStatus(desired *v1alpha1.Metric) (*v1alpha1.Metric, e existing, err = r.ServingClientSet.AutoscalingV1alpha1().Metrics(desired.Namespace).Get(desired.Name, metav1.GetOptions{}) } if err != nil { - return nil, err + return err } // If there's nothing to update, just return. if reflect.DeepEqual(existing.Status, desired.Status) { - return existing, nil + return nil } existing.Status = desired.Status - existing, err = r.ServingClientSet.AutoscalingV1alpha1().Metrics(existing.Namespace).UpdateStatus(existing) - if !apierrs.IsConflict(err) { - return existing, err - } - } - return nil, err + _, err = r.ServingClientSet.AutoscalingV1alpha1().Metrics(existing.Namespace).UpdateStatus(existing) + return err + }) } diff --git a/pkg/reconciler/retry.go b/pkg/reconciler/retry.go new file mode 100644 index 000000000000..67a7aa540911 --- /dev/null +++ b/pkg/reconciler/retry.go @@ -0,0 +1,32 @@ +/* +Copyright 2018 The Knative Authors + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + https://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package reconciler + +import apierrs "k8s.io/apimachinery/pkg/api/errors" + +// RetryUpdateConflicts retries the inner function if it returns conflict errors. +// This can be used to retry status updates without constantly reenqueuing keys. +func RetryUpdateConflicts(updater func(int) error) error { + var err error + for i := 0; i < 4; i++ { + err = updater(i) + if err == nil || !apierrs.IsConflict(err) { + return err + } + } + return err +} diff --git a/pkg/reconciler/revision/revision.go b/pkg/reconciler/revision/revision.go index 65d604764b81..56e9fb8d5a1e 100644 --- a/pkg/reconciler/revision/revision.go +++ b/pkg/reconciler/revision/revision.go @@ -105,7 +105,7 @@ func (c *Reconciler) Reconcile(ctx context.Context, key string) error { // This is important because the copy we loaded from the informer's // cache may be stale and we don't want to overwrite a prior update // to status with this stale state. - } else if _, err = c.updateStatus(rev); err != nil { + } else if err = c.updateStatus(rev); err != nil { logger.Warnw("Failed to update revision status", zap.Error(err)) c.Recorder.Eventf(rev, corev1.EventTypeWarning, "UpdateFailed", "Failed to update status for Revision %q: %v", rev.Name, err) @@ -232,11 +232,13 @@ func (c *Reconciler) updateRevisionLoggingURL( "${REVISION_UID}", uid, -1) } -func (c *Reconciler) updateStatus(desired *v1alpha1.Revision) (*v1alpha1.Revision, error) { - var err error - for i := 0; i < 4; i++ { +func (c *Reconciler) updateStatus(desired *v1alpha1.Revision) error { + return reconciler.RetryUpdateConflicts(func(i int) error { + var ( + existing *v1alpha1.Revision + err error + ) // The first iteration tries to use the informer's state. - var existing *v1alpha1.Revision if i == 0 { existing, err = c.revisionLister.Revisions(desired.Namespace).Get(desired.Name) existing = existing.DeepCopy() @@ -244,19 +246,16 @@ func (c *Reconciler) updateStatus(desired *v1alpha1.Revision) (*v1alpha1.Revisio existing, err = c.ServingClientSet.ServingV1alpha1().Revisions(desired.Namespace).Get(desired.Name, metav1.GetOptions{}) } if err != nil { - return nil, err + return err } // If there's nothing to update, just return. if reflect.DeepEqual(existing.Status, desired.Status) { - return existing, nil + return nil } existing.Status = desired.Status - existing, err = c.ServingClientSet.ServingV1alpha1().Revisions(desired.Namespace).UpdateStatus(existing) - if !apierrs.IsConflict(err) { - return existing, err - } - } - return nil, err + _, err = c.ServingClientSet.ServingV1alpha1().Revisions(desired.Namespace).UpdateStatus(existing) + return err + }) } diff --git a/pkg/reconciler/route/reconcile_resources.go b/pkg/reconciler/route/reconcile_resources.go index 8515a6803da3..979402f8d851 100644 --- a/pkg/reconciler/route/reconcile_resources.go +++ b/pkg/reconciler/route/reconcile_resources.go @@ -36,6 +36,7 @@ import ( netv1alpha1 "knative.dev/serving/pkg/apis/networking/v1alpha1" "knative.dev/serving/pkg/apis/serving" "knative.dev/serving/pkg/apis/serving/v1alpha1" + "knative.dev/serving/pkg/reconciler" "knative.dev/serving/pkg/reconciler/route/config" "knative.dev/serving/pkg/reconciler/route/resources" "knative.dev/serving/pkg/reconciler/route/traffic" @@ -187,11 +188,13 @@ func (c *Reconciler) updatePlaceholderServices(ctx context.Context, route *v1alp return eg.Wait() } -func (c *Reconciler) updateStatus(desired *v1alpha1.Route) (*v1alpha1.Route, error) { - var err error - for i := 0; i < 4; i++ { +func (c *Reconciler) updateStatus(desired *v1alpha1.Route) error { + return reconciler.RetryUpdateConflicts(func(i int) error { + var ( + existing *v1alpha1.Route + err error + ) // The first iteration tries to use the informer's state. - var existing *v1alpha1.Route if i == 0 { existing, err = c.routeLister.Routes(desired.Namespace).Get(desired.Name) existing = existing.DeepCopy() @@ -199,21 +202,18 @@ func (c *Reconciler) updateStatus(desired *v1alpha1.Route) (*v1alpha1.Route, err existing, err = c.ServingClientSet.ServingV1alpha1().Routes(desired.Namespace).Get(desired.Name, metav1.GetOptions{}) } if err != nil { - return nil, err + return err } // If there's nothing to update, just return. if reflect.DeepEqual(existing.Status, desired.Status) { - return existing, nil + return nil } existing.Status = desired.Status - existing, err = c.ServingClientSet.ServingV1alpha1().Routes(desired.Namespace).UpdateStatus(existing) - if !apierrs.IsConflict(err) { - return existing, err - } - } - return nil, err + _, err = c.ServingClientSet.ServingV1alpha1().Routes(desired.Namespace).UpdateStatus(existing) + return err + }) } // Update the lastPinned annotation on revisions we target so they don't get GC'd. diff --git a/pkg/reconciler/route/route.go b/pkg/reconciler/route/route.go index 93a0f0c8b3f9..a6cc050f0c5e 100644 --- a/pkg/reconciler/route/route.go +++ b/pkg/reconciler/route/route.go @@ -120,7 +120,7 @@ func (c *Reconciler) Reconcile(ctx context.Context, key string) error { // This is important because the copy we loaded from the informer's // cache may be stale and we don't want to overwrite a prior update // to status with this stale state. - } else if _, err = c.updateStatus(route); err != nil { + } else if err = c.updateStatus(route); err != nil { logger.Warnw("Failed to update route status", zap.Error(err)) c.Recorder.Eventf(route, corev1.EventTypeWarning, "UpdateFailed", "Failed to update status for Route %q: %v", route.Name, err) diff --git a/pkg/reconciler/serverlessservice/serverlessservice.go b/pkg/reconciler/serverlessservice/serverlessservice.go index f3129691b984..20c397bd1c81 100644 --- a/pkg/reconciler/serverlessservice/serverlessservice.go +++ b/pkg/reconciler/serverlessservice/serverlessservice.go @@ -94,7 +94,7 @@ func (r *reconciler) Reconcile(ctx context.Context, key string) error { r.Recorder.Eventf(sks, corev1.EventTypeWarning, "UpdateFailed", "InternalError: %v", reconcileErr.Error()) } if !equality.Semantic.DeepEqual(sks.Status, original.Status) { - if _, err := r.updateStatus(sks); err != nil { + if err := r.updateStatus(sks); err != nil { r.Recorder.Eventf(sks, corev1.EventTypeWarning, "UpdateFailed", "Failed to update status: %v", err) return err } @@ -127,11 +127,13 @@ func (r *reconciler) reconcile(ctx context.Context, sks *netv1alpha1.ServerlessS return nil } -func (r *reconciler) updateStatus(desired *netv1alpha1.ServerlessService) (*netv1alpha1.ServerlessService, error) { - var err error - for i := 0; i < 4; i++ { +func (r *reconciler) updateStatus(desired *netv1alpha1.ServerlessService) error { + return rbase.RetryUpdateConflicts(func(i int) error { + var ( + existing *netv1alpha1.ServerlessService + err error + ) // The first iteration tries to use the informer's state. - var existing *netv1alpha1.ServerlessService if i == 0 { existing, err = r.sksLister.ServerlessServices(desired.Namespace).Get(desired.Name) existing = existing.DeepCopy() @@ -139,21 +141,18 @@ func (r *reconciler) updateStatus(desired *netv1alpha1.ServerlessService) (*netv existing, err = r.ServingClientSet.NetworkingV1alpha1().ServerlessServices(desired.Namespace).Get(desired.Name, metav1.GetOptions{}) } if err != nil { - return nil, err + return err } // If there's nothing to update, just return. if reflect.DeepEqual(existing.Status, desired.Status) { - return existing, nil + return nil } existing.Status = desired.Status - existing, err = r.ServingClientSet.NetworkingV1alpha1().ServerlessServices(existing.Namespace).UpdateStatus(existing) - if !apierrs.IsConflict(err) { - return existing, err - } - } - return nil, err + _, err = r.ServingClientSet.NetworkingV1alpha1().ServerlessServices(existing.Namespace).UpdateStatus(existing) + return err + }) } func (r *reconciler) reconcilePublicService(ctx context.Context, sks *netv1alpha1.ServerlessService) error { diff --git a/pkg/reconciler/service/service.go b/pkg/reconciler/service/service.go index deb60d6a731f..9ce969c3425b 100644 --- a/pkg/reconciler/service/service.go +++ b/pkg/reconciler/service/service.go @@ -101,7 +101,7 @@ func (c *Reconciler) Reconcile(ctx context.Context, key string) error { // cache may be stale and we don't want to overwrite a prior update // to status with this stale state. - } else if _, uErr := c.updateStatus(service, logger); uErr != nil { + } else if uErr := c.updateStatus(service, logger); uErr != nil { logger.Warnw("Failed to update service status", zap.Error(uErr)) c.Recorder.Eventf(service, corev1.EventTypeWarning, "UpdateFailed", "Failed to update status for Service %q: %v", service.Name, uErr) @@ -273,11 +273,13 @@ func (c *Reconciler) checkRoutesNotReady(config *v1alpha1.Configuration, logger } } -func (c *Reconciler) updateStatus(desired *v1alpha1.Service, logger *zap.SugaredLogger) (*v1alpha1.Service, error) { - var err error - for i := 0; i < 4; i++ { +func (c *Reconciler) updateStatus(desired *v1alpha1.Service, logger *zap.SugaredLogger) error { + return reconciler.RetryUpdateConflicts(func(i int) error { + var ( + existing *v1alpha1.Service + err error + ) // The first iteration tries to use the informer's state. - var existing *v1alpha1.Service if i == 0 { existing, err = c.serviceLister.Services(desired.Namespace).Get(desired.Name) existing = existing.DeepCopy() @@ -285,27 +287,24 @@ func (c *Reconciler) updateStatus(desired *v1alpha1.Service, logger *zap.Sugared existing, err = c.ServingClientSet.ServingV1alpha1().Services(desired.Namespace).Get(desired.Name, metav1.GetOptions{}) } if err != nil { - return nil, err + return err } // If there's nothing to update, just return. if reflect.DeepEqual(existing.Status, desired.Status) { - return existing, nil + return nil } becomesReady := desired.Status.IsReady() && !existing.Status.IsReady() existing.Status = desired.Status - existing, err = c.ServingClientSet.ServingV1alpha1().Services(desired.Namespace).UpdateStatus(existing) + _, err = c.ServingClientSet.ServingV1alpha1().Services(desired.Namespace).UpdateStatus(existing) if err == nil && becomesReady { duration := time.Since(existing.ObjectMeta.CreationTimestamp.Time) logger.Infof("Service became ready after %v", duration) c.StatsReporter.ReportServiceReady(existing.Namespace, existing.Name, duration) } - if !apierrs.IsConflict(err) { - return existing, err - } - } - return nil, err + return err + }) } func (c *Reconciler) createConfiguration(service *v1alpha1.Service) (*v1alpha1.Configuration, error) { From afaecc31c5cd18c54367516801d460660139ecd1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Markus=20Th=C3=B6mmes?= Date: Mon, 9 Dec 2019 16:56:40 +0100 Subject: [PATCH 4/8] Review comments. --- pkg/reconciler/autoscaling/reconciler.go | 4 ++-- pkg/reconciler/certificate/certificate.go | 4 ++-- pkg/reconciler/configuration/configuration.go | 4 ++-- pkg/reconciler/ingress/ingress.go | 4 ++-- pkg/reconciler/metric/metric.go | 4 ++-- pkg/reconciler/retry.go | 6 +++--- pkg/reconciler/revision/revision.go | 4 ++-- pkg/reconciler/route/reconcile_resources.go | 4 ++-- pkg/reconciler/serverlessservice/serverlessservice.go | 4 ++-- pkg/reconciler/service/service.go | 4 ++-- 10 files changed, 21 insertions(+), 21 deletions(-) diff --git a/pkg/reconciler/autoscaling/reconciler.go b/pkg/reconciler/autoscaling/reconciler.go index 86f492416eb3..08e740018020 100644 --- a/pkg/reconciler/autoscaling/reconciler.go +++ b/pkg/reconciler/autoscaling/reconciler.go @@ -139,13 +139,13 @@ func (c *Base) ReconcileMetric(ctx context.Context, pa *pav1alpha1.PodAutoscaler // UpdateStatus updates the status of the given PodAutoscaler. func (c *Base) UpdateStatus(desired *pav1alpha1.PodAutoscaler) error { - return reconciler.RetryUpdateConflicts(func(i int) error { + return reconciler.RetryUpdateConflicts(func(attempts int) error { var ( existing *pav1alpha1.PodAutoscaler err error ) // The first iteration tries to use the informer's state. - if i == 0 { + if attempts == 0 { existing, err = c.PALister.PodAutoscalers(desired.Namespace).Get(desired.Name) existing = existing.DeepCopy() } else { diff --git a/pkg/reconciler/certificate/certificate.go b/pkg/reconciler/certificate/certificate.go index 868049034134..42de782a16ab 100644 --- a/pkg/reconciler/certificate/certificate.go +++ b/pkg/reconciler/certificate/certificate.go @@ -174,13 +174,13 @@ func (c *Reconciler) reconcileCMCertificate(ctx context.Context, knCert *v1alpha } func (c *Reconciler) updateStatus(desired *v1alpha1.Certificate) error { - return reconciler.RetryUpdateConflicts(func(i int) error { + return reconciler.RetryUpdateConflicts(func(attempts int) error { var ( existing *v1alpha1.Certificate err error ) // The first iteration tries to use the informer's state. - if i == 0 { + if attempts == 0 { existing, err = c.knCertificateLister.Certificates(desired.Namespace).Get(desired.Name) existing = existing.DeepCopy() } else { diff --git a/pkg/reconciler/configuration/configuration.go b/pkg/reconciler/configuration/configuration.go index f58ac7d13dde..d06e924e80ac 100644 --- a/pkg/reconciler/configuration/configuration.go +++ b/pkg/reconciler/configuration/configuration.go @@ -342,13 +342,13 @@ func (c *Reconciler) createRevision(ctx context.Context, config *v1alpha1.Config } func (c *Reconciler) updateStatus(desired *v1alpha1.Configuration) error { - return reconciler.RetryUpdateConflicts(func(i int) error { + return reconciler.RetryUpdateConflicts(func(attempts int) error { var ( existing *v1alpha1.Configuration err error ) // The first iteration tries to use the informer's state. - if i == 0 { + if attempts == 0 { existing, err = c.configurationLister.Configurations(desired.Namespace).Get(desired.Name) existing = existing.DeepCopy() } else { diff --git a/pkg/reconciler/ingress/ingress.go b/pkg/reconciler/ingress/ingress.go index fd96b30373e7..75c1a9cc2144 100644 --- a/pkg/reconciler/ingress/ingress.go +++ b/pkg/reconciler/ingress/ingress.go @@ -307,13 +307,13 @@ func (r *Reconciler) reconcileDeletion(ctx context.Context, ia *v1alpha1.Ingress // Update the Status of the Ingress. Caller is responsible for checking // for semantic differences before calling. func (r *Reconciler) updateStatus(desired *v1alpha1.Ingress) error { - return reconciler.RetryUpdateConflicts(func(i int) error { + return reconciler.RetryUpdateConflicts(func(attempts int) error { var ( existing *v1alpha1.Ingress err error ) // The first iteration tries to use the informer's state. - if i == 0 { + if attempts == 0 { existing, err = r.ingressLister.Ingresses(desired.Namespace).Get(desired.Name) existing = existing.DeepCopy() } else { diff --git a/pkg/reconciler/metric/metric.go b/pkg/reconciler/metric/metric.go index 734cd33203df..74b3a62bae3d 100644 --- a/pkg/reconciler/metric/metric.go +++ b/pkg/reconciler/metric/metric.go @@ -105,13 +105,13 @@ func (r *reconciler) reconcileCollection(ctx context.Context, metric *v1alpha1.M } func (r *reconciler) updateStatus(desired *v1alpha1.Metric) error { - return rbase.RetryUpdateConflicts(func(i int) error { + return rbase.RetryUpdateConflicts(func(attempts int) error { var ( existing *v1alpha1.Metric err error ) // The first iteration tries to use the informer's state. - if i == 0 { + if attempts == 0 { existing, err = r.metricLister.Metrics(desired.Namespace).Get(desired.Name) existing = existing.DeepCopy() } else { diff --git a/pkg/reconciler/retry.go b/pkg/reconciler/retry.go index 67a7aa540911..9caea3d73fe8 100644 --- a/pkg/reconciler/retry.go +++ b/pkg/reconciler/retry.go @@ -1,5 +1,5 @@ /* -Copyright 2018 The Knative Authors +Copyright 2019 The Knative Authors Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. @@ -22,8 +22,8 @@ import apierrs "k8s.io/apimachinery/pkg/api/errors" // This can be used to retry status updates without constantly reenqueuing keys. func RetryUpdateConflicts(updater func(int) error) error { var err error - for i := 0; i < 4; i++ { - err = updater(i) + for attempts := 0; attempts < 4; attempts++ { + err = updater(attempts) if err == nil || !apierrs.IsConflict(err) { return err } diff --git a/pkg/reconciler/revision/revision.go b/pkg/reconciler/revision/revision.go index 56e9fb8d5a1e..7c18bb3f15f1 100644 --- a/pkg/reconciler/revision/revision.go +++ b/pkg/reconciler/revision/revision.go @@ -233,13 +233,13 @@ func (c *Reconciler) updateRevisionLoggingURL( } func (c *Reconciler) updateStatus(desired *v1alpha1.Revision) error { - return reconciler.RetryUpdateConflicts(func(i int) error { + return reconciler.RetryUpdateConflicts(func(attempts int) error { var ( existing *v1alpha1.Revision err error ) // The first iteration tries to use the informer's state. - if i == 0 { + if attempts == 0 { existing, err = c.revisionLister.Revisions(desired.Namespace).Get(desired.Name) existing = existing.DeepCopy() } else { diff --git a/pkg/reconciler/route/reconcile_resources.go b/pkg/reconciler/route/reconcile_resources.go index 979402f8d851..04b09d230fbb 100644 --- a/pkg/reconciler/route/reconcile_resources.go +++ b/pkg/reconciler/route/reconcile_resources.go @@ -189,13 +189,13 @@ func (c *Reconciler) updatePlaceholderServices(ctx context.Context, route *v1alp } func (c *Reconciler) updateStatus(desired *v1alpha1.Route) error { - return reconciler.RetryUpdateConflicts(func(i int) error { + return reconciler.RetryUpdateConflicts(func(attempts int) error { var ( existing *v1alpha1.Route err error ) // The first iteration tries to use the informer's state. - if i == 0 { + if attempts == 0 { existing, err = c.routeLister.Routes(desired.Namespace).Get(desired.Name) existing = existing.DeepCopy() } else { diff --git a/pkg/reconciler/serverlessservice/serverlessservice.go b/pkg/reconciler/serverlessservice/serverlessservice.go index 20c397bd1c81..d733656ccf2e 100644 --- a/pkg/reconciler/serverlessservice/serverlessservice.go +++ b/pkg/reconciler/serverlessservice/serverlessservice.go @@ -128,13 +128,13 @@ func (r *reconciler) reconcile(ctx context.Context, sks *netv1alpha1.ServerlessS } func (r *reconciler) updateStatus(desired *netv1alpha1.ServerlessService) error { - return rbase.RetryUpdateConflicts(func(i int) error { + return rbase.RetryUpdateConflicts(func(attempts int) error { var ( existing *netv1alpha1.ServerlessService err error ) // The first iteration tries to use the informer's state. - if i == 0 { + if attempts == 0 { existing, err = r.sksLister.ServerlessServices(desired.Namespace).Get(desired.Name) existing = existing.DeepCopy() } else { diff --git a/pkg/reconciler/service/service.go b/pkg/reconciler/service/service.go index 9ce969c3425b..3e0f0f1488ea 100644 --- a/pkg/reconciler/service/service.go +++ b/pkg/reconciler/service/service.go @@ -274,13 +274,13 @@ func (c *Reconciler) checkRoutesNotReady(config *v1alpha1.Configuration, logger } func (c *Reconciler) updateStatus(desired *v1alpha1.Service, logger *zap.SugaredLogger) error { - return reconciler.RetryUpdateConflicts(func(i int) error { + return reconciler.RetryUpdateConflicts(func(attempts int) error { var ( existing *v1alpha1.Service err error ) // The first iteration tries to use the informer's state. - if i == 0 { + if attempts == 0 { existing, err = c.serviceLister.Services(desired.Namespace).Get(desired.Name) existing = existing.DeepCopy() } else { From 6f9e69c47338a9f0551731181352f20f598b2144 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Markus=20Th=C3=B6mmes?= Date: Mon, 9 Dec 2019 17:12:00 +0100 Subject: [PATCH 5/8] Add a test for the retrier. --- pkg/reconciler/retry_test.go | 80 ++++++++++++++++++++++++++++++++++++ 1 file changed, 80 insertions(+) create mode 100644 pkg/reconciler/retry_test.go diff --git a/pkg/reconciler/retry_test.go b/pkg/reconciler/retry_test.go new file mode 100644 index 000000000000..acee2dd8bdc5 --- /dev/null +++ b/pkg/reconciler/retry_test.go @@ -0,0 +1,80 @@ +/* +Copyright 2019 The Knative Authors + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package reconciler + +import ( + "errors" + "testing" + + apierrs "k8s.io/apimachinery/pkg/api/errors" + + v1alpha1 "knative.dev/serving/pkg/apis/autoscaling/v1alpha1" +) + +func TestRetryUpdateConflicts(t *testing.T) { + errAny := errors.New("foo") + errConflict := apierrs.NewConflict(v1alpha1.Resource("foo"), "bar", errAny) + + tests := []struct { + name string + returns []error + want error + wantAttempts int + }{{ + name: "all good", + returns: []error{nil}, + want: nil, + wantAttempts: 1, + }, { + name: "not retry on non-conflict error", + returns: []error{errAny}, + want: errAny, + wantAttempts: 1, + }, { + name: "retry up to 4 times on conflicts", + returns: []error{errConflict, errConflict, errConflict, errConflict, errConflict}, + want: errConflict, + wantAttempts: 4, + }, { + name: "eventually succeed", + returns: []error{errConflict, errConflict, nil}, + want: nil, + wantAttempts: 3, + }, { + name: "eventually fail", + returns: []error{errConflict, errConflict, errAny}, + want: errAny, + wantAttempts: 3, + }} + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + attempts := 0 + got := RetryUpdateConflicts(func(i int) error { + attempts++ + return test.returns[i] + }) + + if got != test.want { + t.Errorf("RetryUpdateConflicts() = %v, want %v", got, test.want) + } + if attempts != test.wantAttempts { + t.Errorf("attempts = %d, want %d", attempts, test.wantAttempts) + } + }) + } +} From a93051745c9720346f496b044486a1c900a1bdb8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Markus=20Th=C3=B6mmes?= Date: Mon, 9 Dec 2019 17:40:12 +0100 Subject: [PATCH 6/8] Use client-go helper for retries with default backoff. --- pkg/reconciler/retry.go | 18 +++++++++--------- pkg/reconciler/retry_test.go | 6 +++--- 2 files changed, 12 insertions(+), 12 deletions(-) diff --git a/pkg/reconciler/retry.go b/pkg/reconciler/retry.go index 9caea3d73fe8..26e99b74a40b 100644 --- a/pkg/reconciler/retry.go +++ b/pkg/reconciler/retry.go @@ -16,17 +16,17 @@ limitations under the License. package reconciler -import apierrs "k8s.io/apimachinery/pkg/api/errors" +import ( + "k8s.io/client-go/util/retry" +) // RetryUpdateConflicts retries the inner function if it returns conflict errors. // This can be used to retry status updates without constantly reenqueuing keys. func RetryUpdateConflicts(updater func(int) error) error { - var err error - for attempts := 0; attempts < 4; attempts++ { - err = updater(attempts) - if err == nil || !apierrs.IsConflict(err) { - return err - } - } - return err + attempts := 0 + return retry.RetryOnConflict(retry.DefaultRetry, func() error { + err := updater(attempts) + attempts++ + return err + }) } diff --git a/pkg/reconciler/retry_test.go b/pkg/reconciler/retry_test.go index acee2dd8bdc5..7166ac978b7a 100644 --- a/pkg/reconciler/retry_test.go +++ b/pkg/reconciler/retry_test.go @@ -45,10 +45,10 @@ func TestRetryUpdateConflicts(t *testing.T) { want: errAny, wantAttempts: 1, }, { - name: "retry up to 4 times on conflicts", - returns: []error{errConflict, errConflict, errConflict, errConflict, errConflict}, + name: "retry up to 5 times on conflicts", + returns: []error{errConflict, errConflict, errConflict, errConflict, errConflict, errConflict}, want: errConflict, - wantAttempts: 4, + wantAttempts: 5, }, { name: "eventually succeed", returns: []error{errConflict, errConflict, nil}, From 0e1b2b9376ea9f05d0e49212f84180328c75f531 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Markus=20Th=C3=B6mmes?= Date: Tue, 10 Dec 2019 10:34:03 +0100 Subject: [PATCH 7/8] Use named error to avoid another variable definition. --- pkg/reconciler/autoscaling/reconciler.go | 7 ++----- pkg/reconciler/certificate/certificate.go | 7 ++----- pkg/reconciler/configuration/configuration.go | 7 ++----- pkg/reconciler/ingress/ingress.go | 7 ++----- pkg/reconciler/metric/metric.go | 7 ++----- pkg/reconciler/revision/revision.go | 7 ++----- pkg/reconciler/route/reconcile_resources.go | 7 ++----- pkg/reconciler/serverlessservice/serverlessservice.go | 7 ++----- pkg/reconciler/service/service.go | 7 ++----- 9 files changed, 18 insertions(+), 45 deletions(-) diff --git a/pkg/reconciler/autoscaling/reconciler.go b/pkg/reconciler/autoscaling/reconciler.go index 08e740018020..541dcc18bea0 100644 --- a/pkg/reconciler/autoscaling/reconciler.go +++ b/pkg/reconciler/autoscaling/reconciler.go @@ -139,11 +139,8 @@ func (c *Base) ReconcileMetric(ctx context.Context, pa *pav1alpha1.PodAutoscaler // UpdateStatus updates the status of the given PodAutoscaler. func (c *Base) UpdateStatus(desired *pav1alpha1.PodAutoscaler) error { - return reconciler.RetryUpdateConflicts(func(attempts int) error { - var ( - existing *pav1alpha1.PodAutoscaler - err error - ) + return reconciler.RetryUpdateConflicts(func(attempts int) (err error) { + var existing *pav1alpha1.PodAutoscaler // The first iteration tries to use the informer's state. if attempts == 0 { existing, err = c.PALister.PodAutoscalers(desired.Namespace).Get(desired.Name) diff --git a/pkg/reconciler/certificate/certificate.go b/pkg/reconciler/certificate/certificate.go index 42de782a16ab..abfeb84acbaf 100644 --- a/pkg/reconciler/certificate/certificate.go +++ b/pkg/reconciler/certificate/certificate.go @@ -174,11 +174,8 @@ func (c *Reconciler) reconcileCMCertificate(ctx context.Context, knCert *v1alpha } func (c *Reconciler) updateStatus(desired *v1alpha1.Certificate) error { - return reconciler.RetryUpdateConflicts(func(attempts int) error { - var ( - existing *v1alpha1.Certificate - err error - ) + return reconciler.RetryUpdateConflicts(func(attempts int) (err error) { + var existing *v1alpha1.Certificate // The first iteration tries to use the informer's state. if attempts == 0 { existing, err = c.knCertificateLister.Certificates(desired.Namespace).Get(desired.Name) diff --git a/pkg/reconciler/configuration/configuration.go b/pkg/reconciler/configuration/configuration.go index d06e924e80ac..432fd236b876 100644 --- a/pkg/reconciler/configuration/configuration.go +++ b/pkg/reconciler/configuration/configuration.go @@ -342,11 +342,8 @@ func (c *Reconciler) createRevision(ctx context.Context, config *v1alpha1.Config } func (c *Reconciler) updateStatus(desired *v1alpha1.Configuration) error { - return reconciler.RetryUpdateConflicts(func(attempts int) error { - var ( - existing *v1alpha1.Configuration - err error - ) + return reconciler.RetryUpdateConflicts(func(attempts int) (err error) { + var existing *v1alpha1.Configuration // The first iteration tries to use the informer's state. if attempts == 0 { existing, err = c.configurationLister.Configurations(desired.Namespace).Get(desired.Name) diff --git a/pkg/reconciler/ingress/ingress.go b/pkg/reconciler/ingress/ingress.go index 75c1a9cc2144..c1d883636fe3 100644 --- a/pkg/reconciler/ingress/ingress.go +++ b/pkg/reconciler/ingress/ingress.go @@ -307,11 +307,8 @@ func (r *Reconciler) reconcileDeletion(ctx context.Context, ia *v1alpha1.Ingress // Update the Status of the Ingress. Caller is responsible for checking // for semantic differences before calling. func (r *Reconciler) updateStatus(desired *v1alpha1.Ingress) error { - return reconciler.RetryUpdateConflicts(func(attempts int) error { - var ( - existing *v1alpha1.Ingress - err error - ) + return reconciler.RetryUpdateConflicts(func(attempts int) (err error) { + var existing *v1alpha1.Ingress // The first iteration tries to use the informer's state. if attempts == 0 { existing, err = r.ingressLister.Ingresses(desired.Namespace).Get(desired.Name) diff --git a/pkg/reconciler/metric/metric.go b/pkg/reconciler/metric/metric.go index 74b3a62bae3d..107a0ce38595 100644 --- a/pkg/reconciler/metric/metric.go +++ b/pkg/reconciler/metric/metric.go @@ -105,11 +105,8 @@ func (r *reconciler) reconcileCollection(ctx context.Context, metric *v1alpha1.M } func (r *reconciler) updateStatus(desired *v1alpha1.Metric) error { - return rbase.RetryUpdateConflicts(func(attempts int) error { - var ( - existing *v1alpha1.Metric - err error - ) + return rbase.RetryUpdateConflicts(func(attempts int) (err error) { + var existing *v1alpha1.Metric // The first iteration tries to use the informer's state. if attempts == 0 { existing, err = r.metricLister.Metrics(desired.Namespace).Get(desired.Name) diff --git a/pkg/reconciler/revision/revision.go b/pkg/reconciler/revision/revision.go index 7c18bb3f15f1..3d30990f8217 100644 --- a/pkg/reconciler/revision/revision.go +++ b/pkg/reconciler/revision/revision.go @@ -233,11 +233,8 @@ func (c *Reconciler) updateRevisionLoggingURL( } func (c *Reconciler) updateStatus(desired *v1alpha1.Revision) error { - return reconciler.RetryUpdateConflicts(func(attempts int) error { - var ( - existing *v1alpha1.Revision - err error - ) + return reconciler.RetryUpdateConflicts(func(attempts int) (err error) { + var existing *v1alpha1.Revision // The first iteration tries to use the informer's state. if attempts == 0 { existing, err = c.revisionLister.Revisions(desired.Namespace).Get(desired.Name) diff --git a/pkg/reconciler/route/reconcile_resources.go b/pkg/reconciler/route/reconcile_resources.go index 04b09d230fbb..5ce47fc2851d 100644 --- a/pkg/reconciler/route/reconcile_resources.go +++ b/pkg/reconciler/route/reconcile_resources.go @@ -189,11 +189,8 @@ func (c *Reconciler) updatePlaceholderServices(ctx context.Context, route *v1alp } func (c *Reconciler) updateStatus(desired *v1alpha1.Route) error { - return reconciler.RetryUpdateConflicts(func(attempts int) error { - var ( - existing *v1alpha1.Route - err error - ) + return reconciler.RetryUpdateConflicts(func(attempts int) (err error) { + var existing *v1alpha1.Route // The first iteration tries to use the informer's state. if attempts == 0 { existing, err = c.routeLister.Routes(desired.Namespace).Get(desired.Name) diff --git a/pkg/reconciler/serverlessservice/serverlessservice.go b/pkg/reconciler/serverlessservice/serverlessservice.go index d733656ccf2e..3befe9c24148 100644 --- a/pkg/reconciler/serverlessservice/serverlessservice.go +++ b/pkg/reconciler/serverlessservice/serverlessservice.go @@ -128,11 +128,8 @@ func (r *reconciler) reconcile(ctx context.Context, sks *netv1alpha1.ServerlessS } func (r *reconciler) updateStatus(desired *netv1alpha1.ServerlessService) error { - return rbase.RetryUpdateConflicts(func(attempts int) error { - var ( - existing *netv1alpha1.ServerlessService - err error - ) + return rbase.RetryUpdateConflicts(func(attempts int) (err error) { + var existing *netv1alpha1.ServerlessService // The first iteration tries to use the informer's state. if attempts == 0 { existing, err = r.sksLister.ServerlessServices(desired.Namespace).Get(desired.Name) diff --git a/pkg/reconciler/service/service.go b/pkg/reconciler/service/service.go index 3e0f0f1488ea..d77fcf2b46d2 100644 --- a/pkg/reconciler/service/service.go +++ b/pkg/reconciler/service/service.go @@ -274,11 +274,8 @@ func (c *Reconciler) checkRoutesNotReady(config *v1alpha1.Configuration, logger } func (c *Reconciler) updateStatus(desired *v1alpha1.Service, logger *zap.SugaredLogger) error { - return reconciler.RetryUpdateConflicts(func(attempts int) error { - var ( - existing *v1alpha1.Service - err error - ) + return reconciler.RetryUpdateConflicts(func(attempts int) (err error) { + var existing *v1alpha1.Service // The first iteration tries to use the informer's state. if attempts == 0 { existing, err = c.serviceLister.Services(desired.Namespace).Get(desired.Name) From 2bfa75df88dce19c43db03619fd1470230b784c2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Markus=20Th=C3=B6mmes?= Date: Tue, 10 Dec 2019 10:54:29 +0100 Subject: [PATCH 8/8] Remove lister access from retry function. --- pkg/reconciler/autoscaling/hpa/hpa.go | 2 +- pkg/reconciler/autoscaling/kpa/kpa.go | 2 +- pkg/reconciler/autoscaling/reconciler.go | 17 +++++++---------- pkg/reconciler/certificate/certificate.go | 19 ++++++++----------- pkg/reconciler/configuration/configuration.go | 19 ++++++++----------- pkg/reconciler/ingress/ingress.go | 19 ++++++++----------- pkg/reconciler/metric/metric.go | 19 ++++++++----------- pkg/reconciler/revision/revision.go | 19 ++++++++----------- pkg/reconciler/route/reconcile_resources.go | 17 +++++++---------- pkg/reconciler/route/route.go | 2 +- .../serverlessservice/serverlessservice.go | 19 ++++++++----------- pkg/reconciler/service/service.go | 19 ++++++++----------- 12 files changed, 73 insertions(+), 100 deletions(-) diff --git a/pkg/reconciler/autoscaling/hpa/hpa.go b/pkg/reconciler/autoscaling/hpa/hpa.go index c4cc209176fd..3451d8655f9f 100644 --- a/pkg/reconciler/autoscaling/hpa/hpa.go +++ b/pkg/reconciler/autoscaling/hpa/hpa.go @@ -78,7 +78,7 @@ func (c *Reconciler) Reconcile(ctx context.Context, key string) error { // This is important because the copy we loaded from the informer's // cache may be stale and we don't want to overwrite a prior update // to status with this stale state. - } else if err = c.UpdateStatus(pa); err != nil { + } else if err = c.UpdateStatus(original, pa); err != nil { logger.Warnw("Failed to update pa status", zap.Error(err)) c.Recorder.Eventf(pa, corev1.EventTypeWarning, "UpdateFailed", "Failed to update status for PA %q: %v", pa.Name, err) diff --git a/pkg/reconciler/autoscaling/kpa/kpa.go b/pkg/reconciler/autoscaling/kpa/kpa.go index ab4ebbb69e22..9775dc3a4002 100644 --- a/pkg/reconciler/autoscaling/kpa/kpa.go +++ b/pkg/reconciler/autoscaling/kpa/kpa.go @@ -89,7 +89,7 @@ func (c *Reconciler) Reconcile(ctx context.Context, key string) error { // This is important because the copy we loaded from the informer's // cache may be stale and we don't want to overwrite a prior update // to status with this stale state. - } else if err = c.UpdateStatus(pa); err != nil { + } else if err = c.UpdateStatus(original, pa); err != nil { logger.Warnw("Failed to update pa status", zap.Error(err)) c.Recorder.Eventf(pa, corev1.EventTypeWarning, "UpdateFailed", "Failed to update status for PA %q: %v", pa.Name, err) diff --git a/pkg/reconciler/autoscaling/reconciler.go b/pkg/reconciler/autoscaling/reconciler.go index 541dcc18bea0..1ebc03271a53 100644 --- a/pkg/reconciler/autoscaling/reconciler.go +++ b/pkg/reconciler/autoscaling/reconciler.go @@ -138,18 +138,15 @@ func (c *Base) ReconcileMetric(ctx context.Context, pa *pav1alpha1.PodAutoscaler } // UpdateStatus updates the status of the given PodAutoscaler. -func (c *Base) UpdateStatus(desired *pav1alpha1.PodAutoscaler) error { +func (c *Base) UpdateStatus(existing *pav1alpha1.PodAutoscaler, desired *pav1alpha1.PodAutoscaler) error { + existing = existing.DeepCopy() return reconciler.RetryUpdateConflicts(func(attempts int) (err error) { - var existing *pav1alpha1.PodAutoscaler - // The first iteration tries to use the informer's state. - if attempts == 0 { - existing, err = c.PALister.PodAutoscalers(desired.Namespace).Get(desired.Name) - existing = existing.DeepCopy() - } else { + // The first iteration tries to use the informer's state, subsequent attempts fetch the latest state via API. + if attempts > 0 { existing, err = c.ServingClientSet.AutoscalingV1alpha1().PodAutoscalers(desired.Namespace).Get(desired.Name, metav1.GetOptions{}) - } - if err != nil { - return err + if err != nil { + return err + } } // If there's nothing to update, just return. diff --git a/pkg/reconciler/certificate/certificate.go b/pkg/reconciler/certificate/certificate.go index abfeb84acbaf..f6b248221b3a 100644 --- a/pkg/reconciler/certificate/certificate.go +++ b/pkg/reconciler/certificate/certificate.go @@ -100,7 +100,7 @@ func (c *Reconciler) Reconcile(ctx context.Context, key string) error { // This is important because the copy we loaded from the informer's // cache may be stale and we don't want to overwrite a prior update // to status with this stale state. - } else if err := c.updateStatus(knCert); err != nil { + } else if err := c.updateStatus(original, knCert); err != nil { logger.Warnw("Failed to update certificate status", zap.Error(err)) c.Recorder.Eventf(knCert, corev1.EventTypeWarning, "UpdateFailed", "Failed to update status for Certificate %s: %v", key, err) @@ -173,18 +173,15 @@ func (c *Reconciler) reconcileCMCertificate(ctx context.Context, knCert *v1alpha return cmCert, nil } -func (c *Reconciler) updateStatus(desired *v1alpha1.Certificate) error { +func (c *Reconciler) updateStatus(existing *v1alpha1.Certificate, desired *v1alpha1.Certificate) error { + existing = existing.DeepCopy() return reconciler.RetryUpdateConflicts(func(attempts int) (err error) { - var existing *v1alpha1.Certificate - // The first iteration tries to use the informer's state. - if attempts == 0 { - existing, err = c.knCertificateLister.Certificates(desired.Namespace).Get(desired.Name) - existing = existing.DeepCopy() - } else { + // The first iteration tries to use the informer's state, subsequent attempts fetch the latest state via API. + if attempts > 0 { existing, err = c.ServingClientSet.NetworkingV1alpha1().Certificates(desired.Namespace).Get(desired.Name, metav1.GetOptions{}) - } - if err != nil { - return err + if err != nil { + return err + } } // If there's nothing to update, just return. diff --git a/pkg/reconciler/configuration/configuration.go b/pkg/reconciler/configuration/configuration.go index 432fd236b876..0390b20b6a13 100644 --- a/pkg/reconciler/configuration/configuration.go +++ b/pkg/reconciler/configuration/configuration.go @@ -87,7 +87,7 @@ func (c *Reconciler) Reconcile(ctx context.Context, key string) error { // This is important because the copy we loaded from the informer's // cache may be stale and we don't want to overwrite a prior update // to status with this stale state. - } else if err = c.updateStatus(config); err != nil { + } else if err = c.updateStatus(original, config); err != nil { logger.Warnw("Failed to update configuration status", zap.Error(err)) c.Recorder.Eventf(config, corev1.EventTypeWarning, "UpdateFailed", "Failed to update status: %v", err) return err @@ -341,18 +341,15 @@ func (c *Reconciler) createRevision(ctx context.Context, config *v1alpha1.Config return created, nil } -func (c *Reconciler) updateStatus(desired *v1alpha1.Configuration) error { +func (c *Reconciler) updateStatus(existing *v1alpha1.Configuration, desired *v1alpha1.Configuration) error { + existing = existing.DeepCopy() return reconciler.RetryUpdateConflicts(func(attempts int) (err error) { - var existing *v1alpha1.Configuration - // The first iteration tries to use the informer's state. - if attempts == 0 { - existing, err = c.configurationLister.Configurations(desired.Namespace).Get(desired.Name) - existing = existing.DeepCopy() - } else { + // The first iteration tries to use the informer's state, subsequent attempts fetch the latest state via API. + if attempts > 0 { existing, err = c.ServingClientSet.ServingV1alpha1().Configurations(desired.Namespace).Get(desired.Name, metav1.GetOptions{}) - } - if err != nil { - return err + if err != nil { + return err + } } // If there's nothing to update, just return. diff --git a/pkg/reconciler/ingress/ingress.go b/pkg/reconciler/ingress/ingress.go index c1d883636fe3..6b5e64796300 100644 --- a/pkg/reconciler/ingress/ingress.go +++ b/pkg/reconciler/ingress/ingress.go @@ -130,7 +130,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, key string) error { // cache may be stale and we don't want to overwrite a prior update // to status with this stale state. } else { - if err = r.updateStatus(ingress); err != nil { + if err = r.updateStatus(original, ingress); err != nil { logger.Warnw("Failed to update Ingress status", zap.Error(err)) r.Recorder.Eventf(ingress, corev1.EventTypeWarning, "UpdateFailed", "Failed to update status for Ingress %q: %v", ingress.GetName(), err) @@ -306,18 +306,15 @@ func (r *Reconciler) reconcileDeletion(ctx context.Context, ia *v1alpha1.Ingress // Update the Status of the Ingress. Caller is responsible for checking // for semantic differences before calling. -func (r *Reconciler) updateStatus(desired *v1alpha1.Ingress) error { +func (r *Reconciler) updateStatus(existing *v1alpha1.Ingress, desired *v1alpha1.Ingress) error { + existing = existing.DeepCopy() return reconciler.RetryUpdateConflicts(func(attempts int) (err error) { - var existing *v1alpha1.Ingress - // The first iteration tries to use the informer's state. - if attempts == 0 { - existing, err = r.ingressLister.Ingresses(desired.Namespace).Get(desired.Name) - existing = existing.DeepCopy() - } else { + // The first iteration tries to use the informer's state, subsequent attempts fetch the latest state via API. + if attempts > 0 { existing, err = r.ServingClientSet.NetworkingV1alpha1().Ingresses(desired.GetNamespace()).Get(desired.GetName(), metav1.GetOptions{}) - } - if err != nil { - return err + if err != nil { + return err + } } // If there's nothing to update, just return. diff --git a/pkg/reconciler/metric/metric.go b/pkg/reconciler/metric/metric.go index 107a0ce38595..d753b232397d 100644 --- a/pkg/reconciler/metric/metric.go +++ b/pkg/reconciler/metric/metric.go @@ -83,7 +83,7 @@ func (r *reconciler) Reconcile(ctx context.Context, key string) error { if !equality.Semantic.DeepEqual(original.Status, metric.Status) { // Change of status, need to update the object. - if uErr := r.updateStatus(metric); uErr != nil { + if uErr := r.updateStatus(original, metric); uErr != nil { logger.Warnw("Failed to update metric status", zap.Error(uErr)) r.Recorder.Eventf(metric, corev1.EventTypeWarning, "UpdateFailed", "Failed to update metric status: %v", uErr) @@ -104,18 +104,15 @@ func (r *reconciler) reconcileCollection(ctx context.Context, metric *v1alpha1.M return nil } -func (r *reconciler) updateStatus(desired *v1alpha1.Metric) error { +func (r *reconciler) updateStatus(existing *v1alpha1.Metric, desired *v1alpha1.Metric) error { + existing = existing.DeepCopy() return rbase.RetryUpdateConflicts(func(attempts int) (err error) { - var existing *v1alpha1.Metric - // The first iteration tries to use the informer's state. - if attempts == 0 { - existing, err = r.metricLister.Metrics(desired.Namespace).Get(desired.Name) - existing = existing.DeepCopy() - } else { + // The first iteration tries to use the informer's state, subsequent attempts fetch the latest state via API. + if attempts > 0 { existing, err = r.ServingClientSet.AutoscalingV1alpha1().Metrics(desired.Namespace).Get(desired.Name, metav1.GetOptions{}) - } - if err != nil { - return err + if err != nil { + return err + } } // If there's nothing to update, just return. diff --git a/pkg/reconciler/revision/revision.go b/pkg/reconciler/revision/revision.go index 3d30990f8217..14a68672eb6a 100644 --- a/pkg/reconciler/revision/revision.go +++ b/pkg/reconciler/revision/revision.go @@ -105,7 +105,7 @@ func (c *Reconciler) Reconcile(ctx context.Context, key string) error { // This is important because the copy we loaded from the informer's // cache may be stale and we don't want to overwrite a prior update // to status with this stale state. - } else if err = c.updateStatus(rev); err != nil { + } else if err = c.updateStatus(original, rev); err != nil { logger.Warnw("Failed to update revision status", zap.Error(err)) c.Recorder.Eventf(rev, corev1.EventTypeWarning, "UpdateFailed", "Failed to update status for Revision %q: %v", rev.Name, err) @@ -232,18 +232,15 @@ func (c *Reconciler) updateRevisionLoggingURL( "${REVISION_UID}", uid, -1) } -func (c *Reconciler) updateStatus(desired *v1alpha1.Revision) error { +func (c *Reconciler) updateStatus(existing *v1alpha1.Revision, desired *v1alpha1.Revision) error { + existing = existing.DeepCopy() return reconciler.RetryUpdateConflicts(func(attempts int) (err error) { - var existing *v1alpha1.Revision - // The first iteration tries to use the informer's state. - if attempts == 0 { - existing, err = c.revisionLister.Revisions(desired.Namespace).Get(desired.Name) - existing = existing.DeepCopy() - } else { + // The first iteration tries to use the informer's state, subsequent attempts fetch the latest state via API. + if attempts > 0 { existing, err = c.ServingClientSet.ServingV1alpha1().Revisions(desired.Namespace).Get(desired.Name, metav1.GetOptions{}) - } - if err != nil { - return err + if err != nil { + return err + } } // If there's nothing to update, just return. diff --git a/pkg/reconciler/route/reconcile_resources.go b/pkg/reconciler/route/reconcile_resources.go index 5ce47fc2851d..7d085c735ea6 100644 --- a/pkg/reconciler/route/reconcile_resources.go +++ b/pkg/reconciler/route/reconcile_resources.go @@ -188,18 +188,15 @@ func (c *Reconciler) updatePlaceholderServices(ctx context.Context, route *v1alp return eg.Wait() } -func (c *Reconciler) updateStatus(desired *v1alpha1.Route) error { +func (c *Reconciler) updateStatus(existing *v1alpha1.Route, desired *v1alpha1.Route) error { + existing = existing.DeepCopy() return reconciler.RetryUpdateConflicts(func(attempts int) (err error) { - var existing *v1alpha1.Route - // The first iteration tries to use the informer's state. - if attempts == 0 { - existing, err = c.routeLister.Routes(desired.Namespace).Get(desired.Name) - existing = existing.DeepCopy() - } else { + // The first iteration tries to use the informer's state, subsequent attempts fetch the latest state via API. + if attempts > 0 { existing, err = c.ServingClientSet.ServingV1alpha1().Routes(desired.Namespace).Get(desired.Name, metav1.GetOptions{}) - } - if err != nil { - return err + if err != nil { + return err + } } // If there's nothing to update, just return. diff --git a/pkg/reconciler/route/route.go b/pkg/reconciler/route/route.go index a6cc050f0c5e..b04df2c86b7d 100644 --- a/pkg/reconciler/route/route.go +++ b/pkg/reconciler/route/route.go @@ -120,7 +120,7 @@ func (c *Reconciler) Reconcile(ctx context.Context, key string) error { // This is important because the copy we loaded from the informer's // cache may be stale and we don't want to overwrite a prior update // to status with this stale state. - } else if err = c.updateStatus(route); err != nil { + } else if err = c.updateStatus(original, route); err != nil { logger.Warnw("Failed to update route status", zap.Error(err)) c.Recorder.Eventf(route, corev1.EventTypeWarning, "UpdateFailed", "Failed to update status for Route %q: %v", route.Name, err) diff --git a/pkg/reconciler/serverlessservice/serverlessservice.go b/pkg/reconciler/serverlessservice/serverlessservice.go index 3befe9c24148..0e375d64d3a8 100644 --- a/pkg/reconciler/serverlessservice/serverlessservice.go +++ b/pkg/reconciler/serverlessservice/serverlessservice.go @@ -94,7 +94,7 @@ func (r *reconciler) Reconcile(ctx context.Context, key string) error { r.Recorder.Eventf(sks, corev1.EventTypeWarning, "UpdateFailed", "InternalError: %v", reconcileErr.Error()) } if !equality.Semantic.DeepEqual(sks.Status, original.Status) { - if err := r.updateStatus(sks); err != nil { + if err := r.updateStatus(original, sks); err != nil { r.Recorder.Eventf(sks, corev1.EventTypeWarning, "UpdateFailed", "Failed to update status: %v", err) return err } @@ -127,18 +127,15 @@ func (r *reconciler) reconcile(ctx context.Context, sks *netv1alpha1.ServerlessS return nil } -func (r *reconciler) updateStatus(desired *netv1alpha1.ServerlessService) error { +func (r *reconciler) updateStatus(existing *netv1alpha1.ServerlessService, desired *netv1alpha1.ServerlessService) error { + existing = existing.DeepCopy() return rbase.RetryUpdateConflicts(func(attempts int) (err error) { - var existing *netv1alpha1.ServerlessService - // The first iteration tries to use the informer's state. - if attempts == 0 { - existing, err = r.sksLister.ServerlessServices(desired.Namespace).Get(desired.Name) - existing = existing.DeepCopy() - } else { + // The first iteration tries to use the informer's state, subsequent attempts fetch the latest state via API. + if attempts > 0 { existing, err = r.ServingClientSet.NetworkingV1alpha1().ServerlessServices(desired.Namespace).Get(desired.Name, metav1.GetOptions{}) - } - if err != nil { - return err + if err != nil { + return err + } } // If there's nothing to update, just return. diff --git a/pkg/reconciler/service/service.go b/pkg/reconciler/service/service.go index d77fcf2b46d2..3e819f884647 100644 --- a/pkg/reconciler/service/service.go +++ b/pkg/reconciler/service/service.go @@ -101,7 +101,7 @@ func (c *Reconciler) Reconcile(ctx context.Context, key string) error { // cache may be stale and we don't want to overwrite a prior update // to status with this stale state. - } else if uErr := c.updateStatus(service, logger); uErr != nil { + } else if uErr := c.updateStatus(original, service, logger); uErr != nil { logger.Warnw("Failed to update service status", zap.Error(uErr)) c.Recorder.Eventf(service, corev1.EventTypeWarning, "UpdateFailed", "Failed to update status for Service %q: %v", service.Name, uErr) @@ -273,18 +273,15 @@ func (c *Reconciler) checkRoutesNotReady(config *v1alpha1.Configuration, logger } } -func (c *Reconciler) updateStatus(desired *v1alpha1.Service, logger *zap.SugaredLogger) error { +func (c *Reconciler) updateStatus(existing *v1alpha1.Service, desired *v1alpha1.Service, logger *zap.SugaredLogger) error { + existing = existing.DeepCopy() return reconciler.RetryUpdateConflicts(func(attempts int) (err error) { - var existing *v1alpha1.Service - // The first iteration tries to use the informer's state. - if attempts == 0 { - existing, err = c.serviceLister.Services(desired.Namespace).Get(desired.Name) - existing = existing.DeepCopy() - } else { + // The first iteration tries to use the informer's state, subsequent attempts fetch the latest state via API. + if attempts > 0 { existing, err = c.ServingClientSet.ServingV1alpha1().Services(desired.Namespace).Get(desired.Name, metav1.GetOptions{}) - } - if err != nil { - return err + if err != nil { + return err + } } // If there's nothing to update, just return.