Fix labels for Service Monitors

.chloggen/bug_2877.yaml

@@ -0,0 +1,18 @@
+# One of 'breaking', 'deprecation', 'new_component', 'enhancement', 'bug_fix'
+change_type: bug_fix
+
+# The name of the component, or a single word describing the area of concern, (e.g. collector, target allocator, auto-instrumentation, opamp, github action)
+component: collector
+
+# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`).
+note: |
+  Create a Service Monitor for the collector Service when Prometheus exporter is used. A different Service Monitor is created for the monitoring service.
+  This helps excluding the headless service (duplicating the metrics collectiong) and splits responsibilities between the two Service Monitors.
+
+# One or more tracking issues related to the change
+issues: [2877]
+
+# (Optional) One or more lines of additional information to render under the primary note.
+# These lines will be padded with 2 spaces and then inserted directly into the document.
+# Use pipe (|) for multiline entries.
+subtext:

Makefile

@@ -312,10 +312,6 @@ endif
 install-metrics-server:
 	./hack/install-metrics-server.sh
 
-.PHONY: install-prometheus-operator
-install-prometheus-operator:
-	./hack/install-prometheus-operator.sh
-
 # This only installs the CRDs Target Allocator supports
 .PHONY: install-targetallocator-prometheus-crds
 install-targetallocator-prometheus-crds:

controllers/builder_test.go

@@ -259,12 +259,13 @@ service:
 						Name:      "test-collector",
 						Namespace: "test",
 						Labels: map[string]string{
-							"app.kubernetes.io/component":  "opentelemetry-collector",
-							"app.kubernetes.io/instance":   "test.test",
-							"app.kubernetes.io/managed-by": "opentelemetry-operator",
-							"app.kubernetes.io/name":       "test-collector",
-							"app.kubernetes.io/part-of":    "opentelemetry",
-							"app.kubernetes.io/version":    "latest",
+							"app.kubernetes.io/component":                 "opentelemetry-collector",
+							"app.kubernetes.io/instance":                  "test.test",
+							"app.kubernetes.io/managed-by":                "opentelemetry-operator",
+							"app.kubernetes.io/name":                      "test-collector",
+							"app.kubernetes.io/part-of":                   "opentelemetry",
+							"app.kubernetes.io/version":                   "latest",
+							"operator.opentelemetry.io/collector-service": "Exists",
 						},
 						Annotations: nil,
 					},
@@ -506,12 +507,13 @@ service:
 						Name:      "test-collector",
 						Namespace: "test",
 						Labels: map[string]string{
-							"app.kubernetes.io/component":  "opentelemetry-collector",
-							"app.kubernetes.io/instance":   "test.test",
-							"app.kubernetes.io/managed-by": "opentelemetry-operator",
-							"app.kubernetes.io/name":       "test-collector",
-							"app.kubernetes.io/part-of":    "opentelemetry",
-							"app.kubernetes.io/version":    "latest",
+							"app.kubernetes.io/component":                 "opentelemetry-collector",
+							"app.kubernetes.io/instance":                  "test.test",
+							"app.kubernetes.io/managed-by":                "opentelemetry-operator",
+							"app.kubernetes.io/name":                      "test-collector",
+							"app.kubernetes.io/part-of":                   "opentelemetry",
+							"app.kubernetes.io/version":                   "latest",
+							"operator.opentelemetry.io/collector-service": "Exists",
 						},
 						Annotations: nil,
 					},
@@ -774,12 +776,13 @@ service:
 						Name:      "test-collector",
 						Namespace: "test",
 						Labels: map[string]string{
-							"app.kubernetes.io/component":  "opentelemetry-collector",
-							"app.kubernetes.io/instance":   "test.test",
-							"app.kubernetes.io/managed-by": "opentelemetry-operator",
-							"app.kubernetes.io/name":       "test-collector",
-							"app.kubernetes.io/part-of":    "opentelemetry",
-							"app.kubernetes.io/version":    "latest",
+							"app.kubernetes.io/component":                 "opentelemetry-collector",
+							"app.kubernetes.io/instance":                  "test.test",
+							"app.kubernetes.io/managed-by":                "opentelemetry-operator",
+							"app.kubernetes.io/name":                      "test-collector",
+							"app.kubernetes.io/part-of":                   "opentelemetry",
+							"app.kubernetes.io/version":                   "latest",
+							"operator.opentelemetry.io/collector-service": "Exists",
 						},
 						Annotations: nil,
 					},

