From aa430e7f106949e1ee3108bc08c2e604d3e25a9b Mon Sep 17 00:00:00 2001 From: odubajDT <93584209+odubajDT@users.noreply.github.com> Date: Wed, 6 Sep 2023 14:33:31 +0200 Subject: [PATCH] feat(metrics-operator): support combination of OR criteria in SLO converter (#2023) Signed-off-by: odubajDT --- metrics-operator/converter/sli_converter.go | 2 +- metrics-operator/converter/slo_converter.go | 51 +++++-- .../converter/slo_converter_test.go | 127 ++++++++++++++++-- metrics-operator/main.go | 8 +- 4 files changed, 159 insertions(+), 29 deletions(-) diff --git a/metrics-operator/converter/sli_converter.go b/metrics-operator/converter/sli_converter.go index 125c22328a..777e993d7b 100644 --- a/metrics-operator/converter/sli_converter.go +++ b/metrics-operator/converter/sli_converter.go @@ -24,7 +24,7 @@ func NewSLIConverter() *SLIConverter { func (c *SLIConverter) Convert(fileContent []byte, provider string, namespace string) (string, error) { //check that provider and namespace is set if provider == "" || namespace == "" { - return "", fmt.Errorf("--sli-provider and --sli-namespace needs to be set for conversion") + return "", fmt.Errorf("missing arguments: 'keptn-provider-name' and 'keptn-provider-namespace' needs to be set for conversion") } // unmarshall content diff --git a/metrics-operator/converter/slo_converter.go b/metrics-operator/converter/slo_converter.go index 382d52f2d8..d23214d4fe 100644 --- a/metrics-operator/converter/slo_converter.go +++ b/metrics-operator/converter/slo_converter.go @@ -43,13 +43,27 @@ type Criteria struct { } func (o *Objective) hasNotSupportedCriteria() bool { - return len(o.Pass) > 1 || len(o.Warning) > 1 + // no pass criteria -> informative + if len(o.Pass) == 0 { + return true + } + // support only warning criteria with a single criteria element + if len(o.Warning) > 1 { + return true + } + // warning criteria == 1, pass can be only 1 + if len(o.Warning) == 1 { + return len(o.Pass) > 1 + } + + // warn criteria == 0, pass can be anything + return false } func (c *SLOConverter) Convert(fileContent []byte, analysisDef string, namespace string) (string, error) { //check that provider and namespace is set if analysisDef == "" || namespace == "" { - return "", fmt.Errorf("missing arguments: 'definition' and 'namespace' needs to be set for conversion") + return "", fmt.Errorf("missing arguments: 'analysis-definition-name' and 'analysis-value-template-namespace' needs to be set for conversion") } // unmarshall content @@ -142,10 +156,19 @@ func removePercentage(str string) (int, error) { // nolint:gocognit,gocyclo func setupTarget(o *Objective) (*metricsapi.Target, error) { target := &metricsapi.Target{} - // skip objective target conversion if it has criteria combined with logical OR -> not supported - // this way the SLO will become "informative" - // it will become informative as well when the pass criteria are not defined - if o.hasNotSupportedCriteria() || len(o.Pass) == 0 { + // skip unsupported combination of criteria and informative objectives + if o.hasNotSupportedCriteria() { + return target, nil + } + + // multiple criteria combined with logical OR operator + if len(o.Pass) > 1 { + ops := []string{o.Pass[0].Operators[0], o.Pass[1].Operators[0]} + op, err := newOperator(ops, true) + if err != nil { + return nil, err + } + target.Failure = op return target, nil } @@ -155,7 +178,7 @@ func setupTarget(o *Objective) (*metricsapi.Target, error) { if len(o.Pass[0].Operators) > 0 { op, err := newOperator(o.Pass[0].Operators, true) if err != nil { - return target, err + return nil, err } target.Failure = op return target, nil @@ -168,16 +191,16 @@ func setupTarget(o *Objective) (*metricsapi.Target, error) { // !(pass criteria) -> warn criteria isWarningSuperInterval, err := isSuperInterval(o.Warning[0].Operators, o.Pass[0].Operators) if err != nil { - return target, err + return nil, err } if (len(o.Pass[0].Operators) == 1 && len(o.Warning[0].Operators) == 1) || isWarningSuperInterval { op1, err := newOperator(o.Pass[0].Operators, true) if err != nil { - return target, err + return nil, err } op2, err := newOperator(o.Warning[0].Operators, true) if err != nil { - return target, err + return nil, err } target.Failure = op2 target.Warning = op1 @@ -189,16 +212,16 @@ func setupTarget(o *Objective) (*metricsapi.Target, error) { // warn criteria -> warn criteria isPassSuperInterval, err := isSuperInterval(o.Pass[0].Operators, o.Warning[0].Operators) if err != nil { - return target, err + return nil, err } if isPassSuperInterval { op1, err := newOperator(o.Warning[0].Operators, false) if err != nil { - return target, err + return nil, err } op2, err := newOperator(o.Pass[0].Operators, true) if err != nil { - return target, err + return nil, err } target.Failure = op2 target.Warning = op1 @@ -396,7 +419,7 @@ func negateSingleOperator(op string, value string) (*metricsapi.Operator, error) }, nil } - return &metricsapi.Operator{}, NewInvalidOperatorErr(op) + return nil, NewInvalidOperatorErr(op) } // checks and creates single operator diff --git a/metrics-operator/converter/slo_converter_test.go b/metrics-operator/converter/slo_converter_test.go index 4beaaec1da..1d642042bf 100644 --- a/metrics-operator/converter/slo_converter_test.go +++ b/metrics-operator/converter/slo_converter_test.go @@ -23,14 +23,22 @@ objectives: - sli: "response_time_p90" key_sli: false pass: - - criteria: + - criteria: - ">600" - "<800" warning: - - criteria: + - criteria: - "<=1000" - ">500" weight: 2 + - sli: "response_time_p91" + key_sli: false + pass: + - criteria: + - "<600" + - criteria: + - ">800" + weight: 5 - sli: "response_time_p80" key_sli: false pass: @@ -66,10 +74,10 @@ objectives: pass: - criteria: - "<=+100%" - - ">=80" + - ">=100" - criteria: - "<=+100%" - - ">=80" + - "<=80" - sli: "throughput" pass: - criteria: @@ -100,6 +108,15 @@ spec: highBound: "800" lowBound: "600" weight: 2 + - analysisValueTemplateRef: + name: response_time_p91 + namespace: default + target: + failure: + inRange: + highBound: "800" + lowBound: "600" + weight: 5 - analysisValueTemplateRef: name: response_time_p80 namespace: default @@ -138,7 +155,11 @@ spec: - analysisValueTemplateRef: name: cpu namespace: default - target: {} + target: + failure: + inRange: + highBound: "100" + lowBound: "80" - analysisValueTemplateRef: name: throughput namespace: default @@ -840,7 +861,7 @@ func TestNewOperator(t *testing.T) { } } -func TestShouldIgnoreObjective(t *testing.T) { +func TestHasNotSupportedCriteria(t *testing.T) { tests := []struct { name string o *Objective @@ -852,10 +873,34 @@ func TestShouldIgnoreObjective(t *testing.T) { Pass: []Criteria{}, Warning: []Criteria{}, }, + want: true, + }, + { + name: "pass == 1, warn == 0", + o: &Objective{ + Pass: []Criteria{ + { + Operators: []string{}, + }, + }, + Warning: []Criteria{}, + }, want: false, }, { - name: "valid criteria", + name: "pass == 0, warn == 1", + o: &Objective{ + Pass: []Criteria{}, + Warning: []Criteria{ + { + Operators: []string{}, + }, + }, + }, + want: true, + }, + { + name: "pass == 1; warn == 1", o: &Objective{ Pass: []Criteria{ { @@ -871,7 +916,7 @@ func TestShouldIgnoreObjective(t *testing.T) { want: false, }, { - name: "OR criteria", + name: "pass == 2; warn == 1", o: &Objective{ Pass: []Criteria{ { @@ -889,6 +934,21 @@ func TestShouldIgnoreObjective(t *testing.T) { }, want: true, }, + { + name: "pass == 2; warn == 0", + o: &Objective{ + Pass: []Criteria{ + { + Operators: []string{}, + }, + { + Operators: []string{}, + }, + }, + Warning: []Criteria{}, + }, + want: false, + }, } for _, tt := range tests { @@ -1154,15 +1214,62 @@ func TestSetupTarget(t *testing.T) { wantErr: true, }, { - name: "logical OR criteria -> informative slo", + name: "logical OR criteria -> with pass only", o: &Objective{ Name: "criteria", Pass: []Criteria{ + { + Operators: []string{">15"}, + }, { Operators: []string{"<10"}, }, + }, + }, + want: &metricsapi.Target{ + Failure: &metricsapi.Operator{ + InRange: &metricsapi.RangeValue{ + LowBound: *resource.NewDecimalQuantity(*dec10, resource.DecimalSI), + HighBound: *resource.NewDecimalQuantity(*dec15, resource.DecimalSI), + }, + }, + }, + wantErr: false, + }, + { + name: "logical OR criteria -> with pass only - error", + o: &Objective{ + Name: "criteria", + Pass: []Criteria{ { - Operators: []string{">1"}, + Operators: []string{">====15"}, + }, + { + Operators: []string{"<10"}, + }, + }, + }, + want: &metricsapi.Target{}, + wantErr: true, + }, + { + name: "logical OR criteria with pass and warn -> informative", + o: &Objective{ + Name: "criteria", + Pass: []Criteria{ + { + Operators: []string{">15"}, + }, + { + Operators: []string{"<10"}, + }, + }, + Warning: []Criteria{ + { + Operators: []string{">15"}, + }, + { + Operators: []string{"<10"}, }, }, }, diff --git a/metrics-operator/main.go b/metrics-operator/main.go index ced779a28a..6beac86a10 100644 --- a/metrics-operator/main.go +++ b/metrics-operator/main.go @@ -92,11 +92,11 @@ func main() { var disableWebhook bool var probeAddr string flag.StringVar(&SLIFilePath, "convert-sli", "", "The path the the SLI file to be converted") - flag.StringVar(&provider, "sli-provider", "", "The name of KeptnMetricsProvider referenced in KeptnValueTemplates") - flag.StringVar(&namespace, "sli-namespace", "", "The namespace of the referenced KeptnMetricsProvider") + flag.StringVar(&provider, "keptn-provider-name", "", "The name of KeptnMetricsProvider referenced in KeptnValueTemplates") + flag.StringVar(&namespace, "keptn-provider-namespace", "", "The namespace of the referenced KeptnMetricsProvider") flag.StringVar(&SLOFilePath, "convert-slo", "", "The path the the SLO file to be converted") - flag.StringVar(&analysisDefinition, "definition", "", "The name of AnalysisDefinition to be created") - flag.StringVar(&namespace, "slo-namespace", "", "The namespace of the referenced AnalysisValueTemplate") + flag.StringVar(&analysisDefinition, "analysis-definition-name", "", "The name of AnalysisDefinition to be created") + flag.StringVar(&namespace, "analysis-value-template-namespace", "", "The namespace of the referenced AnalysisValueTemplate") flag.StringVar(&metricsAddr, "metrics-bind-address", ":8080", "The address the metric endpoint binds to.") flag.StringVar(&probeAddr, "health-probe-bind-address", ":8081", "The address the probe endpoint binds to.") flag.BoolVar(&disableWebhook, "disable-webhook", false, "Disable the registration of webhooks.")