Keep multiple versions of Collector Config (#2946)

open-telemetry · May 28, 2024 · 06e00c6 · 06e00c6
1 parent 542e67d
commit 06e00c6
Show file tree

Hide file tree

Showing 29 changed files with 317 additions and 38 deletions.
diff --git a/.chloggen/matth.versioned_config.yaml b/.chloggen/matth.versioned_config.yaml
@@ -0,0 +1,17 @@
+# One of 'breaking', 'deprecation', 'new_component', 'enhancement', 'bug_fix'
+change_type: enhancement
+
+# 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: Keep multiple previous versions of the Collector ConfigMap, configurable via the ConfigVersions field.
+
+# One or more tracking issues related to the change
+issues: [2871]
+
+# (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: |
+  This change introduces a new field in the Collector ConfigMap, `ConfigVersions`, which allows users to specify the number of previous versions of the Collector ConfigMap to keep. The default value is 1, which means that the current and one previous version of the Collector ConfigMap are kept. By keeping historical versions of the configuration, we ensure that during a config upgrade the previous configuration is still available for running (non-upgraded) pods as well as for rollbacks. If we overwrite the original ConfigMap with the new configuration, any pod which restarts for any reason will get the new configuration, which makes rollouts impossible to control.
diff --git a/apis/v1beta1/opentelemetrycollector_types.go b/apis/v1beta1/opentelemetrycollector_types.go
@@ -94,6 +94,12 @@ type OpenTelemetryCollectorSpec struct {
 	// +required
 	// +kubebuilder:pruning:PreserveUnknownFields
 	Config Config `json:"config"`
+	// ConfigVersions defines the number versions to keep for the collector config. Each config version is stored in a separate ConfigMap.
+	// Defaults to 3. The minimum value is 1.
+	// +optional
+	// +kubebuilder:default:=3
+	// +kubebuilder:validation:Minimum:=1
+	ConfigVersions int `json:"configVersions,omitempty"`
 	// Ingress is used to specify how OpenTelemetry Collector is exposed. This
 	// functionality is only available if one of the valid modes is set.
 	// Valid modes are: deployment, daemonset and statefulset.

diff --git a/bundle/manifests/opentelemetry.io_opentelemetrycollectors.yaml b/bundle/manifests/opentelemetry.io_opentelemetrycollectors.yaml
@@ -5555,6 +5555,10 @@ spec:
                 - service
                 type: object
                 x-kubernetes-preserve-unknown-fields: true
+              configVersions:
+                default: 3
+                minimum: 1
+                type: integer
               configmaps:
                 items:
                   properties:

diff --git a/config/crd/bases/opentelemetry.io_opentelemetrycollectors.yaml b/config/crd/bases/opentelemetry.io_opentelemetrycollectors.yaml
@@ -5541,6 +5541,10 @@ spec:
                 - service
                 type: object
                 x-kubernetes-preserve-unknown-fields: true
+              configVersions:
+                default: 3
+                minimum: 1
+                type: integer
               configmaps:
                 items:
                   properties:

diff --git a/controllers/builder_test.go b/controllers/builder_test.go
@@ -37,6 +37,7 @@ import (
 	"github.com/open-telemetry/opentelemetry-operator/internal/config"
 	"github.com/open-telemetry/opentelemetry-operator/internal/manifests"
 	"github.com/open-telemetry/opentelemetry-operator/internal/manifests/collector"
+	"github.com/open-telemetry/opentelemetry-operator/internal/manifests/manifestutils"
 	"github.com/open-telemetry/opentelemetry-operator/pkg/featuregate"
 )
 
@@ -86,6 +87,10 @@ service:
 	goodConfig := v1beta1.Config{}
 	err := go_yaml.Unmarshal([]byte(goodConfigYaml), &goodConfig)
 	require.NoError(t, err)
+
+	goodConfigHash, _ := manifestutils.GetConfigMapSHA(goodConfig)
+	goodConfigHash = goodConfigHash[:8]
+
 	one := int32(1)
 	type args struct {
 		instance v1beta1.OpenTelemetryCollector
@@ -164,7 +169,7 @@ service:
 										VolumeSource: corev1.VolumeSource{
 											ConfigMap: &corev1.ConfigMapVolumeSource{
 												LocalObjectReference: corev1.LocalObjectReference{
-													Name: "test-collector",
+													Name: "test-collector-" + goodConfigHash,
 												},
 												Items: []corev1.KeyToPath{
 													{
@@ -223,7 +228,7 @@ service:
 				},
 				&corev1.ConfigMap{
 					ObjectMeta: metav1.ObjectMeta{
-						Name:      "test-collector",
+						Name:      "test-collector-" + goodConfigHash,
 						Namespace: "test",
 						Labels: map[string]string{
 							"app.kubernetes.io/component":  "opentelemetry-collector",
@@ -414,7 +419,7 @@ service:
 										VolumeSource: corev1.VolumeSource{
 											ConfigMap: &corev1.ConfigMapVolumeSource{
 												LocalObjectReference: corev1.LocalObjectReference{
-													Name: "test-collector",
+													Name: "test-collector-" + goodConfigHash,
 												},
 												Items: []corev1.KeyToPath{
 													{
@@ -473,7 +478,7 @@ service:
 				},
 				&corev1.ConfigMap{
 					ObjectMeta: metav1.ObjectMeta{
-						Name:      "test-collector",
+						Name:      "test-collector-" + goodConfigHash,
 						Namespace: "test",
 						Labels: map[string]string{
 							"app.kubernetes.io/component":  "opentelemetry-collector",
@@ -700,7 +705,7 @@ service:
 										VolumeSource: corev1.VolumeSource{
 											ConfigMap: &corev1.ConfigMapVolumeSource{
 												LocalObjectReference: corev1.LocalObjectReference{
-													Name: "test-collector",
+													Name: "test-collector-" + goodConfigHash,
 												},
 												Items: []corev1.KeyToPath{
 													{
@@ -759,7 +764,7 @@ service:
 				},
 				&corev1.ConfigMap{
 					ObjectMeta: metav1.ObjectMeta{
-						Name:      "test-collector",
+						Name:      "test-collector-" + goodConfigHash,
 						Namespace: "test",
 						Labels: map[string]string{
 							"app.kubernetes.io/component":  "opentelemetry-collector",
@@ -1138,6 +1143,10 @@ service:
 	goodConfig := v1beta1.Config{}
 	err := go_yaml.Unmarshal([]byte(goodConfigYaml), &goodConfig)
 	require.NoError(t, err)
+
+	goodConfigHash, _ := manifestutils.GetConfigMapSHA(goodConfig)
+	goodConfigHash = goodConfigHash[:8]
+
 	one := int32(1)
 	type args struct {
 		instance v1beta1.OpenTelemetryCollector
@@ -1225,7 +1234,7 @@ service:
 										VolumeSource: corev1.VolumeSource{
 											ConfigMap: &corev1.ConfigMapVolumeSource{
 												LocalObjectReference: corev1.LocalObjectReference{
-													Name: "test-collector",
+													Name: "test-collector-" + goodConfigHash,
 												},
 												Items: []corev1.KeyToPath{
 													{
@@ -1284,7 +1293,7 @@ service:
 				},
 				&corev1.ConfigMap{
 					ObjectMeta: metav1.ObjectMeta{
-						Name:      "test-collector",
+						Name:      "test-collector-" + goodConfigHash,
 						Namespace: "test",
 						Labels: map[string]string{
 							"app.kubernetes.io/component":  "opentelemetry-collector",
@@ -1620,7 +1629,7 @@ prometheus_cr:
 										VolumeSource: corev1.VolumeSource{
 											ConfigMap: &corev1.ConfigMapVolumeSource{
 												LocalObjectReference: corev1.LocalObjectReference{
-													Name: "test-collector",
+													Name: "test-collector-" + goodConfigHash,
 												},
 												Items: []corev1.KeyToPath{
 													{
@@ -1679,7 +1688,7 @@ prometheus_cr:
 				},
 				&corev1.ConfigMap{
 					ObjectMeta: metav1.ObjectMeta{
-						Name:      "test-collector",
+						Name:      "test-collector-" + goodConfigHash,
 						Namespace: "test",
 						Labels: map[string]string{
 							"app.kubernetes.io/component":  "opentelemetry-collector",

diff --git a/controllers/opentelemetrycollector_controller.go b/controllers/opentelemetrycollector_controller.go
@@ -17,6 +17,8 @@ package controllers
 
 import (
 	"context"
+	"fmt"
+	"sort"
 
 	"github.com/go-logr/logr"
 	routev1 "github.com/openshift/api/route/v1"
@@ -111,6 +113,17 @@ func (r *OpenTelemetryCollectorReconciler) findOtelOwnedObjects(ctx context.Cont
 			ownedObjects[uid] = object
 		}
 	}
+
+	configMapList := &corev1.ConfigMapList{}
+	err := r.List(ctx, configMapList, listOps)
+	if err != nil {
+		return nil, fmt.Errorf("error listing ConfigMaps: %w", err)
+	}
+	ownedConfigMaps := r.getConfigMapsToRemove(params.OtelCol.Spec.ConfigVersions, configMapList)
+	for i := range ownedConfigMaps {
+		ownedObjects[ownedConfigMaps[i].GetUID()] = &ownedConfigMaps[i]
+	}
+
 	return ownedObjects, nil
 }
 
@@ -134,6 +147,27 @@ func (r *OpenTelemetryCollectorReconciler) findClusterRoleObjects(ctx context.Co
 	return ownedObjects, nil
 }
 
+// getConfigMapsToRemove returns a list of ConfigMaps to remove based on the number of ConfigMaps to keep.
+// It keeps the newest ConfigMap, the `configVersionsToKeep` next newest ConfigMaps, and returns the remainder.
+func (r *OpenTelemetryCollectorReconciler) getConfigMapsToRemove(configVersionsToKeep int, configMapList *corev1.ConfigMapList) []corev1.ConfigMap {
+	configVersionsToKeep = max(1, configVersionsToKeep)
+	ownedConfigMaps := []corev1.ConfigMap{}
+	sort.Slice(configMapList.Items, func(i, j int) bool {
+		iTime := configMapList.Items[i].GetCreationTimestamp().Time
+		jTime := configMapList.Items[j].GetCreationTimestamp().Time
+		// sort the ConfigMaps newest to oldest
+		return iTime.After(jTime)
+	})
+
+	for i := range configMapList.Items {
+		if i > configVersionsToKeep {
+			ownedConfigMaps = append(ownedConfigMaps, configMapList.Items[i])
+		}
+	}
+
+	return ownedConfigMaps
+}
+
 func (r *OpenTelemetryCollectorReconciler) getParams(instance v1beta1.OpenTelemetryCollector) (manifests.Params, error) {
 	p := manifests.Params{
 		Config:   r.config,

diff --git a/controllers/reconcile_test.go b/controllers/reconcile_test.go
@@ -430,7 +430,9 @@ func TestOpenTelemetryCollectorReconciler_Reconcile(t *testing.T) {
 					result: controllerruntime.Result{},
 					checks: []check[v1alpha1.OpenTelemetryCollector]{
 						func(t *testing.T, params v1alpha1.OpenTelemetryCollector) {
-							exists, err := populateObjectIfExists(t, &v1.ConfigMap{}, namespacedObjectName(naming.Collector(params.Name), params.Namespace))
+							configHash, _ := getConfigMapSHAFromString(params.Spec.Config)
+							configHash = configHash[:8]
+							exists, err := populateObjectIfExists(t, &v1.ConfigMap{}, namespacedObjectName(naming.ConfigMap(params.Name, configHash), params.Namespace))
 							assert.NoError(t, err)
 							assert.True(t, exists)
 							exists, err = populateObjectIfExists(t, &appsv1.StatefulSet{}, namespacedObjectName(naming.Collector(params.Name), params.Namespace))
@@ -452,7 +454,9 @@ func TestOpenTelemetryCollectorReconciler_Reconcile(t *testing.T) {
 					result: controllerruntime.Result{},
 					checks: []check[v1alpha1.OpenTelemetryCollector]{
 						func(t *testing.T, params v1alpha1.OpenTelemetryCollector) {
-							exists, err := populateObjectIfExists(t, &v1.ConfigMap{}, namespacedObjectName(naming.Collector(params.Name), params.Namespace))
+							configHash, _ := getConfigMapSHAFromString(params.Spec.Config)
+							configHash = configHash[:8]
+							exists, err := populateObjectIfExists(t, &v1.ConfigMap{}, namespacedObjectName(naming.ConfigMap(params.Name, configHash), params.Namespace))
 							assert.NoError(t, err)
 							assert.True(t, exists)
 							actual := v1.ConfigMap{}
@@ -497,7 +501,9 @@ func TestOpenTelemetryCollectorReconciler_Reconcile(t *testing.T) {
 					result: controllerruntime.Result{},
 					checks: []check[v1alpha1.OpenTelemetryCollector]{
 						func(t *testing.T, params v1alpha1.OpenTelemetryCollector) {
-							exists, err := populateObjectIfExists(t, &v1.ConfigMap{}, namespacedObjectName(naming.Collector(params.Name), params.Namespace))
+							configHash, _ := getConfigMapSHAFromString(params.Spec.Config)
+							configHash = configHash[:8]
+							exists, err := populateObjectIfExists(t, &v1.ConfigMap{}, namespacedObjectName(naming.ConfigMap(params.Name, configHash), params.Namespace))
 							assert.NoError(t, err)
 							assert.True(t, exists)
 							actual := v1.ConfigMap{}

diff --git a/controllers/suite_test.go b/controllers/suite_test.go
@@ -50,6 +50,7 @@ import (
 	logf "sigs.k8s.io/controller-runtime/pkg/log"
 	metricsserver "sigs.k8s.io/controller-runtime/pkg/metrics/server"
 	"sigs.k8s.io/controller-runtime/pkg/webhook"
+	"sigs.k8s.io/yaml"
 
 	"github.com/open-telemetry/opentelemetry-operator/apis/v1alpha1"
 	"github.com/open-telemetry/opentelemetry-operator/apis/v1beta1"
@@ -60,6 +61,7 @@ import (
 	"github.com/open-telemetry/opentelemetry-operator/internal/config"
 	"github.com/open-telemetry/opentelemetry-operator/internal/manifests"
 	"github.com/open-telemetry/opentelemetry-operator/internal/manifests/collector/testdata"
+	"github.com/open-telemetry/opentelemetry-operator/internal/manifests/manifestutils"
 	"github.com/open-telemetry/opentelemetry-operator/internal/rbac"
 	// +kubebuilder:scaffold:imports
 )
@@ -480,3 +482,12 @@ func populateObjectIfExists(t testing.TB, object client.Object, namespacedName t
 	}
 	return true, nil
 }
+
+func getConfigMapSHAFromString(configStr string) (string, error) {
+	var config v1beta1.Config
+	err := yaml.Unmarshal([]byte(configStr), &config)
+	if err != nil {
+		return "", err
+	}
+	return manifestutils.GetConfigMapSHA(config)
+}
diff --git a/docs/api.md b/docs/api.md
@@ -29872,6 +29872,17 @@ doing so, you wil accept the risk of it breaking things.<br/>
 for the workload.<br/>
         </td>
         <td>false</td>
+      </tr><tr>
+        <td><b>configVersions</b></td>
+        <td>integer</td>
+        <td>
+          ConfigVersions defines the number versions to keep for the collector config. Each config version is stored in a separate ConfigMap.
+Defaults to 3. The minimum value is 1.<br/>
+          <br/>
+            <i>Default</i>: 3<br/>
+            <i>Minimum</i>: 1<br/>
+        </td>
+        <td>false</td>
       </tr><tr>
         <td><b><a href="#opentelemetrycollectorspecconfigmapsindex-1">configmaps</a></b></td>
         <td>[]object</td>

diff --git a/internal/manifests/collector/configmap.go b/internal/manifests/collector/configmap.go
@@ -24,8 +24,13 @@ import (
 )
 
 func ConfigMap(params manifests.Params) (*corev1.ConfigMap, error) {
-	name := naming.ConfigMap(params.OtelCol.Name)
-	labels := manifestutils.Labels(params.OtelCol.ObjectMeta, name, params.OtelCol.Spec.Image, ComponentOpenTelemetryCollector, []string{})
+	hash, err := manifestutils.GetConfigMapSHA(params.OtelCol.Spec.Config)
+	if err != nil {
+		return nil, err
+	}
+	name := naming.ConfigMap(params.OtelCol.Name, hash)
+	collectorName := naming.Collector(params.OtelCol.Name)
+	labels := manifestutils.Labels(params.OtelCol.ObjectMeta, collectorName, params.OtelCol.Spec.Image, ComponentOpenTelemetryCollector, []string{})
 
 	replacedConf, err := ReplaceConfig(params.OtelCol)
 	if err != nil {

diff --git a/internal/manifests/collector/configmap_test.go b/internal/manifests/collector/configmap_test.go
@@ -18,6 +18,9 @@ import (
 	"testing"
 
 	"github.com/stretchr/testify/assert"
+
+	"github.com/open-telemetry/opentelemetry-operator/internal/manifests/manifestutils"
+	"github.com/open-telemetry/opentelemetry-operator/internal/naming"
 )
 
 func TestDesiredConfigMap(t *testing.T) {
@@ -29,9 +32,6 @@ func TestDesiredConfigMap(t *testing.T) {
 	}
 
 	t.Run("should return expected collector config map", func(t *testing.T) {
-		expectedLables["app.kubernetes.io/component"] = "opentelemetry-collector"
-		expectedLables["app.kubernetes.io/name"] = "test-collector"
-		expectedLables["app.kubernetes.io/version"] = "0.47.0"
 
 		expectedData := map[string]string{
 			"collector.yaml": `receivers:
@@ -58,10 +58,17 @@ service:
 		}
 
 		param := deploymentParams()
+		hash, _ := manifestutils.GetConfigMapSHA(param.OtelCol.Spec.Config)
+		expectedName := naming.ConfigMap("test", hash)
+
+		expectedLables["app.kubernetes.io/component"] = "opentelemetry-collector"
+		expectedLables["app.kubernetes.io/name"] = "test-collector"
+		expectedLables["app.kubernetes.io/version"] = "0.47.0"
+
 		actual, err := ConfigMap(param)
 
 		assert.NoError(t, err)
-		assert.Equal(t, "test-collector", actual.Name)
+		assert.Equal(t, expectedName, actual.Name)
 		assert.Equal(t, expectedLables, actual.Labels)
 		assert.Equal(t, len(expectedData), len(actual.Data))
 		for k, expected := range expectedData {
@@ -70,10 +77,6 @@ service:
 	})
 
 	t.Run("should return expected escaped collector config map with target_allocator config block", func(t *testing.T) {
-		expectedLables["app.kubernetes.io/component"] = "opentelemetry-collector"
-		expectedLables["app.kubernetes.io/name"] = "test-collector"
-		expectedLables["app.kubernetes.io/version"] = "latest"
-
 		expectedData := map[string]string{
 			"collector.yaml": `exporters:
   debug:
@@ -97,11 +100,19 @@ service:
 
 		param, err := newParams("test/test-img", "testdata/http_sd_config_servicemonitor_test.yaml")
 		assert.NoError(t, err)
+
+		hash, _ := manifestutils.GetConfigMapSHA(param.OtelCol.Spec.Config)
+		expectedName := naming.ConfigMap("test", hash)
+
+		expectedLables["app.kubernetes.io/component"] = "opentelemetry-collector"
+		expectedLables["app.kubernetes.io/name"] = "test-collector"
+		expectedLables["app.kubernetes.io/version"] = "latest"
+
 		param.OtelCol.Spec.TargetAllocator.Enabled = true
 		actual, err := ConfigMap(param)
 
 		assert.NoError(t, err)
-		assert.Equal(t, "test-collector", actual.Name)
+		assert.Equal(t, expectedName, actual.Name)
 		assert.Equal(t, expectedLables, actual.Labels)
 		assert.Equal(t, len(expectedData), len(actual.Data))
 		for k, expected := range expectedData {