Sanity check GC revision timeout

pkg/gc/config.go

@@ -21,6 +21,7 @@ import (
 	"strconv"
 	"time"
 
+	"github.com/knative/pkg/configmap"
 	corev1 "k8s.io/api/core/v1"
 )
 
@@ -40,44 +41,51 @@ type Config struct {
 	StaleRevisionLastpinnedDebounce time.Duration
 }
 
-func NewConfigFromConfigMap(configMap *corev1.ConfigMap) (*Config, error) {
-	c := Config{}
+func NewConfigFromConfigMapFunc(logger configmap.Logger, minRevisionTimeout time.Duration) func(configMap *corev1.ConfigMap) (*Config, error) {
+	return func(configMap *corev1.ConfigMap) (*Config, error) {
+		c := Config{}
 
-	for _, dur := range []struct {
-		key          string
-		field        *time.Duration
-		defaultValue time.Duration
-	}{{
-		key:          "stale-revision-create-delay",
-		field:        &c.StaleRevisionCreateDelay,
-		defaultValue: 24 * time.Hour,
-	}, {
-		key:          "stale-revision-timeout",
-		field:        &c.StaleRevisionTimeout,
-		defaultValue: 15 * time.Hour,
-	}, {
-		key:          "stale-revision-lastpinned-debounce",
-		field:        &c.StaleRevisionLastpinnedDebounce,
-		defaultValue: 5 * time.Hour,
-	}} {
-		if raw, ok := configMap.Data[dur.key]; !ok {
-			*dur.field = dur.defaultValue
-		} else if val, err := time.ParseDuration(raw); err != nil {
+		for _, dur := range []struct {
+			key          string
+			field        *time.Duration
+			defaultValue time.Duration
+		}{{
+			key:          "stale-revision-create-delay",
+			field:        &c.StaleRevisionCreateDelay,
+			defaultValue: 24 * time.Hour,
+		}, {
+			key:          "stale-revision-timeout",
+			field:        &c.StaleRevisionTimeout,
+			defaultValue: 15 * time.Hour,
+		}, {
+			key:          "stale-revision-lastpinned-debounce",
+			field:        &c.StaleRevisionLastpinnedDebounce,
+			defaultValue: 5 * time.Hour,
+		}} {
+			if raw, ok := configMap.Data[dur.key]; !ok {
+				*dur.field = dur.defaultValue
+			} else if val, err := time.ParseDuration(raw); err != nil {
+				return nil, err
+			} else {
+				*dur.field = val
+			}
+		}
+
+		if raw, ok := configMap.Data["stale-revision-minimum-generations"]; !ok {
+			c.StaleRevisionMinimumGenerations = 1
+		} else if val, err := strconv.ParseInt(raw, 10, 64); err != nil {
 			return nil, err
+		} else if val < 0 {
+			return nil, errors.New("stale-revision-minimum-generations must be zero or greater")
 		} else {
-			*dur.field = val
+			c.StaleRevisionMinimumGenerations = val
 		}
-	}
 
-	if raw, ok := configMap.Data["stale-revision-minimum-generations"]; !ok {
-		c.StaleRevisionMinimumGenerations = 1
-	} else if val, err := strconv.ParseInt(raw, 10, 64); err != nil {
-		return nil, err
-	} else if val < 0 {
-		return nil, errors.New("stale-revision-minimum-generations must be zero or greater")
-	} else {
-		c.StaleRevisionMinimumGenerations = val
+		if c.StaleRevisionTimeout-c.StaleRevisionLastpinnedDebounce < minRevisionTimeout {
+			logger.Errorf("Got revision timeout of %v, minimum supported value is %v", c.StaleRevisionTimeout, minRevisionTimeout+c.StaleRevisionLastpinnedDebounce)
+			c.StaleRevisionTimeout = minRevisionTimeout + c.StaleRevisionLastpinnedDebounce
+			return &c, nil
+		}
+		return &c, nil
 	}
-
-	return &c, nil
 }

pkg/gc/config_test.go

@@ -21,6 +21,7 @@ import (
 	"time"
 
 	"github.com/google/go-cmp/cmp"
+	logtesting "github.com/knative/pkg/logging/testing"
 	corev1 "k8s.io/api/core/v1"
 
 	. "github.com/knative/pkg/configmap/testing"
@@ -31,12 +32,12 @@ func TestOurConfig(t *testing.T) {
 	for _, tt := range []struct {
 		name string
 		fail bool
-		want Config
+		want *Config
 		data *corev1.ConfigMap
 	}{{
 		name: "actual config",
 		fail: false,
-		want: Config{
+		want: &Config{
 			StaleRevisionCreateDelay:        24 * time.Hour,
 			StaleRevisionTimeout:            15 * time.Hour,
 			StaleRevisionMinimumGenerations: 1,
@@ -46,7 +47,7 @@ func TestOurConfig(t *testing.T) {
 	}, {
 		name: "example config",
 		fail: false,
-		want: Config{
+		want: &Config{
 			StaleRevisionCreateDelay:        24 * time.Hour,
 			StaleRevisionTimeout:            15 * time.Hour,
 			StaleRevisionMinimumGenerations: 1,
@@ -55,7 +56,7 @@ func TestOurConfig(t *testing.T) {
 		data: example,
 	}, {
 		name: "With value overrides",
-		want: Config{
+		want: &Config{
 			StaleRevisionCreateDelay:        15 * time.Hour,
 			StaleRevisionTimeout:            15 * time.Hour,
 			StaleRevisionMinimumGenerations: 10,
@@ -70,7 +71,7 @@ func TestOurConfig(t *testing.T) {
 	}, {
 		name: "Invalid duration",
 		fail: true,
-		want: Config{},
+		want: nil,
 		data: &corev1.ConfigMap{
 			Data: map[string]string{
 				"stale-revision-create-delay": "invalid",
@@ -79,7 +80,7 @@ func TestOurConfig(t *testing.T) {
 	}, {
 		name: "Invalid negative minimum generation",
 		fail: true,
-		want: Config{},
+		want: nil,
 		data: &corev1.ConfigMap{
 			Data: map[string]string{
 				"stale-revision-minimum-generations": "-1",
@@ -88,24 +89,36 @@ func TestOurConfig(t *testing.T) {
 	}, {
 		name: "Invalid minimum generation",
 		fail: true,
-		want: Config{},
+		want: nil,
 		data: &corev1.ConfigMap{
 			Data: map[string]string{
 				"stale-revision-minimum-generations": "invalid",
 			},
 		},
+	}, {
+		name: "Below minimum timeout",
+		fail: false,
+		want: &Config{
+			StaleRevisionCreateDelay:        15 * time.Hour,
+			StaleRevisionTimeout:            15 * time.Hour,
+			StaleRevisionMinimumGenerations: 10,
+			StaleRevisionLastpinnedDebounce: 5 * time.Hour,
+		},
+		data: &corev1.ConfigMap{
+			Data: map[string]string{
+				"stale-revision-create-delay":        "15h",
+				"stale-revision-minimum-generations": "10",
+				"stale-revision-timeout":             "1h",
+			},
+		},
 	}} {
 		t.Run(tt.name, func(t *testing.T) {
-			testConfig, err := NewConfigFromConfigMap(tt.data)
+			testConfig, err := NewConfigFromConfigMapFunc(logtesting.TestLogger(t), 10*time.Hour)(tt.data)
 			if tt.fail != (err != nil) {
 				t.Errorf("Unexpected error value: %v", err)
 			}
 
-			if tt.fail {
-				return
-			}
-
-			if diff := cmp.Diff(tt.want, *testConfig); diff != "" {
+			if diff := cmp.Diff(tt.want, testConfig); diff != "" {
 				t.Errorf("Unexpected controller config (-want, +got): %v", diff)
 			}
 		})

pkg/reconciler/configuration/config/store.go

@@ -18,6 +18,7 @@ package config
 
 import (
 	"context"
+	"time"
 
 	"github.com/knative/pkg/configmap"
 	"github.com/knative/serving/pkg/gc"
@@ -53,13 +54,13 @@ func (s *Store) Load() *Config {
 	}
 }
 
-func NewStore(logger configmap.Logger) *Store {
+func NewStore(logger configmap.Logger, minRevisionTimeout time.Duration) *Store {
 	return &Store{
 		UntypedStore: configmap.NewUntypedStore(
 			"configuration",
 			logger,
 			configmap.Constructors{
-				gc.ConfigName: gc.NewConfigFromConfigMap,
+				gc.ConfigName: gc.NewConfigFromConfigMapFunc(logger, minRevisionTimeout),
 			},
 		),
 	}

pkg/reconciler/configuration/config/store_test.go

@@ -19,6 +19,7 @@ package config
 import (
 	"context"
 	"testing"
+	"time"
 
 	"github.com/google/go-cmp/cmp"
 
@@ -30,7 +31,7 @@ import (
 
 func TestStoreLoadWithContext(t *testing.T) {
 	defer logtesting.ClearAll()
-	store := NewStore(logtesting.TestLogger(t))
+	store := NewStore(logtesting.TestLogger(t), 10*time.Hour)
 
 	gcConfig := ConfigMapFromTestFile(t, "config-gc")
 
@@ -39,7 +40,7 @@ func TestStoreLoadWithContext(t *testing.T) {
 	config := FromContext(store.ToContext(context.Background()))
 
 	t.Run("revision-gc", func(t *testing.T) {
-		expected, _ := gc.NewConfigFromConfigMap(gcConfig)
+		expected, _ := gc.NewConfigFromConfigMapFunc(logtesting.TestLogger(t), 10*time.Hour)(gcConfig)
 		if diff := cmp.Diff(expected, config.RevisionGC); diff != "" {
 			t.Errorf("Unexpected controller config (-want, +got): %v", diff)
 		}

pkg/reconciler/configuration/controller.go

@@ -58,7 +58,7 @@ func NewController(
 	})
 
 	c.Logger.Info("Setting up ConfigMap receivers")
-	c.configStore = configns.NewStore(c.Logger.Named("config-store"))
+	c.configStore = configns.NewStore(c.Logger.Named("config-store"), opt.ResyncPeriod)
 	c.configStore.WatchConfigs(opt.ConfigMapWatcher)
 	return impl
 }

pkg/reconciler/route/config/store.go

@@ -18,6 +18,7 @@ package config
 
 import (
 	"context"
+	"time"
 
 	"github.com/knative/pkg/configmap"
 	"github.com/knative/serving/pkg/gc"
@@ -58,14 +59,14 @@ type Store struct {
 // after the ConfigMap has been processed and stored.
 //
 // See also: configmap.NewUntypedStore().
-func NewStore(logger configmap.Logger, onAfterStore ...func(name string, value interface{})) *Store {
+func NewStore(logger configmap.Logger, minRevisionTimeout time.Duration, onAfterStore ...func(name string, value interface{})) *Store {
 	store := &Store{
 		UntypedStore: configmap.NewUntypedStore(
 			"route",
 			logger,
 			configmap.Constructors{
 				DomainConfigName:   NewDomainFromConfigMap,
-				gc.ConfigName:      gc.NewConfigFromConfigMap,
+				gc.ConfigName:      gc.NewConfigFromConfigMapFunc(logger, minRevisionTimeout),
 				network.ConfigName: network.NewConfigFromConfigMap,
 			},
 			onAfterStore...,

pkg/reconciler/route/config/store_test.go

@@ -19,6 +19,7 @@ package config
 import (
 	"context"
 	"testing"
+	"time"
 
 	"github.com/google/go-cmp/cmp"
 	logtesting "github.com/knative/pkg/logging/testing"
@@ -30,7 +31,7 @@ import (
 
 func TestStoreLoadWithContext(t *testing.T) {
 	defer logtesting.ClearAll()
-	store := NewStore(logtesting.TestLogger(t))
+	store := NewStore(logtesting.TestLogger(t), 10*time.Hour)
 
 	domainConfig := ConfigMapFromTestFile(t, DomainConfigName)
 	gcConfig := ConfigMapFromTestFile(t, gc.ConfigName)
@@ -50,16 +51,32 @@ func TestStoreLoadWithContext(t *testing.T) {
 	})
 
 	t.Run("gc", func(t *testing.T) {
-		expected, _ := gc.NewConfigFromConfigMap(gcConfig)
+		expected, err := gc.NewConfigFromConfigMapFunc(logtesting.TestLogger(t), 10*time.Hour)(gcConfig)
+		if err != nil {
+			t.Errorf("Parsing configmap: %v", err)
+		}
 		if diff := cmp.Diff(expected, config.GC); diff != "" {
 			t.Errorf("Unexpected controller config (-want, +got): %v", diff)
 		}
 	})
+
+	t.Run("gc invalid timeout", func(t *testing.T) {
+		gcConfig.Data["stale-revision-timeout"] = "1h"
+		expected, err := gc.NewConfigFromConfigMapFunc(logtesting.TestLogger(t), 10*time.Hour)(gcConfig)
+
+		if err != nil {
+			t.Errorf("Got error parsing gc config with invalid timeout: %v", err)
+		}
+
+		if expected.StaleRevisionTimeout != 15*time.Hour {
+			t.Errorf("Expected revision timeout of %v, got %v", 15*time.Hour, expected.StaleRevisionTimeout)
+		}
+	})
 }
 
 func TestStoreImmutableConfig(t *testing.T) {
 	defer logtesting.ClearAll()
-	store := NewStore(logtesting.TestLogger(t))
+	store := NewStore(logtesting.TestLogger(t), 10*time.Hour)
 	store.OnConfigChanged(ConfigMapFromTestFile(t, DomainConfigName))
 	store.OnConfigChanged(ConfigMapFromTestFile(t, network.ConfigName))
 	store.OnConfigChanged(ConfigMapFromTestFile(t, gc.ConfigName))

pkg/reconciler/route/controller.go

@@ -130,7 +130,7 @@ func NewControllerWithClock(
 	resync := configmap.TypeFilter(configsToResync...)(func(string, interface{}) {
 		impl.GlobalResync(routeInformer.Informer())
 	})
-	c.configStore = config.NewStore(c.Logger.Named("config-store"), resync)
+	c.configStore = config.NewStore(c.Logger.Named("config-store"), opt.ResyncPeriod, resync)
 	c.configStore.WatchConfigs(opt.ConfigMapWatcher)
 	return impl
 }