OCPBUGS-17113: Lazy updates for Prometheus

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,6 +249,10 @@ check-runbooks:
 # Testing #
 ###########
 
+.PHONY: bench
+bench:
+	go test -run NONE -bench=. -benchmem $(PKGS)
+
 .PHONY: test
 test: test-unit test-rules test-e2e
 

go.mod

@@ -12,6 +12,7 @@ require (
 	github.com/ghodss/yaml v1.0.0
 	github.com/go-kit/log v0.2.1
 	github.com/go-openapi/strfmt v0.23.0
+	github.com/google/go-cmp v0.6.0
 	github.com/google/uuid v1.6.0
 	github.com/imdario/mergo v0.3.16
 	github.com/mattn/go-shellwords v1.0.12
@@ -82,7 +83,6 @@ require (
 	github.com/golang/protobuf v1.5.4 // indirect
 	github.com/google/cel-go v0.20.1 // indirect
 	github.com/google/gnostic-models v0.6.8 // indirect
-	github.com/google/go-cmp v0.6.0 // indirect
 	github.com/google/gofuzz v1.2.0 // indirect
 	github.com/grafana/regexp v0.0.0-20240518133315-a468a5bfb3bc // indirect
 	github.com/grpc-ecosystem/go-grpc-prometheus v1.2.0 // indirect

pkg/client/client.go

@@ -50,6 +50,7 @@ import (
 	apiextensionsclient "k8s.io/apiextensions-apiserver/pkg/client/clientset/clientset"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 	"k8s.io/apimachinery/pkg/fields"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/runtime/schema"
@@ -58,6 +59,7 @@ import (
 	"k8s.io/apimachinery/pkg/util/intstr"
 	"k8s.io/apimachinery/pkg/util/wait"
 	"k8s.io/apimachinery/pkg/watch"
+	"k8s.io/client-go/dynamic"
 	"k8s.io/client-go/kubernetes"
 	"k8s.io/client-go/metadata"
 	"k8s.io/client-go/rest"
@@ -81,6 +83,7 @@ type Client struct {
 	userWorkloadNamespace string
 
 	kclient     kubernetes.Interface
+	dclient     dynamic.Interface
 	mdataclient metadata.Interface
 	osmclient   openshiftmonitoringclientset.Interface
 	oscclient   openshiftconfigclientset.Interface
@@ -107,6 +110,14 @@ func NewForConfig(cfg *rest.Config, version string, namespace, userWorkloadNames
 		client.kclient = kclient
 	}
 
+	if client.dclient == nil {
+		dclient, err := dynamic.NewForConfig(cfg)
+		if err != nil {
+			return nil, fmt.Errorf("creating dynamic clientset client: %w", err)
+		}
+		client.dclient = dclient
+	}
+
 	if client.eclient == nil {
 		eclient, err := apiextensionsclient.NewForConfig(cfg)
 		if err != nil {
@@ -203,6 +214,12 @@ func KubernetesClient(kclient kubernetes.Interface) Option {
 	}
 }
 
+func DynamicClient(dclient *dynamic.DynamicClient) Option {
+	return func(c *Client) {
+		c.dclient = dclient
+	}
+}
+
 func OpenshiftMonitoringClient(osmclient openshiftmonitoringclientset.Interface) Option {
 	return func(c *Client) {
 		c.osmclient = osmclient
@@ -632,29 +649,75 @@ func (c *Client) GetAlertingRule(ctx context.Context, namespace, name string) (*
 	return c.osmclient.MonitoringV1().AlertingRules(namespace).Get(ctx, name, metav1.GetOptions{})
 }
 
-func (c *Client) CreateOrUpdatePrometheus(ctx context.Context, p *monv1.Prometheus) error {
-	pclient := c.mclient.MonitoringV1().Prometheuses(p.GetNamespace())
-	existing, err := pclient.Get(ctx, p.GetName(), metav1.GetOptions{})
+// CreateOrUpdatePrometheus does not use a defaulting function, and resorts 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.
+// NOTE: The return values establish the following matrix (w.r.t. the create or update verbs):
+// * true, nil   : verb action needed; operation successful,
+// * false, nil  : verb action not needed; operation skipped,
+// * true, error : verb action needed, operation unsuccessful.
+// * false, error: verb action may or may not be needed; operation unsuccessful,
+func (c *Client) CreateOrUpdatePrometheus(ctx context.Context, structuredRequiredPrometheus *monv1.Prometheus) (bool, error) {
+	unstructuredRequiredPrometheusObject, err := runtime.DefaultUnstructuredConverter.ToUnstructured(structuredRequiredPrometheus)
+	if err != nil {
+		return false, fmt.Errorf("converting Prometheus object to unstructured failed: %w", err)
+	}
+	unstructuredRequiredPrometheus := &unstructured.Unstructured{}
+	unstructuredRequiredPrometheus.SetUnstructuredContent(unstructuredRequiredPrometheusObject)
+
+	// Set the GVK for the unstructured object, since these are not inferred automatically, unlike their structured counterparts.
+	unstructuredRequiredPrometheus.SetGroupVersionKind(monv1.SchemeGroupVersion.WithKind(monv1.PrometheusesKind))
+
+	// Preserve the original behavior: always merge the metadata, never replace.
+	// Refer: https://github.com/openshift/cluster-monitoring-operator/pull/942.
+	prometheusGVR := unstructuredRequiredPrometheus.GroupVersionKind().GroupVersion().WithResource(monv1.PrometheusName)
+	unstructuredExistingPrometheus, err := c.dclient.
+		Resource(prometheusGVR).
+		Namespace(structuredRequiredPrometheus.GetNamespace()).
+		Get(ctx, structuredRequiredPrometheus.GetName(), metav1.GetOptions{})
 	if apierrors.IsNotFound(err) {
-		_, err := pclient.Create(ctx, p, metav1.CreateOptions{})
+		_, didUpdate, err := resourceapply.ApplyUnstructuredResourceImproved(
+			ctx,
+			c.dclient,
+			c.eventRecorder,
+			unstructuredRequiredPrometheus,
+			c.resourceCache,
+			prometheusGVR,
+			nil,
+			nil,
+		)
 		if err != nil {
-			return fmt.Errorf("creating Prometheus object failed: %w", err)
+			return didUpdate, fmt.Errorf("creating Prometheus object failed: %w", err)
 		}
-		return nil
+
+		return true, nil
 	}
 	if err != nil {
-		return fmt.Errorf("retrieving Prometheus object failed: %w", err)
+		return false, fmt.Errorf("retrieving Prometheus object failed: %w", err)
 	}
+	unstructuredRequiredPrometheusMetadataLabels := unstructuredRequiredPrometheus.GetLabels()
+	unstructuredRequiredPrometheusMetadataAnnotations := unstructuredRequiredPrometheus.GetAnnotations()
+	mergeMetadataLabels(unstructuredRequiredPrometheusMetadataLabels, unstructuredExistingPrometheus.GetLabels())
+	mergeMetadataAnnotations(unstructuredRequiredPrometheusMetadataAnnotations, unstructuredExistingPrometheus.GetAnnotations())
+	unstructuredRequiredPrometheus.SetLabels(unstructuredRequiredPrometheusMetadataLabels)
+	unstructuredRequiredPrometheus.SetAnnotations(unstructuredRequiredPrometheusMetadataAnnotations)
+	unstructuredRequiredPrometheus.SetResourceVersion(unstructuredExistingPrometheus.GetResourceVersion())
 
-	required := p.DeepCopy()
-	mergeMetadata(&required.ObjectMeta, existing.ObjectMeta)
-
-	required.ResourceVersion = existing.ResourceVersion
-	_, err = pclient.Update(ctx, required, metav1.UpdateOptions{})
+	_, didUpdate, err := resourceapply.ApplyUnstructuredResourceImproved(
+		ctx,
+		c.dclient,
+		c.eventRecorder,
+		unstructuredRequiredPrometheus,
+		c.resourceCache,
+		prometheusGVR,
+		nil,
+		nil,
+	)
 	if err != nil {
-		return fmt.Errorf("updating Prometheus object failed: %w", err)
+		return didUpdate, fmt.Errorf("updating Prometheus object failed: %w", err)
 	}
-	return nil
+
+	return didUpdate, nil
 }
 
 func (c *Client) CreateOrUpdatePrometheusRule(ctx context.Context, p *monv1.PrometheusRule) error {
@@ -1878,20 +1941,28 @@ func (c *Client) VPACustomResourceDefinitionPresent(ctx context.Context, lastKno
 // where keys starting from string defined in `metadataPrefix` are deleted. This prevents issues with preserving stale
 // metadata defined by the operator
 func mergeMetadata(required *metav1.ObjectMeta, existing metav1.ObjectMeta) {
-	for k := range existing.Annotations {
+	mergeMetadataLabels(required.Labels, existing.Labels)
+	mergeMetadataAnnotations(required.Annotations, existing.Annotations)
+}
+
+func mergeMetadataLabels(requiredLabels map[string]string, existingLabels map[string]string) {
+	for k := range existingLabels {
 		if strings.HasPrefix(k, metadataPrefix) {
-			delete(existing.Annotations, k)
+			delete(existingLabels, k)
 		}
 	}
 
-	for k := range existing.Labels {
+	_ = mergo.Merge(&requiredLabels, existingLabels)
+}
+
+func mergeMetadataAnnotations(requiredAnnotations map[string]string, existingAnnotations map[string]string) {
+	for k := range existingAnnotations {
 		if strings.HasPrefix(k, metadataPrefix) {
-			delete(existing.Labels, k)
+			delete(existingAnnotations, k)
 		}
 	}
 
-	_ = mergo.Merge(&required.Annotations, existing.Annotations)
-	_ = mergo.Merge(&required.Labels, existing.Labels)
+	_ = mergo.Merge(&requiredAnnotations, existingAnnotations)
 }
 
 type jsonPatch struct {