From 3eca5e49ace20d81bf7ebfbff1ae72d734e0f310 Mon Sep 17 00:00:00 2001 From: Vincent Behar Date: Fri, 4 Oct 2019 11:48:53 +0200 Subject: [PATCH 1/6] Allow per-deployment metrics check interval Add support for a new annotation `osiris.deislabs.io/metricsCheckInterval` on deployments, to override the default metricsCheckInterval on a per-deployment basis. I kept the same name and unit (seconds) to be consistent. --- example/hello-osiris.yaml | 1 + pkg/deployments/zeroscaler/zeroscaler.go | 10 ++++-- pkg/kubernetes/osiris.go | 19 +++++++++++ pkg/kubernetes/osiris_test.go | 41 ++++++++++++++++++++++++ 4 files changed, 69 insertions(+), 2 deletions(-) diff --git a/example/hello-osiris.yaml b/example/hello-osiris.yaml index 53c945d0..b0727a2e 100644 --- a/example/hello-osiris.yaml +++ b/example/hello-osiris.yaml @@ -45,6 +45,7 @@ metadata: annotations: osiris.deislabs.io/enabled: "true" osiris.deislabs.io/minReplicas: "1" + osiris.deislabs.io/metricsCheckInterval: "120" # seconds spec: replicas: 1 selector: diff --git a/pkg/deployments/zeroscaler/zeroscaler.go b/pkg/deployments/zeroscaler/zeroscaler.go index d48b9cfc..76dfefe7 100644 --- a/pkg/deployments/zeroscaler/zeroscaler.go +++ b/pkg/deployments/zeroscaler/zeroscaler.go @@ -131,17 +131,23 @@ func (z *zeroscaler) ensureMetricsCollection(deployment *appsv1.Deployment) { if ok { collector.stop() } + metricsCheckInterval := k8s.GetMetricsCheckInterval( + deployment.Annotations, + z.cfg.MetricsCheckInterval, + ) glog.Infof( - "Using new metrics collector for deployment %s in namespace %s", + "Using new metrics collector for deployment %s in namespace %s "+ + "with metrics check interval of %d seconds", deployment.Name, deployment.Namespace, + metricsCheckInterval, ) collector := newMetricsCollector( z.kubeClient, deployment.Name, deployment.Namespace, selector, - time.Duration(z.cfg.MetricsCheckInterval)*time.Second, + time.Duration(metricsCheckInterval)*time.Second, ) go func() { collector.run(z.ctx) diff --git a/pkg/kubernetes/osiris.go b/pkg/kubernetes/osiris.go index b9c713fb..55a0370c 100644 --- a/pkg/kubernetes/osiris.go +++ b/pkg/kubernetes/osiris.go @@ -34,3 +34,22 @@ func GetMinReplicas(annotations map[string]string, defaultVal int32) int32 { } return int32(minReplicas) } + +// GetMetricsCheckInterval gets the interval in which the zeroScaler would +// repeatedly track the pod http request metrics. The value is the number +// of seconds of the interval. If it fails to do so, it returns the default +// value instead. +func GetMetricsCheckInterval( + annotations map[string]string, + defaultVal int, +) int { + val, ok := annotations["osiris.deislabs.io/metricsCheckInterval"] + if !ok { + return defaultVal + } + metricsCheckInterval, err := strconv.Atoi(val) + if err != nil { + return defaultVal + } + return metricsCheckInterval +} diff --git a/pkg/kubernetes/osiris_test.go b/pkg/kubernetes/osiris_test.go index b7e223bc..c8d6aa84 100644 --- a/pkg/kubernetes/osiris_test.go +++ b/pkg/kubernetes/osiris_test.go @@ -113,3 +113,44 @@ func TestGetMinReplicas(t *testing.T) { }) } } + +func TestGetMetricsCheckInterval(t *testing.T) { + testcases := []struct { + name string + annotations map[string]string + expectedResult int + }{ + { + name: "map with metrics check interval entry", + annotations: map[string]string{ + "osiris.deislabs.io/metricsCheckInterval": "60", + }, + expectedResult: 60, + }, + { + name: "map with no metrics check interval entry", + annotations: map[string]string{ + "osiris.deislabs.io/notmetricsCheckInterval": "60", + }, + expectedResult: 150, + }, + { + name: "map with invalid metrics check interval entry", + annotations: map[string]string{ + "osiris.deislabs.io/metricsCheckInterval": "invalid", + }, + expectedResult: 150, + }, + } + + for _, test := range testcases { + t.Run(test.name, func(t *testing.T) { + actual := GetMetricsCheckInterval(test.annotations, 150) + if actual != test.expectedResult { + t.Errorf( + "expected GetMetricsCheckInterval to return %d, but got %d", + test.expectedResult, actual) + } + }) + } +} From d3dce7b7d08a1473088822cf266ee4c548f30cdd Mon Sep 17 00:00:00 2001 From: Vincent Behar Date: Tue, 8 Oct 2019 11:40:02 +0200 Subject: [PATCH 2/6] expose error from parsing annotation value --- pkg/deployments/zeroscaler/zeroscaler.go | 16 +++++- pkg/kubernetes/osiris.go | 30 +++++++---- pkg/kubernetes/osiris_test.go | 63 ++++++++++++++++++++---- 3 files changed, 88 insertions(+), 21 deletions(-) diff --git a/pkg/deployments/zeroscaler/zeroscaler.go b/pkg/deployments/zeroscaler/zeroscaler.go index 76dfefe7..e5b3b471 100644 --- a/pkg/deployments/zeroscaler/zeroscaler.go +++ b/pkg/deployments/zeroscaler/zeroscaler.go @@ -131,10 +131,22 @@ func (z *zeroscaler) ensureMetricsCollection(deployment *appsv1.Deployment) { if ok { collector.stop() } - metricsCheckInterval := k8s.GetMetricsCheckInterval( + metricsCheckInterval, err := k8s.GetMetricsCheckInterval( deployment.Annotations, - z.cfg.MetricsCheckInterval, ) + if err != nil { + glog.Warningf( + "Invalid custom metrics check interval value in deployment %s,"+ + " falling back to the default value of %d seconds; error: %s", + deployment.Name, + z.cfg.MetricsCheckInterval, + err, + ) + metricsCheckInterval = z.cfg.MetricsCheckInterval + } + if metricsCheckInterval <= 0 { + metricsCheckInterval = z.cfg.MetricsCheckInterval + } glog.Infof( "Using new metrics collector for deployment %s in namespace %s "+ "with metrics check interval of %d seconds", diff --git a/pkg/kubernetes/osiris.go b/pkg/kubernetes/osiris.go index 55a0370c..5e098fc6 100644 --- a/pkg/kubernetes/osiris.go +++ b/pkg/kubernetes/osiris.go @@ -1,10 +1,15 @@ package kubernetes import ( + "fmt" "strconv" "strings" ) +const ( + metricsCheckIntervalAnnotationName = "osiris.deislabs.io/metricsCheckInterval" +) + // ResourceIsOsirisEnabled checks the annotations to see if the // kube resource is enabled for osiris or not. func ResourceIsOsirisEnabled(annotations map[string]string) bool { @@ -37,19 +42,24 @@ func GetMinReplicas(annotations map[string]string, defaultVal int32) int32 { // GetMetricsCheckInterval gets the interval in which the zeroScaler would // repeatedly track the pod http request metrics. The value is the number -// of seconds of the interval. If it fails to do so, it returns the default -// value instead. -func GetMetricsCheckInterval( - annotations map[string]string, - defaultVal int, -) int { - val, ok := annotations["osiris.deislabs.io/metricsCheckInterval"] +// of seconds of the interval. If it fails to do so, it returns an error. +func GetMetricsCheckInterval(annotations map[string]string) (int, error) { + if len(annotations) == 0 { + return 0, nil + } + val, ok := annotations[metricsCheckIntervalAnnotationName] if !ok { - return defaultVal + return 0, nil } metricsCheckInterval, err := strconv.Atoi(val) if err != nil { - return defaultVal + return 0, fmt.Errorf("invalid int value '%s' for '%s' annotation: %s", + val, metricsCheckIntervalAnnotationName, err) + } + if metricsCheckInterval <= 0 { + return 0, fmt.Errorf("metricsCheckInterval should be positive, "+ + "'%d' is not a valid value", + metricsCheckInterval) } - return metricsCheckInterval + return metricsCheckInterval, nil } diff --git a/pkg/kubernetes/osiris_test.go b/pkg/kubernetes/osiris_test.go index c8d6aa84..8b7582e5 100644 --- a/pkg/kubernetes/osiris_test.go +++ b/pkg/kubernetes/osiris_test.go @@ -119,33 +119,78 @@ func TestGetMetricsCheckInterval(t *testing.T) { name string annotations map[string]string expectedResult int + expectedError bool }{ { - name: "map with metrics check interval entry", - annotations: map[string]string{ - "osiris.deislabs.io/metricsCheckInterval": "60", - }, - expectedResult: 60, + name: "nil map", + annotations: nil, + expectedResult: 0, + expectedError: false, + }, + { + name: "empty map", + annotations: map[string]string{}, + expectedResult: 0, + expectedError: false, }, { name: "map with no metrics check interval entry", annotations: map[string]string{ - "osiris.deislabs.io/notmetricsCheckInterval": "60", + "whatever": "60", }, - expectedResult: 150, + expectedResult: 0, + expectedError: false, }, { name: "map with invalid metrics check interval entry", annotations: map[string]string{ "osiris.deislabs.io/metricsCheckInterval": "invalid", }, - expectedResult: 150, + expectedResult: 0, + expectedError: true, + }, + { + name: "map with negative metrics check interval entry", + annotations: map[string]string{ + "osiris.deislabs.io/metricsCheckInterval": "-1", + }, + expectedResult: 0, + expectedError: true, + }, + { + name: "map with zero metrics check interval entry", + annotations: map[string]string{ + "osiris.deislabs.io/metricsCheckInterval": "0", + }, + expectedResult: 0, + expectedError: true, + }, + { + name: "map with valid metrics check interval entry", + annotations: map[string]string{ + "osiris.deislabs.io/metricsCheckInterval": "60", + }, + expectedResult: 60, + expectedError: false, }, } for _, test := range testcases { t.Run(test.name, func(t *testing.T) { - actual := GetMetricsCheckInterval(test.annotations, 150) + actual, err := GetMetricsCheckInterval(test.annotations) + if err != nil { + if !test.expectedError { + t.Errorf( + "expected GetMetricsCheckInterval to return %d, but got error %v", + test.expectedResult, err) + } + } else { + if test.expectedError { + t.Error( + "expected GetMetricsCheckInterval to return an error, but got none") + } + } + if actual != test.expectedResult { t.Errorf( "expected GetMetricsCheckInterval to return %d, but got %d", From 8d46b8ce57bee0b8886f4cbadfe454f9ed27fc21 Mon Sep 17 00:00:00 2001 From: Vincent Behar Date: Mon, 14 Oct 2019 09:01:17 +0200 Subject: [PATCH 3/6] add documentation --- README.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/README.md b/README.md index 17da9ebd..075558ce 100644 --- a/README.md +++ b/README.md @@ -101,7 +101,7 @@ The following table lists the configurable parameters of the Helm chart and thei | Parameter | Description | Default | | --------- | ----------- | ------- | -| `zeroscaler.metricsCheckInterval` | The interval in which the zeroScaler would repeatedly track the pod http request metrics. The value is the number of seconds of the interval. | `150` | +| `zeroscaler.metricsCheckInterval` | The interval in which the zeroScaler would repeatedly track the pod http request metrics. The value is the number of seconds of the interval. Note that this can also be set on a per-deployment basis, with an annotation. | `150` | Example of installation with Helm and a custom configuration: @@ -178,6 +178,7 @@ The following table lists the supported annotations for Kubernetes `Deployments` | ---------- | ----------- | ------- | | `osiris.deislabs.io/enabled` | Enable Osiris for this deployment. Allowed values: `y`, `yes`, `true`, `on`, `1`. | _no value_ (= disabled) | | `osiris.deislabs.io/minReplicas` | The minimum number of replicas to set on the deployment when Osiris will scale up. If you set `2`, Osiris will scale the deployment from `0` to `2` replicas directly. Osiris won't collect metrics from deployments which have more than `minReplicas` replicas - to avoid useless collections of metrics. | `1` | +| `osiris.deislabs.io/metricsCheckInterval` | The interval in which Osiris would repeatedly track the pod http request metrics. The value is the number of seconds of the interval. Note that this value override the global value defined by the `zeroscaler.metricsCheckInterval` Helm value. | _value of the `zeroscaler.metricsCheckInterval` Helm value_ | #### Service Annotations From fa782a7feee3c3c10cbc0160ac38f6b8ab3e52db Mon Sep 17 00:00:00 2001 From: Vincent Behar Date: Thu, 17 Oct 2019 14:35:56 +0200 Subject: [PATCH 4/6] add a warning if metricsCheckInterval is invalid --- pkg/deployments/zeroscaler/zeroscaler.go | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/pkg/deployments/zeroscaler/zeroscaler.go b/pkg/deployments/zeroscaler/zeroscaler.go index e5b3b471..5276b937 100644 --- a/pkg/deployments/zeroscaler/zeroscaler.go +++ b/pkg/deployments/zeroscaler/zeroscaler.go @@ -145,6 +145,13 @@ func (z *zeroscaler) ensureMetricsCollection(deployment *appsv1.Deployment) { metricsCheckInterval = z.cfg.MetricsCheckInterval } if metricsCheckInterval <= 0 { + glog.Warningf( + "Invalid custom metrics check interval value %d in deployment %s,"+ + " falling back to the default value of %d seconds", + metricsCheckInterval, + deployment.Name, + z.cfg.MetricsCheckInterval, + ) metricsCheckInterval = z.cfg.MetricsCheckInterval } glog.Infof( From e99cf3c2766424f9cdb882ffab4d343cb1bc9c7d Mon Sep 17 00:00:00 2001 From: Vincent Behar Date: Thu, 17 Oct 2019 14:40:54 +0200 Subject: [PATCH 5/6] improve warning message --- pkg/deployments/zeroscaler/zeroscaler.go | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/pkg/deployments/zeroscaler/zeroscaler.go b/pkg/deployments/zeroscaler/zeroscaler.go index 5276b937..ef42e991 100644 --- a/pkg/deployments/zeroscaler/zeroscaler.go +++ b/pkg/deployments/zeroscaler/zeroscaler.go @@ -136,8 +136,9 @@ func (z *zeroscaler) ensureMetricsCollection(deployment *appsv1.Deployment) { ) if err != nil { glog.Warningf( - "Invalid custom metrics check interval value in deployment %s,"+ - " falling back to the default value of %d seconds; error: %s", + "There was an error getting custom metrics check interval value "+ + "in deployment %s, falling back to the default value of %d "+ + "seconds; error: %s", deployment.Name, z.cfg.MetricsCheckInterval, err, From 1ddfe313883c382a13de4646969a5bf256b93c06 Mon Sep 17 00:00:00 2001 From: Vincent Behar Date: Thu, 17 Oct 2019 15:37:28 +0200 Subject: [PATCH 6/6] switch to testify for tests --- Gopkg.lock | 1 + pkg/kubernetes/osiris_test.go | 46 +++++++++++++++++------------------ 2 files changed, 23 insertions(+), 24 deletions(-) diff --git a/Gopkg.lock b/Gopkg.lock index 6a753d59..df256664 100644 --- a/Gopkg.lock +++ b/Gopkg.lock @@ -483,6 +483,7 @@ "github.com/kelseyhightower/envconfig", "github.com/phayes/freeport", "github.com/satori/go.uuid", + "github.com/stretchr/testify/assert", "github.com/stretchr/testify/require", "golang.org/x/net/http/httpguts", "golang.org/x/net/http2", diff --git a/pkg/kubernetes/osiris_test.go b/pkg/kubernetes/osiris_test.go index 8b7582e5..9fb4a3b0 100644 --- a/pkg/kubernetes/osiris_test.go +++ b/pkg/kubernetes/osiris_test.go @@ -1,6 +1,11 @@ package kubernetes -import "testing" +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) func TestResourceIsOsirisEnabled(t *testing.T) { testcases := []struct { @@ -119,19 +124,19 @@ func TestGetMetricsCheckInterval(t *testing.T) { name string annotations map[string]string expectedResult int - expectedError bool + expectedError string }{ { name: "nil map", annotations: nil, expectedResult: 0, - expectedError: false, + expectedError: "", }, { name: "empty map", annotations: map[string]string{}, expectedResult: 0, - expectedError: false, + expectedError: "", }, { name: "map with no metrics check interval entry", @@ -139,7 +144,7 @@ func TestGetMetricsCheckInterval(t *testing.T) { "whatever": "60", }, expectedResult: 0, - expectedError: false, + expectedError: "", }, { name: "map with invalid metrics check interval entry", @@ -147,7 +152,9 @@ func TestGetMetricsCheckInterval(t *testing.T) { "osiris.deislabs.io/metricsCheckInterval": "invalid", }, expectedResult: 0, - expectedError: true, + expectedError: "invalid int value 'invalid' for " + + "'osiris.deislabs.io/metricsCheckInterval' annotation: " + + "strconv.Atoi: parsing \"invalid\": invalid syntax", }, { name: "map with negative metrics check interval entry", @@ -155,7 +162,8 @@ func TestGetMetricsCheckInterval(t *testing.T) { "osiris.deislabs.io/metricsCheckInterval": "-1", }, expectedResult: 0, - expectedError: true, + expectedError: "metricsCheckInterval should be positive, " + + "'-1' is not a valid value", }, { name: "map with zero metrics check interval entry", @@ -163,7 +171,8 @@ func TestGetMetricsCheckInterval(t *testing.T) { "osiris.deislabs.io/metricsCheckInterval": "0", }, expectedResult: 0, - expectedError: true, + expectedError: "metricsCheckInterval should be positive, " + + "'0' is not a valid value", }, { name: "map with valid metrics check interval entry", @@ -171,31 +180,20 @@ func TestGetMetricsCheckInterval(t *testing.T) { "osiris.deislabs.io/metricsCheckInterval": "60", }, expectedResult: 60, - expectedError: false, + expectedError: "", }, } for _, test := range testcases { t.Run(test.name, func(t *testing.T) { actual, err := GetMetricsCheckInterval(test.annotations) - if err != nil { - if !test.expectedError { - t.Errorf( - "expected GetMetricsCheckInterval to return %d, but got error %v", - test.expectedResult, err) - } + if len(test.expectedError) > 0 { + require.EqualError(t, err, test.expectedError, "") } else { - if test.expectedError { - t.Error( - "expected GetMetricsCheckInterval to return an error, but got none") - } + require.NoError(t, err) } - if actual != test.expectedResult { - t.Errorf( - "expected GetMetricsCheckInterval to return %d, but got %d", - test.expectedResult, actual) - } + assert.Equal(t, test.expectedResult, actual) }) } }