feat(metrics-operator): support combination of OR criteria in SLO con…

…verter (#2023)

Signed-off-by: odubajDT <ondrej.dubaj@dynatrace.com>

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

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

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"},
 					},
 				},
 			},

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.")