fixup! fixup! OCPBUGS-17113: Lazy updates for Prometheus

Author: rexagod

Diff for: Makefile

@@ -12,7 +12,7 @@ GOPROXY?=http://proxy.golang.org
 export GO111MODULE
 export GOPROXY
 
-# go pakages for unit tests, excluding e2e tests
+# go packages for unit tests, excluding e2e tests
 PKGS=$(shell go list ./... | grep -v /test/e2e)
 GOLANG_FILES:=$(shell find . -name \*.go -print)
 # NOTE: grep -v %.yaml is needed  because "%s-policy.yaml" is used
@@ -249,17 +249,21 @@ check-runbooks:
 # Testing #
 ###########
 
+.PHONY: bench
+bench:
+	go test -run NONE -bench ^Bench -benchmem $(PKGS)
+
 .PHONY: test
 test: test-unit test-rules test-e2e
 
 .PHONY: test-unit
 test-unit:
-	go test -race -short $(PKGS) -count=1
+	go test -run ^Test -race -short $(PKGS) -count=1
 
 .PHONY: test-e2e
 test-e2e: KUBECONFIG?=$(HOME)/.kube/config
 test-e2e:
-	go test -v -timeout=150m ./test/e2e/ --kubeconfig $(KUBECONFIG)
+	go test -run ^Test -v -timeout=150m ./test/e2e/ --kubeconfig $(KUBECONFIG)
 
 $(BIN_DIR):
 	mkdir -p $(BIN_DIR)

Diff for: pkg/client/client.go

