From 7f80241ac5919deefa624f507346cee3089c39f0 Mon Sep 17 00:00:00 2001 From: Tara Gu Date: Mon, 28 Oct 2019 08:32:09 -0400 Subject: [PATCH 1/4] Pass config defaults into revision reconciler; add test case to verify --- pkg/reconciler/revision/config/store.go | 4 ++++ pkg/reconciler/revision/config/store_test.go | 11 +++++++++++ 2 files changed, 15 insertions(+) diff --git a/pkg/reconciler/revision/config/store.go b/pkg/reconciler/revision/config/store.go index f555753d709e..2ea89935c353 100644 --- a/pkg/reconciler/revision/config/store.go +++ b/pkg/reconciler/revision/config/store.go @@ -18,6 +18,7 @@ package config import ( "context" + "knative.dev/serving/pkg/apis/config" "knative.dev/pkg/configmap" "knative.dev/pkg/logging" @@ -37,6 +38,7 @@ type Config struct { Observability *metrics.ObservabilityConfig Logging *logging.Config Tracing *pkgtracing.Config + Defaults *config.Defaults } func FromContext(ctx context.Context) *Config { @@ -64,6 +66,7 @@ func NewStore(logger configmap.Logger, onAfterStore ...func(name string, value i pkgmetrics.ConfigMapName(): metrics.NewObservabilityConfigFromConfigMap, logging.ConfigMapName(): logging.NewConfigFromConfigMap, pkgtracing.ConfigName: pkgtracing.NewTracingConfigFromConfigMap, + config.DefaultsConfigName: config.NewDefaultsConfigFromConfigMap, }, onAfterStore..., ), @@ -84,5 +87,6 @@ func (s *Store) Load() *Config { Observability: s.UntypedLoad(pkgmetrics.ConfigMapName()).(*metrics.ObservabilityConfig).DeepCopy(), Logging: s.UntypedLoad((logging.ConfigMapName())).(*logging.Config).DeepCopy(), Tracing: s.UntypedLoad(pkgtracing.ConfigName).(*pkgtracing.Config).DeepCopy(), + Defaults: s.UntypedLoad(config.DefaultsConfigName).(*config.Defaults).DeepCopy(), } } diff --git a/pkg/reconciler/revision/config/store_test.go b/pkg/reconciler/revision/config/store_test.go index 870dad777bcb..69403b201a3b 100644 --- a/pkg/reconciler/revision/config/store_test.go +++ b/pkg/reconciler/revision/config/store_test.go @@ -26,6 +26,7 @@ import ( logtesting "knative.dev/pkg/logging/testing" pkgmetrics "knative.dev/pkg/metrics" pkgtracing "knative.dev/pkg/tracing/config" + apisconfig "knative.dev/serving/pkg/apis/config" deployment "knative.dev/serving/pkg/deployment" "knative.dev/serving/pkg/metrics" "knative.dev/serving/pkg/network" @@ -41,12 +42,14 @@ func TestStoreLoadWithContext(t *testing.T) { observabilityConfig := ConfigMapFromTestFile(t, pkgmetrics.ConfigMapName()) loggingConfig := ConfigMapFromTestFile(t, logging.ConfigMapName()) tracingConfig := ConfigMapFromTestFile(t, pkgtracing.ConfigName) + defaultConfig := ConfigMapFromTestFile(t, apisconfig.DefaultsConfigName) store.OnConfigChanged(deploymentConfig) store.OnConfigChanged(networkConfig) store.OnConfigChanged(observabilityConfig) store.OnConfigChanged(loggingConfig) store.OnConfigChanged(tracingConfig) + store.OnConfigChanged(defaultConfig) config := FromContext(store.ToContext(context.Background())) @@ -86,6 +89,13 @@ func TestStoreLoadWithContext(t *testing.T) { t.Errorf("Unexpected tracing config (-want, +got): %v", diff) } }) + + t.Run("defaults", func(t *testing.T) { + expected, _ := apisconfig.NewDefaultsConfigFromConfigMap(defaultConfig) + if diff := cmp.Diff(expected, config.Defaults); diff != "" { + t.Errorf("Unexpected defaults config (-want, +got): %v", diff) + } + }) } func TestStoreImmutableConfig(t *testing.T) { @@ -96,6 +106,7 @@ func TestStoreImmutableConfig(t *testing.T) { store.OnConfigChanged(ConfigMapFromTestFile(t, pkgmetrics.ConfigMapName())) store.OnConfigChanged(ConfigMapFromTestFile(t, logging.ConfigMapName())) store.OnConfigChanged(ConfigMapFromTestFile(t, pkgtracing.ConfigName)) + store.OnConfigChanged(ConfigMapFromTestFile(t, apisconfig.DefaultsConfigName)) config := store.Load() From dee85c3a4c345c942ff973f364d78cd36da47504 Mon Sep 17 00:00:00 2001 From: Tara Gu Date: Mon, 28 Oct 2019 11:49:03 -0400 Subject: [PATCH 2/4] add testdata --- pkg/reconciler/revision/config/testdata/config-defaults.yaml | 1 + 1 file changed, 1 insertion(+) create mode 120000 pkg/reconciler/revision/config/testdata/config-defaults.yaml diff --git a/pkg/reconciler/revision/config/testdata/config-defaults.yaml b/pkg/reconciler/revision/config/testdata/config-defaults.yaml new file mode 120000 index 000000000000..93ce67f647a6 --- /dev/null +++ b/pkg/reconciler/revision/config/testdata/config-defaults.yaml @@ -0,0 +1 @@ +../../../../../config/config-defaults.yaml \ No newline at end of file From 18933ceb4ce0be33383aedf677af68cf19f3031b Mon Sep 17 00:00:00 2001 From: Tara Gu Date: Mon, 28 Oct 2019 16:37:32 -0400 Subject: [PATCH 3/4] Update existing tests --- pkg/reconciler/revision/queueing_test.go | 20 ++++++++++++++++++++ pkg/reconciler/revision/revision_test.go | 12 ++++++++---- 2 files changed, 28 insertions(+), 4 deletions(-) diff --git a/pkg/reconciler/revision/queueing_test.go b/pkg/reconciler/revision/queueing_test.go index 564b803d9ff0..ded3326cfac1 100644 --- a/pkg/reconciler/revision/queueing_test.go +++ b/pkg/reconciler/revision/queueing_test.go @@ -18,6 +18,7 @@ package revision import ( "context" + "knative.dev/serving/pkg/apis/config" "testing" "time" @@ -133,6 +134,24 @@ func getTestDeploymentConfigMap() *corev1.ConfigMap { } } +func getTestDefaultsConfig() *config.Defaults { + c, _ := config.NewDefaultsConfigFromConfigMap(getTestDefaultsConfigMap()) + // ignoring error as test controller is generated + return c +} + +func getTestDefaultsConfigMap() *corev1.ConfigMap { + return &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: config.DefaultsConfigName, + Namespace: system.Namespace(), + }, + Data: map[string]string{ + "container-concurrency": "1", + }, + } +} + func newTestController(t *testing.T) ( context.Context, context.CancelFunc, @@ -148,6 +167,7 @@ func newTestController(t *testing.T) ( configs := []*corev1.ConfigMap{ getTestDeploymentConfigMap(), + getTestDefaultsConfigMap(), { ObjectMeta: metav1.ObjectMeta{ Namespace: system.Namespace(), diff --git a/pkg/reconciler/revision/revision_test.go b/pkg/reconciler/revision/revision_test.go index a88106780456..2ef6cf0dca4b 100644 --- a/pkg/reconciler/revision/revision_test.go +++ b/pkg/reconciler/revision/revision_test.go @@ -308,7 +308,8 @@ func TestResolutionFailed(t *testing.T) { // TODO(mattmoor): add coverage of a Reconcile fixing a stale logging URL func TestUpdateRevWithWithUpdatedLoggingURL(t *testing.T) { deploymentConfig := getTestDeploymentConfig() - ctx, _, controller, watcher := newTestControllerWithConfig(t, deploymentConfig, &corev1.ConfigMap{ + defaultsConfig := getTestDefaultsConfigMap() + ctx, _, controller, watcher := newTestControllerWithConfig(t, deploymentConfig, defaultsConfig, &corev1.ConfigMap{ ObjectMeta: metav1.ObjectMeta{ Namespace: system.Namespace(), Name: metrics.ConfigMapName(), @@ -482,7 +483,8 @@ func TestIstioOutboundIPRangesInjection(t *testing.T) { func getPodAnnotationsForConfig(t *testing.T, configMapValue string, configAnnotationOverride string) map[string]string { controllerConfig := getTestDeploymentConfig() - ctx, _, controller, watcher := newTestControllerWithConfig(t, controllerConfig) + defaultsConfig := getTestDefaultsConfigMap() + ctx, _, controller, watcher := newTestControllerWithConfig(t, controllerConfig, defaultsConfig) // Resolve image references to this "digest" digest := "foo@sha256:deadbeef" @@ -557,7 +559,8 @@ func TestGlobalResyncOnConfigMapUpdateRevision(t *testing.T) { for _, test := range tests { t.Run(test.name, func(t *testing.T) { controllerConfig := getTestDeploymentConfig() - ctx, informers, ctrl, watcher := newTestControllerWithConfig(t, controllerConfig) + defaultsConfig := getTestDefaultsConfigMap() + ctx, informers, ctrl, watcher := newTestControllerWithConfig(t, controllerConfig, defaultsConfig) ctx, cancel := context.WithCancel(ctx) grp := errgroup.Group{} @@ -713,7 +716,8 @@ func TestGlobalResyncOnConfigMapUpdateDeployment(t *testing.T) { for _, test := range tests { t.Run(test.name, func(t *testing.T) { controllerConfig := getTestDeploymentConfig() - ctx, informers, ctrl, watcher := newTestControllerWithConfig(t, controllerConfig) + defaultsConfig := getTestDefaultsConfigMap() + ctx, informers, ctrl, watcher := newTestControllerWithConfig(t, controllerConfig, defaultsConfig) ctx, cancel := context.WithCancel(ctx) grp := errgroup.Group{} From 5bb421f7d002d10fc39aaca23081225243e9778d Mon Sep 17 00:00:00 2001 From: Tara Gu Date: Tue, 29 Oct 2019 11:00:38 -0400 Subject: [PATCH 4/4] Review: add test case for updating config-defaults --- pkg/reconciler/revision/config/store_test.go | 5 +++ pkg/reconciler/revision/controller.go | 2 + pkg/reconciler/revision/queueing_test.go | 8 +--- pkg/reconciler/revision/revision_test.go | 41 +++++++++++++++----- 4 files changed, 40 insertions(+), 16 deletions(-) diff --git a/pkg/reconciler/revision/config/store_test.go b/pkg/reconciler/revision/config/store_test.go index 69403b201a3b..65410f8b8c44 100644 --- a/pkg/reconciler/revision/config/store_test.go +++ b/pkg/reconciler/revision/config/store_test.go @@ -113,6 +113,8 @@ func TestStoreImmutableConfig(t *testing.T) { config.Deployment.QueueSidecarImage = "mutated" config.Network.IstioOutboundIPRanges = "mutated" config.Logging.LoggingConfig = "mutated" + ccMutated := int64(4) + config.Defaults.ContainerConcurrency = ccMutated newConfig := store.Load() @@ -125,4 +127,7 @@ func TestStoreImmutableConfig(t *testing.T) { if newConfig.Logging.LoggingConfig == "mutated" { t.Error("Logging config is not immutable") } + if newConfig.Defaults.ContainerConcurrency == ccMutated { + t.Error("Defaults config is not immutable") + } } diff --git a/pkg/reconciler/revision/controller.go b/pkg/reconciler/revision/controller.go index f78b4ec277cf..39f1d3d7233d 100644 --- a/pkg/reconciler/revision/controller.go +++ b/pkg/reconciler/revision/controller.go @@ -32,6 +32,7 @@ import ( "knative.dev/pkg/configmap" "knative.dev/pkg/controller" "knative.dev/pkg/logging" + apisconfig "knative.dev/serving/pkg/apis/config" "knative.dev/serving/pkg/apis/serving/v1alpha1" "knative.dev/serving/pkg/deployment" "knative.dev/serving/pkg/metrics" @@ -106,6 +107,7 @@ func NewController( &network.Config{}, &metrics.ObservabilityConfig{}, &deployment.Config{}, + &apisconfig.Defaults{}, } resync := configmap.TypeFilter(configsToResync...)(func(string, interface{}) { diff --git a/pkg/reconciler/revision/queueing_test.go b/pkg/reconciler/revision/queueing_test.go index ded3326cfac1..3fd5ef0d4863 100644 --- a/pkg/reconciler/revision/queueing_test.go +++ b/pkg/reconciler/revision/queueing_test.go @@ -134,12 +134,6 @@ func getTestDeploymentConfigMap() *corev1.ConfigMap { } } -func getTestDefaultsConfig() *config.Defaults { - c, _ := config.NewDefaultsConfigFromConfigMap(getTestDefaultsConfigMap()) - // ignoring error as test controller is generated - return c -} - func getTestDefaultsConfigMap() *corev1.ConfigMap { return &corev1.ConfigMap{ ObjectMeta: metav1.ObjectMeta{ @@ -147,7 +141,7 @@ func getTestDefaultsConfigMap() *corev1.ConfigMap { Namespace: system.Namespace(), }, Data: map[string]string{ - "container-concurrency": "1", + "container-name-template": "user-container", }, } } diff --git a/pkg/reconciler/revision/revision_test.go b/pkg/reconciler/revision/revision_test.go index 2ef6cf0dca4b..61c027e89342 100644 --- a/pkg/reconciler/revision/revision_test.go +++ b/pkg/reconciler/revision/revision_test.go @@ -20,6 +20,7 @@ import ( "context" "errors" "fmt" + "knative.dev/serving/pkg/apis/config" "strconv" "strings" "testing" @@ -164,7 +165,7 @@ func newTestControllerWithConfig(t *testing.T, deploymentConfig *deployment.Conf "panic-window": "10s", "tick-interval": "2s", }, - }, getTestDeploymentConfigMap()} + }, getTestDeploymentConfigMap(), getTestDefaultsConfigMap()} cms = append(cms, configs...) @@ -308,8 +309,7 @@ func TestResolutionFailed(t *testing.T) { // TODO(mattmoor): add coverage of a Reconcile fixing a stale logging URL func TestUpdateRevWithWithUpdatedLoggingURL(t *testing.T) { deploymentConfig := getTestDeploymentConfig() - defaultsConfig := getTestDefaultsConfigMap() - ctx, _, controller, watcher := newTestControllerWithConfig(t, deploymentConfig, defaultsConfig, &corev1.ConfigMap{ + ctx, _, controller, watcher := newTestControllerWithConfig(t, deploymentConfig, &corev1.ConfigMap{ ObjectMeta: metav1.ObjectMeta{ Namespace: system.Namespace(), Name: metrics.ConfigMapName(), @@ -483,8 +483,7 @@ func TestIstioOutboundIPRangesInjection(t *testing.T) { func getPodAnnotationsForConfig(t *testing.T, configMapValue string, configAnnotationOverride string) map[string]string { controllerConfig := getTestDeploymentConfig() - defaultsConfig := getTestDefaultsConfigMap() - ctx, _, controller, watcher := newTestControllerWithConfig(t, controllerConfig, defaultsConfig) + ctx, _, controller, watcher := newTestControllerWithConfig(t, controllerConfig) // Resolve image references to this "digest" digest := "foo@sha256:deadbeef" @@ -554,13 +553,38 @@ func TestGlobalResyncOnConfigMapUpdateRevision(t *testing.T) { return HookIncomplete } }, + }, { + name: "Update ContainerConcurrency", // Should update ContainerConcurrency on revision spec + configMapToUpdate: &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: system.Namespace(), + Name: config.DefaultsConfigName, + }, + Data: map[string]string{ + "container-concurrency": "3", + }, + }, + callback: func(t *testing.T) func(runtime.Object) HookResult { + return func(obj runtime.Object) HookResult { + revision := obj.(*v1alpha1.Revision) + t.Logf("Revision updated: %v", revision.Name) + + expected := int64(3) + got := *(revision.Spec.ContainerConcurrency) + if got != expected { + return HookComplete + } + + t.Logf("No update occurred; expected: %d got: %d", expected, got) + return HookIncomplete + } + }, }} for _, test := range tests { t.Run(test.name, func(t *testing.T) { controllerConfig := getTestDeploymentConfig() - defaultsConfig := getTestDefaultsConfigMap() - ctx, informers, ctrl, watcher := newTestControllerWithConfig(t, controllerConfig, defaultsConfig) + ctx, informers, ctrl, watcher := newTestControllerWithConfig(t, controllerConfig) ctx, cancel := context.WithCancel(ctx) grp := errgroup.Group{} @@ -716,8 +740,7 @@ func TestGlobalResyncOnConfigMapUpdateDeployment(t *testing.T) { for _, test := range tests { t.Run(test.name, func(t *testing.T) { controllerConfig := getTestDeploymentConfig() - defaultsConfig := getTestDefaultsConfigMap() - ctx, informers, ctrl, watcher := newTestControllerWithConfig(t, controllerConfig, defaultsConfig) + ctx, informers, ctrl, watcher := newTestControllerWithConfig(t, controllerConfig) ctx, cancel := context.WithCancel(ctx) grp := errgroup.Group{}