From cfbc8976cfd86c87aaba6d8c0532315375086330 Mon Sep 17 00:00:00 2001 From: Jon Johnson Date: Thu, 8 Nov 2018 22:10:50 +0000 Subject: [PATCH 1/4] Add service events For Config and Route creation. --- pkg/reconciler/v1alpha1/service/resources/route.go | 1 - pkg/reconciler/v1alpha1/service/service.go | 3 ++- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/reconciler/v1alpha1/service/resources/route.go b/pkg/reconciler/v1alpha1/service/resources/route.go index a93a48b0e09e..baad884d0abd 100644 --- a/pkg/reconciler/v1alpha1/service/resources/route.go +++ b/pkg/reconciler/v1alpha1/service/resources/route.go @@ -28,7 +28,6 @@ import ( // MakeRoute creates a Route from a Service object. func MakeRoute(service *v1alpha1.Service) (*v1alpha1.Route, error) { - c := &v1alpha1.Route{ ObjectMeta: metav1.ObjectMeta{ Name: names.Route(service), diff --git a/pkg/reconciler/v1alpha1/service/service.go b/pkg/reconciler/v1alpha1/service/service.go index 2b76c1e78c87..a4c8df49a389 100644 --- a/pkg/reconciler/v1alpha1/service/service.go +++ b/pkg/reconciler/v1alpha1/service/service.go @@ -158,6 +158,7 @@ func (c *Reconciler) reconcile(ctx context.Context, service *v1alpha1.Service) e c.Recorder.Eventf(service, corev1.EventTypeWarning, "CreationFailed", "Failed to create Configuration %q: %v", configName, err) return err } + c.Recorder.Eventf(service, corev1.EventTypeNormal, "Created", "Created Configuration %q", config.GetName()) } else if err != nil { logger.Errorf("Failed to reconcile Service: %q failed to Get Configuration: %q; %v", service.Name, configName, zap.Error(err)) return err @@ -178,6 +179,7 @@ func (c *Reconciler) reconcile(ctx context.Context, service *v1alpha1.Service) e c.Recorder.Eventf(service, corev1.EventTypeWarning, "CreationFailed", "Failed to create Route %q: %v", routeName, err) return err } + c.Recorder.Eventf(service, corev1.EventTypeNormal, "Created", "Created Route %q", route.GetName()) } else if err != nil { logger.Errorf("Failed to reconcile Service: %q failed to Get Route: %q", service.Name, routeName) return err @@ -223,7 +225,6 @@ func (c *Reconciler) createConfiguration(service *v1alpha1.Service) (*v1alpha1.C } func (c *Reconciler) reconcileConfiguration(ctx context.Context, service *v1alpha1.Service, config *v1alpha1.Configuration) (*v1alpha1.Configuration, error) { - logger := logging.FromContext(ctx) desiredConfig, err := resources.MakeConfiguration(service) if err != nil { From 5ffc20983cc5cb557664b2e6a4ce8a3d30d526bf Mon Sep 17 00:00:00 2001 From: Jon Johnson Date: Thu, 8 Nov 2018 22:43:46 +0000 Subject: [PATCH 2/4] Make updateStatus consistent These had two different implementation styles, one of which emitted events. Changed all of them to share the same style. --- .../v1alpha1/autoscaling/autoscaling.go | 36 ++++++++++--------- .../v1alpha1/clusteringress/clusteringress.go | 6 ++-- .../v1alpha1/configuration/configuration.go | 26 +++++++++----- pkg/reconciler/v1alpha1/revision/revision.go | 35 +++++++++--------- .../v1alpha1/route/reconcile_resources.go | 17 ++++----- pkg/reconciler/v1alpha1/route/route.go | 4 +-- pkg/reconciler/v1alpha1/service/service.go | 25 ++++++++----- 7 files changed, 86 insertions(+), 63 deletions(-) diff --git a/pkg/reconciler/v1alpha1/autoscaling/autoscaling.go b/pkg/reconciler/v1alpha1/autoscaling/autoscaling.go index 5cc3e3af4f93..da49c7e917c1 100644 --- a/pkg/reconciler/v1alpha1/autoscaling/autoscaling.go +++ b/pkg/reconciler/v1alpha1/autoscaling/autoscaling.go @@ -158,12 +158,11 @@ 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 { - // logger.Infof("Updating Status (-old, +new): %v", cmp.Diff(original, kpa)) - if _, err := c.updateStatus(kpa); err != nil { - logger.Warn("Failed to update kpa status", zap.Error(err)) - return err - } + } else if _, err := c.updateStatus(kpa); err != nil { + logger.Warn("Failed to update kpa status", zap.Error(err)) + c.Recorder.Eventf(kpa, corev1.EventTypeWarning, "UpdateFailed", + "Failed to update status for KPA %q: %v", kpa.Name, err) + return err } return err } @@ -247,15 +246,20 @@ func (c *Reconciler) updateStatus(desired *kpa.PodAutoscaler) (*kpa.PodAutoscale if err != nil { return nil, err } - // Check if there is anything to update. - if !reflect.DeepEqual(kpa.Status, desired.Status) { - // Don't modify the informers copy - existing := kpa.DeepCopy() - existing.Status = desired.Status - - // TODO: for CRD there's no updatestatus, so use normal update - return c.ServingClientSet.AutoscalingV1alpha1().PodAutoscalers(kpa.Namespace).Update(existing) - // return prClient.UpdateStatus(newKPA) + // If there's nothing to update, just return. + if reflect.DeepEqual(kpa.Status, desired.Status) { + return kpa, nil } - return kpa, nil + // Don't modify the informers copy + existing := kpa.DeepCopy() + existing.Status = desired.Status + + // TODO: for CRD there's no updatestatus, so use normal update + updated, err := c.ServingClientSet.AutoscalingV1alpha1().PodAutoscalers(kpa.Namespace).Update(existing) + if err != nil { + return nil, err + } + + c.Recorder.Eventf(desired, corev1.EventTypeNormal, "Updated", "Updated status for KPA %q", desired.Name) + return updated, nil } diff --git a/pkg/reconciler/v1alpha1/clusteringress/clusteringress.go b/pkg/reconciler/v1alpha1/clusteringress/clusteringress.go index 7207d8242e3c..0d8735b8db77 100644 --- a/pkg/reconciler/v1alpha1/clusteringress/clusteringress.go +++ b/pkg/reconciler/v1alpha1/clusteringress/clusteringress.go @@ -117,7 +117,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(ctx, ci); err != nil { + } else if _, err := c.updateStatus(ci); err != nil { logger.Warn("Failed to update clusterIngress status", zap.Error(err)) c.Recorder.Eventf(ci, corev1.EventTypeWarning, "UpdateFailed", "Failed to update status for ClusterIngress %q: %v", ci.Name, err) @@ -128,7 +128,7 @@ func (c *Reconciler) Reconcile(ctx context.Context, key string) error { // Update the Status of the ClusterIngress. Caller is responsible for checking // for semantic differences before calling. -func (c *Reconciler) updateStatus(ctx context.Context, desired *v1alpha1.ClusterIngress) (*v1alpha1.ClusterIngress, error) { +func (c *Reconciler) updateStatus(desired *v1alpha1.ClusterIngress) (*v1alpha1.ClusterIngress, error) { ci, err := c.clusterIngressLister.Get(desired.Name) if err != nil { return nil, err @@ -146,7 +146,7 @@ func (c *Reconciler) updateStatus(ctx context.Context, desired *v1alpha1.Cluster return nil, err } - c.Recorder.Eventf(desired, corev1.EventTypeNormal, "Updated", "Updated status for clusterIngress %q", desired.Name) + c.Recorder.Eventf(desired, corev1.EventTypeNormal, "Updated", "Updated status for ClusterIngress %q", desired.Name) return updated, nil } diff --git a/pkg/reconciler/v1alpha1/configuration/configuration.go b/pkg/reconciler/v1alpha1/configuration/configuration.go index e1df0c5c1e36..8c298ed44e96 100644 --- a/pkg/reconciler/v1alpha1/configuration/configuration.go +++ b/pkg/reconciler/v1alpha1/configuration/configuration.go @@ -139,6 +139,8 @@ func (c *Reconciler) Reconcile(ctx context.Context, key string) error { // to status with this stale state. } else if _, err := c.updateStatus(config); err != nil { logger.Warn("Failed to update configuration status", zap.Error(err)) + c.Recorder.Eventf(config, corev1.EventTypeWarning, "UpdateFailed", + "Failed to update status for Configuration %q: %v", config.Name, err) return err } return err @@ -266,19 +268,25 @@ func (c *Reconciler) createRevision(ctx context.Context, config *v1alpha1.Config } func (c *Reconciler) updateStatus(desired *v1alpha1.Configuration) (*v1alpha1.Configuration, error) { - u, err := c.configurationLister.Configurations(desired.Namespace).Get(desired.Name) + config, err := c.configurationLister.Configurations(desired.Namespace).Get(desired.Name) if err != nil { return nil, err } - if !reflect.DeepEqual(u.Status, desired.Status) { - // Don't modify the informers copy - existing := u.DeepCopy() - existing.Status = desired.Status - // TODO: for CRD there's no updatestatus, so use normal update - return c.ServingClientSet.ServingV1alpha1().Configurations(desired.Namespace).Update(existing) - // return configClient.UpdateStatus(newu) + // If there's nothing to update, just return. + if reflect.DeepEqual(config.Status, desired.Status) { + return config, nil } - return u, nil + // Don't modify the informers copy + existing := config.DeepCopy() + existing.Status = desired.Status + // TODO: for CRD there's no updatestatus, so use normal update + updated, err := c.ServingClientSet.ServingV1alpha1().Configurations(desired.Namespace).Update(existing) + if err != nil { + return nil, err + } + + c.Recorder.Eventf(desired, corev1.EventTypeNormal, "Updated", "Updated status for Configuration %q", desired.Name) + return updated, nil } func (c *Reconciler) gcRevisions(ctx context.Context, config *v1alpha1.Configuration) error { diff --git a/pkg/reconciler/v1alpha1/revision/revision.go b/pkg/reconciler/v1alpha1/revision/revision.go index eb782ceec05b..66d9ced0b126 100644 --- a/pkg/reconciler/v1alpha1/revision/revision.go +++ b/pkg/reconciler/v1alpha1/revision/revision.go @@ -249,12 +249,11 @@ 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 { - // logger.Infof("Updating Status (-old, +new): %v", cmp.Diff(original, rev)) - if _, err := c.updateStatus(rev); err != nil { - logger.Warn("Failed to update revision status", zap.Error(err)) - return err - } + } else if _, err := c.updateStatus(rev); err != nil { + logger.Warn("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) + return err } return err } @@ -411,15 +410,19 @@ func (c *Reconciler) updateStatus(desired *v1alpha1.Revision) (*v1alpha1.Revisio if err != nil { return nil, err } - // Check if there is anything to update. - if !reflect.DeepEqual(rev.Status, desired.Status) { - // Don't modify the informers copy - existing := rev.DeepCopy() - existing.Status = desired.Status - - // TODO: for CRD there's no updatestatus, so use normal update - return c.ServingClientSet.ServingV1alpha1().Revisions(desired.Namespace).Update(existing) - // return prClient.UpdateStatus(newRev) + // If there's nothing to update, just return. + if reflect.DeepEqual(rev.Status, desired.Status) { + return rev, nil + } + // Don't modify the informers copy + existing := rev.DeepCopy() + existing.Status = desired.Status + // TODO: for CRD there's no updatestatus, so use normal update + updated, err := c.ServingClientSet.ServingV1alpha1().Revisions(desired.Namespace).Update(existing) + if err != nil { + return nil, err } - return rev, nil + + c.Recorder.Eventf(desired, corev1.EventTypeNormal, "Updated", "Updated status for Revision %q", desired.Name) + return updated, nil } diff --git a/pkg/reconciler/v1alpha1/route/reconcile_resources.go b/pkg/reconciler/v1alpha1/route/reconcile_resources.go index 56f059ce3080..e56474bfd48b 100644 --- a/pkg/reconciler/v1alpha1/route/reconcile_resources.go +++ b/pkg/reconciler/v1alpha1/route/reconcile_resources.go @@ -141,24 +141,25 @@ func (c *Reconciler) reconcilePlaceholderService(ctx context.Context, route *v1a // Update the Status of the route. Caller is responsible for checking // for semantic differences before calling. -func (c *Reconciler) updateStatus(ctx context.Context, route *v1alpha1.Route) (*v1alpha1.Route, error) { - existing, err := c.routeLister.Routes(route.Namespace).Get(route.Name) +func (c *Reconciler) updateStatus(desired *v1alpha1.Route) (*v1alpha1.Route, error) { + route, err := c.routeLister.Routes(desired.Namespace).Get(desired.Name) if err != nil { return nil, err } - existing = existing.DeepCopy() // If there's nothing to update, just return. - if reflect.DeepEqual(existing.Status, route.Status) { - return existing, nil + if reflect.DeepEqual(route.Status, desired.Status) { + return route, nil } - existing.Status = route.Status + // Don't modify the informers copy + existing := route.DeepCopy() + existing.Status = desired.Status // TODO: for CRD there's no updatestatus, so use normal update. - updated, err := c.ServingClientSet.ServingV1alpha1().Routes(route.Namespace).Update(existing) + updated, err := c.ServingClientSet.ServingV1alpha1().Routes(desired.Namespace).Update(existing) if err != nil { return nil, err } - c.Recorder.Eventf(route, corev1.EventTypeNormal, "Updated", "Updated status for route %q", route.Name) + c.Recorder.Eventf(desired, corev1.EventTypeNormal, "Updated", "Updated status for route %q", desired.Name) return updated, nil } diff --git a/pkg/reconciler/v1alpha1/route/route.go b/pkg/reconciler/v1alpha1/route/route.go index 9af7af9316e7..476be6116409 100644 --- a/pkg/reconciler/v1alpha1/route/route.go +++ b/pkg/reconciler/v1alpha1/route/route.go @@ -200,10 +200,10 @@ 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(ctx, route); err != nil { + } else if _, err := c.updateStatus(route); err != nil { logger.Warn("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) + "Failed to update status for Route %q: %v", route.Name, err) return err } return err diff --git a/pkg/reconciler/v1alpha1/service/service.go b/pkg/reconciler/v1alpha1/service/service.go index a4c8df49a389..8003fd3249f9 100644 --- a/pkg/reconciler/v1alpha1/service/service.go +++ b/pkg/reconciler/v1alpha1/service/service.go @@ -140,6 +140,8 @@ func (c *Reconciler) Reconcile(ctx context.Context, key string) error { // to status with this stale state. } else if _, err := c.updateStatus(service); err != nil { logger.Warn("Failed to update service status", zap.Error(err)) + c.Recorder.Eventf(service, corev1.EventTypeWarning, "UpdateFailed", + "Failed to update status for Service %q: %v", service.Name, err) return err } return err @@ -204,16 +206,21 @@ func (c *Reconciler) updateStatus(desired *v1alpha1.Service) (*v1alpha1.Service, if err != nil { return nil, err } - // Check if there is anything to update. - if !reflect.DeepEqual(service.Status, desired.Status) { - // Don't modify the informers copy - existing := service.DeepCopy() - existing.Status = desired.Status - serviceClient := c.ServingClientSet.ServingV1alpha1().Services(desired.Namespace) - // TODO: for CRD there's no updatestatus, so use normal update. - return serviceClient.Update(existing) + // If there's nothing to update, just return. + if reflect.DeepEqual(service.Status, desired.Status) { + return service, nil } - return service, nil + // Don't modify the informers copy + existing := service.DeepCopy() + existing.Status = desired.Status + // TODO: for CRD there's no updatestatus, so use normal update. + updated, err := c.ServingClientSet.ServingV1alpha1().Services(desired.Namespace).Update(existing) + if err != nil { + return nil, err + } + + c.Recorder.Eventf(desired, corev1.EventTypeNormal, "Updated", "Updated status for Service %q", desired.Name) + return updated, nil } func (c *Reconciler) createConfiguration(service *v1alpha1.Service) (*v1alpha1.Configuration, error) { From ab4f95ad089ea6f881f2a0d5e9f93f395648428e Mon Sep 17 00:00:00 2001 From: Jon Johnson Date: Fri, 9 Nov 2018 18:41:46 +0000 Subject: [PATCH 3/4] Don't emit events for status updates --- .../v1alpha1/autoscaling/autoscaling.go | 23 ++++++----------- .../v1alpha1/clusteringress/clusteringress.go | 19 +++++--------- .../v1alpha1/configuration/configuration.go | 25 +++++++------------ pkg/reconciler/v1alpha1/revision/revision.go | 8 +----- .../v1alpha1/route/reconcile_resources.go | 8 +----- pkg/reconciler/v1alpha1/service/service.go | 19 +++++--------- 6 files changed, 31 insertions(+), 71 deletions(-) diff --git a/pkg/reconciler/v1alpha1/autoscaling/autoscaling.go b/pkg/reconciler/v1alpha1/autoscaling/autoscaling.go index da49c7e917c1..a8bde60fec58 100644 --- a/pkg/reconciler/v1alpha1/autoscaling/autoscaling.go +++ b/pkg/reconciler/v1alpha1/autoscaling/autoscaling.go @@ -23,6 +23,13 @@ import ( "github.com/knative/pkg/controller" "github.com/knative/pkg/logging" + "github.com/knative/serving/pkg/apis/autoscaling" + kpa "github.com/knative/serving/pkg/apis/autoscaling/v1alpha1" + "github.com/knative/serving/pkg/apis/serving" + "github.com/knative/serving/pkg/autoscaler" + informers "github.com/knative/serving/pkg/client/informers/externalversions/autoscaling/v1alpha1" + listers "github.com/knative/serving/pkg/client/listers/autoscaling/v1alpha1" + "github.com/knative/serving/pkg/reconciler" "go.uber.org/zap" corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/equality" @@ -31,14 +38,6 @@ import ( corev1informers "k8s.io/client-go/informers/core/v1" corev1listers "k8s.io/client-go/listers/core/v1" "k8s.io/client-go/tools/cache" - - "github.com/knative/serving/pkg/apis/autoscaling" - kpa "github.com/knative/serving/pkg/apis/autoscaling/v1alpha1" - "github.com/knative/serving/pkg/apis/serving" - "github.com/knative/serving/pkg/autoscaler" - informers "github.com/knative/serving/pkg/client/informers/externalversions/autoscaling/v1alpha1" - listers "github.com/knative/serving/pkg/client/listers/autoscaling/v1alpha1" - "github.com/knative/serving/pkg/reconciler" ) const ( @@ -255,11 +254,5 @@ func (c *Reconciler) updateStatus(desired *kpa.PodAutoscaler) (*kpa.PodAutoscale existing.Status = desired.Status // TODO: for CRD there's no updatestatus, so use normal update - updated, err := c.ServingClientSet.AutoscalingV1alpha1().PodAutoscalers(kpa.Namespace).Update(existing) - if err != nil { - return nil, err - } - - c.Recorder.Eventf(desired, corev1.EventTypeNormal, "Updated", "Updated status for KPA %q", desired.Name) - return updated, nil + return c.ServingClientSet.AutoscalingV1alpha1().PodAutoscalers(kpa.Namespace).Update(existing) } diff --git a/pkg/reconciler/v1alpha1/clusteringress/clusteringress.go b/pkg/reconciler/v1alpha1/clusteringress/clusteringress.go index 0d8735b8db77..4b43ffb95c91 100644 --- a/pkg/reconciler/v1alpha1/clusteringress/clusteringress.go +++ b/pkg/reconciler/v1alpha1/clusteringress/clusteringress.go @@ -20,12 +20,6 @@ import ( "context" "reflect" - "go.uber.org/zap" - corev1 "k8s.io/api/core/v1" - "k8s.io/apimachinery/pkg/api/equality" - apierrs "k8s.io/apimachinery/pkg/api/errors" - "k8s.io/client-go/tools/cache" - "github.com/knative/pkg/apis/istio/v1alpha3" istioinformers "github.com/knative/pkg/client/informers/externalversions/istio/v1alpha3" istiolisters "github.com/knative/pkg/client/listers/istio/v1alpha3" @@ -38,6 +32,11 @@ import ( "github.com/knative/serving/pkg/reconciler" "github.com/knative/serving/pkg/reconciler/v1alpha1/clusteringress/resources" "github.com/knative/serving/pkg/reconciler/v1alpha1/clusteringress/resources/names" + "go.uber.org/zap" + corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/api/equality" + apierrs "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/client-go/tools/cache" ) const controllerAgentName = "clusteringress-controller" @@ -141,13 +140,7 @@ func (c *Reconciler) updateStatus(desired *v1alpha1.ClusterIngress) (*v1alpha1.C existing := ci.DeepCopy() existing.Status = desired.Status // TODO: for CRD there's no updatestatus, so use normal update. - updated, err := c.ServingClientSet.NetworkingV1alpha1().ClusterIngresses().Update(existing) - if err != nil { - return nil, err - } - - c.Recorder.Eventf(desired, corev1.EventTypeNormal, "Updated", "Updated status for ClusterIngress %q", desired.Name) - return updated, nil + return c.ServingClientSet.NetworkingV1alpha1().ClusterIngresses().Update(existing) } func (c *Reconciler) reconcile(ctx context.Context, ci *v1alpha1.ClusterIngress) error { diff --git a/pkg/reconciler/v1alpha1/configuration/configuration.go b/pkg/reconciler/v1alpha1/configuration/configuration.go index 8c298ed44e96..2171eb2b5718 100644 --- a/pkg/reconciler/v1alpha1/configuration/configuration.go +++ b/pkg/reconciler/v1alpha1/configuration/configuration.go @@ -25,6 +25,14 @@ import ( "github.com/knative/pkg/configmap" "github.com/knative/pkg/controller" "github.com/knative/pkg/logging" + "github.com/knative/serving/pkg/apis/serving" + "github.com/knative/serving/pkg/apis/serving/v1alpha1" + servinginformers "github.com/knative/serving/pkg/client/informers/externalversions/serving/v1alpha1" + listers "github.com/knative/serving/pkg/client/listers/serving/v1alpha1" + "github.com/knative/serving/pkg/reconciler" + configns "github.com/knative/serving/pkg/reconciler/v1alpha1/configuration/config" + "github.com/knative/serving/pkg/reconciler/v1alpha1/configuration/resources" + resourcenames "github.com/knative/serving/pkg/reconciler/v1alpha1/configuration/resources/names" "go.uber.org/zap" corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/equality" @@ -34,15 +42,6 @@ import ( "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/labels" "k8s.io/client-go/tools/cache" - - "github.com/knative/serving/pkg/apis/serving" - "github.com/knative/serving/pkg/apis/serving/v1alpha1" - servinginformers "github.com/knative/serving/pkg/client/informers/externalversions/serving/v1alpha1" - listers "github.com/knative/serving/pkg/client/listers/serving/v1alpha1" - "github.com/knative/serving/pkg/reconciler" - configns "github.com/knative/serving/pkg/reconciler/v1alpha1/configuration/config" - "github.com/knative/serving/pkg/reconciler/v1alpha1/configuration/resources" - resourcenames "github.com/knative/serving/pkg/reconciler/v1alpha1/configuration/resources/names" ) const controllerAgentName = "configuration-controller" @@ -280,13 +279,7 @@ func (c *Reconciler) updateStatus(desired *v1alpha1.Configuration) (*v1alpha1.Co existing := config.DeepCopy() existing.Status = desired.Status // TODO: for CRD there's no updatestatus, so use normal update - updated, err := c.ServingClientSet.ServingV1alpha1().Configurations(desired.Namespace).Update(existing) - if err != nil { - return nil, err - } - - c.Recorder.Eventf(desired, corev1.EventTypeNormal, "Updated", "Updated status for Configuration %q", desired.Name) - return updated, nil + return c.ServingClientSet.ServingV1alpha1().Configurations(desired.Namespace).Update(existing) } func (c *Reconciler) gcRevisions(ctx context.Context, config *v1alpha1.Configuration) error { diff --git a/pkg/reconciler/v1alpha1/revision/revision.go b/pkg/reconciler/v1alpha1/revision/revision.go index 66d9ced0b126..adaae1f9bb51 100644 --- a/pkg/reconciler/v1alpha1/revision/revision.go +++ b/pkg/reconciler/v1alpha1/revision/revision.go @@ -418,11 +418,5 @@ func (c *Reconciler) updateStatus(desired *v1alpha1.Revision) (*v1alpha1.Revisio existing := rev.DeepCopy() existing.Status = desired.Status // TODO: for CRD there's no updatestatus, so use normal update - updated, err := c.ServingClientSet.ServingV1alpha1().Revisions(desired.Namespace).Update(existing) - if err != nil { - return nil, err - } - - c.Recorder.Eventf(desired, corev1.EventTypeNormal, "Updated", "Updated status for Revision %q", desired.Name) - return updated, nil + return c.ServingClientSet.ServingV1alpha1().Revisions(desired.Namespace).Update(existing) } diff --git a/pkg/reconciler/v1alpha1/route/reconcile_resources.go b/pkg/reconciler/v1alpha1/route/reconcile_resources.go index e56474bfd48b..4a2ffc7ba6e1 100644 --- a/pkg/reconciler/v1alpha1/route/reconcile_resources.go +++ b/pkg/reconciler/v1alpha1/route/reconcile_resources.go @@ -154,13 +154,7 @@ func (c *Reconciler) updateStatus(desired *v1alpha1.Route) (*v1alpha1.Route, err existing := route.DeepCopy() existing.Status = desired.Status // TODO: for CRD there's no updatestatus, so use normal update. - updated, err := c.ServingClientSet.ServingV1alpha1().Routes(desired.Namespace).Update(existing) - if err != nil { - return nil, err - } - - c.Recorder.Eventf(desired, corev1.EventTypeNormal, "Updated", "Updated status for route %q", desired.Name) - return updated, nil + return c.ServingClientSet.ServingV1alpha1().Routes(desired.Namespace).Update(existing) } // Update the lastPinned annotation on revisions we target so they don't get GC'd. diff --git a/pkg/reconciler/v1alpha1/service/service.go b/pkg/reconciler/v1alpha1/service/service.go index 8003fd3249f9..ec243fca7f68 100644 --- a/pkg/reconciler/v1alpha1/service/service.go +++ b/pkg/reconciler/v1alpha1/service/service.go @@ -23,6 +23,11 @@ import ( "github.com/google/go-cmp/cmp" "github.com/knative/pkg/controller" "github.com/knative/pkg/logging" + "github.com/knative/serving/pkg/apis/serving/v1alpha1" + servinginformers "github.com/knative/serving/pkg/client/informers/externalversions/serving/v1alpha1" + listers "github.com/knative/serving/pkg/client/listers/serving/v1alpha1" + "github.com/knative/serving/pkg/reconciler" + "github.com/knative/serving/pkg/reconciler/v1alpha1/service/resources" resourcenames "github.com/knative/serving/pkg/reconciler/v1alpha1/service/resources/names" "go.uber.org/zap" corev1 "k8s.io/api/core/v1" @@ -30,12 +35,6 @@ import ( "k8s.io/apimachinery/pkg/api/errors" apierrs "k8s.io/apimachinery/pkg/api/errors" "k8s.io/client-go/tools/cache" - - "github.com/knative/serving/pkg/apis/serving/v1alpha1" - servinginformers "github.com/knative/serving/pkg/client/informers/externalversions/serving/v1alpha1" - listers "github.com/knative/serving/pkg/client/listers/serving/v1alpha1" - "github.com/knative/serving/pkg/reconciler" - "github.com/knative/serving/pkg/reconciler/v1alpha1/service/resources" ) const controllerAgentName = "service-controller" @@ -214,13 +213,7 @@ func (c *Reconciler) updateStatus(desired *v1alpha1.Service) (*v1alpha1.Service, existing := service.DeepCopy() existing.Status = desired.Status // TODO: for CRD there's no updatestatus, so use normal update. - updated, err := c.ServingClientSet.ServingV1alpha1().Services(desired.Namespace).Update(existing) - if err != nil { - return nil, err - } - - c.Recorder.Eventf(desired, corev1.EventTypeNormal, "Updated", "Updated status for Service %q", desired.Name) - return updated, nil + return c.ServingClientSet.ServingV1alpha1().Services(desired.Namespace).Update(existing) } func (c *Reconciler) createConfiguration(service *v1alpha1.Service) (*v1alpha1.Configuration, error) { From 989573c388ce25204c631fad659f848d28b32dcc Mon Sep 17 00:00:00 2001 From: Jon Johnson Date: Fri, 9 Nov 2018 18:51:52 +0000 Subject: [PATCH 4/4] Fix unit test A route test was expecting a status update event. --- pkg/reconciler/v1alpha1/route/route_test.go | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/pkg/reconciler/v1alpha1/route/route_test.go b/pkg/reconciler/v1alpha1/route/route_test.go index 14b8942f01d4..86c7b6437457 100644 --- a/pkg/reconciler/v1alpha1/route/route_test.go +++ b/pkg/reconciler/v1alpha1/route/route_test.go @@ -38,6 +38,7 @@ import ( "github.com/knative/serving/pkg/gc" rclr "github.com/knative/serving/pkg/reconciler" "github.com/knative/serving/pkg/reconciler/v1alpha1/route/config" + . "github.com/knative/serving/pkg/reconciler/v1alpha1/testing" "github.com/knative/serving/pkg/system" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -46,8 +47,6 @@ import ( "k8s.io/apimachinery/pkg/watch" kubeinformers "k8s.io/client-go/informers" fakekubeclientset "k8s.io/client-go/kubernetes/fake" - - . "github.com/knative/serving/pkg/reconciler/v1alpha1/testing" ) const ( @@ -273,7 +272,6 @@ func TestCreateRouteForOneReserveRevision(t *testing.T) { // hooks here. Each hook tests for a specific event. h.OnCreate(&kubeClient.Fake, "events", ExpectNormalEventDelivery(t, `Created service "test-route"`)) h.OnCreate(&kubeClient.Fake, "events", ExpectNormalEventDelivery(t, "^Created ClusterIngress.*" /*ingress name is unset in test*/)) - h.OnCreate(&kubeClient.Fake, "events", ExpectNormalEventDelivery(t, `Updated status for route "test-route"`)) // An inactive revision rev := getTestRevisionWithCondition("test-rev",