diff --git a/pkg/controller/route/cruds.go b/pkg/controller/route/cruds.go index 131f252cce05..27ad828e46b6 100644 --- a/pkg/controller/route/cruds.go +++ b/pkg/controller/route/cruds.go @@ -71,6 +71,8 @@ func (c *Controller) reconcilePlaceholderService(ctx context.Context, route *v1a return nil } +// Update the Status of the route. Caller is responsible for checking +// for semantic differences before calling. func (c *Controller) updateStatus(ctx context.Context, route *v1alpha1.Route) (*v1alpha1.Route, error) { logger := logging.FromContext(ctx) @@ -81,12 +83,10 @@ func (c *Controller) updateStatus(ctx context.Context, route *v1alpha1.Route) (* c.Recorder.Eventf(route, corev1.EventTypeWarning, "UpdateFailed", "Failed to get current status for route %q: %v", route.Name, err) return nil, err } - // If there's nothing to update, just return. if reflect.DeepEqual(existing.Status, route.Status) { return existing, nil } - existing.Status = route.Status // TODO: for CRD there's no updatestatus, so use normal update. updated, err := routeClient.Update(existing) diff --git a/pkg/controller/route/queueing_test.go b/pkg/controller/route/queueing_test.go index 6de563e2f0dd..4ac24b8fa632 100644 --- a/pkg/controller/route/queueing_test.go +++ b/pkg/controller/route/queueing_test.go @@ -22,7 +22,6 @@ import ( "github.com/knative/serving/pkg" "github.com/knative/serving/pkg/apis/serving/v1alpha1" - "github.com/knative/serving/pkg/autoscaler" fakeclientset "github.com/knative/serving/pkg/client/clientset/versioned/fake" informers "github.com/knative/serving/pkg/client/informers/externalversions" "github.com/knative/serving/pkg/configmap" @@ -72,20 +71,6 @@ func TestNewRouteCallsSyncHandler(t *testing.T) { defaultDomainSuffix: "", prodDomainSuffix: "selector:\n app: prod", }, - }, &corev1.ConfigMap{ - ObjectMeta: metav1.ObjectMeta{ - Namespace: pkg.GetServingSystemNamespace(), - Name: autoscaler.ConfigName, - }, - Data: map[string]string{ - "max-scale-up-rate": "1.0", - "single-concurrency-target": "1.0", - "multi-concurrency-target": "1.0", - "stable-window": "5m", - "panic-window": "10s", - "scale-to-zero-threshold": "10m", - "concurrency-quantum-of-time": "100ms", - }, }) servingClient := fakeclientset.NewSimpleClientset(rev, route) diff --git a/pkg/controller/route/route.go b/pkg/controller/route/route.go index 4d6c6b3560a7..3ef54154b321 100644 --- a/pkg/controller/route/route.go +++ b/pkg/controller/route/route.go @@ -21,8 +21,6 @@ import ( "fmt" "sync" - "github.com/knative/serving/pkg/autoscaler" - corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/equality" apierrs "k8s.io/apimachinery/pkg/api/errors" @@ -56,11 +54,6 @@ type Controller struct { // must go through domainConfigMutex domainConfig *DomainConfig domainConfigMutex sync.Mutex - - // autoscalerConfig could change over time and access to it - // must go through autoscalerConfigMutex - autoscalerConfig *autoscaler.Config - autoscalerConfigMutex sync.Mutex } // NewController initializes the controller and is called by the generated code @@ -83,24 +76,16 @@ func NewController( c.Logger.Info("Setting up event handlers") routeInformer.Informer().AddEventHandler(cache.ResourceEventHandlerFuncs{ - AddFunc: c.Enqueue, - UpdateFunc: func(old, new interface{}) { - c.Enqueue(new) - }, + AddFunc: c.Enqueue, + UpdateFunc: controller.PassNew(c.Enqueue), DeleteFunc: c.Enqueue, }) - configInformer.Informer().AddEventHandler(cache.ResourceEventHandlerFuncs{ - AddFunc: func(obj interface{}) { - c.SyncConfiguration(obj.(*v1alpha1.Configuration)) - }, - UpdateFunc: func(old, new interface{}) { - c.SyncConfiguration(new.(*v1alpha1.Configuration)) - }, + AddFunc: c.EnqueueReferringRoute, + UpdateFunc: controller.PassNew(c.EnqueueReferringRoute), }) + c.Logger.Info("Setting up ConfigMap receivers") opt.ConfigMapWatcher.Watch(controller.GetDomainConfigMapName(), c.receiveDomainConfig) - opt.ConfigMapWatcher.Watch(autoscaler.ConfigName, c.receiveAutoscalerConfig) - return c } @@ -109,17 +94,17 @@ func NewController( // is closed, at which point it will shutdown the workqueue and wait for // workers to finish processing their current work items. func (c *Controller) Run(threadiness int, stopCh <-chan struct{}) error { - return c.RunController(threadiness, stopCh, c.updateRouteEvent, "Route") + return c.RunController(threadiness, stopCh, c.Reconcile, "Route") } ///////////////////////////////////////// // Event handlers ///////////////////////////////////////// -// updateRouteEvent compares the actual state with the desired, and attempts to +// Reconcile compares the actual state with the desired, and attempts to // converge the two. It then updates the Status block of the Route resource // with the current status of the resource. -func (c *Controller) updateRouteEvent(key string) error { +func (c *Controller) Reconcile(key string) error { // Convert the namespace/name string into a distinct namespace and name namespace, name, err := cache.SplitMetaNamespaceKey(key) if err != nil { @@ -169,23 +154,23 @@ func (c *Controller) reconcile(ctx context.Context, route *v1alpha1.Route) error } // Call configureTrafficAndUpdateRouteStatus, which also updates the Route.Status // to contain the domain we will use for Gateway creation. - if _, err := c.configureTrafficAndUpdateRouteStatus(ctx, route); err != nil { + if _, err := c.configureTraffic(ctx, route); err != nil { return err } logger.Info("Route successfully synced") return nil } -// configureTrafficAndUpdateRouteStatus attempts to configure traffic based on the RouteSpec. If there are missing -// targets (e.g. Configurations without a Ready Revision, or Revision that isn't Ready or Inactive), no traffic will be -// configured. +// configureTraffic attempts to configure traffic based on the RouteSpec. If there are missing +// targets (e.g. Configurations without a Ready Revision, or Revision that isn't Ready or Inactive), +// no traffic will be configured. // -// If traffic is configured we update the RouteStatus with AllTrafficAssigned = True. Otherwise we mark -// AllTrafficAssigned = False, with a message referring to one of the missing target. +// If traffic is configured we update the RouteStatus with AllTrafficAssigned = True. Otherwise we +// mark AllTrafficAssigned = False, with a message referring to one of the missing target. // -// In all cases we will add annotations to the referred targets. This is so that when they become routable we can know -// (through a listener) and attempt traffic configuration again. -func (c *Controller) configureTrafficAndUpdateRouteStatus(ctx context.Context, r *v1alpha1.Route) (*v1alpha1.Route, error) { +// In all cases we will add annotations to the referred targets. This is so that when they become +// routable we can know (through a listener) and attempt traffic configuration again. +func (c *Controller) configureTraffic(ctx context.Context, r *v1alpha1.Route) (*v1alpha1.Route, error) { r.Status.Domain = c.routeDomain(r) logger := logging.FromContext(ctx) t, targetErr := traffic.BuildTrafficConfiguration( @@ -212,34 +197,31 @@ func (c *Controller) configureTrafficAndUpdateRouteStatus(ctx context.Context, r return r, nil } -func (c *Controller) SyncConfiguration(config *v1alpha1.Configuration) { - configName := config.Name - ns := config.Namespace - +func (c *Controller) EnqueueReferringRoute(obj interface{}) { + config, ok := obj.(*v1alpha1.Configuration) + if !ok { + c.Logger.Infof("Ignoring non-Configuration objects %v", obj) + return + } if config.Status.LatestReadyRevisionName == "" { - c.Logger.Infof("Configuration %s is not ready", configName) + fmt.Printf("Configuration %s is not ready\n", config.Name) + c.Logger.Infof("Configuration %s is not ready", config.Name) return } // Check whether is configuration is referred by a route. routeName, ok := config.Labels[serving.RouteLabelKey] if !ok { - c.Logger.Infof("Configuration %s does not have label %s", configName, serving.RouteLabelKey) + c.Logger.Infof("Configuration %s does not have a referrring route", config.Name) return } // Configuration is referred by a Route. Update such Route. - logger := loggerWithRouteInfo(c.Logger, ns, routeName) - ctx := logging.WithLogger(context.TODO(), logger) - route, err := c.routeLister.Routes(ns).Get(routeName) + route, err := c.routeLister.Routes(config.Namespace).Get(routeName) if err != nil { - logger.Error("Error fetching route upon configuration becoming ready", zap.Error(err)) + loggerWithRouteInfo(c.Logger, config.Namespace, routeName).Error( + "Error fetching route upon configuration becoming ready", zap.Error(err)) return } - // Don't modify the informers copy. - route = route.DeepCopy() - if _, err := c.configureTrafficAndUpdateRouteStatus(ctx, route); err != nil { - logger.Error("Error updating route upon configuration becoming ready", zap.Error(err)) - } - c.updateStatus(ctx, route) + c.Enqueue(route) } ///////////////////////////////////////// @@ -272,25 +254,3 @@ func (c *Controller) receiveDomainConfig(configMap *corev1.ConfigMap) { defer c.domainConfigMutex.Unlock() c.domainConfig = newDomainConfig } - -func (c *Controller) receiveAutoscalerConfig(configMap *corev1.ConfigMap) { - newAutoscalerConfig, err := autoscaler.NewConfigFromConfigMap(configMap) - c.autoscalerConfigMutex.Lock() - defer c.autoscalerConfigMutex.Unlock() - if err != nil { - if c.autoscalerConfig != nil { - c.Logger.Errorf("Error updating Autoscaler ConfigMap: %v", err) - } else { - c.Logger.Fatalf("Error initializing Autoscaler ConfigMap: %v", err) - } - return - } - c.Logger.Infof("Autoscaler config map is added or updated: %v", configMap) - c.autoscalerConfig = newAutoscalerConfig -} - -func (c *Controller) getAutoscalerConfig() *autoscaler.Config { - c.autoscalerConfigMutex.Lock() - defer c.autoscalerConfigMutex.Unlock() - return c.autoscalerConfig -} diff --git a/pkg/controller/route/route_test.go b/pkg/controller/route/route_test.go index ac4bca44e176..d66b2fb32a8f 100644 --- a/pkg/controller/route/route_test.go +++ b/pkg/controller/route/route_test.go @@ -31,7 +31,6 @@ import ( "time" "github.com/knative/serving/pkg" - "github.com/knative/serving/pkg/autoscaler" "github.com/knative/serving/pkg/configmap" "go.uber.org/zap" @@ -185,20 +184,6 @@ func newTestController(configs ...*corev1.ConfigMap) ( defaultDomainSuffix: "", prodDomainSuffix: "selector:\n app: prod", }, - }, &corev1.ConfigMap{ - ObjectMeta: metav1.ObjectMeta{ - Namespace: pkg.GetServingSystemNamespace(), - Name: autoscaler.ConfigName, - }, - Data: map[string]string{ - "max-scale-up-rate": "1.0", - "single-concurrency-target": "1.0", - "multi-concurrency-target": "1.0", - "stable-window": "5m", - "panic-window": "10s", - "scale-to-zero-threshold": "10m", - "concurrency-quantum-of-time": "100ms", - }, }) for _, cm := range configs { cms = append(cms, cm) @@ -248,10 +233,10 @@ func TestCreateRouteCreatesStuff(t *testing.T) { }}, ) servingClient.ServingV1alpha1().Routes(testNamespace).Create(route) - // Since updateRouteEvent looks in the lister, we need to add it to the informer + // Since Reconcile looks in the lister, we need to add it to the informer servingInformer.Serving().V1alpha1().Routes().Informer().GetIndexer().Add(route) - controller.updateRouteEvent(KeyOrDie(route)) + controller.Reconcile(KeyOrDie(route)) // Look for the placeholder service. expectedServiceName := fmt.Sprintf("%s-service", route.Name) @@ -333,22 +318,7 @@ func TestCreateRouteCreatesStuff(t *testing.T) { // Test the only revision in the route is in Reserve (inactive) serving status. func TestCreateRouteForOneReserveRevision(t *testing.T) { - kubeClient, servingClient, controller, _, servingInformer, _ := newTestController(&corev1.ConfigMap{ - ObjectMeta: metav1.ObjectMeta{ - Namespace: pkg.GetServingSystemNamespace(), - Name: autoscaler.ConfigName, - }, - Data: map[string]string{ - "enable-scale-to-zero": "true", - "max-scale-up-rate": "1.0", - "single-concurrency-target": "1.0", - "multi-concurrency-target": "1.0", - "stable-window": "5m", - "panic-window": "10s", - "scale-to-zero-threshold": "10m", - "concurrency-quantum-of-time": "100ms", - }, - }) + kubeClient, servingClient, controller, _, servingInformer, _ := newTestController() h := NewHooks() // Look for the events. Events are delivered asynchronously so we need to use @@ -374,10 +344,10 @@ func TestCreateRouteForOneReserveRevision(t *testing.T) { }}, ) servingClient.ServingV1alpha1().Routes(testNamespace).Create(route) - // Since updateRouteEvent looks in the lister, we need to add it to the informer + // Since Reconcile looks in the lister, we need to add it to the informer servingInformer.Serving().V1alpha1().Routes().Informer().GetIndexer().Add(route) - controller.updateRouteEvent(KeyOrDie(route)) + controller.Reconcile(KeyOrDie(route)) // Look for the route rule with activator as the destination. vs, err := servingClient.NetworkingV1alpha3().VirtualServices(testNamespace).Get(ctrl.GetVirtualServiceName(route), metav1.GetOptions{}) @@ -452,7 +422,7 @@ func TestCreateRouteWithMultipleTargets(t *testing.T) { cfgrev := getTestRevisionForConfig(config) config.Status.LatestReadyRevisionName = cfgrev.Name servingClient.ServingV1alpha1().Configurations(testNamespace).Create(config) - // Since updateRouteEvent looks in the lister, we need to add it to the informer + // Since Reconcile looks in the lister, we need to add it to the informer servingInformer.Serving().V1alpha1().Configurations().Informer().GetIndexer().Add(config) servingClient.ServingV1alpha1().Revisions(testNamespace).Create(cfgrev) @@ -467,10 +437,10 @@ func TestCreateRouteWithMultipleTargets(t *testing.T) { }}, ) servingClient.ServingV1alpha1().Routes(testNamespace).Create(route) - // Since updateRouteEvent looks in the lister, we need to add it to the informer + // Since Reconcile looks in the lister, we need to add it to the informer servingInformer.Serving().V1alpha1().Routes().Informer().GetIndexer().Add(route) - controller.updateRouteEvent(KeyOrDie(route)) + controller.Reconcile(KeyOrDie(route)) vs, err := servingClient.NetworkingV1alpha3().VirtualServices(testNamespace).Get(ctrl.GetVirtualServiceName(route), metav1.GetOptions{}) if err != nil { @@ -521,23 +491,7 @@ func TestCreateRouteWithMultipleTargets(t *testing.T) { // Test one out of multiple target revisions is in Reserve serving state. func TestCreateRouteWithOneTargetReserve(t *testing.T) { - _, servingClient, controller, _, servingInformer, _ := newTestController(&corev1.ConfigMap{ - ObjectMeta: metav1.ObjectMeta{ - Namespace: pkg.GetServingSystemNamespace(), - Name: autoscaler.ConfigName, - }, - Data: map[string]string{ - "enable-scale-to-zero": "true", - "max-scale-up-rate": "1.0", - "single-concurrency-target": "1.0", - "multi-concurrency-target": "1.0", - "stable-window": "5m", - "panic-window": "10s", - "scale-to-zero-threshold": "10m", - "concurrency-quantum-of-time": "100ms", - }, - }) - + _, servingClient, controller, _, servingInformer, _ := newTestController() // A standalone inactive revision rev := getTestRevisionWithCondition("test-rev", v1alpha1.RevisionCondition{ @@ -553,7 +507,7 @@ func TestCreateRouteWithOneTargetReserve(t *testing.T) { cfgrev := getTestRevisionForConfig(config) config.Status.LatestReadyRevisionName = cfgrev.Name servingClient.ServingV1alpha1().Configurations(testNamespace).Create(config) - // Since updateRouteEvent looks in the lister, we need to add it to the informer + // Since Reconcile looks in the lister, we need to add it to the informer servingInformer.Serving().V1alpha1().Configurations().Informer().GetIndexer().Add(config) servingClient.ServingV1alpha1().Revisions(testNamespace).Create(cfgrev) @@ -568,10 +522,10 @@ func TestCreateRouteWithOneTargetReserve(t *testing.T) { }}, ) servingClient.ServingV1alpha1().Routes(testNamespace).Create(route) - // Since updateRouteEvent looks in the lister, we need to add it to the informer + // Since Reconcile looks in the lister, we need to add it to the informer servingInformer.Serving().V1alpha1().Routes().Informer().GetIndexer().Add(route) - controller.updateRouteEvent(KeyOrDie(route)) + controller.Reconcile(KeyOrDie(route)) vs, err := servingClient.NetworkingV1alpha3().VirtualServices(testNamespace).Get(ctrl.GetVirtualServiceName(route), metav1.GetOptions{}) if err != nil { @@ -633,7 +587,7 @@ func TestCreateRouteWithDuplicateTargets(t *testing.T) { cfgrev := getTestRevisionForConfig(config) config.Status.LatestReadyRevisionName = cfgrev.Name servingClient.ServingV1alpha1().Configurations(testNamespace).Create(config) - // Since updateRouteEvent looks in the lister, we need to add it to the informer + // Since Reconcile looks in the lister, we need to add it to the informer servingInformer.Serving().V1alpha1().Configurations().Informer().GetIndexer().Add(config) servingClient.ServingV1alpha1().Revisions(testNamespace).Create(cfgrev) @@ -666,10 +620,10 @@ func TestCreateRouteWithDuplicateTargets(t *testing.T) { }}, ) servingClient.ServingV1alpha1().Routes(testNamespace).Create(route) - // Since updateRouteEvent looks in the lister, we need to add it to the informer + // Since Reconcile looks in the lister, we need to add it to the informer servingInformer.Serving().V1alpha1().Routes().Informer().GetIndexer().Add(route) - controller.updateRouteEvent(KeyOrDie(route)) + controller.Reconcile(KeyOrDie(route)) vs, err := servingClient.NetworkingV1alpha3().VirtualServices(testNamespace).Get(ctrl.GetVirtualServiceName(route), metav1.GetOptions{}) if err != nil { @@ -749,7 +703,7 @@ func TestCreateRouteWithNamedTargets(t *testing.T) { cfgrev := getTestRevisionForConfig(config) config.Status.LatestReadyRevisionName = cfgrev.Name servingClient.ServingV1alpha1().Configurations(testNamespace).Create(config) - // Since updateRouteEvent looks in the lister, we need to add it to the informer + // Since Reconcile looks in the lister, we need to add it to the informer servingInformer.Serving().V1alpha1().Configurations().Informer().GetIndexer().Add(config) servingClient.ServingV1alpha1().Revisions(testNamespace).Create(cfgrev) @@ -768,10 +722,10 @@ func TestCreateRouteWithNamedTargets(t *testing.T) { ) servingClient.ServingV1alpha1().Routes(testNamespace).Create(route) - // Since updateRouteEvent looks in the lister, we need to add it to the informer + // Since Reconcile looks in the lister, we need to add it to the informer servingInformer.Serving().V1alpha1().Routes().Informer().GetIndexer().Add(route) - controller.updateRouteEvent(KeyOrDie(route)) + controller.Reconcile(KeyOrDie(route)) vs, err := servingClient.NetworkingV1alpha3().VirtualServices(testNamespace).Get(ctrl.GetVirtualServiceName(route), metav1.GetOptions{}) if err != nil { @@ -852,9 +806,9 @@ func TestSetLabelToConfigurationDirectlyConfigured(t *testing.T) { servingClient.ServingV1alpha1().Configurations(testNamespace).Create(config) servingClient.ServingV1alpha1().Revisions(testNamespace).Create(rev) servingClient.ServingV1alpha1().Routes(testNamespace).Create(route) - // Since updateRouteEvent looks in the lister, we need to add it to the informer + // Since Reconcile looks in the lister, we need to add it to the informer servingInformer.Serving().V1alpha1().Routes().Informer().GetIndexer().Add(route) - controller.updateRouteEvent(KeyOrDie(route)) + controller.Reconcile(KeyOrDie(route)) config, err := servingClient.ServingV1alpha1().Configurations(testNamespace).Get(config.Name, metav1.GetOptions{}) if err != nil { @@ -884,7 +838,7 @@ func TestCreateRouteWithInvalidConfigurationShouldReturnError(t *testing.T) { servingClient.ServingV1alpha1().Configurations(testNamespace).Create(config) servingClient.ServingV1alpha1().Revisions(testNamespace).Create(rev) servingClient.ServingV1alpha1().Routes(testNamespace).Create(route) - // Since updateRouteEvent looks in the lister, we need to add it to the informer + // Since Reconcile looks in the lister, we need to add it to the informer servingInformer.Serving().V1alpha1().Routes().Informer().GetIndexer().Add(route) // No configuration updates. @@ -905,7 +859,7 @@ func TestCreateRouteWithInvalidConfigurationShouldReturnError(t *testing.T) { expectedErrMsg := "Configuration \"test-config\" is already in use by \"another-route\", and cannot be used by \"test-route\"" // Should return error. - err := controller.updateRouteEvent(route.Namespace + "/" + route.Name) + err := controller.Reconcile(route.Namespace + "/" + route.Name) if wanted, got := expectedErrMsg, err.Error(); wanted != got { t.Errorf("unexpected error: %q expected: %q", got, wanted) } @@ -927,12 +881,12 @@ func TestCreateRouteRevisionMissingCondition(t *testing.T) { servingClient.ServingV1alpha1().Configurations(testNamespace).Create(config) servingClient.ServingV1alpha1().Revisions(testNamespace).Create(rev) servingClient.ServingV1alpha1().Routes(testNamespace).Create(route) - // Since updateRouteEvent looks in the lister, we need to add it to the informer + // Since Reconcile looks in the lister, we need to add it to the informer servingInformer.Serving().V1alpha1().Routes().Informer().GetIndexer().Add(route) expectedErrMsg := `revisions.serving.knative.dev "does-not-exist" not found` // Should return error. - err := controller.updateRouteEvent(route.Namespace + "/" + route.Name) + err := controller.Reconcile(route.Namespace + "/" + route.Name) if wanted, got := expectedErrMsg, err.Error(); wanted != got { t.Errorf("unexpected error: %q expected: %q", got, wanted) } @@ -971,12 +925,12 @@ func TestCreateRouteConfigurationMissingCondition(t *testing.T) { servingClient.ServingV1alpha1().Configurations(testNamespace).Create(config) servingClient.ServingV1alpha1().Revisions(testNamespace).Create(rev) servingClient.ServingV1alpha1().Routes(testNamespace).Create(route) - // Since updateRouteEvent looks in the lister, we need to add it to the informer + // Since Reconcile looks in the lister, we need to add it to the informer servingInformer.Serving().V1alpha1().Routes().Informer().GetIndexer().Add(route) expectedErrMsg := `configurations.serving.knative.dev "does-not-exist" not found` // Should return error. - err := controller.updateRouteEvent(route.Namespace + "/" + route.Name) + err := controller.Reconcile(route.Namespace + "/" + route.Name) if wanted, got := expectedErrMsg, err.Error(); wanted != got { t.Errorf("unexpected error: %q expected: %q", got, wanted) } @@ -1016,10 +970,10 @@ func TestSetLabelNotChangeConfigurationLabelIfLabelExists(t *testing.T) { servingClient.ServingV1alpha1().Configurations(testNamespace).Create(config) servingClient.ServingV1alpha1().Revisions(testNamespace).Create(rev) servingClient.ServingV1alpha1().Routes(testNamespace).Create(route) - // Since updateRouteEvent looks in the lister, we need to add it to the informer + // Since Reconcile looks in the lister, we need to add it to the informer servingInformer.Serving().V1alpha1().Routes().Informer().GetIndexer().Add(route) - // Assert that the configuration is not updated when updateRouteEvent is called. + // Assert that the configuration is not updated when Reconcile is called. servingClient.Fake.PrependReactor("update", "configurations", func(a kubetesting.Action) (bool, runtime.Object, error) { t.Error("Configuration was updated unexpectedly") @@ -1027,7 +981,7 @@ func TestSetLabelNotChangeConfigurationLabelIfLabelExists(t *testing.T) { }, ) - // Assert that the revision is not updated when updateRouteEvent is called. + // Assert that the revision is not updated when Reconcile is called. servingClient.Fake.PrependReactor("update", "revision", func(a kubetesting.Action) (bool, runtime.Object, error) { t.Error("Revision was updated unexpectedly") @@ -1035,7 +989,7 @@ func TestSetLabelNotChangeConfigurationLabelIfLabelExists(t *testing.T) { }, ) - controller.updateRouteEvent(route.Namespace + "/" + route.Name) + controller.Reconcile(route.Namespace + "/" + route.Name) } func TestDeleteLabelOfConfigurationWhenUnconfigured(t *testing.T) { @@ -1048,10 +1002,10 @@ func TestDeleteLabelOfConfigurationWhenUnconfigured(t *testing.T) { servingClient.ServingV1alpha1().Configurations(testNamespace).Create(config) servingClient.ServingV1alpha1().Revisions(testNamespace).Create(rev) - // Since updateRouteEvent looks in the lister, we need to add it to the informer + // Since Reconcile looks in the lister, we need to add it to the informer servingInformer.Serving().V1alpha1().Routes().Informer().GetIndexer().Add(route) servingClient.ServingV1alpha1().Routes(testNamespace).Create(route) - controller.updateRouteEvent(KeyOrDie(route)) + controller.Reconcile(KeyOrDie(route)) config, err := servingClient.ServingV1alpha1().Configurations(testNamespace).Get(config.Name, metav1.GetOptions{}) if err != nil { @@ -1078,7 +1032,7 @@ func TestUpdateRouteDomainWhenRouteLabelChanges(t *testing.T) { // Create a route. servingInformer.Serving().V1alpha1().Routes().Informer().GetIndexer().Add(route) routeClient.Create(route) - controller.updateRouteEvent(KeyOrDie(route)) + controller.Reconcile(KeyOrDie(route)) // Confirms that by default the route get the defaultDomainSuffix. expectations := []struct { @@ -1095,7 +1049,7 @@ func TestUpdateRouteDomainWhenRouteLabelChanges(t *testing.T) { route.ObjectMeta.Labels = expectation.labels servingInformer.Serving().V1alpha1().Routes().Informer().GetIndexer().Add(route) routeClient.Update(route) - controller.updateRouteEvent(KeyOrDie(route)) + controller.Reconcile(KeyOrDie(route)) // Confirms that the new route get the correct domain. route, _ = routeClient.Get(route.Name, metav1.GetOptions{}) @@ -1106,7 +1060,7 @@ func TestUpdateRouteDomainWhenRouteLabelChanges(t *testing.T) { } } -func TestUpdateRouteWhenConfigurationChanges(t *testing.T) { +func TestEnqueueReferringRoute(t *testing.T) { _, servingClient, controller, _, servingInformer, _ := newTestController() routeClient := servingClient.ServingV1alpha1().Routes(testNamespace) @@ -1120,65 +1074,26 @@ func TestUpdateRouteWhenConfigurationChanges(t *testing.T) { ) routeClient.Create(route) - // Since updateRouteEvent looks in the lister, we need to add it to the informer + // Since EnqueueReferringRoute looks in the lister, we need to add it to the informer servingInformer.Serving().V1alpha1().Routes().Informer().GetIndexer().Add(route) - servingClient.ServingV1alpha1().Configurations(testNamespace).Create(config) - // Since SyncConfiguration looks in the lister, we need to add it to the - // informer - servingInformer.Serving().V1alpha1().Configurations().Informer().GetIndexer().Add(config) - servingClient.ServingV1alpha1().Revisions(testNamespace).Create(rev) - - controller.SyncConfiguration(config) - - route, err := routeClient.Get(route.Name, metav1.GetOptions{}) - if err != nil { - t.Fatalf("Couldn't get route: %v", err) - } - - // The configuration has no LatestReadyRevisionName, so there should be no - // routed targets. - var expectedTrafficTargets []v1alpha1.TrafficTarget - if diff := cmp.Diff(expectedTrafficTargets, route.Status.Traffic); diff != "" { - t.Errorf("Unexpected label diff (-want +got): %v", diff) - } // Update config to have LatestReadyRevisionName and route label. config.Status.LatestReadyRevisionName = rev.Name config.Labels = map[string]string{ serving.RouteLabelKey: route.Name, } - - // We need to update the config in the client since getDirectTrafficTargets - // gets the configuration from there - servingClient.ServingV1alpha1().Configurations(testNamespace).Update(config) - // Since SyncConfiguration looks in the lister, we need to add it to the - // informer - servingInformer.Serving().V1alpha1().Configurations().Informer().GetIndexer().Add(config) - controller.SyncConfiguration(config) - - route, err = routeClient.Get(route.Name, metav1.GetOptions{}) - if err != nil { - t.Fatalf("Couldn't get route: %v", err) - } - // Now the configuration has a LatestReadyRevisionName, so its revision should - // be targeted - expectedTrafficTargets = []v1alpha1.TrafficTarget{{ - ConfigurationName: config.Name, - RevisionName: rev.Name, - Percent: 100, - }} - if diff := cmp.Diff(expectedTrafficTargets, route.Status.Traffic); diff != "" { - t.Errorf("Unexpected traffic target diff (-want +got): %v", diff) + controller.EnqueueReferringRoute(config) + // add this fake queue end marker. + controller.WorkQueue.AddRateLimited("queue-has-no-work") + expected := fmt.Sprintf("%s/%s", route.Namespace, route.Name) + if k, _ := controller.WorkQueue.Get(); k != expected { + t.Errorf("Expected %q, saw %q", expected, k) } - expectedDomainPrefix := fmt.Sprintf("%s.%s.", route.Name, route.Namespace) - if !strings.HasPrefix(route.Status.Domain, expectedDomainPrefix) { - t.Errorf("Route domain %q must have prefix %q", route.Status.Domain, expectedDomainPrefix) - } - } -func TestAddConfigurationEventNotUpdateAnythingIfHasNoLatestReady(t *testing.T) { - _, servingClient, controller, _, servingInformer, _ := newTestController() +func TestEnqueueReferringRouteNotEnqueueIfCannotFindRoute(t *testing.T) { + _, _, controller, _, _, _ := newTestController() + config := getTestConfiguration() rev := getTestRevisionForConfig(config) route := getTestRouteWithTrafficTargets( @@ -1187,33 +1102,82 @@ func TestAddConfigurationEventNotUpdateAnythingIfHasNoLatestReady(t *testing.T) Percent: 100, }}, ) - // If set config.Status.LatestReadyRevisionName = rev.Name, the test should fail. - config.Status.LatestCreatedRevisionName = rev.Name - config.Labels = map[string]string{serving.RouteLabelKey: route.Name} - servingClient.ServingV1alpha1().Configurations(testNamespace).Create(config) - servingClient.ServingV1alpha1().Revisions(testNamespace).Create(rev) - servingClient.ServingV1alpha1().Routes(testNamespace).Create(route) - // Since updateRouteEvent looks in the lister, we need to add it to the informer - servingInformer.Serving().V1alpha1().Routes().Informer().GetIndexer().Add(route) + // Update config to have LatestReadyRevisionName and route label. + config.Status.LatestReadyRevisionName = rev.Name + config.Labels = map[string]string{ + serving.RouteLabelKey: route.Name, + } + controller.EnqueueReferringRoute(config) + // add this item to avoid being blocked by queue. + expected := "queue-has-no-work" + controller.WorkQueue.AddRateLimited(expected) + if k, _ := controller.WorkQueue.Get(); k != expected { + t.Errorf("Expected %v, saw %v", expected, k) + } +} - // No configuration updates - servingClient.Fake.PrependReactor("update", "configurations", - func(a kubetesting.Action) (bool, runtime.Object, error) { - t.Error("Configuration was updated unexpectedly") - return true, nil, nil - }, - ) +func TestReconcileIgnoresMissingRoute(t *testing.T) { + _, _, controller, _, _, _ := newTestController() + if controller.Reconcile("nonexisting/route") != nil { + t.Error("Expected missing routes to be silently ignored") + } +} - // No route updates - servingClient.Fake.PrependReactor("update", "routes", - func(a kubetesting.Action) (bool, runtime.Object, error) { - t.Error("Route was updated unexpectedly") - return true, nil, nil - }, - ) +func TestReconcileRejectsInvalidKey(t *testing.T) { + _, _, controller, _, _, _ := newTestController() + if controller.Reconcile("invalid/key/should/be/silently/ignored") != nil { + t.Error("Expected invalid key to be silently ignored") + } +} - controller.SyncConfiguration(config) +func TestEnqueueReferringRouteNotEnqueueIfHasNoLatestReady(t *testing.T) { + _, _, controller, _, _, _ := newTestController() + config := getTestConfiguration() + + controller.EnqueueReferringRoute(config) + // add this item to avoid being blocked by queue. + expected := "queue-has-no-work" + controller.WorkQueue.AddRateLimited(expected) + if k, _ := controller.WorkQueue.Get(); k != expected { + t.Errorf("Expected %v, saw %v", expected, k) + } +} + +func TestEnqueueReferringRouteNotEnqueueIfHavingNoRouteLabel(t *testing.T) { + _, _, controller, _, _, _ := newTestController() + config := getTestConfiguration() + rev := getTestRevisionForConfig(config) + fmt.Println(rev.Name) + config.Status.LatestReadyRevisionName = rev.Name + + if controller.WorkQueue.Len() > 0 { + t.Errorf("Expecting no route sync work prior to config change") + } + controller.EnqueueReferringRoute(config) + // add this item to avoid being blocked by queue. + expected := "queue-has-no-work" + controller.WorkQueue.AddRateLimited(expected) + if k, _ := controller.WorkQueue.Get(); k != expected { + t.Errorf("Expected %v, saw %v", expected, k) + } +} + +func TestEnqueueReferringRouteNotEnqueueIfNotGivenAConfig(t *testing.T) { + _, _, controller, _, _, _ := newTestController() + config := getTestConfiguration() + rev := getTestRevisionForConfig(config) + + if controller.WorkQueue.Len() > 0 { + t.Errorf("Expecting no route sync work prior to config change") + } + controller.EnqueueReferringRoute(rev) + // add this item to avoid being blocked by queue. + expected := "queue-has-no-work" + controller.WorkQueue.AddRateLimited(expected) + if k, _ := controller.WorkQueue.Get(); k != expected { + t.Errorf("Expected %v, saw %v", expected, k) + } } func TestUpdateDomainConfigMap(t *testing.T) { @@ -1224,7 +1188,7 @@ func TestUpdateDomainConfigMap(t *testing.T) { // Create a route. servingInformer.Serving().V1alpha1().Routes().Informer().GetIndexer().Add(route) routeClient.Create(route) - controller.updateRouteEvent(KeyOrDie(route)) + controller.Reconcile(KeyOrDie(route)) route.ObjectMeta.Labels = map[string]string{"app": "prod"} @@ -1287,7 +1251,7 @@ func TestUpdateDomainConfigMap(t *testing.T) { expectation.apply() servingInformer.Serving().V1alpha1().Routes().Informer().GetIndexer().Add(route) routeClient.Update(route) - controller.updateRouteEvent(KeyOrDie(route)) + controller.Reconcile(KeyOrDie(route)) route, _ = routeClient.Get(route.Name, metav1.GetOptions{}) expectedDomain := fmt.Sprintf("%s.%s.%s", route.Name, route.Namespace, expectation.expectedDomainSuffix)