From 37fa1e90db42539dcca4a628b2c49e108a17df7a Mon Sep 17 00:00:00 2001 From: Tanzeeb Khalili Date: Thu, 27 Feb 2020 17:07:46 -0500 Subject: [PATCH 1/8] Test minScale during update to configuration --- test/e2e/minscale_readiness_test.go | 104 ++++++++++++++++++++++++---- test/v1alpha1/configuration.go | 16 +++++ 2 files changed, 105 insertions(+), 15 deletions(-) diff --git a/test/e2e/minscale_readiness_test.go b/test/e2e/minscale_readiness_test.go index 7fc7d61f0a42..0d6603a892eb 100644 --- a/test/e2e/minscale_readiness_test.go +++ b/test/e2e/minscale_readiness_test.go @@ -19,10 +19,12 @@ limitations under the License. package e2e import ( + "fmt" "strconv" "testing" "time" + v1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/util/wait" "knative.dev/pkg/test/logstream" @@ -54,18 +56,18 @@ func TestMinScale(t *testing.T) { defer test.TearDown(clients, names) t.Log("Creating configuration") - if _, err := v1a1test.CreateConfiguration(t, clients, names, withMinScale(minScale)); err != nil { + cfg, err := v1a1test.CreateConfiguration(t, clients, names, withMinScale(minScale)) + if err != nil { t.Fatalf("Failed to create Configuration: %v", err) } - revName := latestRevisionName(t, clients, names.Config) - deploymentName := revName + "-deployment" - privateServiceName := serverlessServicesName(t, clients, revName) + revName := latestRevisionName(t, clients, names.Config, "") + serviceName := serverlessServicesName(t, clients, revName) // Before becoming ready, observe minScale t.Log("Waiting for revision to scale to minScale before becoming ready") - if err := waitForDesiredScale(t, clients, privateServiceName, gte(minScale)); err != nil { - t.Fatalf("The deployment %q did not scale >= %d before becoming ready: %v", deploymentName, minScale, err) + if err := waitForDesiredScale(t, clients, serviceName, gte(minScale)); err != nil { + t.Fatalf("The revision %q did not scale >= %d before becoming ready: %v", revName, minScale, err) } // Revision becomes ready @@ -78,8 +80,8 @@ func TestMinScale(t *testing.T) { // Without a route, ignore minScale t.Log("Waiting for revision to scale below minScale after becoming ready") - if err := waitForDesiredScale(t, clients, privateServiceName, lt(minScale)); err != nil { - t.Fatalf("The deployment %q did not scale < minScale after becoming ready: %v", deploymentName, err) + if err := waitForDesiredScale(t, clients, serviceName, lt(minScale)); err != nil { + t.Fatalf("The revision %q did not scale < minScale after becoming ready: %v", revName, err) } // Create route @@ -98,8 +100,42 @@ func TestMinScale(t *testing.T) { // With a route, observe minScale t.Log("Waiting for revision to scale to minScale after creating route") - if err := waitForDesiredScale(t, clients, privateServiceName, gte(minScale)); err != nil { - t.Fatalf("The deployment %q did not scale >= %d after creating route: %v", deploymentName, minScale, err) + if err := waitForDesiredScale(t, clients, serviceName, gte(minScale)); err != nil { + t.Fatalf("The revision %q did not scale >= %d after creating route: %v", revName, minScale, err) + } + + t.Log("Updating configuration") + if _, err := v1a1test.PatchConfig(clients, cfg, withEnv("FOO", "BAR")); err != nil { + t.Fatalf("Failed to update Configuration: %v", err) + } + + newRevName := latestRevisionName(t, clients, names.Config, revName) + newServiceName := serverlessServicesName(t, clients, newRevName) + + // After update, observe minScale in new revision + t.Log("Waiting for latest revision to scale to minScale after update") + if err := waitForDesiredScale(t, clients, newServiceName, gte(minScale)); err != nil { + t.Fatalf("The revision %q did not scale >= %d after creating route: %v", newRevName, minScale, err) + } + + // Revision becomes ready + t.Log("Waiting for revision to become ready") + if err := v1a1test.WaitForRevisionState( + clients.ServingAlphaClient, newRevName, v1a1test.IsRevisionReady, "RevisionIsReady", + ); err != nil { + t.Fatalf("The Revision %q did not become ready: %v", newRevName, err) + } + + // After update, ensure new revision holds minScale + t.Log("Hold minScale after update") + if err := ensureDesiredScale(t, clients, newServiceName, gte(minScale)); err != nil { + t.Fatalf("The revision %q did not stay at scale >= %d after creating route: %v", newRevName, minScale, err) + } + + // After update, ensure old revision ignores minScale + t.Log("Waiting for old revision to scale below minScale after being replaced") + if err := waitForDesiredScale(t, clients, serviceName, lt(minScale)); err != nil { + t.Fatalf("The revision %q did not scale < minScale after being replaced: %v", revName, err) } } @@ -115,6 +151,12 @@ func lt(m int) func(int) bool { } } +func withEnv(name, value string) func(cfg *v1alpha1.Configuration) { + return func(cfg *v1alpha1.Configuration) { + cfg.Spec.GetTemplate().Spec.GetContainer().Env = []v1.EnvVar{{Name: name, Value: value}} + } +} + func withMinScale(minScale int) func(cfg *v1alpha1.Configuration) { return func(cfg *v1alpha1.Configuration) { if cfg.Spec.Template.Annotations == nil { @@ -124,13 +166,19 @@ func withMinScale(minScale int) func(cfg *v1alpha1.Configuration) { } } -func latestRevisionName(t *testing.T, clients *test.Clients, configName string) string { +func lrcChanged(oldName string) func(c *v1alpha1.Configuration) (bool, error) { + return func(c *v1alpha1.Configuration) (bool, error) { + return c.Status.LatestCreatedRevisionName != oldName, nil + } +} + +func latestRevisionName(t *testing.T, clients *test.Clients, configName, oldRevName string) string { // Wait for the Config have a LatestCreatedRevisionName if err := v1a1test.WaitForConfigurationState( clients.ServingAlphaClient, configName, - v1a1test.ConfigurationHasCreatedRevision, "ConfigurationHasCreatedRevision", + lrcChanged(oldRevName), "ConfigurationHasUpdatedCreatedRevision", ); err != nil { - t.Fatalf("The Configuration %q does not have a LatestCreatedRevisionName: %v", configName, err) + t.Fatalf("The Configuration %q has not updated LatestCreatedRevisionName from %q: %v", configName, oldRevName, err) } config, err := clients.ServingAlphaClient.Configs.Get(configName, metav1.GetOptions{}) @@ -143,6 +191,7 @@ func latestRevisionName(t *testing.T, clients *test.Clients, configName string) func serverlessServicesName(t *testing.T, clients *test.Clients, revisionName string) string { var privateServiceName string + if err := wait.PollImmediate(time.Second, 1*time.Minute, func() (bool, error) { sks, err := clients.NetworkingClient.ServerlessServices.Get(revisionName, metav1.GetOptions{}) if err != nil { @@ -156,17 +205,42 @@ func serverlessServicesName(t *testing.T, clients *test.Clients, revisionName st }); err != nil { t.Fatalf("Error retrieving sks %q: %v", revisionName, err) } + return privateServiceName } -func waitForDesiredScale(t *testing.T, clients *test.Clients, privateServiceName string, cond func(int) bool) error { +func waitForDesiredScale(t *testing.T, clients *test.Clients, serviceName string, cond func(int) bool) error { endpoints := clients.KubeClient.Kube.CoreV1().Endpoints(test.ServingNamespace) return wait.PollImmediate(time.Second, 1*time.Minute, func() (bool, error) { - endpoint, err := endpoints.Get(privateServiceName, metav1.GetOptions{}) + endpoint, err := endpoints.Get(serviceName, metav1.GetOptions{}) if err != nil { return false, nil } return cond(resources.ReadyAddressCount(endpoint)), nil }) + +} + +func ensureDesiredScale(t *testing.T, clients *test.Clients, serviceName string, cond func(int) bool) error { + endpoints := clients.KubeClient.Kube.CoreV1().Endpoints(test.ServingNamespace) + + err := wait.PollImmediate(time.Second, 5*time.Second, func() (bool, error) { + endpoint, err := endpoints.Get(serviceName, metav1.GetOptions{}) + if err != nil { + return false, nil + } + + if scale := resources.ReadyAddressCount(endpoint); !cond(scale) { + return false, fmt.Errorf("scaled to %d", scale) + } + + return false, nil + }) + + if err != wait.ErrWaitTimeout { + return err + } + + return nil } diff --git a/test/v1alpha1/configuration.go b/test/v1alpha1/configuration.go index 472020018862..1d28c1b5fd4c 100644 --- a/test/v1alpha1/configuration.go +++ b/test/v1alpha1/configuration.go @@ -44,6 +44,22 @@ func CreateConfiguration(t pkgTest.T, clients *test.Clients, names test.Resource return clients.ServingAlphaClient.Configs.Create(config) } +// PatchConfig patches the existing config with the provided options. Returns the latest Configuration object +func PatchConfig(clients *test.Clients, cfg *v1alpha1.Configuration, fopt ...v1alpha1testing.ConfigOption) (*v1alpha1.Configuration, error) { + newCfg := cfg.DeepCopy() + + for _, opt := range fopt { + opt(newCfg) + } + + patchBytes, err := duck.CreateBytePatch(cfg, newCfg) + if err != nil { + return nil, err + } + + return clients.ServingAlphaClient.Configs.Patch(cfg.ObjectMeta.Name, types.JSONPatchType, patchBytes, "") +} + // PatchConfigImage patches the existing config passed in with a new imagePath. Returns the latest Configuration object func PatchConfigImage(clients *test.Clients, cfg *v1alpha1.Configuration, imagePath string) (*v1alpha1.Configuration, error) { newCfg := cfg.DeepCopy() From b4a3bf089952d2dc982c19eda57eb4dae3cc7012 Mon Sep 17 00:00:00 2001 From: Tanzeeb Khalili Date: Thu, 27 Feb 2020 19:20:35 -0500 Subject: [PATCH 2/8] Add route label to LatestCreatedRevision --- pkg/reconciler/labeler/controller.go | 16 +++++++++++++++- pkg/reconciler/labeler/labeler.go | 2 ++ pkg/reconciler/labeler/labeler_test.go | 20 +++++++++++++++++++- pkg/reconciler/labeler/labels.go | 13 +++++++++++++ 4 files changed, 49 insertions(+), 2 deletions(-) diff --git a/pkg/reconciler/labeler/controller.go b/pkg/reconciler/labeler/controller.go index 623839f81efb..7edcf06e1b56 100644 --- a/pkg/reconciler/labeler/controller.go +++ b/pkg/reconciler/labeler/controller.go @@ -19,16 +19,18 @@ package labeler import ( "context" + v1 "knative.dev/serving/pkg/apis/serving/v1" servingclient "knative.dev/serving/pkg/client/injection/client" configurationinformer "knative.dev/serving/pkg/client/injection/informers/serving/v1/configuration" revisioninformer "knative.dev/serving/pkg/client/injection/informers/serving/v1/revision" routeinformer "knative.dev/serving/pkg/client/injection/informers/serving/v1/route" routereconciler "knative.dev/serving/pkg/client/injection/reconciler/serving/v1/route" + servingreconciler "knative.dev/serving/pkg/reconciler" "knative.dev/pkg/configmap" "knative.dev/pkg/controller" "knative.dev/pkg/logging" - servingreconciler "knative.dev/serving/pkg/reconciler" + "knative.dev/pkg/tracker" ) const controllerAgentName = "labeler-controller" @@ -56,5 +58,17 @@ func NewController( logger.Info("Setting up event handlers") routeInformer.Informer().AddEventHandler(controller.HandleAll(impl.Enqueue)) + c.tracker = tracker.New(impl.EnqueueKey, controller.GetTrackerLease(ctx)) + + configInformer.Informer().AddEventHandler(controller.HandleAll( + // Call the tracker's OnChanged method, but we've seen the objects + // coming through this path missing TypeMeta, so ensure it is properly + // populated. + controller.EnsureTypeMeta( + c.tracker.OnChanged, + v1.SchemeGroupVersion.WithKind("Configuration"), + ), + )) + return impl } diff --git a/pkg/reconciler/labeler/labeler.go b/pkg/reconciler/labeler/labeler.go index e45d5b6a9b94..39caf0b37538 100644 --- a/pkg/reconciler/labeler/labeler.go +++ b/pkg/reconciler/labeler/labeler.go @@ -20,6 +20,7 @@ import ( "context" pkgreconciler "knative.dev/pkg/reconciler" + "knative.dev/pkg/tracker" v1 "knative.dev/serving/pkg/apis/serving/v1" clientset "knative.dev/serving/pkg/client/clientset/versioned" routereconciler "knative.dev/serving/pkg/client/injection/reconciler/serving/v1/route" @@ -33,6 +34,7 @@ type Reconciler struct { // Listers index properties about resources configurationLister listers.ConfigurationLister revisionLister listers.RevisionLister + tracker tracker.Interface } // Check that our Reconciler implements routereconciler.Interface diff --git a/pkg/reconciler/labeler/labeler_test.go b/pkg/reconciler/labeler/labeler_test.go index f899f80e72bc..753b731553ce 100644 --- a/pkg/reconciler/labeler/labeler_test.go +++ b/pkg/reconciler/labeler/labeler_test.go @@ -161,6 +161,23 @@ func TestReconcile(t *testing.T) { patchAddLabel("default", "new-config", "serving.knative.dev/route", "config-change"), }, Key: "default/config-change", + }, { + Name: "updating configuration", + Objects: []runtime.Object{ + simpleRunLatest("default", "config-update", "the-config", WithRouteFinalizer), + simpleConfig("default", "the-config", + WithLatestCreated("the-config-ecoge"), + WithConfigLabel("serving.knative.dev/route", "config-update")), + rev("default", "the-config", + WithRevisionLabel("serving.knative.dev/route", "config-update")), + rev("default", "the-config", + WithRevName("the-config-ecoge")), + }, + WantPatches: []clientgotesting.PatchActionImpl{ + patchAddLabel("default", "the-config-ecoge", + "serving.knative.dev/route", "config-update"), + }, + Key: "default/config-update", }, { Name: "delete route", Objects: []runtime.Object{ @@ -235,6 +252,7 @@ func TestReconcile(t *testing.T) { client: servingclient.Get(ctx), configurationLister: listers.GetConfigurationLister(), revisionLister: listers.GetRevisionLister(), + tracker: &NullTracker{}, } return routereconciler.NewReconciler(ctx, logging.FromContext(ctx), servingclient.Get(ctx), @@ -276,7 +294,7 @@ func rev(namespace, name string, opts ...RevisionOption) *v1.Revision { rev := &v1.Revision{ ObjectMeta: metav1.ObjectMeta{ Namespace: namespace, - Name: cfg.Status.LatestCreatedRevisionName, + Name: cfg.Status.LatestReadyRevisionName, ResourceVersion: "v1", OwnerReferences: []metav1.OwnerReference{*kmeta.NewControllerRef(cfg)}, }, diff --git a/pkg/reconciler/labeler/labels.go b/pkg/reconciler/labeler/labels.go index dd0f5ed97ffb..0ec194242ffd 100644 --- a/pkg/reconciler/labeler/labels.go +++ b/pkg/reconciler/labeler/labels.go @@ -53,9 +53,22 @@ func (c *Reconciler) syncLabels(ctx context.Context, r *v1.Route) error { return err } revisions.Insert(tt.RevisionName) + + // If the owner reference is a configuration, add it to the list of configurations + // and add its LatestCreatedRevision to the list of revisions owner := metav1.GetControllerOf(rev) if owner != nil && owner.Kind == "Configuration" { configs.Insert(owner.Name) + + config, err := c.configurationLister.Configurations(r.Namespace).Get(owner.Name) + if err != nil { + return err + } + + revisions.Insert(config.Status.LatestCreatedRevisionName) + + // Track future changes to the configuration + c.tracker.Track(kmeta.ObjectReference(config), r) } } From 9f347c2e1ef67a5752aedc64e4ca4184f166855f Mon Sep 17 00:00:00 2001 From: Tanzeeb Khalili Date: Wed, 4 Mar 2020 12:22:26 -0500 Subject: [PATCH 3/8] Use impl.Enqueue instead of tracker.OnChanged in labeler reconciler --- pkg/reconciler/labeler/controller.go | 15 +-------------- pkg/reconciler/labeler/labeler.go | 2 -- pkg/reconciler/labeler/labeler_test.go | 1 - pkg/reconciler/labeler/labels.go | 6 +++--- 4 files changed, 4 insertions(+), 20 deletions(-) diff --git a/pkg/reconciler/labeler/controller.go b/pkg/reconciler/labeler/controller.go index 7edcf06e1b56..6046dd6d759b 100644 --- a/pkg/reconciler/labeler/controller.go +++ b/pkg/reconciler/labeler/controller.go @@ -19,7 +19,6 @@ package labeler import ( "context" - v1 "knative.dev/serving/pkg/apis/serving/v1" servingclient "knative.dev/serving/pkg/client/injection/client" configurationinformer "knative.dev/serving/pkg/client/injection/informers/serving/v1/configuration" revisioninformer "knative.dev/serving/pkg/client/injection/informers/serving/v1/revision" @@ -30,7 +29,6 @@ import ( "knative.dev/pkg/configmap" "knative.dev/pkg/controller" "knative.dev/pkg/logging" - "knative.dev/pkg/tracker" ) const controllerAgentName = "labeler-controller" @@ -57,18 +55,7 @@ func NewController( logger.Info("Setting up event handlers") routeInformer.Informer().AddEventHandler(controller.HandleAll(impl.Enqueue)) - - c.tracker = tracker.New(impl.EnqueueKey, controller.GetTrackerLease(ctx)) - - configInformer.Informer().AddEventHandler(controller.HandleAll( - // Call the tracker's OnChanged method, but we've seen the objects - // coming through this path missing TypeMeta, so ensure it is properly - // populated. - controller.EnsureTypeMeta( - c.tracker.OnChanged, - v1.SchemeGroupVersion.WithKind("Configuration"), - ), - )) + configInformer.Informer().AddEventHandler(controller.HandleAll(impl.Enqueue)) return impl } diff --git a/pkg/reconciler/labeler/labeler.go b/pkg/reconciler/labeler/labeler.go index 39caf0b37538..e45d5b6a9b94 100644 --- a/pkg/reconciler/labeler/labeler.go +++ b/pkg/reconciler/labeler/labeler.go @@ -20,7 +20,6 @@ import ( "context" pkgreconciler "knative.dev/pkg/reconciler" - "knative.dev/pkg/tracker" v1 "knative.dev/serving/pkg/apis/serving/v1" clientset "knative.dev/serving/pkg/client/clientset/versioned" routereconciler "knative.dev/serving/pkg/client/injection/reconciler/serving/v1/route" @@ -34,7 +33,6 @@ type Reconciler struct { // Listers index properties about resources configurationLister listers.ConfigurationLister revisionLister listers.RevisionLister - tracker tracker.Interface } // Check that our Reconciler implements routereconciler.Interface diff --git a/pkg/reconciler/labeler/labeler_test.go b/pkg/reconciler/labeler/labeler_test.go index 753b731553ce..7662cbaf4b1c 100644 --- a/pkg/reconciler/labeler/labeler_test.go +++ b/pkg/reconciler/labeler/labeler_test.go @@ -252,7 +252,6 @@ func TestReconcile(t *testing.T) { client: servingclient.Get(ctx), configurationLister: listers.GetConfigurationLister(), revisionLister: listers.GetRevisionLister(), - tracker: &NullTracker{}, } return routereconciler.NewReconciler(ctx, logging.FromContext(ctx), servingclient.Get(ctx), diff --git a/pkg/reconciler/labeler/labels.go b/pkg/reconciler/labeler/labels.go index 0ec194242ffd..cddfd8defab6 100644 --- a/pkg/reconciler/labeler/labels.go +++ b/pkg/reconciler/labeler/labels.go @@ -65,10 +65,10 @@ func (c *Reconciler) syncLabels(ctx context.Context, r *v1.Route) error { return err } + // If the configuration is being updated, the route's .status.traffic will + // only incude the latest ready revision. We need to label the latest created revision + // as well so that there is a smooth transition when the new revision becomes ready. revisions.Insert(config.Status.LatestCreatedRevisionName) - - // Track future changes to the configuration - c.tracker.Track(kmeta.ObjectReference(config), r) } } From 246494c96e789e4672eece75bf190a2cf3582405 Mon Sep 17 00:00:00 2001 From: Tanzeeb Khalili Date: Wed, 4 Mar 2020 14:05:02 -0500 Subject: [PATCH 4/8] Update minscale readiness e2e test * faster polling (250 ms) * more descriptive error for ensureDesiredScale * inlined latestCreatedRevisionName check --- test/e2e/minscale_readiness_test.go | 16 ++++++---------- 1 file changed, 6 insertions(+), 10 deletions(-) diff --git a/test/e2e/minscale_readiness_test.go b/test/e2e/minscale_readiness_test.go index 0d6603a892eb..4fd74c6bdb81 100644 --- a/test/e2e/minscale_readiness_test.go +++ b/test/e2e/minscale_readiness_test.go @@ -166,17 +166,13 @@ func withMinScale(minScale int) func(cfg *v1alpha1.Configuration) { } } -func lrcChanged(oldName string) func(c *v1alpha1.Configuration) (bool, error) { - return func(c *v1alpha1.Configuration) (bool, error) { - return c.Status.LatestCreatedRevisionName != oldName, nil - } -} - func latestRevisionName(t *testing.T, clients *test.Clients, configName, oldRevName string) string { // Wait for the Config have a LatestCreatedRevisionName if err := v1a1test.WaitForConfigurationState( clients.ServingAlphaClient, configName, - lrcChanged(oldRevName), "ConfigurationHasUpdatedCreatedRevision", + func(c *v1alpha1.Configuration) (bool, error) { + return c.Status.LatestCreatedRevisionName != oldRevName, nil + }, "ConfigurationHasUpdatedCreatedRevision", ); err != nil { t.Fatalf("The Configuration %q has not updated LatestCreatedRevisionName from %q: %v", configName, oldRevName, err) } @@ -212,7 +208,7 @@ func serverlessServicesName(t *testing.T, clients *test.Clients, revisionName st func waitForDesiredScale(t *testing.T, clients *test.Clients, serviceName string, cond func(int) bool) error { endpoints := clients.KubeClient.Kube.CoreV1().Endpoints(test.ServingNamespace) - return wait.PollImmediate(time.Second, 1*time.Minute, func() (bool, error) { + return wait.PollImmediate(250*time.Millisecond, 1*time.Minute, func() (bool, error) { endpoint, err := endpoints.Get(serviceName, metav1.GetOptions{}) if err != nil { return false, nil @@ -225,14 +221,14 @@ func waitForDesiredScale(t *testing.T, clients *test.Clients, serviceName string func ensureDesiredScale(t *testing.T, clients *test.Clients, serviceName string, cond func(int) bool) error { endpoints := clients.KubeClient.Kube.CoreV1().Endpoints(test.ServingNamespace) - err := wait.PollImmediate(time.Second, 5*time.Second, func() (bool, error) { + err := wait.PollImmediate(250*time.Millisecond, 5*time.Second, func() (bool, error) { endpoint, err := endpoints.Get(serviceName, metav1.GetOptions{}) if err != nil { return false, nil } if scale := resources.ReadyAddressCount(endpoint); !cond(scale) { - return false, fmt.Errorf("scaled to %d", scale) + return false, fmt.Errorf("scale %d didn't meet condition", scale) } return false, nil From 35aa45e86b07a915b841e19116debae882e5f317 Mon Sep 17 00:00:00 2001 From: Tanzeeb Khalili Date: Thu, 5 Mar 2020 16:07:31 -0500 Subject: [PATCH 5/8] Enqueue route instead of config in labeler reconciler --- pkg/reconciler/labeler/controller.go | 10 +++++++++- test/e2e/minscale_readiness_test.go | 7 +++---- 2 files changed, 12 insertions(+), 5 deletions(-) diff --git a/pkg/reconciler/labeler/controller.go b/pkg/reconciler/labeler/controller.go index 6046dd6d759b..f19328b2fc13 100644 --- a/pkg/reconciler/labeler/controller.go +++ b/pkg/reconciler/labeler/controller.go @@ -19,6 +19,9 @@ package labeler import ( "context" + "k8s.io/client-go/tools/cache" + + "knative.dev/serving/pkg/apis/serving" servingclient "knative.dev/serving/pkg/client/injection/client" configurationinformer "knative.dev/serving/pkg/client/injection/informers/serving/v1/configuration" revisioninformer "knative.dev/serving/pkg/client/injection/informers/serving/v1/revision" @@ -29,6 +32,7 @@ import ( "knative.dev/pkg/configmap" "knative.dev/pkg/controller" "knative.dev/pkg/logging" + pkgreconciler "knative.dev/pkg/reconciler" ) const controllerAgentName = "labeler-controller" @@ -55,7 +59,11 @@ func NewController( logger.Info("Setting up event handlers") routeInformer.Informer().AddEventHandler(controller.HandleAll(impl.Enqueue)) - configInformer.Informer().AddEventHandler(controller.HandleAll(impl.Enqueue)) + + configInformer.Informer().AddEventHandler(cache.FilteringResourceEventHandler{ + FilterFunc: pkgreconciler.LabelExistsFilterFunc(serving.RouteLabelKey), + Handler: controller.HandleAll(impl.EnqueueLabelOfNamespaceScopedResource("", serving.RouteLabelKey)), + }) return impl } diff --git a/test/e2e/minscale_readiness_test.go b/test/e2e/minscale_readiness_test.go index 4fd74c6bdb81..0f4487d88c8c 100644 --- a/test/e2e/minscale_readiness_test.go +++ b/test/e2e/minscale_readiness_test.go @@ -44,11 +44,10 @@ func TestMinScale(t *testing.T) { clients := Setup(t) - name := test.ObjectNameForTest(t) - names := test.ResourceNames{ - Config: name, - Route: name, + // Config and Route have different names to avoid false positives + Config: test.ObjectNameForTest(t), + Route: test.ObjectNameForTest(t), Image: "helloworld", } From 05ee2c74b88f8693efd720e242f2a0326f085a27 Mon Sep 17 00:00:00 2001 From: Tanzeeb Khalili Date: Thu, 5 Mar 2020 18:24:32 -0500 Subject: [PATCH 6/8] Only label configurations of RunLatest traffic targets --- pkg/reconciler/labeler/labeler_test.go | 30 ++++++++++++++++++++++++-- pkg/reconciler/labeler/labels.go | 8 +++---- 2 files changed, 32 insertions(+), 6 deletions(-) diff --git a/pkg/reconciler/labeler/labeler_test.go b/pkg/reconciler/labeler/labeler_test.go index 7662cbaf4b1c..b63bf9fb01ed 100644 --- a/pkg/reconciler/labeler/labeler_test.go +++ b/pkg/reconciler/labeler/labeler_test.go @@ -74,6 +74,24 @@ func TestReconcile(t *testing.T) { Eventf(corev1.EventTypeNormal, "FinalizerUpdate", "Updated %q finalizers", "first-reconcile"), }, Key: "default/first-reconcile", + }, { + Name: "label pinned revision", + Objects: []runtime.Object{ + simpleRoute("default", "pinned-revision", "the-revision"), + simpleConfig("default", "red-herring", WithLatestCreated("red-herring-ecoge")), + rev("default", "red-herring"), + rev("default", "red-herring", WithRevName("red-herring-ecoge")), + rev("default", "red-herring", WithRevName("the-revision")), + }, + WantPatches: []clientgotesting.PatchActionImpl{ + patchAddFinalizerAction("default", "pinned-revision"), + patchAddLabel("default", "the-revision", + "serving.knative.dev/route", "pinned-revision"), + }, + WantEvents: []string{ + Eventf(corev1.EventTypeNormal, "FinalizerUpdate", "Updated %q finalizers", "pinned-revision"), + }, + Key: "default/pinned-revision", }, { Name: "steady state", Objects: []runtime.Object{ @@ -162,7 +180,7 @@ func TestReconcile(t *testing.T) { }, Key: "default/config-change", }, { - Name: "updating configuration", + Name: "update configuration", Objects: []runtime.Object{ simpleRunLatest("default", "config-update", "the-config", WithRouteFinalizer), simpleConfig("default", "the-config", @@ -265,7 +283,15 @@ func routeWithTraffic(namespace, name string, traffic v1.TrafficTarget, opts ... func simpleRunLatest(namespace, name, config string, opts ...RouteOption) *v1.Route { return routeWithTraffic(namespace, name, v1.TrafficTarget{ - RevisionName: config + "-dbnfd", + RevisionName: config + "-dbnfd", + Percent: ptr.Int64(100), + LatestRevision: ptr.Bool(true), + }, opts...) +} + +func simpleRoute(namespace, name, revision string, opts ...RouteOption) *v1.Route { + return routeWithTraffic(namespace, name, v1.TrafficTarget{ + RevisionName: revision, Percent: ptr.Int64(100), }, opts...) } diff --git a/pkg/reconciler/labeler/labels.go b/pkg/reconciler/labeler/labels.go index cddfd8defab6..d08a1ced811e 100644 --- a/pkg/reconciler/labeler/labels.go +++ b/pkg/reconciler/labeler/labels.go @@ -46,7 +46,7 @@ func (c *Reconciler) syncLabels(ctx context.Context, r *v1.Route) error { configs := sets.NewString() // Walk the revisions in Route's .status.traffic and build a list - // of Configurations to label from their OwnerReferences. + // of RunLatest Configurations to label from their OwnerReferences. for _, tt := range r.Status.Traffic { rev, err := c.revisionLister.Revisions(r.Namespace).Get(tt.RevisionName) if err != nil { @@ -54,10 +54,10 @@ func (c *Reconciler) syncLabels(ctx context.Context, r *v1.Route) error { } revisions.Insert(tt.RevisionName) - // If the owner reference is a configuration, add it to the list of configurations - // and add its LatestCreatedRevision to the list of revisions + // If the .traffic is a RunLatest and the revision's owner is a configuration, + // add the configuration to the list along with its LatestCreatedRevisionName owner := metav1.GetControllerOf(rev) - if owner != nil && owner.Kind == "Configuration" { + if owner != nil && owner.Kind == "Configuration" && tt.LatestRevision != nil && *tt.LatestRevision { configs.Insert(owner.Name) config, err := c.configurationLister.Configurations(r.Namespace).Get(owner.Name) From b7eec55f12d1f453c89e3ae7c6fc87a38925d94f Mon Sep 17 00:00:00 2001 From: Tanzeeb Khalili Date: Fri, 6 Mar 2020 18:17:39 -0500 Subject: [PATCH 7/8] Label configuration if it or it's revision is in a route --- pkg/reconciler/labeler/labeler_test.go | 9 +++++---- pkg/reconciler/labeler/labels.go | 13 +++++++------ 2 files changed, 12 insertions(+), 10 deletions(-) diff --git a/pkg/reconciler/labeler/labeler_test.go b/pkg/reconciler/labeler/labeler_test.go index b63bf9fb01ed..5b6ec3819769 100644 --- a/pkg/reconciler/labeler/labeler_test.go +++ b/pkg/reconciler/labeler/labeler_test.go @@ -78,15 +78,16 @@ func TestReconcile(t *testing.T) { Name: "label pinned revision", Objects: []runtime.Object{ simpleRoute("default", "pinned-revision", "the-revision"), - simpleConfig("default", "red-herring", WithLatestCreated("red-herring-ecoge")), - rev("default", "red-herring"), - rev("default", "red-herring", WithRevName("red-herring-ecoge")), - rev("default", "red-herring", WithRevName("the-revision")), + simpleConfig("default", "the-config"), + rev("default", "the-config"), + rev("default", "the-config", WithRevName("the-revision")), }, WantPatches: []clientgotesting.PatchActionImpl{ patchAddFinalizerAction("default", "pinned-revision"), patchAddLabel("default", "the-revision", "serving.knative.dev/route", "pinned-revision"), + patchAddLabel("default", "the-config", + "serving.knative.dev/route", "pinned-revision"), }, WantEvents: []string{ Eventf(corev1.EventTypeNormal, "FinalizerUpdate", "Updated %q finalizers", "pinned-revision"), diff --git a/pkg/reconciler/labeler/labels.go b/pkg/reconciler/labeler/labels.go index d08a1ced811e..0d610ddacf5b 100644 --- a/pkg/reconciler/labeler/labels.go +++ b/pkg/reconciler/labeler/labels.go @@ -54,10 +54,10 @@ func (c *Reconciler) syncLabels(ctx context.Context, r *v1.Route) error { } revisions.Insert(tt.RevisionName) - // If the .traffic is a RunLatest and the revision's owner is a configuration, - // add the configuration to the list along with its LatestCreatedRevisionName + // If the owner reference is a configuration, add it to the list of configurations + // and add its LatestCreatedRevision to the list of revisions owner := metav1.GetControllerOf(rev) - if owner != nil && owner.Kind == "Configuration" && tt.LatestRevision != nil && *tt.LatestRevision { + if owner != nil && owner.Kind == "Configuration" { configs.Insert(owner.Name) config, err := c.configurationLister.Configurations(r.Namespace).Get(owner.Name) @@ -65,10 +65,11 @@ func (c *Reconciler) syncLabels(ctx context.Context, r *v1.Route) error { return err } - // If the configuration is being updated, the route's .status.traffic will - // only incude the latest ready revision. We need to label the latest created revision + // If we are tracking the latest revision, we need to label the latest created revision // as well so that there is a smooth transition when the new revision becomes ready. - revisions.Insert(config.Status.LatestCreatedRevisionName) + if tt.LatestRevision != nil && *tt.LatestRevision { + revisions.Insert(config.Status.LatestCreatedRevisionName) + } } } From e4a1182c50c26a2a2027fd8c4c699a711090826e Mon Sep 17 00:00:00 2001 From: Tanzeeb Khalili Date: Mon, 9 Mar 2020 10:19:16 -0400 Subject: [PATCH 8/8] Only pull config for runLatest in labeler --- pkg/reconciler/labeler/labels.go | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-) diff --git a/pkg/reconciler/labeler/labels.go b/pkg/reconciler/labeler/labels.go index 0d610ddacf5b..ba69a4fadfbf 100644 --- a/pkg/reconciler/labeler/labels.go +++ b/pkg/reconciler/labeler/labels.go @@ -46,7 +46,7 @@ func (c *Reconciler) syncLabels(ctx context.Context, r *v1.Route) error { configs := sets.NewString() // Walk the revisions in Route's .status.traffic and build a list - // of RunLatest Configurations to label from their OwnerReferences. + // of Configurations to label from their OwnerReferences. for _, tt := range r.Status.Traffic { rev, err := c.revisionLister.Revisions(r.Namespace).Get(tt.RevisionName) if err != nil { @@ -55,19 +55,18 @@ func (c *Reconciler) syncLabels(ctx context.Context, r *v1.Route) error { revisions.Insert(tt.RevisionName) // If the owner reference is a configuration, add it to the list of configurations - // and add its LatestCreatedRevision to the list of revisions owner := metav1.GetControllerOf(rev) if owner != nil && owner.Kind == "Configuration" { configs.Insert(owner.Name) - config, err := c.configurationLister.Configurations(r.Namespace).Get(owner.Name) - if err != nil { - return err - } - - // If we are tracking the latest revision, we need to label the latest created revision - // as well so that there is a smooth transition when the new revision becomes ready. + // If we are tracking the latest revision, add the latest created revision as well + // so that there is a smooth transition when the new revision becomes ready. if tt.LatestRevision != nil && *tt.LatestRevision { + config, err := c.configurationLister.Configurations(r.Namespace).Get(owner.Name) + if err != nil { + return err + } + revisions.Insert(config.Status.LatestCreatedRevisionName) } }