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 {