Skip to content
This repository was archived by the owner on Jan 27, 2021. It is now read-only.
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Gopkg.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 2 additions & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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:

Expand Down Expand Up @@ -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

Expand Down
1 change: 1 addition & 0 deletions example/hello-osiris.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down
30 changes: 28 additions & 2 deletions pkg/deployments/zeroscaler/zeroscaler.go
Original file line number Diff line number Diff line change
Expand Up @@ -131,17 +131,43 @@ func (z *zeroscaler) ensureMetricsCollection(deployment *appsv1.Deployment) {
if ok {
collector.stop()
}
metricsCheckInterval, err := k8s.GetMetricsCheckInterval(
deployment.Annotations,
)
if err != nil {
glog.Warningf(
"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,
)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The error you return from GetMetricsCheckInterval() already indicates it was an issue with paring the value. Since you're including the text of the error at the end of the warning message, this information basically appears twice.

I would refrain from mentioning the value as "invalid" here and just note "there was an error getting.... falling back to." This way, also, if GetMetricsCheckInterval() ever changes and produces other possible errors as well, there won't be text elsewhere in the code (e.g. right here) that needs to be adjusted to account for the different sorts of errors that function can produce.

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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is probably worthy of a warning.

}
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)
Expand Down
29 changes: 29 additions & 0 deletions pkg/kubernetes/osiris.go
Original file line number Diff line number Diff line change
@@ -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 {
Expand Down Expand Up @@ -34,3 +39,27 @@ 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 an error.
func GetMetricsCheckInterval(annotations map[string]string) (int, error) {
if len(annotations) == 0 {
return 0, nil
}
val, ok := annotations[metricsCheckIntervalAnnotationName]
if !ok {
return 0, nil
}
metricsCheckInterval, err := strconv.Atoi(val)
if err != nil {
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, nil
}
86 changes: 85 additions & 1 deletion pkg/kubernetes/osiris_test.go
Original file line number Diff line number Diff line change
@@ -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 {
Expand Down Expand Up @@ -113,3 +118,82 @@ func TestGetMinReplicas(t *testing.T) {
})
}
}

func TestGetMetricsCheckInterval(t *testing.T) {
testcases := []struct {
name string
annotations map[string]string
expectedResult int
expectedError string
}{
{
name: "nil map",
annotations: nil,
expectedResult: 0,
expectedError: "",
},
{
name: "empty map",
annotations: map[string]string{},
expectedResult: 0,
expectedError: "",
},
{
name: "map with no metrics check interval entry",
annotations: map[string]string{
"whatever": "60",
},
expectedResult: 0,
expectedError: "",
},
{
name: "map with invalid metrics check interval entry",
annotations: map[string]string{
"osiris.deislabs.io/metricsCheckInterval": "invalid",
},
expectedResult: 0,
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",
annotations: map[string]string{
"osiris.deislabs.io/metricsCheckInterval": "-1",
},
expectedResult: 0,
expectedError: "metricsCheckInterval should be positive, " +
"'-1' is not a valid value",
},
{
name: "map with zero metrics check interval entry",
annotations: map[string]string{
"osiris.deislabs.io/metricsCheckInterval": "0",
},
expectedResult: 0,
expectedError: "metricsCheckInterval should be positive, " +
"'0' is not a valid value",
},
{
name: "map with valid metrics check interval entry",
annotations: map[string]string{
"osiris.deislabs.io/metricsCheckInterval": "60",
},
expectedResult: 60,
expectedError: "",
},
}

for _, test := range testcases {
t.Run(test.name, func(t *testing.T) {
actual, err := GetMetricsCheckInterval(test.annotations)
if len(test.expectedError) > 0 {
require.EqualError(t, err, test.expectedError, "")
} else {
require.NoError(t, err)
}

assert.Equal(t, test.expectedResult, actual)
})
}
}