diff --git a/pkg/apis/serving/metadata_validation.go b/pkg/apis/serving/metadata_validation.go index 43cdea380587..0aede7a3be66 100644 --- a/pkg/apis/serving/metadata_validation.go +++ b/pkg/apis/serving/metadata_validation.go @@ -37,6 +37,7 @@ var ( UpdaterAnnotation, CreatorAnnotation, RevisionLastPinnedAnnotationKey, + RoutingStateModifiedAnnotationKey, GroupNamePrefix+"forceUpgrade", ) ) diff --git a/pkg/apis/serving/v1/revision_helpers.go b/pkg/apis/serving/v1/revision_helpers.go index ae65823181d4..5a5473ac6c4a 100644 --- a/pkg/apis/serving/v1/revision_helpers.go +++ b/pkg/apis/serving/v1/revision_helpers.go @@ -21,6 +21,7 @@ import ( "time" corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/util/clock" net "knative.dev/networking/pkg/apis/networking" "knative.dev/pkg/kmeta" "knative.dev/serving/pkg/apis/serving" @@ -122,10 +123,15 @@ func (r *Revision) SetRoutingState(state RoutingState) { r.Annotations = kmeta.UnionMaps(r.Annotations, map[string]string{ - serving.RoutingStateModifiedAnnotationKey: time.Now().UTC().Format(time.RFC3339), + serving.RoutingStateModifiedAnnotationKey: RoutingStateModifiedString(clock.RealClock{}), }) } +// RoutingStateModifiedString gives a formatted now timestamp. +func RoutingStateModifiedString(clock clock.Clock) string { + return clock.Now().UTC().Format(time.RFC3339) +} + // GetRoutingState retrieves the RoutingState label. func (r *Revision) GetRoutingState() RoutingState { return RoutingState(r.ObjectMeta.Labels[serving.RoutingStateLabelKey]) diff --git a/pkg/reconciler/gc/v2/gc_test.go b/pkg/reconciler/gc/v2/gc_test.go index 9a77c167722f..e519ca3c70b3 100644 --- a/pkg/reconciler/gc/v2/gc_test.go +++ b/pkg/reconciler/gc/v2/gc_test.go @@ -143,12 +143,12 @@ func TestCollect(t *testing.T) { WithCreationTimestamp(oldest)), rev("keep-no-last-pinned", "foo", 5555, MarkRevisionReady, WithRevName("5555"), - WithCreationTimestamp(older), - WithLastPinned(tenMinutesAgo)), + WithCreationTimestamp(oldest), + WithLastPinned(older)), rev("keep-no-last-pinned", "foo", 5556, MarkRevisionReady, WithRevName("5556"), - WithCreationTimestamp(old), - WithLastPinned(tenMinutesAgo)), + WithCreationTimestamp(oldest), + WithLastPinned(old)), }, }, { name: "keep recent lastPinned", @@ -206,7 +206,7 @@ func TestCollect(t *testing.T) { rev("keep-all", "foo", 5554, WithRevName("keep-all"), WithCreationTimestamp(oldest), - WithLastPinned(tenMinutesAgo)), + WithLastPinned(old)), }, }} diff --git a/pkg/reconciler/labeler/controller.go b/pkg/reconciler/labeler/controller.go index eefc439ed184..ee7e19c15ebb 100644 --- a/pkg/reconciler/labeler/controller.go +++ b/pkg/reconciler/labeler/controller.go @@ -19,6 +19,7 @@ package labeler import ( "context" + "k8s.io/apimachinery/pkg/util/clock" "k8s.io/client-go/tools/cache" v1 "knative.dev/serving/pkg/apis/serving/v1" @@ -43,7 +44,14 @@ func NewController( ctx context.Context, cmw configmap.Watcher, ) *controller.Impl { + return newControllerWithClock(ctx, cmw, clock.RealClock{}) +} +func newControllerWithClock( + ctx context.Context, + cmw configmap.Watcher, + clock clock.Clock, +) *controller.Impl { ctx = servingreconciler.AnnotateLoggerWithName(ctx, controllerAgentName) logger := logging.FromContext(ctx) routeInformer := routeinformer.Get(ctx) @@ -54,6 +62,7 @@ func NewController( client: servingclient.Get(ctx), configurationLister: configInformer.Lister(), revisionLister: revisionInformer.Lister(), + clock: clock, } impl := routereconciler.NewImpl(ctx, c, func(*controller.Impl) controller.Options { return controller.Options{ diff --git a/pkg/reconciler/labeler/labeler.go b/pkg/reconciler/labeler/labeler.go index 7dc28c2032f1..cf27ca68288c 100644 --- a/pkg/reconciler/labeler/labeler.go +++ b/pkg/reconciler/labeler/labeler.go @@ -19,6 +19,7 @@ package labeler import ( "context" + "k8s.io/apimachinery/pkg/util/clock" pkgreconciler "knative.dev/pkg/reconciler" "knative.dev/pkg/tracker" cfgmap "knative.dev/serving/pkg/apis/config" @@ -40,6 +41,7 @@ type Reconciler struct { // Listers index properties about resources configurationLister listers.ConfigurationLister revisionLister listers.RevisionLister + clock clock.Clock } // Check that our Reconciler implements routereconciler.Interface @@ -50,8 +52,8 @@ func (c *Reconciler) FinalizeKind(ctx context.Context, r *v1.Route) pkgreconcile switch cfgmap.FromContextOrDefaults(ctx).Features.ResponsiveRevisionGC { case cfgmap.Enabled: // v2 logic - cacc := labelerv2.NewConfigurationAccessor(c.client, c.tracker, c.configurationLister) - racc := labelerv2.NewRevisionAccessor(c.client, c.tracker, c.revisionLister) + cacc := labelerv2.NewConfigurationAccessor(c.client, c.tracker, c.configurationLister, c.clock) + racc := labelerv2.NewRevisionAccessor(c.client, c.tracker, c.revisionLister, c.clock) return labelerv2.ClearLabels(r.Namespace, r.Name, cacc, racc) default: // v1 logic @@ -65,8 +67,8 @@ func (c *Reconciler) ReconcileKind(ctx context.Context, r *v1.Route) pkgreconcil switch cfgmap.FromContextOrDefaults(ctx).Features.ResponsiveRevisionGC { case cfgmap.Enabled: // v2 logic - cacc := labelerv2.NewConfigurationAccessor(c.client, c.tracker, c.configurationLister) - racc := labelerv2.NewRevisionAccessor(c.client, c.tracker, c.revisionLister) + cacc := labelerv2.NewConfigurationAccessor(c.client, c.tracker, c.configurationLister, c.clock) + racc := labelerv2.NewRevisionAccessor(c.client, c.tracker, c.revisionLister, c.clock) return labelerv2.SyncLabels(r, cacc, racc) default: // v1 logic diff --git a/pkg/reconciler/labeler/labelerv2_test.go b/pkg/reconciler/labeler/labelerv2_test.go index 31d3015eea3d..a3d9c589aa24 100644 --- a/pkg/reconciler/labeler/labelerv2_test.go +++ b/pkg/reconciler/labeler/labelerv2_test.go @@ -20,6 +20,7 @@ import ( "context" "fmt" "testing" + "time" // Inject the fake informers that this controller needs. servingclient "knative.dev/serving/pkg/client/injection/client/fake" @@ -30,6 +31,7 @@ import ( corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/util/clock" clientgotesting "k8s.io/client-go/testing" routereconciler "knative.dev/serving/pkg/client/injection/reconciler/serving/v1/route" @@ -49,6 +51,7 @@ import ( // This is heavily based on the way the OpenShift Ingress controller tests its reconciliation method. func TestV2Reconcile(t *testing.T) { now := metav1.Now() + fakeTime := now.Time table := TableTest{{ Name: "bad workqueue key", @@ -70,8 +73,8 @@ func TestV2Reconcile(t *testing.T) { }, WantPatches: []clientgotesting.PatchActionImpl{ patchAddFinalizerAction("default", "first-reconcile"), - patchAddLabel("default", rev("default", "the-config").Name, - "serving.knative.dev/route", "first-reconcile"), + patchAddRouteAndServingStateLabel( + "default", rev("default", "the-config").Name, "first-reconcile", now.Time), patchAddLabel("default", "the-config", "serving.knative.dev/route", "first-reconcile"), }, WantEvents: []string{ @@ -89,8 +92,8 @@ func TestV2Reconcile(t *testing.T) { }, WantPatches: []clientgotesting.PatchActionImpl{ patchAddFinalizerAction("default", "pinned-revision"), - patchAddLabel("default", "the-revision", - "serving.knative.dev/route", "pinned-revision"), + patchAddRouteAndServingStateLabel( + "default", "the-revision", "pinned-revision", now.Time), patchAddLabel("default", "the-config", "serving.knative.dev/route", "pinned-revision"), }, @@ -106,7 +109,9 @@ func TestV2Reconcile(t *testing.T) { simpleConfig("default", "the-config", WithConfigLabel("serving.knative.dev/route", "steady-state")), rev("default", "the-config", - WithRevisionLabel("serving.knative.dev/route", "steady-state")), + WithRevisionLabel("serving.knative.dev/route", "steady-state"), + WithRoutingState(v1.RoutingStateActive), + WithRoutingStateModified(now.Time)), }, Key: "default/steady-state", }, { @@ -119,8 +124,8 @@ func TestV2Reconcile(t *testing.T) { }, WantPatches: []clientgotesting.PatchActionImpl{ patchAddFinalizerAction("default", "no-ready-revision"), - patchAddLabel("default", rev("default", "the-config").Name, - "serving.knative.dev/route", "no-ready-revision"), + patchAddRouteAndServingStateLabel( + "default", rev("default", "the-config").Name, "no-ready-revision", now.Time), patchAddLabel("default", "the-config", "serving.knative.dev/route", "no-ready-revision"), }, @@ -137,13 +142,14 @@ func TestV2Reconcile(t *testing.T) { simpleConfig("default", "old", WithConfigLabel("serving.knative.dev/route", "transitioning-route")), rev("default", "old", - WithRevisionLabel("serving.knative.dev/route", "transitioning-route")), + WithRevisionLabel("serving.knative.dev/route", "transitioning-route"), + WithRoutingState(v1.RoutingStateActive)), simpleConfig("default", "new"), rev("default", "new"), }, WantPatches: []clientgotesting.PatchActionImpl{ - patchAddLabel("default", rev("default", "new").Name, - "serving.knative.dev/route", "transitioning-route"), + patchAddRouteAndServingStateLabel( + "default", rev("default", "new").Name, "transitioning-route", now.Time), patchAddLabel("default", "new", "serving.knative.dev/route", "transitioning-route"), }, @@ -162,8 +168,8 @@ func TestV2Reconcile(t *testing.T) { rev("default", "the-config"), }, WantPatches: []clientgotesting.PatchActionImpl{ - patchAddLabel("default", rev("default", "the-config").Name, - "serving.knative.dev/route", "add-label-failure"), + patchAddRouteAndServingStateLabel( + "default", rev("default", "the-config").Name, "add-label-failure", now.Time), }, WantEvents: []string{ Eventf(corev1.EventTypeWarning, "InternalError", @@ -182,7 +188,9 @@ func TestV2Reconcile(t *testing.T) { simpleRunLatest("default", "add-label-failure", "the-config", WithRouteFinalizer), simpleConfig("default", "the-config"), rev("default", "the-config", - WithRevisionLabel("serving.knative.dev/route", "add-label-failure")), + WithRevisionLabel("serving.knative.dev/route", "add-label-failure"), + WithRoutingState(v1.RoutingStateActive), + WithRoutingStateModified(now.Time)), }, WantPatches: []clientgotesting.PatchActionImpl{ patchAddLabel("default", "the-config", "serving.knative.dev/route", "add-label-failure"), @@ -217,15 +225,16 @@ func TestV2Reconcile(t *testing.T) { simpleConfig("default", "old-config", WithConfigLabel("serving.knative.dev/route", "config-change")), rev("default", "old-config", - WithRevisionLabel("serving.knative.dev/route", "config-change")), + WithRevisionLabel("serving.knative.dev/route", "config-change"), + WithRoutingState(v1.RoutingStateActive)), simpleConfig("default", "new-config"), rev("default", "new-config"), }, WantPatches: []clientgotesting.PatchActionImpl{ - patchRemoveLabel("default", rev("default", "old-config").Name, - "serving.knative.dev/route"), - patchAddLabel("default", rev("default", "new-config").Name, - "serving.knative.dev/route", "config-change"), + patchRemoveRouteAndServingStateLabel( + "default", rev("default", "old-config").Name, now.Time), + patchAddRouteAndServingStateLabel( + "default", rev("default", "new-config").Name, "config-change", now.Time), patchRemoveLabel("default", "old-config", "serving.knative.dev/route"), patchAddLabel("default", "new-config", "serving.knative.dev/route", "config-change"), }, @@ -239,13 +248,14 @@ func TestV2Reconcile(t *testing.T) { WithLatestCreated("the-config-ecoge"), WithConfigLabel("serving.knative.dev/route", "config-update")), rev("default", "the-config", - WithRevisionLabel("serving.knative.dev/route", "config-update")), + WithRevisionLabel("serving.knative.dev/route", "config-update"), + WithRoutingState(v1.RoutingStateActive)), rev("default", "the-config", WithRevName("the-config-ecoge")), }, WantPatches: []clientgotesting.PatchActionImpl{ - patchAddLabel("default", "the-config-ecoge", - "serving.knative.dev/route", "config-update"), + patchAddRouteAndServingStateLabel( + "default", "the-config-ecoge", "config-update", now.Time), }, Key: "default/config-update", }, { @@ -279,7 +289,9 @@ func TestV2Reconcile(t *testing.T) { simpleConfig("default", "new-config", WithConfigLabel("serving.knative.dev/route", "delete-label-failure")), rev("default", "new-config", - WithRevisionLabel("serving.knative.dev/route", "delete-label-failure")), + WithRevisionLabel("serving.knative.dev/route", "delete-label-failure"), + WithRoutingState(v1.RoutingStateActive), + WithRoutingStateModified(now.Time)), rev("default", "old-config"), }, WantPatches: []clientgotesting.PatchActionImpl{ @@ -310,8 +322,8 @@ func TestV2Reconcile(t *testing.T) { WithRevisionLabel("serving.knative.dev/route", "delete-label-failure")), }, WantPatches: []clientgotesting.PatchActionImpl{ - patchRemoveLabel("default", rev("default", "old-config").Name, - "serving.knative.dev/route"), + patchRemoveRouteAndServingStateLabel( + "default", rev("default", "old-config").Name, now.Time), }, WantEvents: []string{ Eventf(corev1.EventTypeWarning, "InternalError", @@ -326,6 +338,7 @@ func TestV2Reconcile(t *testing.T) { configurationLister: listers.GetConfigurationLister(), revisionLister: listers.GetRevisionLister(), tracker: &NullTracker{}, + clock: clock.NewFakeClock(fakeTime), } return routereconciler.NewReconciler(ctx, logging.FromContext(ctx), servingclient.Get(ctx), @@ -413,6 +426,10 @@ func patchRemoveLabel(namespace, name, key string) clientgotesting.PatchActionIm return action } +func patchRemoveRouteAndServingStateLabel(namespace, name string, now time.Time) clientgotesting.PatchActionImpl { + return patchAddRouteAndServingStateLabel(namespace, name, "null", now) +} + func patchAddLabel(namespace, name, key, value string) clientgotesting.PatchActionImpl { action := clientgotesting.PatchActionImpl{} action.Name = name @@ -424,6 +441,29 @@ func patchAddLabel(namespace, name, key, value string) clientgotesting.PatchActi return action } +func patchAddRouteAndServingStateLabel(namespace, name, routeName string, now time.Time) clientgotesting.PatchActionImpl { + action := clientgotesting.PatchActionImpl{} + action.Name = name + action.Namespace = namespace + + state := string(v1.RoutingStateReserve) + + // Note: the raw json `"key": null` removes a value, whereas an actual value + // called "null" would need quotes to parse as a string `"key":"null"`. + if routeName != "null" { + state = string(v1.RoutingStateActive) + routeName = `"` + routeName + `"` + } + + patch := fmt.Sprintf( + `{"metadata":{"annotations":{"serving.knative.dev/routingStateModified":%q},`+ + `"labels":{"serving.knative.dev/route":%s,`+ + `"serving.knative.dev/routingState":%q}}}`, now.UTC().Format(time.RFC3339), routeName, state) + + action.Patch = []byte(patch) + return action +} + func patchAddFinalizerAction(namespace, name string) clientgotesting.PatchActionImpl { resource := v1.Resource("routes") finalizer := resource.String() diff --git a/pkg/reconciler/labeler/v2/accessors.go b/pkg/reconciler/labeler/v2/accessors.go index 17129a79f54e..4f107ed82b91 100644 --- a/pkg/reconciler/labeler/v2/accessors.go +++ b/pkg/reconciler/labeler/v2/accessors.go @@ -21,10 +21,12 @@ import ( "k8s.io/apimachinery/pkg/labels" "k8s.io/apimachinery/pkg/types" + "k8s.io/apimachinery/pkg/util/clock" "knative.dev/pkg/kmeta" "knative.dev/pkg/tracker" "knative.dev/serving/pkg/apis/serving" + v1 "knative.dev/serving/pkg/apis/serving/v1" clientset "knative.dev/serving/pkg/client/clientset/versioned" listers "knative.dev/serving/pkg/client/listers/serving/v1" ) @@ -42,6 +44,7 @@ type Revision struct { client clientset.Interface tracker tracker.Interface revisionLister listers.RevisionLister + clock clock.Clock } // Revision implements Accessor @@ -51,50 +54,76 @@ var _ Accessor = (*Revision)(nil) func NewRevisionAccessor( client clientset.Interface, tracker tracker.Interface, - lister listers.RevisionLister) *Revision { + lister listers.RevisionLister, + clock clock.Clock) *Revision { return &Revision{ client: client, tracker: tracker, revisionLister: lister, + clock: clock, } } // makeMetadataPatch makes a metadata map to be patched or nil if no changes are needed. -func makeMetadataPatch(acc kmeta.Accessor, routeName string) (map[string]interface{}, error) { - labels, err := addRouteLabel(acc, routeName) - if err != nil { +func makeMetadataPatch( + acc kmeta.Accessor, routeName string, addRoutingState bool, clock clock.Clock) (map[string]interface{}, error) { + labels := map[string]interface{}{} + annotations := map[string]interface{}{} + + if err := addRouteLabel(acc, routeName, labels); err != nil { return nil, err } + if addRoutingState { + markRoutingState(acc, routeName != "", clock, labels, annotations) + } + + meta := map[string]interface{}{} if len(labels) > 0 { - meta := map[string]interface{}{ - "metadata": map[string]interface{}{ - "labels": labels, - }, - } - return meta, nil + meta["labels"] = labels + } + if len(annotations) > 0 { + meta["annotations"] = annotations + } + if len(meta) > 0 { + return map[string]interface{}{"metadata": meta}, nil } return nil, nil } +// markRoutingState updates the RoutingStateLabel and bumps the modified time annotation. +func markRoutingState( + acc kmeta.Accessor, hasRoute bool, clock clock.Clock, + diffLabels, diffAnn map[string]interface{}) { + wantState := string(v1.RoutingStateReserve) + if hasRoute { + wantState = string(v1.RoutingStateActive) + } + + if acc.GetLabels()[serving.RoutingStateLabelKey] != wantState { + diffLabels[serving.RoutingStateLabelKey] = wantState + diffAnn[serving.RoutingStateModifiedAnnotationKey] = v1.RoutingStateModifiedString(clock) + } +} + // addRouteLabel appends the route label to the list of labels if needed // or removes the label if routeName is nil. -func addRouteLabel(acc kmeta.Accessor, routeName string) (map[string]interface{}, error) { +func addRouteLabel(acc kmeta.Accessor, routeName string, diffLabels map[string]interface{}) error { if routeName == "" { // remove the label if acc.GetLabels()[serving.RouteLabelKey] != "" { - return map[string]interface{}{serving.RouteLabelKey: nil}, nil + diffLabels[serving.RouteLabelKey] = nil } } else { // add the label if oldLabel := acc.GetLabels()[serving.RouteLabelKey]; oldLabel == "" { - return map[string]interface{}{serving.RouteLabelKey: routeName}, nil + diffLabels[serving.RouteLabelKey] = routeName } else if oldLabel != routeName { // TODO(whaught): this restricts us to only one route -> revision // We can move this to a comma separated list annotation and use the new routingState label. - return nil, fmt.Errorf("resource already has route label %q, and cannot be referenced by %q", oldLabel, routeName) + return fmt.Errorf("resource already has route label %q, and cannot be referenced by %q", oldLabel, routeName) } } - return nil, nil + return nil } // list implements Accessor @@ -124,7 +153,7 @@ func (r *Revision) makeMetadataPatch(ns, name string, routeName string) (map[str if err != nil { return nil, err } - return makeMetadataPatch(rev, routeName) + return makeMetadataPatch(rev, routeName, true /*addRoutingState*/, r.clock) } // Configuration is an implementation of Accessor for Configurations. @@ -132,6 +161,7 @@ type Configuration struct { client clientset.Interface tracker tracker.Interface configurationLister listers.ConfigurationLister + clock clock.Clock } // Configuration implements Accessor @@ -141,11 +171,13 @@ var _ Accessor = (*Configuration)(nil) func NewConfigurationAccessor( client clientset.Interface, tracker tracker.Interface, - lister listers.ConfigurationLister) *Configuration { + lister listers.ConfigurationLister, + clock clock.Clock) *Configuration { return &Configuration{ client: client, tracker: tracker, configurationLister: lister, + clock: clock, } } @@ -176,5 +208,5 @@ func (c *Configuration) makeMetadataPatch(ns, name string, routeName string) (ma if err != nil { return nil, err } - return makeMetadataPatch(config, routeName) + return makeMetadataPatch(config, routeName, false /*addRoutingState*/, c.clock) }