From e3ce5505820a4ca867a78ac89e6a8413e70699d6 Mon Sep 17 00:00:00 2001 From: Tanzeeb Khalili Date: Wed, 11 Mar 2020 15:27:22 -0400 Subject: [PATCH 1/4] Test minscale stability when configuration is created --- test/e2e/minscale_readiness_test.go | 56 ++++++++++++----------------- 1 file changed, 22 insertions(+), 34 deletions(-) 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 { From 92afa46e61c3f9cfa1159f5f646be196e8da7997 Mon Sep 17 00:00:00 2001 From: Tanzeeb Khalili Date: Wed, 11 Mar 2020 18:50:03 -0400 Subject: [PATCH 2/4] Check both .spec.traffic and .status.traffic for resources to label --- pkg/reconciler/labeler/labeler_test.go | 78 +++++++++++++++++++++----- pkg/reconciler/labeler/labels.go | 48 ++++++++++------ 2 files changed, 95 insertions(+), 31 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..5cd7ad9fd8b8 100644 --- a/pkg/reconciler/labeler/labels.go +++ b/pkg/reconciler/labeler/labels.go @@ -45,28 +45,40 @@ 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 + + latest := tt.LatestRevision != nil && *tt.LatestRevision + + 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 + owner := metav1.GetControllerOf(rev) + if 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 latest && config.Status.LatestCreatedRevisionName != "" { revisions.Insert(config.Status.LatestCreatedRevisionName) } } From 8313f82c53487986b81da1664dcb4e25b6f61c69 Mon Sep 17 00:00:00 2001 From: Tanzeeb Khalili Date: Thu, 12 Mar 2020 12:27:49 -0400 Subject: [PATCH 3/4] Inline condition --- pkg/reconciler/labeler/labels.go | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/pkg/reconciler/labeler/labels.go b/pkg/reconciler/labeler/labels.go index 5cd7ad9fd8b8..29d0eccd914f 100644 --- a/pkg/reconciler/labeler/labels.go +++ b/pkg/reconciler/labeler/labels.go @@ -51,8 +51,6 @@ func (c *Reconciler) syncLabels(ctx context.Context, r *v1.Route) error { revName := tt.RevisionName configName := tt.ConfigurationName - latest := tt.LatestRevision != nil && *tt.LatestRevision - if revName != "" { rev, err := c.revisionLister.Revisions(r.Namespace).Get(revName) if err != nil { @@ -78,7 +76,7 @@ func (c *Reconciler) syncLabels(ctx context.Context, r *v1.Route) error { // 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 latest && config.Status.LatestCreatedRevisionName != "" { + if config.Status.LatestCreatedRevisionName != "" && tt.LatestRevision != nil && *tt.LatestRevision { revisions.Insert(config.Status.LatestCreatedRevisionName) } } From df128b4dcff9d683d2c873215bf7d154be03b2f8 Mon Sep 17 00:00:00 2001 From: Tanzeeb Khalili Date: Thu, 12 Mar 2020 14:12:30 -0400 Subject: [PATCH 4/4] Inline owner reference check --- pkg/reconciler/labeler/labels.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/pkg/reconciler/labeler/labels.go b/pkg/reconciler/labeler/labels.go index 29d0eccd914f..a6ddbbc96c5c 100644 --- a/pkg/reconciler/labeler/labels.go +++ b/pkg/reconciler/labeler/labels.go @@ -60,8 +60,7 @@ func (c *Reconciler) syncLabels(ctx context.Context, r *v1.Route) error { revisions.Insert(revName) // If the owner reference is a configuration, treat it like a configuration target - owner := metav1.GetControllerOf(rev) - if owner != nil && owner.Kind == "Configuration" { + if owner := metav1.GetControllerOf(rev); owner != nil && owner.Kind == "Configuration" { configName = owner.Name } }