internal/manifests/collector/collector.go

@@ -56,7 +56,7 @@ func Build(params manifests.Params) ([]client.Object, error) {
 		if params.OtelCol.Spec.Mode == v1beta1.ModeSidecar {
 			manifestFactories = append(manifestFactories, manifests.Factory(PodMonitor))
 		} else {
-			manifestFactories = append(manifestFactories, manifests.Factory(ServiceMonitor))
+			manifestFactories = append(manifestFactories, manifests.Factory(ServiceMonitor), manifests.Factory(ServiceMonitorMonitoring))
 		}
 	}
 

internal/manifests/collector/podmonitor.go

@@ -31,28 +31,14 @@ import (
 
 // PodMonitor returns the pod monitor for the given instance.
 func PodMonitor(params manifests.Params) (*monitoringv1.PodMonitor, error) {
-	if !params.OtelCol.Spec.Observability.Metrics.EnableMetrics {
-		params.Log.V(2).Info("Metrics disabled for this OTEL Collector",
-			"params.OtelCol.name", params.OtelCol.Name,
-			"params.OtelCol.namespace", params.OtelCol.Namespace,
-		)
-		return nil, nil
-	} else if params.Config.PrometheusCRAvailability() == prometheus.NotAvailable {
-		params.Log.V(1).Info("Cannot enable PodMonitor when prometheus CRDs are unavailable",
-			"params.OtelCol.name", params.OtelCol.Name,
-			"params.OtelCol.namespace", params.OtelCol.Namespace,
-		)
+	if !shouldCreatePodMonitor(params) {
 		return nil, nil
 	}
-	var pm monitoringv1.PodMonitor
 
-	if params.OtelCol.Spec.Mode != v1beta1.ModeSidecar {
-		return nil, nil
-	}
 	name := naming.PodMonitor(params.OtelCol.Name)
 	labels := manifestutils.Labels(params.OtelCol.ObjectMeta, name, params.OtelCol.Spec.Image, ComponentOpenTelemetryCollector, nil)
 	selectorLabels := manifestutils.SelectorLabels(params.OtelCol.ObjectMeta, ComponentOpenTelemetryCollector)
-	pm = monitoringv1.PodMonitor{
+	pm := monitoringv1.PodMonitor{
 		ObjectMeta: metav1.ObjectMeta{
 			Namespace: params.OtelCol.Namespace,
 			Name:      name,
@@ -107,3 +93,22 @@ func metricsEndpointsFromConfig(logger logr.Logger, otelcol v1beta1.OpenTelemetr
 	}
 	return metricsEndpoints
 }
+
+func shouldCreatePodMonitor(params manifests.Params) bool {
+	l := params.Log.WithValues(
+		"params.OtelCol.name", params.OtelCol.Name,
+		"params.OtelCol.namespace", params.OtelCol.Namespace,
+	)
+
+	if !params.OtelCol.Spec.Observability.Metrics.EnableMetrics {
+		l.V(2).Info("Metrics disabled for this OTEL Collector. PodMonitor will not ve created")
+		return false
+	} else if params.Config.PrometheusCRAvailability() == prometheus.NotAvailable {
+		l.V(2).Info("Cannot enable PodMonitor when prometheus CRDs are unavailable")
+		return false
+	} else if params.OtelCol.Spec.Mode != v1beta1.ModeSidecar {
+		l.V(2).Info("Not using sidecar mode. PodMonitor will not be created")
+		return false
+	}
+	return true
+}

internal/manifests/collector/service.go

@@ -32,6 +32,7 @@ import (
 // headless and monitoring labels are to differentiate the headless/monitoring services from the clusterIP service.
 const (
 	headlessLabel   = "operator.opentelemetry.io/collector-headless-service"
+	headfulLabel    = "operator.opentelemetry.io/collector-service"
 	monitoringLabel = "operator.opentelemetry.io/collector-monitoring-service"
 	valueExists     = "Exists"
 )
@@ -44,6 +45,7 @@ func HeadlessService(params manifests.Params) (*corev1.Service, error) {
 
 	h.Name = naming.HeadlessService(params.OtelCol.Name)
 	h.Labels[headlessLabel] = valueExists
+	delete(h.Labels, headfulLabel)
 
 	// copy to avoid modifying params.OtelCol.Annotations
 	annotations := map[string]string{
@@ -90,6 +92,7 @@ func MonitoringService(params manifests.Params) (*corev1.Service, error) {
 func Service(params manifests.Params) (*corev1.Service, error) {
 	name := naming.Service(params.OtelCol.Name)
 	labels := manifestutils.Labels(params.OtelCol.ObjectMeta, name, params.OtelCol.Spec.Image, ComponentOpenTelemetryCollector, []string{})
+	labels[headfulLabel] = valueExists
 
 	out, err := params.OtelCol.Spec.Config.Yaml()
 	if err != nil {

internal/manifests/collector/service_test.go

@@ -286,6 +286,7 @@ func service(name string, ports []v1beta1.PortsSpec) v1.Service {
 func serviceWithInternalTrafficPolicy(name string, ports []v1beta1.PortsSpec, internalTrafficPolicy v1.ServiceInternalTrafficPolicyType) v1.Service {
 	params := deploymentParams()
 	labels := manifestutils.Labels(params.OtelCol.ObjectMeta, name, params.OtelCol.Spec.Image, ComponentOpenTelemetryCollector, []string{})
+	labels[headfulLabel] = valueExists
 
 	svcPorts := []v1.ServicePort{}
 	for _, p := range ports {

internal/manifests/collector/servicemonitor.go

@@ -15,6 +15,7 @@
 package collector
 
 import (
+	"fmt"
 	"strings"
 
 	"github.com/go-logr/logr"
@@ -29,30 +30,40 @@ import (
 	"github.com/open-telemetry/opentelemetry-operator/internal/naming"
 )
 
-// ServiceMonitor returns the service monitor for the given instance.
+// ServiceMonitor returns the service monitor for the collector.
 func ServiceMonitor(params manifests.Params) (*monitoringv1.ServiceMonitor, error) {
-	if !params.OtelCol.Spec.Observability.Metrics.EnableMetrics {
-		params.Log.V(2).Info("Metrics disabled for this OTEL Collector",
-			"params.OtelCol.name", params.OtelCol.Name,
-			"params.OtelCol.namespace", params.OtelCol.Namespace,
-		)
-		return nil, nil
-	} else if params.Config.PrometheusCRAvailability() == prometheus.NotAvailable {
-		params.Log.V(1).Info("Cannot enable ServiceMonitor when prometheus CRDs are unavailable",
-			"params.OtelCol.name", params.OtelCol.Name,
-			"params.OtelCol.namespace", params.OtelCol.Namespace,
-		)
-		return nil, nil
+	name := naming.ServiceMonitor(params.OtelCol.Name)
+	endpoints := endpointsFromConfig(params.Log, params.OtelCol)
+	if len(endpoints) > 0 {
+		return createServiceMonitor(name, params, headfulLabel, endpoints)
 	}
-	var sm monitoringv1.ServiceMonitor
+	return nil, nil
+}
+
+// ServiceMonitor returns the service monitor for the monitoring service of the collector.
+func ServiceMonitorMonitoring(params manifests.Params) (*monitoringv1.ServiceMonitor, error) {
+	name := naming.ServiceMonitor(fmt.Sprintf("%s-monitoring", params.OtelCol.Name))
+	endpoints := []monitoringv1.Endpoint{
+		{
+			Port: "monitoring",
+		},
+	}
+	return createServiceMonitor(name, params, monitoringLabel, endpoints)
+}
 
-	if params.OtelCol.Spec.Mode == v1beta1.ModeSidecar {
+// createServiceMonitor creates a Service Monitor using the provided name, the params from the instance, a label to identify the service
+// to target (like the monitoring or the collector services) and the endpoints to scrape.
+func createServiceMonitor(name string, params manifests.Params, label string, endpoints []monitoringv1.Endpoint) (*monitoringv1.ServiceMonitor, error) {
+	if !shouldCreateServiceMonitor(params) {
 		return nil, nil
 	}
-	name := naming.ServiceMonitor(params.OtelCol.Name)
+
+	var sm monitoringv1.ServiceMonitor
+
 	labels := manifestutils.Labels(params.OtelCol.ObjectMeta, name, params.OtelCol.Spec.Image, ComponentOpenTelemetryCollector, []string{})
 	selectorLabels := manifestutils.SelectorLabels(params.OtelCol.ObjectMeta, ComponentOpenTelemetryCollector)
-	selectorLabels[monitoringLabel] = valueExists
+	// This label is the one which differentiates the services
+	selectorLabels[label] = valueExists
 
 	sm = monitoringv1.ServiceMonitor{
 		ObjectMeta: metav1.ObjectMeta{
@@ -61,11 +72,7 @@ func ServiceMonitor(params manifests.Params) (*monitoringv1.ServiceMonitor, erro
 			Labels:    labels,
 		},
 		Spec: monitoringv1.ServiceMonitorSpec{
-			Endpoints: append([]monitoringv1.Endpoint{
-				{
-					Port: "monitoring",
-				},
-			}, endpointsFromConfig(params.Log, params.OtelCol)...),
+			Endpoints: endpoints,
 			NamespaceSelector: monitoringv1.NamespaceSelector{
 				MatchNames: []string{params.OtelCol.Namespace},
 			},
@@ -78,6 +85,25 @@ func ServiceMonitor(params manifests.Params) (*monitoringv1.ServiceMonitor, erro
 	return &sm, nil
 }
 
+func shouldCreateServiceMonitor(params manifests.Params) bool {
+	l := params.Log.WithValues(
+		"params.OtelCol.name", params.OtelCol.Name,
+		"params.OtelCol.namespace", params.OtelCol.Namespace,
+	)
+
+	if !params.OtelCol.Spec.Observability.Metrics.EnableMetrics {
+		l.V(2).Info("Metrics disabled for this OTEL Collector. ServiceMonitor will not ve created")
+		return false
+	} else if params.Config.PrometheusCRAvailability() == prometheus.NotAvailable {
+		l.V(2).Info("Cannot enable ServiceMonitor when prometheus CRDs are unavailable")
+		return false
+	} else if params.OtelCol.Spec.Mode == v1beta1.ModeSidecar {
+		l.V(2).Info("Using sidecar mode. ServiceMonitor will not be created")
+		return false
+	}
+	return true
+}
+
 func endpointsFromConfig(logger logr.Logger, otelcol v1beta1.OpenTelemetryCollector) []monitoringv1.Endpoint {
 	// TODO: https://github.com/open-telemetry/opentelemetry-operator/issues/2603
 	cfgStr, err := otelcol.Spec.Config.Yaml()

internal/manifests/collector/servicemonitor_test.go

@@ -34,18 +34,26 @@ func TestDesiredServiceMonitors(t *testing.T) {
 	params.OtelCol.Spec.Observability.Metrics.EnableMetrics = true
 	actual, err = ServiceMonitor(params)
 	assert.NoError(t, err)
+	assert.Nil(t, actual)
+
+	// Check the monitoring SM
+	actual, err = ServiceMonitorMonitoring(params)
+	assert.NoError(t, err)
 	assert.NotNil(t, actual)
-	assert.Equal(t, fmt.Sprintf("%s-collector", params.OtelCol.Name), actual.Name)
+	assert.Equal(t, fmt.Sprintf("%s-monitoring-collector", params.OtelCol.Name), actual.Name)
 	assert.Equal(t, params.OtelCol.Namespace, actual.Namespace)
 	assert.Equal(t, "monitoring", actual.Spec.Endpoints[0].Port)
-	expectedSelectorLabels := map[string]string{
+	expectedSelectorLabelsMonitor := map[string]string{
 		"app.kubernetes.io/component":                            "opentelemetry-collector",
 		"app.kubernetes.io/instance":                             "default.test",
 		"app.kubernetes.io/managed-by":                           "opentelemetry-operator",
 		"app.kubernetes.io/part-of":                              "opentelemetry",
 		"operator.opentelemetry.io/collector-monitoring-service": "Exists",
 	}
-	assert.Equal(t, expectedSelectorLabels, actual.Spec.Selector.MatchLabels)
+	assert.Equal(t, expectedSelectorLabelsMonitor, actual.Spec.Selector.MatchLabels)
+	assert.NotContains(t, "operator.opentelemetry.io/collector-headless-service", actual.Spec.Selector.MatchLabels)
+	assert.NotContains(t, "operator.opentelemetry.io/collector-service", actual.Spec.Selector.MatchLabels)
+
 }
 
 func TestDesiredServiceMonitorsWithPrometheus(t *testing.T) {
@@ -57,17 +65,18 @@ func TestDesiredServiceMonitorsWithPrometheus(t *testing.T) {
 	assert.NotNil(t, actual)
 	assert.Equal(t, fmt.Sprintf("%s-collector", params.OtelCol.Name), actual.Name)
 	assert.Equal(t, params.OtelCol.Namespace, actual.Namespace)
-	assert.Equal(t, "monitoring", actual.Spec.Endpoints[0].Port)
-	assert.Equal(t, "prometheus-dev", actual.Spec.Endpoints[1].Port)
-	assert.Equal(t, "prometheus-prod", actual.Spec.Endpoints[2].Port)
+	assert.Equal(t, "prometheus-dev", actual.Spec.Endpoints[0].Port)
+	assert.Equal(t, "prometheus-prod", actual.Spec.Endpoints[1].Port)
 	expectedSelectorLabels := map[string]string{
-		"app.kubernetes.io/component":                            "opentelemetry-collector",
-		"app.kubernetes.io/instance":                             "default.test",
-		"app.kubernetes.io/managed-by":                           "opentelemetry-operator",
-		"app.kubernetes.io/part-of":                              "opentelemetry",
-		"operator.opentelemetry.io/collector-monitoring-service": "Exists",
+		"app.kubernetes.io/component":                 "opentelemetry-collector",
+		"app.kubernetes.io/instance":                  "default.test",
+		"app.kubernetes.io/managed-by":                "opentelemetry-operator",
+		"app.kubernetes.io/part-of":                   "opentelemetry",
+		"operator.opentelemetry.io/collector-service": "Exists",
 	}
 	assert.Equal(t, expectedSelectorLabels, actual.Spec.Selector.MatchLabels)
+	assert.NotContains(t, "operator.opentelemetry.io/collector-headless-service", actual.Spec.Selector.MatchLabels)
+	assert.NotContains(t, "operator.opentelemetry.io/collector-monitoring-service", actual.Spec.Selector.MatchLabels)
 }
 
 func TestDesiredServiceMonitorsPrometheusNotAvailable(t *testing.T) {

tests/e2e-openshift/monitoring/01-assert.yaml

@@ -15,22 +15,21 @@ status:
   availableReplicas: 1
   readyReplicas: 1
   replicas: 1
-
 ---
 apiVersion: monitoring.coreos.com/v1
 kind: ServiceMonitor
 metadata:
   labels:
     app.kubernetes.io/managed-by: opentelemetry-operator
-    app.kubernetes.io/name: cluster-collector-collector
-  name: cluster-collector-collector
+    app.kubernetes.io/name: cluster-collector-monitoring-collector
+  name: cluster-collector-monitoring-collector
 spec:
   endpoints:
   - port: monitoring
   selector:
     matchLabels:
       app.kubernetes.io/managed-by: opentelemetry-operator
-
+      operator.opentelemetry.io/collector-monitoring-service: Exists
 ---
 apiVersion: v1
 kind: Service
@@ -87,7 +86,6 @@ spec:
     app.kubernetes.io/managed-by: opentelemetry-operator
     app.kubernetes.io/part-of: opentelemetry
   type: ClusterIP
-
 ---
 apiVersion: v1
 kind: Service

tests/e2e-openshift/monitoring/04-assert.yaml

@@ -0,0 +1,14 @@
+apiVersion: monitoring.coreos.com/v1
+kind: ServiceMonitor
+metadata:
+  labels:
+    app.kubernetes.io/managed-by: opentelemetry-operator
+    app.kubernetes.io/name: cluster-collector2-collector
+  name: cluster-collector2-collector
+spec:
+  endpoints:
+  - port: prometheus
+  selector:
+    matchLabels:
+      app.kubernetes.io/managed-by: opentelemetry-operator
+      operator.opentelemetry.io/collector-service: Exists