From ddaeebb057a26fb2b92d882b24797d3a62020c6d Mon Sep 17 00:00:00 2001 From: Victor Agababov Date: Sun, 19 Apr 2020 14:16:36 -0700 Subject: [PATCH 1/3] Add a new config-default parameter this will guide how long are we allowing the deployment to take before we consider it failed. See #6201 for details. Also make lots of various clean ups in the config-default tests and the code itself. Next we'll theread it through rather than using constants all around. --- config/core/configmaps/deployment.yaml | 4 + pkg/deployment/config.go | 41 +++++- pkg/deployment/config_test.go | 131 +++++++++++------- pkg/reconciler/autoscaling/kpa/scaler.go | 4 +- .../revision/resources/constants.go | 4 - pkg/reconciler/revision/resources/deploy.go | 2 +- .../revision/resources/deploy_test.go | 3 +- 7 files changed, 124 insertions(+), 65 deletions(-) diff --git a/config/core/configmaps/deployment.yaml b/config/core/configmaps/deployment.yaml index ba2d3bef10aa..38542d8a311b 100644 --- a/config/core/configmaps/deployment.yaml +++ b/config/core/configmaps/deployment.yaml @@ -43,3 +43,7 @@ data: # List of repositories for which tag to digest resolving should be skipped registriesSkippingTagResolving: "ko.local,dev.local" + + # ProgressDeadlineSeconds is the time in seconds we wait for the deployment to + # be ready before considering it failed. + progressDeadline: "120" diff --git a/pkg/deployment/config.go b/pkg/deployment/config.go index dcd3b7086047..6a9dc53685a1 100644 --- a/pkg/deployment/config.go +++ b/pkg/deployment/config.go @@ -18,7 +18,10 @@ package deployment import ( "errors" + "fmt" + "strconv" "strings" + "time" corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/util/sets" @@ -29,23 +32,43 @@ const ( ConfigName = "config-deployment" // QueueSidecarImageKey is the config map key for queue sidecar image - QueueSidecarImageKey = "queueSidecarImage" - registriesSkippingTagResolving = "registriesSkippingTagResolving" + QueueSidecarImageKey = "queueSidecarImage" + + // ProgressDeadlineSecondsDefault is the default value for the config's + // ProgressDeadlineSeconds. + ProgressDeadlineSecondsDefault = 120 * time.Second + + registriesSkippingTagResolvingKey = "registriesSkippingTagResolving" + progressDeadlineKey = "progressDeadline" ) +func defaultConfig() *Config { + return &Config{ + ProgressDeadlineSeconds: ProgressDeadlineSecondsDefault, + RegistriesSkippingTagResolving: sets.NewString("ko.local", "dev.local"), + } +} + // NewConfigFromMap creates a DeploymentConfig from the supplied Map func NewConfigFromMap(configMap map[string]string) (*Config, error) { - nc := &Config{} + nc := defaultConfig() qsideCarImage, ok := configMap[QueueSidecarImageKey] if !ok { return nil, errors.New("queue sidecar image is missing") } nc.QueueSidecarImage = qsideCarImage - if registries, ok := configMap[registriesSkippingTagResolving]; !ok { - // It is ok if registries are missing. - nc.RegistriesSkippingTagResolving = sets.NewString("ko.local", "dev.local") - } else { + if pds, ok := configMap[progressDeadlineKey]; ok { + v, err := strconv.ParseInt(pds, 10 /*base*/, 32 /*bits*/) + if err != nil { + return nil, fmt.Errorf("error parsing %s=%s as integer, %w", progressDeadlineKey, pds, err) + } else if v <= 0 { + return nil, fmt.Errorf("%s cannot be non-positive, was %d", progressDeadlineKey, v) + } + nc.ProgressDeadlineSeconds = time.Duration(v) * time.Second + } + + if registries, ok := configMap[registriesSkippingTagResolvingKey]; ok { nc.RegistriesSkippingTagResolving = sets.NewString(strings.Split(registries, ",")...) } return nc, nil @@ -64,4 +87,8 @@ type Config struct { // Repositories for which tag to digest resolving should be skipped RegistriesSkippingTagResolving sets.String + + // ProgressDeadlineSeconds is the time in seconds we wait for the deployment to + // be ready before considering it failed. + ProgressDeadlineSeconds time.Duration } diff --git a/pkg/deployment/config_test.go b/pkg/deployment/config_test.go index 3954b998b920..e092f2a2ce6e 100644 --- a/pkg/deployment/config_test.go +++ b/pkg/deployment/config_test.go @@ -18,92 +18,123 @@ package deployment import ( "testing" + "time" "github.com/google/go-cmp/cmp" + corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/util/sets" - "knative.dev/pkg/system" . "knative.dev/pkg/configmap/testing" + "knative.dev/pkg/system" _ "knative.dev/pkg/system/testing" ) -var noSidecarImage = "" +const noSidecarImage = "" func TestControllerConfigurationFromFile(t *testing.T) { cm, example := ConfigMapsFromTestFile(t, ConfigName, QueueSidecarImageKey) if _, err := NewConfigFromConfigMap(cm); err != nil { - t.Errorf("NewConfigFromConfigMap(actual) = %v", err) + t.Error("NewConfigFromConfigMap(actual) =", err) } - if _, err := NewConfigFromConfigMap(example); err != nil { - t.Errorf("NewConfigFromConfigMap(example) = %v", err) + if got, err := NewConfigFromConfigMap(example); err != nil { + t.Error("NewConfigFromConfigMap(example) =", err) + } else { + want := defaultConfig() + // We require QSI to be explicitly set. So do it here. + want.QueueSidecarImage = "ko://knative.dev/serving/cmd/queue" + if !cmp.Equal(got, want) { + t.Error("Example stanza does not match default, diff(-want,+got):", cmp.Diff(want, got)) + } } } func TestControllerConfiguration(t *testing.T) { configTests := []struct { - name string - wantErr bool - wantController interface{} - config *corev1.ConfigMap + name string + wantErr bool + wantConfig *Config + data map[string]string }{{ - name: "controller configuration with bad registries", - wantErr: false, - wantController: &Config{ + name: "controller configuration with bad registries", + wantConfig: &Config{ RegistriesSkippingTagResolving: sets.NewString("ko.local", ""), QueueSidecarImage: noSidecarImage, + ProgressDeadlineSeconds: ProgressDeadlineSecondsDefault, + }, + data: map[string]string{ + QueueSidecarImageKey: noSidecarImage, + registriesSkippingTagResolvingKey: "ko.local,,", + }, + }, { + name: "controller configuration good progress deadline", + wantConfig: &Config{ + RegistriesSkippingTagResolving: sets.NewString("ko.local", "dev.local"), + QueueSidecarImage: noSidecarImage, + ProgressDeadlineSeconds: 444 * time.Second, + }, + data: map[string]string{ + QueueSidecarImageKey: noSidecarImage, + progressDeadlineKey: "444", }, - config: &corev1.ConfigMap{ - ObjectMeta: metav1.ObjectMeta{ - Namespace: system.Namespace(), - Name: ConfigName, - }, - Data: map[string]string{ - QueueSidecarImageKey: noSidecarImage, - registriesSkippingTagResolving: "ko.local,,", - }, - }}, { - name: "controller configuration with registries", - wantErr: false, - wantController: &Config{ + }, { + name: "controller configuration with registries", + wantConfig: &Config{ RegistriesSkippingTagResolving: sets.NewString("ko.local", "ko.dev"), QueueSidecarImage: noSidecarImage, + ProgressDeadlineSeconds: ProgressDeadlineSecondsDefault, + }, + data: map[string]string{ + QueueSidecarImageKey: noSidecarImage, + registriesSkippingTagResolvingKey: "ko.local,ko.dev", + }, + }, { + name: "controller with no side car image", + wantErr: true, + data: map[string]string{}, + }, { + name: "controller configuration invalid progress deadline", + wantErr: true, + data: map[string]string{ + QueueSidecarImageKey: noSidecarImage, + progressDeadlineKey: "not-a-number", }, - config: &corev1.ConfigMap{ - ObjectMeta: metav1.ObjectMeta{ - Namespace: system.Namespace(), - Name: ConfigName, - }, - Data: map[string]string{ - QueueSidecarImageKey: noSidecarImage, - registriesSkippingTagResolving: "ko.local,ko.dev", - }, + }, { + name: "controller configuration invalid progress deadline II", + wantErr: true, + data: map[string]string{ + QueueSidecarImageKey: noSidecarImage, + progressDeadlineKey: "-21", }, }, { - name: "controller with no side car image", - wantErr: true, - wantController: (*Config)(nil), - config: &corev1.ConfigMap{ - ObjectMeta: metav1.ObjectMeta{ - Namespace: system.Namespace(), - Name: ConfigName, - }, - Data: map[string]string{}, + name: "controller configuration invalid progress deadline III", + wantErr: true, + data: map[string]string{ + QueueSidecarImageKey: noSidecarImage, + progressDeadlineKey: "0", }, }} for _, tt := range configTests { - actualController, err := NewConfigFromConfigMap(tt.config) + t.Run(tt.name, func(t *testing.T) { + gotConfig, err := NewConfigFromConfigMap(&corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: system.Namespace(), + Name: ConfigName, + }, + Data: tt.data, + }) - if (err != nil) != tt.wantErr { - t.Fatalf("Test: %q; NewConfigFromConfigMap() error = %v, WantErr %v", tt.name, err, tt.wantErr) - } + if (err != nil) != tt.wantErr { + t.Fatalf("NewConfigFromConfigMap() error = %v, want: %v", err, tt.wantErr) + } - if diff := cmp.Diff(actualController, tt.wantController); diff != "" { - t.Fatalf("Test: %q; want %v, but got %v", tt.name, tt.wantController, actualController) - } + if got, want := gotConfig, tt.wantConfig; !cmp.Equal(got, want) { + t.Error("Config mismatch, diff(-want,+got):", cmp.Diff(want, got)) + } + }) } } diff --git a/pkg/reconciler/autoscaling/kpa/scaler.go b/pkg/reconciler/autoscaling/kpa/scaler.go index 97f8d89bdcfe..46929607dbeb 100644 --- a/pkg/reconciler/autoscaling/kpa/scaler.go +++ b/pkg/reconciler/autoscaling/kpa/scaler.go @@ -32,10 +32,10 @@ import ( pav1alpha1 "knative.dev/serving/pkg/apis/autoscaling/v1alpha1" "knative.dev/serving/pkg/apis/networking" nv1a1 "knative.dev/serving/pkg/apis/networking/v1alpha1" + "knative.dev/serving/pkg/deployment" "knative.dev/serving/pkg/network" "knative.dev/serving/pkg/reconciler/autoscaling/config" aresources "knative.dev/serving/pkg/reconciler/autoscaling/resources" - rresources "knative.dev/serving/pkg/reconciler/revision/resources" "knative.dev/serving/pkg/resources" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -61,7 +61,7 @@ const ( // race the Revision reconciler and scale down the pods before it can actually surface the pod errors. // We should instead do pod failure diagnostics here immediately before scaling down the Deployment. activationTimeoutBuffer = 10 * time.Second - activationTimeout = time.Duration(rresources.ProgressDeadlineSeconds)*time.Second + activationTimeoutBuffer + activationTimeout = deployment.ProgressDeadlineSecondsDefault + activationTimeoutBuffer ) var probeOptions = []interface{}{ diff --git a/pkg/reconciler/revision/resources/constants.go b/pkg/reconciler/revision/resources/constants.go index 3204f26911fb..396543db5e17 100644 --- a/pkg/reconciler/revision/resources/constants.go +++ b/pkg/reconciler/revision/resources/constants.go @@ -30,10 +30,6 @@ const ( // AppLabelKey is the label defining the application's name. AppLabelKey = "app" - - // ProgressDeadlineSeconds is the time in seconds we wait for the deployment to - // be ready before considering it failed. - ProgressDeadlineSeconds = int32(120) ) var ( diff --git a/pkg/reconciler/revision/resources/deploy.go b/pkg/reconciler/revision/resources/deploy.go index 7f8a9348261e..87ad8c188d85 100644 --- a/pkg/reconciler/revision/resources/deploy.go +++ b/pkg/reconciler/revision/resources/deploy.go @@ -260,7 +260,7 @@ func MakeDeployment(rev *v1.Revision, Spec: appsv1.DeploymentSpec{ Replicas: ptr.Int32(1), Selector: makeSelector(rev), - ProgressDeadlineSeconds: ptr.Int32(ProgressDeadlineSeconds), + ProgressDeadlineSeconds: ptr.Int32(int32(deployment.ProgressDeadlineSecondsDefault.Seconds())), Template: corev1.PodTemplateSpec{ ObjectMeta: metav1.ObjectMeta{ Labels: makeLabels(rev), diff --git a/pkg/reconciler/revision/resources/deploy_test.go b/pkg/reconciler/revision/resources/deploy_test.go index 52700d5e7f35..856867cf4cdf 100644 --- a/pkg/reconciler/revision/resources/deploy_test.go +++ b/pkg/reconciler/revision/resources/deploy_test.go @@ -37,6 +37,7 @@ import ( "knative.dev/serving/pkg/apis/serving" v1 "knative.dev/serving/pkg/apis/serving/v1" autoscalerconfig "knative.dev/serving/pkg/autoscaler/config" + "knative.dev/serving/pkg/deployment" "knative.dev/serving/pkg/network" ) @@ -203,7 +204,7 @@ var ( serving.RevisionUID: "1234", }, }, - ProgressDeadlineSeconds: ptr.Int32(ProgressDeadlineSeconds), + ProgressDeadlineSeconds: ptr.Int32(int32(deployment.ProgressDeadlineSecondsDefault.Seconds())), Template: corev1.PodTemplateSpec{ ObjectMeta: metav1.ObjectMeta{ Labels: map[string]string{ From 9a1a077df00aec78f5af3d592f60ee63e12b828f Mon Sep 17 00:00:00 2001 From: Victor Agababov Date: Mon, 20 Apr 2020 09:24:48 -0700 Subject: [PATCH 2/3] fix issues --- config/core/configmaps/deployment.yaml | 4 ++-- pkg/deployment/config.go | 21 +++++++++---------- pkg/deployment/config_test.go | 8 +++---- pkg/reconciler/autoscaling/kpa/scaler.go | 2 +- pkg/reconciler/revision/resources/deploy.go | 2 +- .../revision/resources/deploy_test.go | 2 +- 6 files changed, 19 insertions(+), 20 deletions(-) diff --git a/config/core/configmaps/deployment.yaml b/config/core/configmaps/deployment.yaml index 38542d8a311b..d255bbbd7795 100644 --- a/config/core/configmaps/deployment.yaml +++ b/config/core/configmaps/deployment.yaml @@ -44,6 +44,6 @@ data: # List of repositories for which tag to digest resolving should be skipped registriesSkippingTagResolving: "ko.local,dev.local" - # ProgressDeadlineSeconds is the time in seconds we wait for the deployment to + # ProgressDeadlineSeconds is the duration we wait for the deployment to # be ready before considering it failed. - progressDeadline: "120" + progressDeadline: "120s" diff --git a/pkg/deployment/config.go b/pkg/deployment/config.go index 6a9dc53685a1..eb6d9596db98 100644 --- a/pkg/deployment/config.go +++ b/pkg/deployment/config.go @@ -19,7 +19,6 @@ package deployment import ( "errors" "fmt" - "strconv" "strings" "time" @@ -34,9 +33,9 @@ const ( // QueueSidecarImageKey is the config map key for queue sidecar image QueueSidecarImageKey = "queueSidecarImage" - // ProgressDeadlineSecondsDefault is the default value for the config's + // ProgressDeadlineDefault is the default value for the config's // ProgressDeadlineSeconds. - ProgressDeadlineSecondsDefault = 120 * time.Second + ProgressDeadlineDefault = 120 * time.Second registriesSkippingTagResolvingKey = "registriesSkippingTagResolving" progressDeadlineKey = "progressDeadline" @@ -44,7 +43,7 @@ const ( func defaultConfig() *Config { return &Config{ - ProgressDeadlineSeconds: ProgressDeadlineSecondsDefault, + ProgressDeadline: ProgressDeadlineDefault, RegistriesSkippingTagResolving: sets.NewString("ko.local", "dev.local"), } } @@ -58,14 +57,14 @@ func NewConfigFromMap(configMap map[string]string) (*Config, error) { } nc.QueueSidecarImage = qsideCarImage - if pds, ok := configMap[progressDeadlineKey]; ok { - v, err := strconv.ParseInt(pds, 10 /*base*/, 32 /*bits*/) + if pd, ok := configMap[progressDeadlineKey]; ok { + v, err := time.ParseDuration(pd) if err != nil { - return nil, fmt.Errorf("error parsing %s=%s as integer, %w", progressDeadlineKey, pds, err) + return nil, fmt.Errorf("error parsing %s=%s as duration, %w", progressDeadlineKey, pd, err) } else if v <= 0 { - return nil, fmt.Errorf("%s cannot be non-positive, was %d", progressDeadlineKey, v) + return nil, fmt.Errorf("%s cannot be non-positive duration, was %v", progressDeadlineKey, v) } - nc.ProgressDeadlineSeconds = time.Duration(v) * time.Second + nc.ProgressDeadline = v } if registries, ok := configMap[registriesSkippingTagResolvingKey]; ok { @@ -88,7 +87,7 @@ type Config struct { // Repositories for which tag to digest resolving should be skipped RegistriesSkippingTagResolving sets.String - // ProgressDeadlineSeconds is the time in seconds we wait for the deployment to + // ProgressDeadline is the time in seconds we wait for the deployment to // be ready before considering it failed. - ProgressDeadlineSeconds time.Duration + ProgressDeadline time.Duration } diff --git a/pkg/deployment/config_test.go b/pkg/deployment/config_test.go index e092f2a2ce6e..186c7b7f9cad 100644 --- a/pkg/deployment/config_test.go +++ b/pkg/deployment/config_test.go @@ -63,7 +63,7 @@ func TestControllerConfiguration(t *testing.T) { wantConfig: &Config{ RegistriesSkippingTagResolving: sets.NewString("ko.local", ""), QueueSidecarImage: noSidecarImage, - ProgressDeadlineSeconds: ProgressDeadlineSecondsDefault, + ProgressDeadline: ProgressDeadlineDefault, }, data: map[string]string{ QueueSidecarImageKey: noSidecarImage, @@ -74,18 +74,18 @@ func TestControllerConfiguration(t *testing.T) { wantConfig: &Config{ RegistriesSkippingTagResolving: sets.NewString("ko.local", "dev.local"), QueueSidecarImage: noSidecarImage, - ProgressDeadlineSeconds: 444 * time.Second, + ProgressDeadline: 444 * time.Second, }, data: map[string]string{ QueueSidecarImageKey: noSidecarImage, - progressDeadlineKey: "444", + progressDeadlineKey: "444s", }, }, { name: "controller configuration with registries", wantConfig: &Config{ RegistriesSkippingTagResolving: sets.NewString("ko.local", "ko.dev"), QueueSidecarImage: noSidecarImage, - ProgressDeadlineSeconds: ProgressDeadlineSecondsDefault, + ProgressDeadline: ProgressDeadlineDefault, }, data: map[string]string{ QueueSidecarImageKey: noSidecarImage, diff --git a/pkg/reconciler/autoscaling/kpa/scaler.go b/pkg/reconciler/autoscaling/kpa/scaler.go index 46929607dbeb..26aa245de3c3 100644 --- a/pkg/reconciler/autoscaling/kpa/scaler.go +++ b/pkg/reconciler/autoscaling/kpa/scaler.go @@ -61,7 +61,7 @@ const ( // race the Revision reconciler and scale down the pods before it can actually surface the pod errors. // We should instead do pod failure diagnostics here immediately before scaling down the Deployment. activationTimeoutBuffer = 10 * time.Second - activationTimeout = deployment.ProgressDeadlineSecondsDefault + activationTimeoutBuffer + activationTimeout = deployment.ProgressDeadlineDefault + activationTimeoutBuffer ) var probeOptions = []interface{}{ diff --git a/pkg/reconciler/revision/resources/deploy.go b/pkg/reconciler/revision/resources/deploy.go index 87ad8c188d85..7037af16c0c6 100644 --- a/pkg/reconciler/revision/resources/deploy.go +++ b/pkg/reconciler/revision/resources/deploy.go @@ -260,7 +260,7 @@ func MakeDeployment(rev *v1.Revision, Spec: appsv1.DeploymentSpec{ Replicas: ptr.Int32(1), Selector: makeSelector(rev), - ProgressDeadlineSeconds: ptr.Int32(int32(deployment.ProgressDeadlineSecondsDefault.Seconds())), + ProgressDeadlineSeconds: ptr.Int32(int32(deployment.ProgressDeadlineDefault.Seconds())), Template: corev1.PodTemplateSpec{ ObjectMeta: metav1.ObjectMeta{ Labels: makeLabels(rev), diff --git a/pkg/reconciler/revision/resources/deploy_test.go b/pkg/reconciler/revision/resources/deploy_test.go index 856867cf4cdf..10b33f71e331 100644 --- a/pkg/reconciler/revision/resources/deploy_test.go +++ b/pkg/reconciler/revision/resources/deploy_test.go @@ -204,7 +204,7 @@ var ( serving.RevisionUID: "1234", }, }, - ProgressDeadlineSeconds: ptr.Int32(int32(deployment.ProgressDeadlineSecondsDefault.Seconds())), + ProgressDeadlineSeconds: ptr.Int32(int32(deployment.ProgressDeadlineDefault.Seconds())), Template: corev1.PodTemplateSpec{ ObjectMeta: metav1.ObjectMeta{ Labels: map[string]string{ From b0143e726dfdd7e00153d4e06e0408ff59c1c8b4 Mon Sep 17 00:00:00 2001 From: Victor Agababov Date: Mon, 20 Apr 2020 09:26:54 -0700 Subject: [PATCH 3/3] fix comment in the yaml --- config/core/configmaps/deployment.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/config/core/configmaps/deployment.yaml b/config/core/configmaps/deployment.yaml index d255bbbd7795..acfa820d8a70 100644 --- a/config/core/configmaps/deployment.yaml +++ b/config/core/configmaps/deployment.yaml @@ -44,6 +44,6 @@ data: # List of repositories for which tag to digest resolving should be skipped registriesSkippingTagResolving: "ko.local,dev.local" - # ProgressDeadlineSeconds is the duration we wait for the deployment to + # ProgressDeadline is the duration we wait for the deployment to # be ready before considering it failed. progressDeadline: "120s"