@@ -25,6 +25,22 @@ import (
 	"strings"
 	"time"
 
+	"github.com/imdario/mergo"
+	configv1 "github.com/openshift/api/config/v1"
+	consolev1 "github.com/openshift/api/console/v1"
+	osmv1 "github.com/openshift/api/monitoring/v1"
+	routev1 "github.com/openshift/api/route/v1"
+	secv1 "github.com/openshift/api/security/v1"
+	openshiftconfigclientset "github.com/openshift/client-go/config/clientset/versioned"
+	openshiftconsoleclientset "github.com/openshift/client-go/console/clientset/versioned"
+	openshiftmonitoringclientset "github.com/openshift/client-go/monitoring/clientset/versioned"
+	openshiftoperatorclientset "github.com/openshift/client-go/operator/clientset/versioned"
+	openshiftrouteclientset "github.com/openshift/client-go/route/clientset/versioned"
+	openshiftsecurityclientset "github.com/openshift/client-go/security/clientset/versioned"
+	"github.com/openshift/library-go/pkg/operator/events"
+	"github.com/openshift/library-go/pkg/operator/resource/resourceapply"
+	monv1 "github.com/prometheus-operator/prometheus-operator/pkg/apis/monitoring/v1"
+	monitoring "github.com/prometheus-operator/prometheus-operator/pkg/client/versioned"
 	admissionv1 "k8s.io/api/admissionregistration/v1"
 	appsv1 "k8s.io/api/apps/v1"
 	v1 "k8s.io/api/core/v1"
@@ -52,23 +68,6 @@ import (
 	apiregistrationv1 "k8s.io/kube-aggregator/pkg/apis/apiregistration/v1"
 	aggregatorclient "k8s.io/kube-aggregator/pkg/client/clientset_generated/clientset"
 	"k8s.io/utils/ptr"
-
-	"github.com/imdario/mergo"
-	configv1 "github.com/openshift/api/config/v1"
-	consolev1 "github.com/openshift/api/console/v1"
-	osmv1 "github.com/openshift/api/monitoring/v1"
-	routev1 "github.com/openshift/api/route/v1"
-	secv1 "github.com/openshift/api/security/v1"
-	openshiftconfigclientset "github.com/openshift/client-go/config/clientset/versioned"
-	openshiftconsoleclientset "github.com/openshift/client-go/console/clientset/versioned"
-	openshiftmonitoringclientset "github.com/openshift/client-go/monitoring/clientset/versioned"
-	openshiftoperatorclientset "github.com/openshift/client-go/operator/clientset/versioned"
-	openshiftrouteclientset "github.com/openshift/client-go/route/clientset/versioned"
-	openshiftsecurityclientset "github.com/openshift/client-go/security/clientset/versioned"
-	"github.com/openshift/library-go/pkg/operator/events"
-	"github.com/openshift/library-go/pkg/operator/resource/resourceapply"
-	monv1 "github.com/prometheus-operator/prometheus-operator/pkg/apis/monitoring/v1"
-	monitoring "github.com/prometheus-operator/prometheus-operator/pkg/client/versioned"
 )
 
 const (
@@ -650,6 +649,9 @@ func (c *Client) GetAlertingRule(ctx context.Context, namespace, name string) (*
 	return c.osmclient.MonitoringV1().AlertingRules(namespace).Get(ctx, name, metav1.GetOptions{})
 }
 
+// NOTE: We don't use a defaulting function here, and resort only to the caching mechanism, since,
+// * defaults for Prometheus should be introduced from its operator level, and,
+// * the defaulting approach generally requires hardcoding the default values, which adds on to the maintenance overhead.
 func (c *Client) CreateOrUpdatePrometheus(ctx context.Context, structuredRequiredPrometheus *monv1.Prometheus) (bool, error) {
 	unstructuredRequiredPrometheusObject, err := runtime.DefaultUnstructuredConverter.ToUnstructured(structuredRequiredPrometheus)
 	if err != nil {
@@ -676,7 +678,7 @@ func (c *Client) CreateOrUpdatePrometheus(ctx context.Context, structuredRequire
 			unstructuredRequiredPrometheus,
 			c.resourceCache,
 			prometheusGVR,
-			prometheusDefaultingFunc,
+			nil,
 			nil,
 		)
 		if err != nil {
@@ -703,7 +705,7 @@ func (c *Client) CreateOrUpdatePrometheus(ctx context.Context, structuredRequire
 		unstructuredRequiredPrometheus,
 		c.resourceCache,
 		prometheusGVR,
-		prometheusDefaultingFunc,
+		nil,
 		nil,
 	)
 	if err != nil {
@@ -713,52 +715,6 @@ func (c *Client) CreateOrUpdatePrometheus(ctx context.Context, structuredRequire
 	return didUpdate, nil
 }
 
-func prometheusDefaultingFunc(unstructuredPrometheus *unstructured.Unstructured) {
-	// Cast to the corresponding structured representation.
-	structuredPrometheus := &monv1.Prometheus{}
-	if err := runtime.DefaultUnstructuredConverter.FromUnstructured(unstructuredPrometheus.UnstructuredContent(), structuredPrometheus); err != nil {
-		klog.ErrorS(err, "failed to convert unstructured to structured Prometheus spec")
-		return
-	}
-
-	// Set defaults.
-	if structuredPrometheus.Spec.CommonPrometheusFields.ScrapeInterval == "" {
-		structuredPrometheus.Spec.CommonPrometheusFields.ScrapeInterval = "30s"
-	}
-	if len(structuredPrometheus.Spec.CommonPrometheusFields.ExternalLabels) == 0 {
-		structuredPrometheus.Spec.CommonPrometheusFields.ExternalLabels = nil
-	}
-	if len(structuredPrometheus.Spec.CommonPrometheusFields.EnableFeatures) == 0 {
-		structuredPrometheus.Spec.CommonPrometheusFields.EnableFeatures = nil
-	}
-	for i, container := range structuredPrometheus.Spec.CommonPrometheusFields.Containers {
-		for j, port := range container.Ports {
-			if port.Protocol == "" {
-				structuredPrometheus.Spec.CommonPrometheusFields.Containers[i].Ports[j].Protocol = "TCP"
-			}
-		}
-	}
-	if structuredPrometheus.Spec.CommonPrometheusFields.PortName == "" {
-		structuredPrometheus.Spec.CommonPrometheusFields.PortName = "web"
-	}
-	if structuredPrometheus.Spec.Thanos == nil {
-		structuredPrometheus.Spec.Thanos = &monv1.ThanosSpec{}
-	}
-	if structuredPrometheus.Spec.Thanos.BlockDuration == "" {
-		structuredPrometheus.Spec.Thanos.BlockDuration = "2h"
-	}
-	if structuredPrometheus.Spec.EvaluationInterval == "" {
-		structuredPrometheus.Spec.EvaluationInterval = "30s"
-	}
-
-	// Convert back to the corresponding unstructured representation and inject.
-	var err error
-	unstructuredPrometheus.Object, err = runtime.DefaultUnstructuredConverter.ToUnstructured(structuredPrometheus)
-	if err != nil {
-		klog.ErrorS(err, "failed to convert structured to unstructured Prometheus")
-	}
-}
-
 func (c *Client) CreateOrUpdatePrometheusRule(ctx context.Context, p *monv1.PrometheusRule) error {
 	pclient := c.mclient.MonitoringV1().PrometheusRules(p.GetNamespace())
 	existing, err := pclient.Get(ctx, p.GetName(), metav1.GetOptions{})

Diff for: pkg/client/client_test.go

@@ -1856,9 +1856,8 @@ func TestCreateOrUpdatePrometheus(t *testing.T) {
 		shouldUpdate bool
 	}{
 		{
-			description:  "initially, apply all default values to the existing prometheus",
-			prometheus:   initialPrometheus,
-			shouldUpdate: true,
+			description: "initially, apply all default values to the existing prometheus",
+			prometheus:  initialPrometheus,
 		},
 		{
 			description: "apply a default value to the existing prometheus",
@@ -1871,13 +1870,15 @@ func TestCreateOrUpdatePrometheus(t *testing.T) {
 					},
 				},
 			},
+			shouldUpdate: true,
 		},
 		{
 			description: "apply no value to the existing prometheus",
 			prometheus: &monv1.Prometheus{
 				TypeMeta:   initialPrometheus.TypeMeta,
 				ObjectMeta: initialPrometheus.ObjectMeta,
 			},
+			shouldUpdate: true,
 		},
 		{
 			description: "apply a non-default value to the existing prometheus",
@@ -2005,7 +2006,7 @@ func BenchmarkCreateOrUpdatePrometheus(b *testing.B) {
 			},
 		},
 		{
-			// Benchmark the custom-defined defaulting behavior.
+			// Benchmark the library-go `spec` behavior.
 			description: "apply the same default value to the existing prometheus",
 			prometheus: func() *monv1.Prometheus {
 				return &monv1.Prometheus{
@@ -2021,7 +2022,7 @@ func BenchmarkCreateOrUpdatePrometheus(b *testing.B) {
 			requiresInitialApply: true,
 		},
 		{
-			// Benchmark the caching behavior.
+			// Benchmark the library-go `metadata` behavior.
 			description: "apply the same custom label to the existing prometheus",
 			prometheus: func() *monv1.Prometheus {
 				return &monv1.Prometheus{