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..d22b3b965f34 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{ + pinnedRoute("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{ @@ -84,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 @@ -161,6 +217,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{ @@ -242,15 +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 revTraffic(name string, latest bool) v1.TrafficTarget { + return v1.TrafficTarget{ + RevisionName: name, + Percent: ptr.Int64(100), + LatestRevision: ptr.Bool(latest), + } +} + +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, v1.TrafficTarget{ - RevisionName: config + "-dbnfd", - Percent: ptr.Int64(100), - }, opts...) + 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 { @@ -276,7 +372,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..a6ddbbc96c5c 100644 --- a/pkg/reconciler/labeler/labels.go +++ b/pkg/reconciler/labeler/labels.go @@ -45,17 +45,39 @@ 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) - 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 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 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 7fc7d61f0a42..d4289bc99a8a 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,33 +44,35 @@ 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", } 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") - 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 := 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, privateServiceName, gte(minScale)); err != nil { - t.Fatalf("The deployment %q did not scale >= %d before becoming ready: %v", deploymentName, minScale, err) + 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", @@ -76,30 +80,49 @@ 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(t, clients, privateServiceName, lt(minScale)); err != nil { - t.Fatalf("The deployment %q did not scale < minScale after becoming ready: %v", deploymentName, 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) } - // Create route - t.Log("Creating route") - if _, err := v1a1test.CreateRoute(t, clients, names); err != nil { - t.Fatalf("Failed to create Route: %v", err) + t.Log("Updating configuration") + if _, err := v1a1test.PatchConfig(clients, cfg, withEnv("FOO", "BAR")); err != nil { + t.Fatalf("Failed to update Configuration: %v", err) } - // Route becomes ready - t.Log("Waiting for route to become ready") - if err := v1a1test.WaitForRouteState( - clients.ServingAlphaClient, names.Route, v1a1test.IsRouteReady, "RouteIsReady", + newRevName := latestRevisionName(t, clients, names.Config, revName) + newServiceName := privateServiceName(t, clients, newRevName) + + 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) + } + + t.Log("Waiting for new revision to become ready") + if err := v1a1test.WaitForRevisionState( + clients.ServingAlphaClient, newRevName, v1a1test.IsRevisionReady, "RevisionIsReady", ); err != nil { - t.Fatalf("The Route %q is not ready: %v", names.Route, err) + t.Fatalf("The Revision %q did not become ready: %v", newRevName, err) + } + + t.Log("Holding new revision at minScale after becoming ready") + if !ensureDesiredScale(clients, newServiceName, gte(minScale)) { + t.Fatalf("The new revision %q did not stay at scale >= %d after becoming ready", newRevName, 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) } - // 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) + 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) } } @@ -115,6 +138,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 +153,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{}) @@ -141,32 +172,48 @@ func latestRevisionName(t *testing.T, clients *test.Clients, configName string) 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) { sks, err := clients.NetworkingClient.ServerlessServices.Get(revisionName, metav1.GetOptions{}) if err != nil { 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) } + return privateServiceName } -func waitForDesiredScale(t *testing.T, clients *test.Clients, privateServiceName 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(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(clients *test.Clients, serviceName string, cond func(int) bool) bool { + endpoints := clients.KubeClient.Kube.CoreV1().Endpoints(test.ServingNamespace) + + 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 + } + + if scale := resources.ReadyAddressCount(endpoint); !cond(scale) { + return false, fmt.Errorf("scale %d didn't meet condition", scale) + } + + return false, nil + }) == wait.ErrWaitTimeout +} 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()