diff --git a/pkg/gc/config.go b/pkg/gc/config.go index 34f6241f0fed..08df6315d694 100644 --- a/pkg/gc/config.go +++ b/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 } diff --git a/pkg/gc/config_test.go b/pkg/gc/config_test.go index e8da81df03fb..96093b822495 100644 --- a/pkg/gc/config_test.go +++ b/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) } }) diff --git a/pkg/reconciler/configuration/config/store.go b/pkg/reconciler/configuration/config/store.go index ae1efdff96aa..a2242f593af4 100644 --- a/pkg/reconciler/configuration/config/store.go +++ b/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), }, ), } diff --git a/pkg/reconciler/configuration/config/store_test.go b/pkg/reconciler/configuration/config/store_test.go index 2dbc9aa351ab..9401e2318ee9 100644 --- a/pkg/reconciler/configuration/config/store_test.go +++ b/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) } diff --git a/pkg/reconciler/configuration/controller.go b/pkg/reconciler/configuration/controller.go index 22289b1502a5..b0eb3a7601d9 100644 --- a/pkg/reconciler/configuration/controller.go +++ b/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 } diff --git a/pkg/reconciler/route/config/store.go b/pkg/reconciler/route/config/store.go index f122edeb97bb..2caf9c746df7 100644 --- a/pkg/reconciler/route/config/store.go +++ b/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..., diff --git a/pkg/reconciler/route/config/store_test.go b/pkg/reconciler/route/config/store_test.go index dddc5bdaeb51..5cf2b130ad77 100644 --- a/pkg/reconciler/route/config/store_test.go +++ b/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)) diff --git a/pkg/reconciler/route/controller.go b/pkg/reconciler/route/controller.go index 89df0dac5904..8742cfb40219 100644 --- a/pkg/reconciler/route/controller.go +++ b/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 }