From 565a887016fdf692f938f5192fa9efe79ed149c2 Mon Sep 17 00:00:00 2001 From: Tanzeeb Khalili Date: Mon, 9 Mar 2020 12:08:28 -0400 Subject: [PATCH 1/4] Add route label to `LatestCreatedRevision` (#7110) * Test minScale during update to configuration * Add route label to LatestCreatedRevision * Use impl.Enqueue instead of tracker.OnChanged in labeler reconciler * Update minscale readiness e2e test * faster polling (250 ms) * more descriptive error for ensureDesiredScale * inlined latestCreatedRevisionName check * Enqueue route instead of config in labeler reconciler * Only label configurations of RunLatest traffic targets * Label configuration if it or it's revision is in a route * Only pull config for runLatest in labeler --- pkg/reconciler/labeler/controller.go | 11 ++- pkg/reconciler/labeler/labeler_test.go | 48 ++++++++++- pkg/reconciler/labeler/labels.go | 13 +++ test/e2e/minscale_readiness_test.go | 109 ++++++++++++++++++++----- test/v1alpha1/configuration.go | 16 ++++ 5 files changed, 174 insertions(+), 23 deletions(-) diff --git a/pkg/reconciler/labeler/controller.go b/pkg/reconciler/labeler/controller.go index 623839f81efb..f19328b2fc13 100644 --- a/pkg/reconciler/labeler/controller.go +++ b/pkg/reconciler/labeler/controller.go @@ -19,16 +19,20 @@ 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" 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" + pkgreconciler "knative.dev/pkg/reconciler" ) const controllerAgentName = "labeler-controller" @@ -56,5 +60,10 @@ func NewController( logger.Info("Setting up event handlers") routeInformer.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/pkg/reconciler/labeler/labeler_test.go b/pkg/reconciler/labeler/labeler_test.go index f899f80e72bc..5b6ec3819769 100644 --- a/pkg/reconciler/labeler/labeler_test.go +++ b/pkg/reconciler/labeler/labeler_test.go @@ -74,6 +74,25 @@ 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", "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"), + }, + Key: "default/pinned-revision", }, { Name: "steady state", Objects: []runtime.Object{ @@ -161,6 +180,23 @@ func TestReconcile(t *testing.T) { patchAddLabel("default", "new-config", "serving.knative.dev/route", "config-change"), }, Key: "default/config-change", + }, { + Name: "update 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{ @@ -248,7 +284,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...) } @@ -276,7 +320,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..ba69a4fadfbf 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 owner := metav1.GetControllerOf(rev) if owner != nil && owner.Kind == "Configuration" { configs.Insert(owner.Name) + + // 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) + } } } diff --git a/test/e2e/minscale_readiness_test.go b/test/e2e/minscale_readiness_test.go index 7fc7d61f0a42..0f4487d88c8c 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" @@ -42,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", } @@ -54,18 +55,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 +79,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 +99,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 +150,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 +165,15 @@ func withMinScale(minScale int) func(cfg *v1alpha1.Configuration) { } } -func latestRevisionName(t *testing.T, clients *test.Clients, configName string) string { +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", + func(c *v1alpha1.Configuration) (bool, error) { + return c.Status.LatestCreatedRevisionName != oldRevName, nil + }, "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 +186,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 +200,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{}) + 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 } 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(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("scale %d didn't meet condition", 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 957a185a22851d2cd351ed4ee95d297f50df4585 Mon Sep 17 00:00:00 2001 From: Victor Agababov Date: Mon, 9 Mar 2020 20:47:28 -0700 Subject: [PATCH 2/4] Some cleanups in the e2e test (#7193) * Some cleanups in the e2e test * Some cleanups in the e2e test * fix nit --- test/e2e/minscale_readiness_test.go | 33 ++++++++++++----------------- 1 file changed, 14 insertions(+), 19 deletions(-) diff --git a/test/e2e/minscale_readiness_test.go b/test/e2e/minscale_readiness_test.go index 0f4487d88c8c..34db9d7e1a94 100644 --- a/test/e2e/minscale_readiness_test.go +++ b/test/e2e/minscale_readiness_test.go @@ -61,11 +61,11 @@ func TestMinScale(t *testing.T) { } revName := latestRevisionName(t, clients, names.Config, "") - serviceName := serverlessServicesName(t, clients, revName) + serviceName := privateServiceName(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, serviceName, gte(minScale)); err != nil { + if err := waitForDesiredScale(clients, serviceName, gte(minScale)); err != nil { t.Fatalf("The revision %q did not scale >= %d before becoming ready: %v", revName, minScale, err) } @@ -79,7 +79,7 @@ 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, serviceName, lt(minScale)); err != nil { + if err := waitForDesiredScale(clients, serviceName, lt(minScale)); err != nil { t.Fatalf("The revision %q did not scale < minScale after becoming ready: %v", revName, err) } @@ -99,7 +99,7 @@ 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, serviceName, gte(minScale)); err != nil { + if err := waitForDesiredScale(clients, serviceName, gte(minScale)); err != nil { t.Fatalf("The revision %q did not scale >= %d after creating route: %v", revName, minScale, err) } @@ -109,11 +109,11 @@ func TestMinScale(t *testing.T) { } newRevName := latestRevisionName(t, clients, names.Config, revName) - newServiceName := serverlessServicesName(t, clients, newRevName) + newServiceName := privateServiceName(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 { + if err := waitForDesiredScale(clients, newServiceName, gte(minScale)); err != nil { t.Fatalf("The revision %q did not scale >= %d after creating route: %v", newRevName, minScale, err) } @@ -127,13 +127,13 @@ func TestMinScale(t *testing.T) { // After update, ensure new revision holds minScale t.Log("Hold minScale after update") - if err := ensureDesiredScale(t, clients, newServiceName, gte(minScale)); err != nil { + if err := ensureDesiredScale(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 { + if err := waitForDesiredScale(clients, serviceName, lt(minScale)); err != nil { t.Fatalf("The revision %q did not scale < minScale after being replaced: %v", revName, err) } } @@ -184,7 +184,7 @@ func latestRevisionName(t *testing.T, clients *test.Clients, configName, oldRevN return config.Status.LatestCreatedRevisionName } -func serverlessServicesName(t *testing.T, clients *test.Clients, revisionName string) string { +func privateServiceName(t *testing.T, clients *test.Clients, revisionName string) string { var privateServiceName string if err := wait.PollImmediate(time.Second, 1*time.Minute, func() (bool, error) { @@ -193,10 +193,7 @@ func serverlessServicesName(t *testing.T, clients *test.Clients, revisionName st return false, nil } privateServiceName = sks.Status.PrivateServiceName - if privateServiceName == "" { - return false, nil - } - return true, nil + return privateServiceName != "", nil }); err != nil { t.Fatalf("Error retrieving sks %q: %v", revisionName, err) } @@ -204,7 +201,7 @@ func serverlessServicesName(t *testing.T, clients *test.Clients, revisionName st return privateServiceName } -func waitForDesiredScale(t *testing.T, clients *test.Clients, serviceName string, cond func(int) bool) error { +func waitForDesiredScale(clients *test.Clients, serviceName string, cond func(int) bool) error { endpoints := clients.KubeClient.Kube.CoreV1().Endpoints(test.ServingNamespace) return wait.PollImmediate(250*time.Millisecond, 1*time.Minute, func() (bool, error) { @@ -217,10 +214,10 @@ 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 { +func ensureDesiredScale(clients *test.Clients, serviceName string, cond func(int) bool) error { endpoints := clients.KubeClient.Kube.CoreV1().Endpoints(test.ServingNamespace) - err := wait.PollImmediate(250*time.Millisecond, 5*time.Second, func() (bool, error) { + if 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 @@ -231,9 +228,7 @@ func ensureDesiredScale(t *testing.T, clients *test.Clients, serviceName string, } return false, nil - }) - - if err != wait.ErrWaitTimeout { + }); err != wait.ErrWaitTimeout { return err } From b00188b4e4a782fa6ce59b89946e38b819b79995 Mon Sep 17 00:00:00 2001 From: Victor Agababov Date: Thu, 12 Mar 2020 19:30:29 -0700 Subject: [PATCH 3/4] Simplify minscale test. (#7235) The only error returned from the function is when we fail to actually verify the scale. So return boolean instead. --- test/e2e/minscale_readiness_test.go | 15 +++++---------- 1 file changed, 5 insertions(+), 10 deletions(-) diff --git a/test/e2e/minscale_readiness_test.go b/test/e2e/minscale_readiness_test.go index 34db9d7e1a94..be748115635c 100644 --- a/test/e2e/minscale_readiness_test.go +++ b/test/e2e/minscale_readiness_test.go @@ -127,8 +127,8 @@ func TestMinScale(t *testing.T) { // After update, ensure new revision holds minScale t.Log("Hold minScale after update") - if err := ensureDesiredScale(clients, newServiceName, gte(minScale)); err != nil { - t.Fatalf("The revision %q did not stay at scale >= %d after creating route: %v", newRevName, minScale, err) + if !ensureDesiredScale(clients, newServiceName, gte(minScale)) { + t.Fatalf("The revision %q did not stay at scale >= %d after creating route", newRevName, minScale) } // After update, ensure old revision ignores minScale @@ -211,13 +211,12 @@ func waitForDesiredScale(clients *test.Clients, serviceName string, cond func(in } return cond(resources.ReadyAddressCount(endpoint)), nil }) - } -func ensureDesiredScale(clients *test.Clients, serviceName string, cond func(int) bool) error { +func ensureDesiredScale(clients *test.Clients, serviceName string, cond func(int) bool) bool { endpoints := clients.KubeClient.Kube.CoreV1().Endpoints(test.ServingNamespace) - if err := wait.PollImmediate(250*time.Millisecond, 5*time.Second, func() (bool, error) { + return wait.PollImmediate(250*time.Millisecond, 5*time.Second, func() (bool, error) { endpoint, err := endpoints.Get(serviceName, metav1.GetOptions{}) if err != nil { return false, nil @@ -228,9 +227,5 @@ func ensureDesiredScale(clients *test.Clients, serviceName string, cond func(int } return false, nil - }); err != wait.ErrWaitTimeout { - return err - } - - return nil + }) == wait.ErrWaitTimeout } From b46767c8ec8aa679d10b9e6d304823a180d1a196 Mon Sep 17 00:00:00 2001 From: Tanzeeb Khalili Date: Fri, 13 Mar 2020 11:41:30 -0400 Subject: [PATCH 4/4] Fix minscale on deploy (#7214) * Test minscale stability when configuration is created * Check both .spec.traffic and .status.traffic for resources to label * Inline condition * Inline owner reference check --- pkg/reconciler/labeler/labeler_test.go | 78 +++++++++++++++++++++----- pkg/reconciler/labeler/labels.go | 45 +++++++++------ test/e2e/minscale_readiness_test.go | 56 ++++++++---------- 3 files changed, 114 insertions(+), 65 deletions(-) diff --git a/pkg/reconciler/labeler/labeler_test.go b/pkg/reconciler/labeler/labeler_test.go index 5b6ec3819769..d22b3b965f34 100644 --- a/pkg/reconciler/labeler/labeler_test.go +++ b/pkg/reconciler/labeler/labeler_test.go @@ -77,7 +77,7 @@ func TestReconcile(t *testing.T) { }, { Name: "label pinned revision", Objects: []runtime.Object{ - simpleRoute("default", "pinned-revision", "the-revision"), + pinnedRoute("default", "pinned-revision", "the-revision"), simpleConfig("default", "the-config"), rev("default", "the-config"), rev("default", "the-config", WithRevName("the-revision")), @@ -103,6 +103,43 @@ func TestReconcile(t *testing.T) { WithRevisionLabel("serving.knative.dev/route", "steady-state")), }, Key: "default/steady-state", + }, { + Name: "no ready revision", + Objects: []runtime.Object{ + simpleRunLatest("default", "no-ready-revision", "the-config", WithStatusTraffic()), + simpleConfig("default", "the-config", WithLatestReady("")), + rev("default", "the-config"), + }, + WantPatches: []clientgotesting.PatchActionImpl{ + patchAddFinalizerAction("default", "no-ready-revision"), + patchAddLabel("default", rev("default", "the-config").Name, + "serving.knative.dev/route", "no-ready-revision"), + patchAddLabel("default", "the-config", + "serving.knative.dev/route", "no-ready-revision"), + }, + WantEvents: []string{ + Eventf(corev1.EventTypeNormal, "FinalizerUpdate", "Updated %q finalizers", "no-ready-revision"), + }, + Key: "default/no-ready-revision", + }, { + Name: "transitioning route", + Objects: []runtime.Object{ + simpleRunLatest("default", "transitioning-route", "old", WithRouteFinalizer, + WithSpecTraffic(configTraffic("new"))), + simpleConfig("default", "old", + WithConfigLabel("serving.knative.dev/route", "transitioning-route")), + rev("default", "old", + WithRevisionLabel("serving.knative.dev/route", "transitioning-route")), + simpleConfig("default", "new"), + rev("default", "new"), + }, + WantPatches: []clientgotesting.PatchActionImpl{ + patchAddLabel("default", rev("default", "new").Name, + "serving.knative.dev/route", "transitioning-route"), + patchAddLabel("default", "new", + "serving.knative.dev/route", "transitioning-route"), + }, + Key: "default/transitioning-route", }, { Name: "failure adding label (revision)", // Induce a failure during patching @@ -278,23 +315,38 @@ func TestReconcile(t *testing.T) { })) } -func routeWithTraffic(namespace, name string, traffic v1.TrafficTarget, opts ...RouteOption) *v1.Route { - return Route(namespace, name, append(opts, WithStatusTraffic(traffic))...) +func configTraffic(name string) v1.TrafficTarget { + return v1.TrafficTarget{ + ConfigurationName: name, + Percent: ptr.Int64(100), + LatestRevision: ptr.Bool(true), + } } -func simpleRunLatest(namespace, name, config string, opts ...RouteOption) *v1.Route { - return routeWithTraffic(namespace, name, v1.TrafficTarget{ - RevisionName: config + "-dbnfd", +func revTraffic(name string, latest bool) v1.TrafficTarget { + return v1.TrafficTarget{ + RevisionName: name, Percent: ptr.Int64(100), - LatestRevision: ptr.Bool(true), - }, opts...) + LatestRevision: ptr.Bool(latest), + } } -func simpleRoute(namespace, name, revision string, opts ...RouteOption) *v1.Route { - return routeWithTraffic(namespace, name, v1.TrafficTarget{ - RevisionName: revision, - Percent: ptr.Int64(100), - }, opts...) +func routeWithTraffic(namespace, name string, spec, status v1.TrafficTarget, opts ...RouteOption) *v1.Route { + return Route(namespace, name, + append([]RouteOption{WithSpecTraffic(spec), WithStatusTraffic(status)}, opts...)...) +} + +func simpleRunLatest(namespace, name, config string, opts ...RouteOption) *v1.Route { + return routeWithTraffic(namespace, name, + configTraffic(config), + revTraffic(config+"-dbnfd", true), + opts...) +} + +func pinnedRoute(namespace, name, revision string, opts ...RouteOption) *v1.Route { + traffic := revTraffic(revision, false) + + return routeWithTraffic(namespace, name, traffic, traffic, opts...) } func simpleConfig(namespace, name string, opts ...ConfigOption) *v1.Configuration { diff --git a/pkg/reconciler/labeler/labels.go b/pkg/reconciler/labeler/labels.go index ba69a4fadfbf..a6ddbbc96c5c 100644 --- a/pkg/reconciler/labeler/labels.go +++ b/pkg/reconciler/labeler/labels.go @@ -45,28 +45,37 @@ func (c *Reconciler) syncLabels(ctx context.Context, r *v1.Route) error { revisions := sets.NewString() configs := sets.NewString() - // Walk the revisions in Route's .status.traffic and build a list - // 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 { - return err + // Walk the Route's .status.traffic and .spec.traffic and build a list + // of revisions and configurations to label + for _, tt := range append(r.Status.Traffic, r.Spec.Traffic...) { + revName := tt.RevisionName + configName := tt.ConfigurationName + + if revName != "" { + rev, err := c.revisionLister.Revisions(r.Namespace).Get(revName) + if err != nil { + return err + } + + revisions.Insert(revName) + + // If the owner reference is a configuration, treat it like a configuration target + if owner := metav1.GetControllerOf(rev); owner != nil && owner.Kind == "Configuration" { + configName = owner.Name + } } - revisions.Insert(tt.RevisionName) - // If the owner reference is a configuration, add it to the list of configurations - owner := metav1.GetControllerOf(rev) - if owner != nil && owner.Kind == "Configuration" { - configs.Insert(owner.Name) + if configName != "" { + config, err := c.configurationLister.Configurations(r.Namespace).Get(configName) + if err != nil { + return err + } + + configs.Insert(configName) - // If we are tracking the latest revision, add the latest created revision as well + // If the target is for the latest revision, add the latest created revision to the list // 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 - } - + if config.Status.LatestCreatedRevisionName != "" && tt.LatestRevision != nil && *tt.LatestRevision { revisions.Insert(config.Status.LatestCreatedRevisionName) } } diff --git a/test/e2e/minscale_readiness_test.go b/test/e2e/minscale_readiness_test.go index be748115635c..d4289bc99a8a 100644 --- a/test/e2e/minscale_readiness_test.go +++ b/test/e2e/minscale_readiness_test.go @@ -54,6 +54,11 @@ func TestMinScale(t *testing.T) { test.CleanupOnInterrupt(func() { test.TearDown(clients, names) }) defer test.TearDown(clients, names) + t.Log("Creating route") + if _, err := v1a1test.CreateRoute(t, clients, names); err != nil { + t.Fatalf("Failed to create Route: %v", err) + } + t.Log("Creating configuration") cfg, err := v1a1test.CreateConfiguration(t, clients, names, withMinScale(minScale)) if err != nil { @@ -63,13 +68,11 @@ func TestMinScale(t *testing.T) { revName := latestRevisionName(t, clients, names.Config, "") serviceName := privateServiceName(t, clients, revName) - // Before becoming ready, observe minScale t.Log("Waiting for revision to scale to minScale before becoming ready") if err := waitForDesiredScale(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 t.Log("Waiting for revision to become ready") if err := v1a1test.WaitForRevisionState( clients.ServingAlphaClient, revName, v1a1test.IsRevisionReady, "RevisionIsReady", @@ -77,30 +80,9 @@ func TestMinScale(t *testing.T) { t.Fatalf("The Revision %q did not become ready: %v", revName, err) } - // Without a route, ignore minScale - t.Log("Waiting for revision to scale below minScale after becoming ready") - if err := waitForDesiredScale(clients, serviceName, lt(minScale)); err != nil { - t.Fatalf("The revision %q did not scale < minScale after becoming ready: %v", revName, err) - } - - // Create route - t.Log("Creating route") - if _, err := v1a1test.CreateRoute(t, clients, names); err != nil { - t.Fatalf("Failed to create Route: %v", err) - } - - // Route becomes ready - t.Log("Waiting for route to become ready") - if err := v1a1test.WaitForRouteState( - clients.ServingAlphaClient, names.Route, v1a1test.IsRouteReady, "RouteIsReady", - ); err != nil { - t.Fatalf("The Route %q is not ready: %v", names.Route, err) - } - - // With a route, observe minScale - t.Log("Waiting for revision to scale to minScale after creating route") - if err := waitForDesiredScale(clients, serviceName, gte(minScale)); err != nil { - t.Fatalf("The revision %q did not scale >= %d after creating route: %v", revName, minScale, err) + t.Log("Holding revision at minScale after becoming ready") + if !ensureDesiredScale(clients, serviceName, gte(minScale)) { + t.Fatalf("The revision %q did not stay at scale >= %d after becoming ready", revName, minScale) } t.Log("Updating configuration") @@ -111,31 +93,37 @@ func TestMinScale(t *testing.T) { newRevName := latestRevisionName(t, clients, names.Config, revName) newServiceName := privateServiceName(t, clients, newRevName) - // After update, observe minScale in new revision - t.Log("Waiting for latest revision to scale to minScale after update") + t.Log("Waiting for new revision to scale to minScale after update") if err := waitForDesiredScale(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") + t.Log("Waiting for new 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") + t.Log("Holding new revision at minScale after becoming ready") if !ensureDesiredScale(clients, newServiceName, gte(minScale)) { - t.Fatalf("The revision %q did not stay at scale >= %d after creating route", newRevName, minScale) + t.Fatalf("The new revision %q did not stay at scale >= %d after becoming ready", newRevName, minScale) } - // After update, ensure old revision ignores minScale t.Log("Waiting for old revision to scale below minScale after being replaced") if err := waitForDesiredScale(clients, serviceName, lt(minScale)); err != nil { t.Fatalf("The revision %q did not scale < minScale after being replaced: %v", revName, err) } + + t.Log("Deleting route") + if err := clients.ServingAlphaClient.Routes.Delete(names.Route, &metav1.DeleteOptions{}); err != nil { + t.Fatalf("Failed to delete route %q: %v", names.Route, err) + } + + t.Log("Waiting for new revision to scale below minScale when there is no route") + if err := waitForDesiredScale(clients, newServiceName, lt(minScale)); err != nil { + t.Fatalf("The revision %q did not scale < minScale after being replaced: %v", newRevName, err) + } } func gte(m int) func(int) bool {