From fd492fcaa5daf0ab80b9bd25220bea6397abbef2 Mon Sep 17 00:00:00 2001 From: Matt Hagenbuch Date: Wed, 1 May 2024 12:05:56 -0400 Subject: [PATCH 01/17] append hash to configmap name --- apis/v1beta1/opentelemetrycollector_types.go | 4 ++++ apis/v1beta1/zz_generated.deepcopy.go | 5 +++++ .../manifests/opentelemetry.io_opentelemetrycollectors.yaml | 3 +++ .../crd/bases/opentelemetry.io_opentelemetrycollectors.yaml | 3 +++ config/manager/kustomization.yaml | 6 ++++++ internal/manifests/collector/configmap.go | 6 +++++- internal/manifests/collector/volume.go | 5 ++++- internal/manifests/manifestutils/annotations.go | 6 +++--- internal/naming/main.go | 5 +++-- 9 files changed, 36 insertions(+), 7 deletions(-) diff --git a/apis/v1beta1/opentelemetrycollector_types.go b/apis/v1beta1/opentelemetrycollector_types.go index 4421165944..349b802f56 100644 --- a/apis/v1beta1/opentelemetrycollector_types.go +++ b/apis/v1beta1/opentelemetrycollector_types.go @@ -94,6 +94,10 @@ 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 + ConfigVersions *int32 `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/apis/v1beta1/zz_generated.deepcopy.go b/apis/v1beta1/zz_generated.deepcopy.go index c5e574127c..0902c1c910 100644 --- a/apis/v1beta1/zz_generated.deepcopy.go +++ b/apis/v1beta1/zz_generated.deepcopy.go @@ -300,6 +300,11 @@ func (in *OpenTelemetryCollectorSpec) DeepCopyInto(out *OpenTelemetryCollectorSp in.OpenTelemetryCommonFields.DeepCopyInto(&out.OpenTelemetryCommonFields) in.TargetAllocator.DeepCopyInto(&out.TargetAllocator) in.Config.DeepCopyInto(&out.Config) + if in.ConfigVersions != nil { + in, out := &in.ConfigVersions, &out.ConfigVersions + *out = new(int32) + **out = **in + } in.Ingress.DeepCopyInto(&out.Ingress) if in.LivenessProbe != nil { in, out := &in.LivenessProbe, &out.LivenessProbe diff --git a/bundle/manifests/opentelemetry.io_opentelemetrycollectors.yaml b/bundle/manifests/opentelemetry.io_opentelemetrycollectors.yaml index 5688544892..938f63344a 100644 --- a/bundle/manifests/opentelemetry.io_opentelemetrycollectors.yaml +++ b/bundle/manifests/opentelemetry.io_opentelemetrycollectors.yaml @@ -5555,6 +5555,9 @@ spec: - service type: object x-kubernetes-preserve-unknown-fields: true + configVersions: + format: int32 + type: integer configmaps: items: properties: diff --git a/config/crd/bases/opentelemetry.io_opentelemetrycollectors.yaml b/config/crd/bases/opentelemetry.io_opentelemetrycollectors.yaml index 03a1360bfa..74dd3bec52 100644 --- a/config/crd/bases/opentelemetry.io_opentelemetrycollectors.yaml +++ b/config/crd/bases/opentelemetry.io_opentelemetrycollectors.yaml @@ -5541,6 +5541,9 @@ spec: - service type: object x-kubernetes-preserve-unknown-fields: true + configVersions: + format: int32 + type: integer configmaps: items: properties: diff --git a/config/manager/kustomization.yaml b/config/manager/kustomization.yaml index 5c5f0b84cb..9396d7d702 100644 --- a/config/manager/kustomization.yaml +++ b/config/manager/kustomization.yaml @@ -1,2 +1,8 @@ resources: - manager.yaml +apiVersion: kustomize.config.k8s.io/v1beta1 +kind: Kustomization +images: +- name: controller + newName: localhost:5001/opentelemetry-operator + newTag: matth-dev-1714578767 diff --git a/internal/manifests/collector/configmap.go b/internal/manifests/collector/configmap.go index 58ee4c1312..137232683d 100644 --- a/internal/manifests/collector/configmap.go +++ b/internal/manifests/collector/configmap.go @@ -24,7 +24,11 @@ import ( ) func ConfigMap(params manifests.Params) (*corev1.ConfigMap, error) { - name := naming.ConfigMap(params.OtelCol.Name) + hash, err := manifestutils.GetConfigMapSHA(params.OtelCol.Spec.Config) + if err != nil { + return nil, err + } + name := naming.ConfigMap(params.OtelCol.Name, hash) labels := manifestutils.Labels(params.OtelCol.ObjectMeta, name, params.OtelCol.Spec.Image, ComponentOpenTelemetryCollector, []string{}) replacedConf, err := ReplaceConfig(params.OtelCol) diff --git a/internal/manifests/collector/volume.go b/internal/manifests/collector/volume.go index 5cf82fde7a..ea033b3a4a 100644 --- a/internal/manifests/collector/volume.go +++ b/internal/manifests/collector/volume.go @@ -20,16 +20,19 @@ import ( "github.com/open-telemetry/opentelemetry-operator/apis/v1beta1" "github.com/open-telemetry/opentelemetry-operator/internal/config" + "github.com/open-telemetry/opentelemetry-operator/internal/manifests/manifestutils" "github.com/open-telemetry/opentelemetry-operator/internal/naming" ) // Volumes builds the volumes for the given instance, including the config map volume. func Volumes(cfg config.Config, otelcol v1beta1.OpenTelemetryCollector) []corev1.Volume { + hash, _ := manifestutils.GetConfigMapSHA(otelcol.Spec.Config) + configMapName := naming.ConfigMap(otelcol.Name, hash) volumes := []corev1.Volume{{ Name: naming.ConfigMapVolume(), VolumeSource: corev1.VolumeSource{ ConfigMap: &corev1.ConfigMapVolumeSource{ - LocalObjectReference: corev1.LocalObjectReference{Name: naming.ConfigMap(otelcol.Name)}, + LocalObjectReference: corev1.LocalObjectReference{Name: configMapName}, Items: []corev1.KeyToPath{{ Key: cfg.CollectorConfigMapEntry(), Path: cfg.CollectorConfigMapEntry(), diff --git a/internal/manifests/manifestutils/annotations.go b/internal/manifests/manifestutils/annotations.go index 06f1baed81..c4243e350d 100644 --- a/internal/manifests/manifestutils/annotations.go +++ b/internal/manifests/manifestutils/annotations.go @@ -44,7 +44,7 @@ func Annotations(instance v1beta1.OpenTelemetryCollector, filterAnnotations []st } } - hash, err := getConfigMapSHA(instance.Spec.Config) + hash, err := GetConfigMapSHA(instance.Spec.Config) if err != nil { return nil, err } @@ -78,7 +78,7 @@ func PodAnnotations(instance v1beta1.OpenTelemetryCollector, filterAnnotations [ } } - hash, err := getConfigMapSHA(instance.Spec.Config) + hash, err := GetConfigMapSHA(instance.Spec.Config) if err != nil { return nil, err } @@ -88,7 +88,7 @@ func PodAnnotations(instance v1beta1.OpenTelemetryCollector, filterAnnotations [ return podAnnotations, nil } -func getConfigMapSHA(config v1beta1.Config) (string, error) { +func GetConfigMapSHA(config v1beta1.Config) (string, error) { b, err := json.Marshal(&config) if err != nil { return "", err diff --git a/internal/naming/main.go b/internal/naming/main.go index 8b801ce75e..ddcc6b1458 100644 --- a/internal/naming/main.go +++ b/internal/naming/main.go @@ -16,8 +16,9 @@ package naming // ConfigMap builds the name for the config map used in the OpenTelemetryCollector containers. -func ConfigMap(otelcol string) string { - return DNSName(Truncate("%s-collector", 63, otelcol)) +// The configHash should be calculated using manifestutils.GetConfigMapSHA +func ConfigMap(otelcol, configHash string) string { + return DNSName(Truncate("%s-collector-%s", 63, otelcol, configHash[:8])) } // TAConfigMap returns the name for the config map used in the TargetAllocator. From dfbbc3b25a2c696831255255220392d6a3833878 Mon Sep 17 00:00:00 2001 From: Matt Hagenbuch Date: Thu, 2 May 2024 11:11:04 -0400 Subject: [PATCH 02/17] fix tests --- ...ntelemetry.io_opentelemetrycollectors.yaml | 1 + config/manager/kustomization.yaml | 2 +- controllers/builder_test.go | 39 ++++++++++++------- .../opentelemetrycollector_controller.go | 25 ++++++++++++ controllers/reconcile_test.go | 12 ++++-- controllers/suite_test.go | 11 ++++++ .../manifests/collector/configmap_test.go | 29 +++++++++----- 7 files changed, 91 insertions(+), 28 deletions(-) diff --git a/bundle/manifests/opentelemetry.io_opentelemetrycollectors.yaml b/bundle/manifests/opentelemetry.io_opentelemetrycollectors.yaml index 938f63344a..5d02664177 100644 --- a/bundle/manifests/opentelemetry.io_opentelemetrycollectors.yaml +++ b/bundle/manifests/opentelemetry.io_opentelemetrycollectors.yaml @@ -5558,6 +5558,7 @@ spec: configVersions: format: int32 type: integer + default: 3 configmaps: items: properties: diff --git a/config/manager/kustomization.yaml b/config/manager/kustomization.yaml index 9396d7d702..79a1399047 100644 --- a/config/manager/kustomization.yaml +++ b/config/manager/kustomization.yaml @@ -5,4 +5,4 @@ kind: Kustomization images: - name: controller newName: localhost:5001/opentelemetry-operator - newTag: matth-dev-1714578767 + newTag: matth-dev-1714590016 diff --git a/controllers/builder_test.go b/controllers/builder_test.go index 5cf49c9cde..2f612e2108 100644 --- 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,13 +228,13 @@ 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", "app.kubernetes.io/instance": "test.test", "app.kubernetes.io/managed-by": "opentelemetry-operator", - "app.kubernetes.io/name": "test-collector", + "app.kubernetes.io/name": "test-collector-" + goodConfigHash, "app.kubernetes.io/part-of": "opentelemetry", "app.kubernetes.io/version": "latest", }, @@ -411,7 +416,7 @@ service: VolumeSource: corev1.VolumeSource{ ConfigMap: &corev1.ConfigMapVolumeSource{ LocalObjectReference: corev1.LocalObjectReference{ - Name: "test-collector", + Name: "test-collector-" + goodConfigHash, }, Items: []corev1.KeyToPath{ { @@ -470,13 +475,13 @@ 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", "app.kubernetes.io/instance": "test.test", "app.kubernetes.io/managed-by": "opentelemetry-operator", - "app.kubernetes.io/name": "test-collector", + "app.kubernetes.io/name": "test-collector-" + goodConfigHash, "app.kubernetes.io/part-of": "opentelemetry", "app.kubernetes.io/version": "latest", }, @@ -694,7 +699,7 @@ service: VolumeSource: corev1.VolumeSource{ ConfigMap: &corev1.ConfigMapVolumeSource{ LocalObjectReference: corev1.LocalObjectReference{ - Name: "test-collector", + Name: "test-collector-" + goodConfigHash, }, Items: []corev1.KeyToPath{ { @@ -753,13 +758,13 @@ 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", "app.kubernetes.io/instance": "test.test", "app.kubernetes.io/managed-by": "opentelemetry-operator", - "app.kubernetes.io/name": "test-collector", + "app.kubernetes.io/name": "test-collector-" + goodConfigHash, "app.kubernetes.io/part-of": "opentelemetry", "app.kubernetes.io/version": "latest", }, @@ -1129,6 +1134,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 @@ -1216,7 +1225,7 @@ service: VolumeSource: corev1.VolumeSource{ ConfigMap: &corev1.ConfigMapVolumeSource{ LocalObjectReference: corev1.LocalObjectReference{ - Name: "test-collector", + Name: "test-collector-" + goodConfigHash, }, Items: []corev1.KeyToPath{ { @@ -1275,13 +1284,13 @@ 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", "app.kubernetes.io/instance": "test.test", "app.kubernetes.io/managed-by": "opentelemetry-operator", - "app.kubernetes.io/name": "test-collector", + "app.kubernetes.io/name": "test-collector-" + goodConfigHash, "app.kubernetes.io/part-of": "opentelemetry", "app.kubernetes.io/version": "latest", }, @@ -1610,7 +1619,7 @@ prometheus_cr: VolumeSource: corev1.VolumeSource{ ConfigMap: &corev1.ConfigMapVolumeSource{ LocalObjectReference: corev1.LocalObjectReference{ - Name: "test-collector", + Name: "test-collector-" + goodConfigHash, }, Items: []corev1.KeyToPath{ { @@ -1669,13 +1678,13 @@ 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", "app.kubernetes.io/instance": "test.test", "app.kubernetes.io/managed-by": "opentelemetry-operator", - "app.kubernetes.io/name": "test-collector", + "app.kubernetes.io/name": "test-collector-" + goodConfigHash, "app.kubernetes.io/part-of": "opentelemetry", "app.kubernetes.io/version": "latest", }, diff --git a/controllers/opentelemetrycollector_controller.go b/controllers/opentelemetrycollector_controller.go index 258c5fadfc..e309a1f943 100644 --- a/controllers/opentelemetrycollector_controller.go +++ b/controllers/opentelemetrycollector_controller.go @@ -18,6 +18,7 @@ package controllers import ( "context" "fmt" + "sort" "github.com/go-logr/logr" routev1 "github.com/openshift/api/route/v1" @@ -127,6 +128,30 @@ func (r *OpenTelemetryCollectorReconciler) findOtelOwnedObjects(ctx context.Cont ownedObjects[pdbList.Items[i].GetUID()] = &pdbList.Items[i] } + configMapList := &corev1.ConfigMapList{} + err = r.List(ctx, configMapList, listOps) + if err != nil { + return nil, fmt.Errorf("error listing ConfigMaps: %w", err) + } + + // only return collector ConfigMaps older than spec.ConfigVersions + 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) + }) + configVersionsToKeep := 3 + if params.OtelCol.Spec.ConfigVersions != nil { + configVersionsToKeep = int(*params.OtelCol.Spec.ConfigVersions) + } + + for i := range configMapList.Items { + if i > configVersionsToKeep { + ownedObjects[configMapList.Items[i].GetUID()] = &configMapList.Items[i] + } + } + return ownedObjects, nil } diff --git a/controllers/reconcile_test.go b/controllers/reconcile_test.go index 8dd272e77f..421a9fd3f1 100644 --- a/controllers/reconcile_test.go +++ b/controllers/reconcile_test.go @@ -428,7 +428,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)) @@ -450,7 +452,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{} @@ -495,7 +499,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 index d20e7be8b2..7ce7eb0ddd 100644 --- 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" @@ -59,6 +60,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 ) @@ -471,3 +473,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/internal/manifests/collector/configmap_test.go b/internal/manifests/collector/configmap_test.go index b850084c53..62bc989771 100644 --- a/internal/manifests/collector/configmap_test.go +++ b/internal/manifests/collector/configmap_test.go @@ -17,6 +17,7 @@ package collector import ( "testing" + "github.com/open-telemetry/opentelemetry-operator/internal/manifests/manifestutils" "github.com/stretchr/testify/assert" ) @@ -29,9 +30,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 +56,18 @@ service: } param := deploymentParams() + hash, _ := manifestutils.GetConfigMapSHA(param.OtelCol.Spec.Config) + hash = hash[:8] + expectedName := "test-collector-" + hash + + expectedLables["app.kubernetes.io/component"] = "opentelemetry-collector" + expectedLables["app.kubernetes.io/name"] = expectedName + 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 +76,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 +99,20 @@ 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) + hash = hash[:8] + expectedName := "test-collector-" + hash + + expectedLables["app.kubernetes.io/component"] = "opentelemetry-collector" + expectedLables["app.kubernetes.io/name"] = expectedName + 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 { From 6095564b24c9da2b8c826d8fbb895e185a1a4f8a Mon Sep 17 00:00:00 2001 From: Matt Hagenbuch Date: Tue, 7 May 2024 10:54:15 -0400 Subject: [PATCH 03/17] use naming --- internal/manifests/collector/configmap_test.go | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/internal/manifests/collector/configmap_test.go b/internal/manifests/collector/configmap_test.go index 62bc989771..2555666bce 100644 --- a/internal/manifests/collector/configmap_test.go +++ b/internal/manifests/collector/configmap_test.go @@ -18,6 +18,7 @@ import ( "testing" "github.com/open-telemetry/opentelemetry-operator/internal/manifests/manifestutils" + "github.com/open-telemetry/opentelemetry-operator/internal/naming" "github.com/stretchr/testify/assert" ) @@ -57,8 +58,7 @@ service: param := deploymentParams() hash, _ := manifestutils.GetConfigMapSHA(param.OtelCol.Spec.Config) - hash = hash[:8] - expectedName := "test-collector-" + hash + expectedName := naming.ConfigMap("test", hash) expectedLables["app.kubernetes.io/component"] = "opentelemetry-collector" expectedLables["app.kubernetes.io/name"] = expectedName @@ -101,8 +101,7 @@ service: assert.NoError(t, err) hash, _ := manifestutils.GetConfigMapSHA(param.OtelCol.Spec.Config) - hash = hash[:8] - expectedName := "test-collector-" + hash + expectedName := naming.ConfigMap("test", hash) expectedLables["app.kubernetes.io/component"] = "opentelemetry-collector" expectedLables["app.kubernetes.io/name"] = expectedName From 5fa002557e2c71cf2219af2eb5d5bd4677038321 Mon Sep 17 00:00:00 2001 From: Matt Hagenbuch Date: Fri, 10 May 2024 09:41:56 -0400 Subject: [PATCH 04/17] e2e tests --- ...ntelemetry.io_opentelemetrycollectors.yaml | 1 - config/manager/kustomization.yaml | 6 ---- tests/e2e/managed-reconcile/02-assert.yaml | 2 +- tests/e2e/multiple-configmaps/00-assert.yaml | 2 +- .../e2e/smoke-targetallocator/00-assert.yaml | 2 +- tests/e2e/statefulset-features/00-assert.yaml | 2 +- tests/e2e/statefulset-features/01-assert.yaml | 2 +- tests/e2e/versioned-configmaps/00-assert.yaml | 28 ++++++++++++++++ .../e2e/versioned-configmaps/00-install.yaml | 27 +++++++++++++++ tests/e2e/versioned-configmaps/01-assert.yaml | 33 +++++++++++++++++++ tests/e2e/versioned-configmaps/01-update.yaml | 27 +++++++++++++++ tests/e2e/versioned-configmaps/02-error.yaml | 5 +++ tests/e2e/versioned-configmaps/02-update.yaml | 27 +++++++++++++++ .../versioned-configmaps/chainsaw-test.yaml | 28 ++++++++++++++++ 14 files changed, 180 insertions(+), 12 deletions(-) create mode 100644 tests/e2e/versioned-configmaps/00-assert.yaml create mode 100644 tests/e2e/versioned-configmaps/00-install.yaml create mode 100644 tests/e2e/versioned-configmaps/01-assert.yaml create mode 100644 tests/e2e/versioned-configmaps/01-update.yaml create mode 100644 tests/e2e/versioned-configmaps/02-error.yaml create mode 100644 tests/e2e/versioned-configmaps/02-update.yaml create mode 100755 tests/e2e/versioned-configmaps/chainsaw-test.yaml diff --git a/bundle/manifests/opentelemetry.io_opentelemetrycollectors.yaml b/bundle/manifests/opentelemetry.io_opentelemetrycollectors.yaml index 5d02664177..938f63344a 100644 --- a/bundle/manifests/opentelemetry.io_opentelemetrycollectors.yaml +++ b/bundle/manifests/opentelemetry.io_opentelemetrycollectors.yaml @@ -5558,7 +5558,6 @@ spec: configVersions: format: int32 type: integer - default: 3 configmaps: items: properties: diff --git a/config/manager/kustomization.yaml b/config/manager/kustomization.yaml index 79a1399047..5c5f0b84cb 100644 --- a/config/manager/kustomization.yaml +++ b/config/manager/kustomization.yaml @@ -1,8 +1,2 @@ resources: - manager.yaml -apiVersion: kustomize.config.k8s.io/v1beta1 -kind: Kustomization -images: -- name: controller - newName: localhost:5001/opentelemetry-operator - newTag: matth-dev-1714590016 diff --git a/tests/e2e/managed-reconcile/02-assert.yaml b/tests/e2e/managed-reconcile/02-assert.yaml index 0a6a83b1a3..0847c94ed8 100644 --- a/tests/e2e/managed-reconcile/02-assert.yaml +++ b/tests/e2e/managed-reconcile/02-assert.yaml @@ -52,7 +52,7 @@ spec: apiVersion: v1 kind: ConfigMap metadata: - name: simplest-collector + name: simplest-collector-17ca6c13 data: collector.yaml: | receivers: diff --git a/tests/e2e/multiple-configmaps/00-assert.yaml b/tests/e2e/multiple-configmaps/00-assert.yaml index 14c929470a..b040f87074 100644 --- a/tests/e2e/multiple-configmaps/00-assert.yaml +++ b/tests/e2e/multiple-configmaps/00-assert.yaml @@ -25,7 +25,7 @@ spec: volumes: - name: otc-internal configMap: - name: simplest-with-configmaps-collector + name: simplest-with-configmaps-collector-17ca6c13 items: - key: collector.yaml path: collector.yaml diff --git a/tests/e2e/smoke-targetallocator/00-assert.yaml b/tests/e2e/smoke-targetallocator/00-assert.yaml index 35a1d6356f..53d8bc5a89 100644 --- a/tests/e2e/smoke-targetallocator/00-assert.yaml +++ b/tests/e2e/smoke-targetallocator/00-assert.yaml @@ -45,4 +45,4 @@ data: - jaeger kind: ConfigMap metadata: - name: stateful-collector + name: stateful-collector-fb278632 diff --git a/tests/e2e/statefulset-features/00-assert.yaml b/tests/e2e/statefulset-features/00-assert.yaml index 744ba76e26..5363d785b9 100644 --- a/tests/e2e/statefulset-features/00-assert.yaml +++ b/tests/e2e/statefulset-features/00-assert.yaml @@ -20,7 +20,7 @@ spec: items: - key: collector.yaml path: collector.yaml - name: stateful-collector + name: stateful-collector-f0fa6faa name: otc-internal - emptyDir: {} name: testvolume diff --git a/tests/e2e/statefulset-features/01-assert.yaml b/tests/e2e/statefulset-features/01-assert.yaml index fdccb0fadd..c68dc9aa80 100644 --- a/tests/e2e/statefulset-features/01-assert.yaml +++ b/tests/e2e/statefulset-features/01-assert.yaml @@ -20,7 +20,7 @@ spec: items: - key: collector.yaml path: collector.yaml - name: stateful-collector + name: stateful-collector-f0fa6faa name: otc-internal - emptyDir: {} name: testvolume diff --git a/tests/e2e/versioned-configmaps/00-assert.yaml b/tests/e2e/versioned-configmaps/00-assert.yaml new file mode 100644 index 0000000000..a363c65a68 --- /dev/null +++ b/tests/e2e/versioned-configmaps/00-assert.yaml @@ -0,0 +1,28 @@ +--- +apiVersion: apps/v1 +kind: Deployment +metadata: + name: simple-collector +spec: + template: + spec: + volumes: + - name: otc-internal + configMap: + name: simple-collector-d6f40475 + items: + - key: collector.yaml + path: collector.yaml + defaultMode: 420 + containers: + - name: otc-container + volumeMounts: + - name: otc-internal + mountPath: /conf +status: + readyReplicas: 1 +--- +apiVersion: v1 +kind: ConfigMap +metadata: + name: simple-collector-d6f40475 diff --git a/tests/e2e/versioned-configmaps/00-install.yaml b/tests/e2e/versioned-configmaps/00-install.yaml new file mode 100644 index 0000000000..a34135e7f0 --- /dev/null +++ b/tests/e2e/versioned-configmaps/00-install.yaml @@ -0,0 +1,27 @@ +--- +apiVersion: opentelemetry.io/v1beta1 +kind: OpenTelemetryCollector +metadata: + name: simple +spec: + mode: "deployment" + configVersions: 1 + config: + receivers: + otlp: + protocols: + grpc: {} + http: {} + processors: + batch: + send_batch_size: 10000 + timeout: 10s + exporters: + debug: {} + + service: + pipelines: + traces: + receivers: [otlp] + processors: [batch] + exporters: [debug] diff --git a/tests/e2e/versioned-configmaps/01-assert.yaml b/tests/e2e/versioned-configmaps/01-assert.yaml new file mode 100644 index 0000000000..cfbb27c31c --- /dev/null +++ b/tests/e2e/versioned-configmaps/01-assert.yaml @@ -0,0 +1,33 @@ +--- +apiVersion: apps/v1 +kind: Deployment +metadata: + name: simple-collector +spec: + template: + spec: + volumes: + - name: otc-internal + configMap: + name: simple-collector-8cd615bf + items: + - key: collector.yaml + path: collector.yaml + defaultMode: 420 + containers: + - name: otc-container + volumeMounts: + - name: otc-internal + mountPath: /conf +status: + readyReplicas: 1 +--- +apiVersion: v1 +kind: ConfigMap +metadata: + name: simple-collector-d6f40475 +--- +apiVersion: v1 +kind: ConfigMap +metadata: + name: simple-collector-8cd615bf diff --git a/tests/e2e/versioned-configmaps/01-update.yaml b/tests/e2e/versioned-configmaps/01-update.yaml new file mode 100644 index 0000000000..8004c4ef55 --- /dev/null +++ b/tests/e2e/versioned-configmaps/01-update.yaml @@ -0,0 +1,27 @@ +--- +apiVersion: opentelemetry.io/v1beta1 +kind: OpenTelemetryCollector +metadata: + name: simple +spec: + mode: "deployment" + configVersions: 1 + config: + receivers: + otlp: + protocols: + grpc: {} + http: {} + processors: + batch: + send_batch_size: 10000 + timeout: 20s + exporters: + debug: {} + + service: + pipelines: + traces: + receivers: [otlp] + processors: [batch] + exporters: [debug] diff --git a/tests/e2e/versioned-configmaps/02-error.yaml b/tests/e2e/versioned-configmaps/02-error.yaml new file mode 100644 index 0000000000..2b63829a6c --- /dev/null +++ b/tests/e2e/versioned-configmaps/02-error.yaml @@ -0,0 +1,5 @@ +--- +apiVersion: v1 +kind: ConfigMap +metadata: + name: simple-collector-d6f40475 diff --git a/tests/e2e/versioned-configmaps/02-update.yaml b/tests/e2e/versioned-configmaps/02-update.yaml new file mode 100644 index 0000000000..7cb8f19060 --- /dev/null +++ b/tests/e2e/versioned-configmaps/02-update.yaml @@ -0,0 +1,27 @@ +--- +apiVersion: opentelemetry.io/v1beta1 +kind: OpenTelemetryCollector +metadata: + name: simple +spec: + mode: "deployment" + configVersions: 1 + config: + receivers: + otlp: + protocols: + grpc: {} + http: {} + processors: + batch: + send_batch_size: 10000 + timeout: 30s + exporters: + debug: {} + + service: + pipelines: + traces: + receivers: [otlp] + processors: [batch] + exporters: [debug] \ No newline at end of file diff --git a/tests/e2e/versioned-configmaps/chainsaw-test.yaml b/tests/e2e/versioned-configmaps/chainsaw-test.yaml new file mode 100755 index 0000000000..f837498100 --- /dev/null +++ b/tests/e2e/versioned-configmaps/chainsaw-test.yaml @@ -0,0 +1,28 @@ +# yaml-language-server: $schema=https://raw.githubusercontent.com/kyverno/chainsaw/main/.schemas/json/test-chainsaw-v1alpha1.json +apiVersion: chainsaw.kyverno.io/v1alpha1 +kind: Test +metadata: + creationTimestamp: null + name: versioned-configmaps +spec: + steps: + - name: step-00 + try: + - apply: + file: 00-install.yaml + - assert: + file: 00-assert.yaml + # Update the collector config and ensure both the new and old confmaps are present + - name: step-01 + try: + - apply: + file: 01-update.yaml + - assert: + file: 01-assert.yaml + # Update the collector config again and ensure the oldest confmap is deleted + - name: step-02 + try: + - apply: + file: 02-update.yaml + - error: + file: 02-error.yaml \ No newline at end of file From a111e27ee815bc3f33383c2800743d8b5e3f7528 Mon Sep 17 00:00:00 2001 From: Matt Hagenbuch Date: Fri, 10 May 2024 10:22:34 -0400 Subject: [PATCH 05/17] default, min, assert less --- .../bases/opentelemetry.io_opentelemetrycollectors.yaml | 1 + controllers/opentelemetrycollector_controller.go | 2 +- tests/e2e/versioned-configmaps/00-assert.yaml | 9 --------- tests/e2e/versioned-configmaps/01-assert.yaml | 9 --------- 4 files changed, 2 insertions(+), 19 deletions(-) diff --git a/config/crd/bases/opentelemetry.io_opentelemetrycollectors.yaml b/config/crd/bases/opentelemetry.io_opentelemetrycollectors.yaml index 74dd3bec52..e5694015c4 100644 --- a/config/crd/bases/opentelemetry.io_opentelemetrycollectors.yaml +++ b/config/crd/bases/opentelemetry.io_opentelemetrycollectors.yaml @@ -5544,6 +5544,7 @@ spec: configVersions: format: int32 type: integer + default: 3 configmaps: items: properties: diff --git a/controllers/opentelemetrycollector_controller.go b/controllers/opentelemetrycollector_controller.go index e309a1f943..f568d70191 100644 --- a/controllers/opentelemetrycollector_controller.go +++ b/controllers/opentelemetrycollector_controller.go @@ -143,7 +143,7 @@ func (r *OpenTelemetryCollectorReconciler) findOtelOwnedObjects(ctx context.Cont }) configVersionsToKeep := 3 if params.OtelCol.Spec.ConfigVersions != nil { - configVersionsToKeep = int(*params.OtelCol.Spec.ConfigVersions) + configVersionsToKeep = max(1, int(*params.OtelCol.Spec.ConfigVersions)) } for i := range configMapList.Items { diff --git a/tests/e2e/versioned-configmaps/00-assert.yaml b/tests/e2e/versioned-configmaps/00-assert.yaml index a363c65a68..09b5d13d9e 100644 --- a/tests/e2e/versioned-configmaps/00-assert.yaml +++ b/tests/e2e/versioned-configmaps/00-assert.yaml @@ -10,15 +10,6 @@ spec: - name: otc-internal configMap: name: simple-collector-d6f40475 - items: - - key: collector.yaml - path: collector.yaml - defaultMode: 420 - containers: - - name: otc-container - volumeMounts: - - name: otc-internal - mountPath: /conf status: readyReplicas: 1 --- diff --git a/tests/e2e/versioned-configmaps/01-assert.yaml b/tests/e2e/versioned-configmaps/01-assert.yaml index cfbb27c31c..b9cb6d35d9 100644 --- a/tests/e2e/versioned-configmaps/01-assert.yaml +++ b/tests/e2e/versioned-configmaps/01-assert.yaml @@ -10,15 +10,6 @@ spec: - name: otc-internal configMap: name: simple-collector-8cd615bf - items: - - key: collector.yaml - path: collector.yaml - defaultMode: 420 - containers: - - name: otc-container - volumeMounts: - - name: otc-internal - mountPath: /conf status: readyReplicas: 1 --- From c06ab534d2261ca874822c05cb26e5a9df013671 Mon Sep 17 00:00:00 2001 From: Matt Hagenbuch Date: Fri, 10 May 2024 10:51:35 -0400 Subject: [PATCH 06/17] defaults --- apis/v1beta1/opentelemetrycollector_types.go | 1 + bundle/manifests/opentelemetry.io_opentelemetrycollectors.yaml | 1 + config/crd/bases/opentelemetry.io_opentelemetrycollectors.yaml | 2 +- 3 files changed, 3 insertions(+), 1 deletion(-) diff --git a/apis/v1beta1/opentelemetrycollector_types.go b/apis/v1beta1/opentelemetrycollector_types.go index 349b802f56..3b603868f6 100644 --- a/apis/v1beta1/opentelemetrycollector_types.go +++ b/apis/v1beta1/opentelemetrycollector_types.go @@ -97,6 +97,7 @@ type OpenTelemetryCollectorSpec struct { // 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 ConfigVersions *int32 `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. diff --git a/bundle/manifests/opentelemetry.io_opentelemetrycollectors.yaml b/bundle/manifests/opentelemetry.io_opentelemetrycollectors.yaml index 938f63344a..5d02664177 100644 --- a/bundle/manifests/opentelemetry.io_opentelemetrycollectors.yaml +++ b/bundle/manifests/opentelemetry.io_opentelemetrycollectors.yaml @@ -5558,6 +5558,7 @@ spec: configVersions: format: int32 type: integer + default: 3 configmaps: items: properties: diff --git a/config/crd/bases/opentelemetry.io_opentelemetrycollectors.yaml b/config/crd/bases/opentelemetry.io_opentelemetrycollectors.yaml index e5694015c4..a55210bfd8 100644 --- a/config/crd/bases/opentelemetry.io_opentelemetrycollectors.yaml +++ b/config/crd/bases/opentelemetry.io_opentelemetrycollectors.yaml @@ -5542,9 +5542,9 @@ spec: type: object x-kubernetes-preserve-unknown-fields: true configVersions: + default: 3 format: int32 type: integer - default: 3 configmaps: items: properties: From b25e45f58845849cb3a2f8f004d8a328f8d32953 Mon Sep 17 00:00:00 2001 From: Matt Hagenbuch Date: Fri, 10 May 2024 11:25:04 -0400 Subject: [PATCH 07/17] Update apis/v1beta1/opentelemetrycollector_types.go --- apis/v1beta1/opentelemetrycollector_types.go | 1 + 1 file changed, 1 insertion(+) diff --git a/apis/v1beta1/opentelemetrycollector_types.go b/apis/v1beta1/opentelemetrycollector_types.go index 3b603868f6..7e14e3167c 100644 --- a/apis/v1beta1/opentelemetrycollector_types.go +++ b/apis/v1beta1/opentelemetrycollector_types.go @@ -98,6 +98,7 @@ type OpenTelemetryCollectorSpec struct { // Defaults to 3. The minimum value is 1. // +optional // +kubebuilder:default:=3 + // +kubebuilder:validation:Minimum:=1 ConfigVersions *int32 `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. From 461925e2f75e311159d52b01014d417ccbadffd5 Mon Sep 17 00:00:00 2001 From: Matt Hagenbuch Date: Fri, 10 May 2024 11:45:16 -0400 Subject: [PATCH 08/17] no pointer, separate func --- apis/v1beta1/opentelemetrycollector_types.go | 2 +- apis/v1beta1/zz_generated.deepcopy.go | 5 ----- ...ntelemetry.io_opentelemetrycollectors.yaml | 2 +- .../opentelemetrycollector_controller.go | 20 ++++++++++++------- 4 files changed, 15 insertions(+), 14 deletions(-) diff --git a/apis/v1beta1/opentelemetrycollector_types.go b/apis/v1beta1/opentelemetrycollector_types.go index 7e14e3167c..8046c743ca 100644 --- a/apis/v1beta1/opentelemetrycollector_types.go +++ b/apis/v1beta1/opentelemetrycollector_types.go @@ -99,7 +99,7 @@ type OpenTelemetryCollectorSpec struct { // +optional // +kubebuilder:default:=3 // +kubebuilder:validation:Minimum:=1 - ConfigVersions *int32 `json:"configVersions,omitempty"` + 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/apis/v1beta1/zz_generated.deepcopy.go b/apis/v1beta1/zz_generated.deepcopy.go index 0902c1c910..c5e574127c 100644 --- a/apis/v1beta1/zz_generated.deepcopy.go +++ b/apis/v1beta1/zz_generated.deepcopy.go @@ -300,11 +300,6 @@ func (in *OpenTelemetryCollectorSpec) DeepCopyInto(out *OpenTelemetryCollectorSp in.OpenTelemetryCommonFields.DeepCopyInto(&out.OpenTelemetryCommonFields) in.TargetAllocator.DeepCopyInto(&out.TargetAllocator) in.Config.DeepCopyInto(&out.Config) - if in.ConfigVersions != nil { - in, out := &in.ConfigVersions, &out.ConfigVersions - *out = new(int32) - **out = **in - } in.Ingress.DeepCopyInto(&out.Ingress) if in.LivenessProbe != nil { in, out := &in.LivenessProbe, &out.LivenessProbe diff --git a/config/crd/bases/opentelemetry.io_opentelemetrycollectors.yaml b/config/crd/bases/opentelemetry.io_opentelemetrycollectors.yaml index a55210bfd8..ab7f14f557 100644 --- a/config/crd/bases/opentelemetry.io_opentelemetrycollectors.yaml +++ b/config/crd/bases/opentelemetry.io_opentelemetrycollectors.yaml @@ -5543,7 +5543,7 @@ spec: x-kubernetes-preserve-unknown-fields: true configVersions: default: 3 - format: int32 + minimum: 1 type: integer configmaps: items: diff --git a/controllers/opentelemetrycollector_controller.go b/controllers/opentelemetrycollector_controller.go index c9eac8ffba..eaf46f2cf0 100644 --- a/controllers/opentelemetrycollector_controller.go +++ b/controllers/opentelemetrycollector_controller.go @@ -134,26 +134,32 @@ func (r *OpenTelemetryCollectorReconciler) findOtelOwnedObjects(ctx context.Cont if err != nil { return nil, fmt.Errorf("error listing ConfigMaps: %w", err) } + ownedConfigMaps := r.getConfigMapsToReconcile(params.OtelCol.Spec.ConfigVersions, configMapList) + for i := range ownedConfigMaps { + ownedObjects[ownedConfigMaps[i].GetUID()] = &ownedConfigMaps[i] + } + + return ownedObjects, nil +} - // only return collector ConfigMaps older than spec.ConfigVersions +// getConfigMapsToReconcile returns a list of ConfigMaps to reconcile 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) getConfigMapsToReconcile(configVersionsToKeep int, configMapList *corev1.ConfigMapList) []corev1.ConfigMap { + 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) }) - configVersionsToKeep := 3 - if params.OtelCol.Spec.ConfigVersions != nil { - configVersionsToKeep = max(1, int(*params.OtelCol.Spec.ConfigVersions)) - } for i := range configMapList.Items { if i > configVersionsToKeep { - ownedObjects[configMapList.Items[i].GetUID()] = &configMapList.Items[i] + ownedConfigMaps = append(ownedConfigMaps, configMapList.Items[i]) } } - return ownedObjects, nil + return ownedConfigMaps } func (r *OpenTelemetryCollectorReconciler) getParams(instance v1beta1.OpenTelemetryCollector) (manifests.Params, error) { From 84ffa49104e1076c6a1f14ebae71eeae8d07454a Mon Sep 17 00:00:00 2001 From: Matt Hagenbuch Date: Fri, 10 May 2024 15:05:35 -0400 Subject: [PATCH 09/17] rename func, name label --- controllers/builder_test.go | 10 +++++----- controllers/opentelemetrycollector_controller.go | 6 +++--- internal/manifests/collector/configmap.go | 3 ++- internal/manifests/collector/configmap_test.go | 4 ++-- 4 files changed, 12 insertions(+), 11 deletions(-) diff --git a/controllers/builder_test.go b/controllers/builder_test.go index 2f612e2108..209f739c29 100644 --- a/controllers/builder_test.go +++ b/controllers/builder_test.go @@ -234,7 +234,7 @@ service: "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-" + goodConfigHash, + "app.kubernetes.io/name": "test-collector", "app.kubernetes.io/part-of": "opentelemetry", "app.kubernetes.io/version": "latest", }, @@ -481,7 +481,7 @@ service: "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-" + goodConfigHash, + "app.kubernetes.io/name": "test-collector", "app.kubernetes.io/part-of": "opentelemetry", "app.kubernetes.io/version": "latest", }, @@ -764,7 +764,7 @@ service: "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-" + goodConfigHash, + "app.kubernetes.io/name": "test-collector", "app.kubernetes.io/part-of": "opentelemetry", "app.kubernetes.io/version": "latest", }, @@ -1290,7 +1290,7 @@ service: "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-" + goodConfigHash, + "app.kubernetes.io/name": "test-collector", "app.kubernetes.io/part-of": "opentelemetry", "app.kubernetes.io/version": "latest", }, @@ -1684,7 +1684,7 @@ prometheus_cr: "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-" + goodConfigHash, + "app.kubernetes.io/name": "test-collector", "app.kubernetes.io/part-of": "opentelemetry", "app.kubernetes.io/version": "latest", }, diff --git a/controllers/opentelemetrycollector_controller.go b/controllers/opentelemetrycollector_controller.go index eaf46f2cf0..ca232e3558 100644 --- a/controllers/opentelemetrycollector_controller.go +++ b/controllers/opentelemetrycollector_controller.go @@ -134,7 +134,7 @@ func (r *OpenTelemetryCollectorReconciler) findOtelOwnedObjects(ctx context.Cont if err != nil { return nil, fmt.Errorf("error listing ConfigMaps: %w", err) } - ownedConfigMaps := r.getConfigMapsToReconcile(params.OtelCol.Spec.ConfigVersions, configMapList) + ownedConfigMaps := r.getConfigMapsToRemove(params.OtelCol.Spec.ConfigVersions, configMapList) for i := range ownedConfigMaps { ownedObjects[ownedConfigMaps[i].GetUID()] = &ownedConfigMaps[i] } @@ -142,9 +142,9 @@ func (r *OpenTelemetryCollectorReconciler) findOtelOwnedObjects(ctx context.Cont return ownedObjects, nil } -// getConfigMapsToReconcile returns a list of ConfigMaps to reconcile based on the number of ConfigMaps to keep. +// 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) getConfigMapsToReconcile(configVersionsToKeep int, configMapList *corev1.ConfigMapList) []corev1.ConfigMap { +func (r *OpenTelemetryCollectorReconciler) getConfigMapsToRemove(configVersionsToKeep int, configMapList *corev1.ConfigMapList) []corev1.ConfigMap { ownedConfigMaps := []corev1.ConfigMap{} sort.Slice(configMapList.Items, func(i, j int) bool { iTime := configMapList.Items[i].GetCreationTimestamp().Time diff --git a/internal/manifests/collector/configmap.go b/internal/manifests/collector/configmap.go index 137232683d..74f90f28d5 100644 --- a/internal/manifests/collector/configmap.go +++ b/internal/manifests/collector/configmap.go @@ -29,7 +29,8 @@ func ConfigMap(params manifests.Params) (*corev1.ConfigMap, error) { return nil, err } name := naming.ConfigMap(params.OtelCol.Name, hash) - labels := manifestutils.Labels(params.OtelCol.ObjectMeta, name, params.OtelCol.Spec.Image, ComponentOpenTelemetryCollector, []string{}) + 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 index 2555666bce..d1c2f91001 100644 --- a/internal/manifests/collector/configmap_test.go +++ b/internal/manifests/collector/configmap_test.go @@ -61,7 +61,7 @@ service: expectedName := naming.ConfigMap("test", hash) expectedLables["app.kubernetes.io/component"] = "opentelemetry-collector" - expectedLables["app.kubernetes.io/name"] = expectedName + expectedLables["app.kubernetes.io/name"] = "test-collector" expectedLables["app.kubernetes.io/version"] = "0.47.0" actual, err := ConfigMap(param) @@ -104,7 +104,7 @@ service: expectedName := naming.ConfigMap("test", hash) expectedLables["app.kubernetes.io/component"] = "opentelemetry-collector" - expectedLables["app.kubernetes.io/name"] = expectedName + expectedLables["app.kubernetes.io/name"] = "test-collector" expectedLables["app.kubernetes.io/version"] = "latest" param.OtelCol.Spec.TargetAllocator.Enabled = true From d970c6e2c1c56a1051695a1819bec97c1e7d0182 Mon Sep 17 00:00:00 2001 From: Matt Hagenbuch Date: Fri, 10 May 2024 15:18:39 -0400 Subject: [PATCH 10/17] chlog --- .chloggen/matth.versioned_config.yaml | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) create mode 100755 .chloggen/matth.versioned_config.yaml diff --git a/.chloggen/matth.versioned_config.yaml b/.chloggen/matth.versioned_config.yaml new file mode 100755 index 0000000000..8f1e8cd3ee --- /dev/null +++ b/.chloggen/matth.versioned_config.yaml @@ -0,0 +1,16 @@ +# 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: From 00e1ae0f7bc37f621a4509000631bd81a4bb2d0e Mon Sep 17 00:00:00 2001 From: Matt Hagenbuch Date: Fri, 10 May 2024 15:25:36 -0400 Subject: [PATCH 11/17] minimum --- controllers/opentelemetrycollector_controller.go | 1 + 1 file changed, 1 insertion(+) diff --git a/controllers/opentelemetrycollector_controller.go b/controllers/opentelemetrycollector_controller.go index ca232e3558..b74aef4ec6 100644 --- a/controllers/opentelemetrycollector_controller.go +++ b/controllers/opentelemetrycollector_controller.go @@ -145,6 +145,7 @@ func (r *OpenTelemetryCollectorReconciler) findOtelOwnedObjects(ctx context.Cont // 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 From 7dbf8af4cf35f609836bfc9b797f1808e76a8a12 Mon Sep 17 00:00:00 2001 From: Matt Hagenbuch Date: Fri, 10 May 2024 20:24:00 -0400 Subject: [PATCH 12/17] bundle --- .../opentelemetry.io_opentelemetrycollectors.yaml | 4 ++-- docs/api.md | 11 +++++++++++ 2 files changed, 13 insertions(+), 2 deletions(-) diff --git a/bundle/manifests/opentelemetry.io_opentelemetrycollectors.yaml b/bundle/manifests/opentelemetry.io_opentelemetrycollectors.yaml index 5d02664177..a9676edca8 100644 --- a/bundle/manifests/opentelemetry.io_opentelemetrycollectors.yaml +++ b/bundle/manifests/opentelemetry.io_opentelemetrycollectors.yaml @@ -5556,9 +5556,9 @@ spec: type: object x-kubernetes-preserve-unknown-fields: true configVersions: - format: int32 - type: integer default: 3 + minimum: 1 + type: integer configmaps: items: properties: diff --git a/docs/api.md b/docs/api.md index ab95401ad4..e0f88fbdb4 100644 --- a/docs/api.md +++ b/docs/api.md @@ -29872,6 +29872,17 @@ doing so, you wil accept the risk of it breaking things.
for the workload.
false + + configVersions + integer + + 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.
+
+ Default: 3
+ Minimum: 1
+ + false configmaps []object From 5787dd606706bf1dd730fc0597ace94615db97ef Mon Sep 17 00:00:00 2001 From: Matt Hagenbuch Date: Mon, 20 May 2024 09:09:45 -0400 Subject: [PATCH 13/17] add subtext and group imports --- .chloggen/matth.versioned_config.yaml | 3 ++- internal/manifests/collector/configmap_test.go | 1 + 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/.chloggen/matth.versioned_config.yaml b/.chloggen/matth.versioned_config.yaml index 8f1e8cd3ee..b49551e923 100755 --- a/.chloggen/matth.versioned_config.yaml +++ b/.chloggen/matth.versioned_config.yaml @@ -13,4 +13,5 @@ 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: +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. \ No newline at end of file diff --git a/internal/manifests/collector/configmap_test.go b/internal/manifests/collector/configmap_test.go index d1c2f91001..8581b5ab20 100644 --- a/internal/manifests/collector/configmap_test.go +++ b/internal/manifests/collector/configmap_test.go @@ -19,6 +19,7 @@ import ( "github.com/open-telemetry/opentelemetry-operator/internal/manifests/manifestutils" "github.com/open-telemetry/opentelemetry-operator/internal/naming" + "github.com/stretchr/testify/assert" ) From 3eaab87894602dfadd24bc7c3c29b6aaa0a04682 Mon Sep 17 00:00:00 2001 From: Matt Hagenbuch Date: Tue, 21 May 2024 08:54:20 -0400 Subject: [PATCH 14/17] add configmap hash to e2e test --- .../targetallocator-prometheuscr/00-assert.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/e2e-targetallocator/targetallocator-prometheuscr/00-assert.yaml b/tests/e2e-targetallocator/targetallocator-prometheuscr/00-assert.yaml index 66a10f2f1d..7593240e16 100644 --- a/tests/e2e-targetallocator/targetallocator-prometheuscr/00-assert.yaml +++ b/tests/e2e-targetallocator/targetallocator-prometheuscr/00-assert.yaml @@ -1,7 +1,7 @@ apiVersion: apps/v1 kind: StatefulSet metadata: - name: prometheus-cr-collector + name: prometheus-cr-collector-b88fa6e7 status: readyReplicas: 1 replicas: 1 From d415fef252ab414c376981085b5d57c8c03dbc50 Mon Sep 17 00:00:00 2001 From: Matt Hagenbuch Date: Tue, 21 May 2024 10:05:37 -0400 Subject: [PATCH 15/17] fix bad merge --- controllers/opentelemetrycollector_controller.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/controllers/opentelemetrycollector_controller.go b/controllers/opentelemetrycollector_controller.go index 977ede0272..b005728199 100644 --- a/controllers/opentelemetrycollector_controller.go +++ b/controllers/opentelemetrycollector_controller.go @@ -113,9 +113,9 @@ func (r *OpenTelemetryCollectorReconciler) findOtelOwnedObjects(ctx context.Cont ownedObjects[uid] = object } } - + configMapList := &corev1.ConfigMapList{} - err = r.List(ctx, configMapList, listOps) + err := r.List(ctx, configMapList, listOps) if err != nil { return nil, fmt.Errorf("error listing ConfigMaps: %w", err) } From f81083f7f91cd5fcf280b585d6f451faa2fea55a Mon Sep 17 00:00:00 2001 From: Matt Hagenbuch Date: Tue, 21 May 2024 10:32:31 -0400 Subject: [PATCH 16/17] lint --- internal/manifests/collector/configmap_test.go | 4 ++-- internal/naming/main.go | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/internal/manifests/collector/configmap_test.go b/internal/manifests/collector/configmap_test.go index 8581b5ab20..d7614c8648 100644 --- a/internal/manifests/collector/configmap_test.go +++ b/internal/manifests/collector/configmap_test.go @@ -17,10 +17,10 @@ package collector import ( "testing" + "github.com/stretchr/testify/assert" + "github.com/open-telemetry/opentelemetry-operator/internal/manifests/manifestutils" "github.com/open-telemetry/opentelemetry-operator/internal/naming" - - "github.com/stretchr/testify/assert" ) func TestDesiredConfigMap(t *testing.T) { diff --git a/internal/naming/main.go b/internal/naming/main.go index fced8aa7b0..7ebd21d9ba 100644 --- a/internal/naming/main.go +++ b/internal/naming/main.go @@ -16,7 +16,7 @@ package naming // ConfigMap builds the name for the config map used in the OpenTelemetryCollector containers. -// The configHash should be calculated using manifestutils.GetConfigMapSHA +// The configHash should be calculated using manifestutils.GetConfigMapSHA. func ConfigMap(otelcol, configHash string) string { return DNSName(Truncate("%s-collector-%s", 63, otelcol, configHash[:8])) } From 7bc3770197392e3684a5b3ade468efb3750c0079 Mon Sep 17 00:00:00 2001 From: Matt Hagenbuch Date: Tue, 21 May 2024 12:27:01 -0400 Subject: [PATCH 17/17] fix e2e tests --- .../targetallocator-features/00-assert.yaml | 2 +- .../targetallocator-kubernetessd/00-assert.yaml | 2 +- .../targetallocator-prometheuscr/00-assert.yaml | 4 ++-- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/tests/e2e-targetallocator/targetallocator-features/00-assert.yaml b/tests/e2e-targetallocator/targetallocator-features/00-assert.yaml index 823f435484..7175937de8 100644 --- a/tests/e2e-targetallocator/targetallocator-features/00-assert.yaml +++ b/tests/e2e-targetallocator/targetallocator-features/00-assert.yaml @@ -20,7 +20,7 @@ spec: items: - key: collector.yaml path: collector.yaml - name: stateful-collector + name: stateful-collector-31b1d381 name: otc-internal - emptyDir: {} name: testvolume diff --git a/tests/e2e-targetallocator/targetallocator-kubernetessd/00-assert.yaml b/tests/e2e-targetallocator/targetallocator-kubernetessd/00-assert.yaml index 88d16ed604..300d2668b4 100644 --- a/tests/e2e-targetallocator/targetallocator-kubernetessd/00-assert.yaml +++ b/tests/e2e-targetallocator/targetallocator-kubernetessd/00-assert.yaml @@ -15,7 +15,7 @@ metadata: apiVersion: v1 kind: ConfigMap metadata: - name: prometheus-kubernetessd-collector + name: prometheus-kubernetessd-collector-8236b782 data: collector.yaml: | exporters: diff --git a/tests/e2e-targetallocator/targetallocator-prometheuscr/00-assert.yaml b/tests/e2e-targetallocator/targetallocator-prometheuscr/00-assert.yaml index 7593240e16..4afae1a5ee 100644 --- a/tests/e2e-targetallocator/targetallocator-prometheuscr/00-assert.yaml +++ b/tests/e2e-targetallocator/targetallocator-prometheuscr/00-assert.yaml @@ -1,7 +1,7 @@ apiVersion: apps/v1 kind: StatefulSet metadata: - name: prometheus-cr-collector-b88fa6e7 + name: prometheus-cr-collector status: readyReplicas: 1 replicas: 1 @@ -43,4 +43,4 @@ data: - prometheus kind: ConfigMap metadata: - name: prometheus-cr-collector + name: prometheus-cr-collector-b88fa6e7