From 85db42d275646c97c86c162c47278f196b73b2cb Mon Sep 17 00:00:00 2001 From: Jacob Aronoff Date: Tue, 25 Jun 2024 15:17:19 -0400 Subject: [PATCH 01/11] remove unnecessary instances of app.kubernetes.io/managed-by for user applied resources --- apis/v1alpha1/instrumentation_webhook.go | 4 --- apis/v1alpha1/opampbridge_webhook.go | 18 ++++------ apis/v1alpha1/opampbridge_webhook_test.go | 12 ++----- apis/v1beta1/collector_webhook.go | 3 -- apis/v1beta1/collector_webhook_test.go | 40 ++++++----------------- apis/v1beta1/metrics.go | 7 +--- pkg/collector/upgrade/upgrade.go | 8 +---- pkg/instrumentation/upgrade/upgrade.go | 8 +---- 8 files changed, 23 insertions(+), 77 deletions(-) diff --git a/apis/v1alpha1/instrumentation_webhook.go b/apis/v1alpha1/instrumentation_webhook.go index d2b4a5a74d..004992f795 100644 --- a/apis/v1alpha1/instrumentation_webhook.go +++ b/apis/v1alpha1/instrumentation_webhook.go @@ -91,10 +91,6 @@ func (w InstrumentationWebhook) defaulter(r *Instrumentation) error { if r.Labels == nil { r.Labels = map[string]string{} } - if r.Labels["app.kubernetes.io/managed-by"] == "" { - r.Labels["app.kubernetes.io/managed-by"] = "opentelemetry-operator" - } - if r.Spec.Java.Image == "" { r.Spec.Java.Image = w.cfg.AutoInstrumentationJavaImage() } diff --git a/apis/v1alpha1/opampbridge_webhook.go b/apis/v1alpha1/opampbridge_webhook.go index 1fbbcf75c0..b27c9bd989 100644 --- a/apis/v1alpha1/opampbridge_webhook.go +++ b/apis/v1alpha1/opampbridge_webhook.go @@ -52,23 +52,23 @@ func (o *OpAMPBridgeWebhook) Default(ctx context.Context, obj runtime.Object) er return o.defaulter(opampBridge) } -func (c OpAMPBridgeWebhook) ValidateCreate(ctx context.Context, obj runtime.Object) (admission.Warnings, error) { +func (o *OpAMPBridgeWebhook) ValidateCreate(ctx context.Context, obj runtime.Object) (admission.Warnings, error) { opampBridge, ok := obj.(*OpAMPBridge) if !ok { return nil, fmt.Errorf("expected an OpAMPBridge, received %T", obj) } - return c.validate(opampBridge) + return o.validate(opampBridge) } -func (c OpAMPBridgeWebhook) ValidateUpdate(ctx context.Context, oldObj, newObj runtime.Object) (admission.Warnings, error) { +func (o *OpAMPBridgeWebhook) ValidateUpdate(ctx context.Context, oldObj, newObj runtime.Object) (admission.Warnings, error) { opampBridge, ok := newObj.(*OpAMPBridge) if !ok { return nil, fmt.Errorf("expected an OpAMPBridge, received %T", newObj) } - return c.validate(opampBridge) + return o.validate(opampBridge) } -func (o OpAMPBridgeWebhook) ValidateDelete(ctx context.Context, obj runtime.Object) (admission.Warnings, error) { +func (o *OpAMPBridgeWebhook) ValidateDelete(ctx context.Context, obj runtime.Object) (admission.Warnings, error) { opampBridge, ok := obj.(*OpAMPBridge) if !ok || opampBridge == nil { return nil, fmt.Errorf("expected an OpAMPBridge, received %T", obj) @@ -76,7 +76,7 @@ func (o OpAMPBridgeWebhook) ValidateDelete(ctx context.Context, obj runtime.Obje return o.validate(opampBridge) } -func (o OpAMPBridgeWebhook) defaulter(r *OpAMPBridge) error { +func (o *OpAMPBridgeWebhook) defaulter(r *OpAMPBridge) error { if len(r.Spec.UpgradeStrategy) == 0 { r.Spec.UpgradeStrategy = UpgradeStrategyAutomatic } @@ -84,10 +84,6 @@ func (o OpAMPBridgeWebhook) defaulter(r *OpAMPBridge) error { if r.Labels == nil { r.Labels = map[string]string{} } - if r.Labels["app.kubernetes.io/managed-by"] == "" { - r.Labels["app.kubernetes.io/managed-by"] = "opentelemetry-operator" - } - one := int32(1) if r.Spec.Replicas == nil { r.Spec.Replicas = &one @@ -104,7 +100,7 @@ func (o OpAMPBridgeWebhook) defaulter(r *OpAMPBridge) error { return nil } -func (o OpAMPBridgeWebhook) validate(r *OpAMPBridge) (admission.Warnings, error) { +func (o *OpAMPBridgeWebhook) validate(r *OpAMPBridge) (admission.Warnings, error) { warnings := admission.Warnings{} // validate OpAMP server endpoint diff --git a/apis/v1alpha1/opampbridge_webhook_test.go b/apis/v1alpha1/opampbridge_webhook_test.go index 34d667b04d..335e055ceb 100644 --- a/apis/v1alpha1/opampbridge_webhook_test.go +++ b/apis/v1alpha1/opampbridge_webhook_test.go @@ -50,9 +50,7 @@ func TestOpAMPBridgeDefaultingWebhook(t *testing.T) { opampBridge: OpAMPBridge{}, expected: OpAMPBridge{ ObjectMeta: metav1.ObjectMeta{ - Labels: map[string]string{ - "app.kubernetes.io/managed-by": "opentelemetry-operator", - }, + Labels: map[string]string{}, }, Spec: OpAMPBridgeSpec{ Replicas: &one, @@ -71,9 +69,7 @@ func TestOpAMPBridgeDefaultingWebhook(t *testing.T) { }, expected: OpAMPBridge{ ObjectMeta: metav1.ObjectMeta{ - Labels: map[string]string{ - "app.kubernetes.io/managed-by": "opentelemetry-operator", - }, + Labels: map[string]string{}, }, Spec: OpAMPBridgeSpec{ Replicas: &five, @@ -93,9 +89,7 @@ func TestOpAMPBridgeDefaultingWebhook(t *testing.T) { }, expected: OpAMPBridge{ ObjectMeta: metav1.ObjectMeta{ - Labels: map[string]string{ - "app.kubernetes.io/managed-by": "opentelemetry-operator", - }, + Labels: map[string]string{}, }, Spec: OpAMPBridgeSpec{ Replicas: &one, diff --git a/apis/v1beta1/collector_webhook.go b/apis/v1beta1/collector_webhook.go index 930c2c819f..ec5bf47ddd 100644 --- a/apis/v1beta1/collector_webhook.go +++ b/apis/v1beta1/collector_webhook.go @@ -94,9 +94,6 @@ func (c CollectorWebhook) Default(_ context.Context, obj runtime.Object) error { if otelcol.Labels == nil { otelcol.Labels = map[string]string{} } - if otelcol.Labels["app.kubernetes.io/managed-by"] == "" { - otelcol.Labels["app.kubernetes.io/managed-by"] = "opentelemetry-operator" - } // We can default to one because dependent objects Deployment and HorizontalPodAutoScaler // default to 1 as well. diff --git a/apis/v1beta1/collector_webhook_test.go b/apis/v1beta1/collector_webhook_test.go index 8262a6fd07..12c2a25bd6 100644 --- a/apis/v1beta1/collector_webhook_test.go +++ b/apis/v1beta1/collector_webhook_test.go @@ -111,9 +111,7 @@ func TestCollectorDefaultingWebhook(t *testing.T) { otelcol: OpenTelemetryCollector{}, expected: OpenTelemetryCollector{ ObjectMeta: metav1.ObjectMeta{ - Labels: map[string]string{ - "app.kubernetes.io/managed-by": "opentelemetry-operator", - }, + Labels: map[string]string{}, }, Spec: OpenTelemetryCollectorSpec{ OpenTelemetryCommonFields: OpenTelemetryCommonFields{ @@ -144,9 +142,7 @@ func TestCollectorDefaultingWebhook(t *testing.T) { }, expected: OpenTelemetryCollector{ ObjectMeta: metav1.ObjectMeta{ - Labels: map[string]string{ - "app.kubernetes.io/managed-by": "opentelemetry-operator", - }, + Labels: map[string]string{}, }, Spec: OpenTelemetryCollectorSpec{ Mode: ModeSidecar, @@ -178,9 +174,7 @@ func TestCollectorDefaultingWebhook(t *testing.T) { }, expected: OpenTelemetryCollector{ ObjectMeta: metav1.ObjectMeta{ - Labels: map[string]string{ - "app.kubernetes.io/managed-by": "opentelemetry-operator", - }, + Labels: map[string]string{}, }, Spec: OpenTelemetryCollectorSpec{ Mode: ModeSidecar, @@ -210,9 +204,7 @@ func TestCollectorDefaultingWebhook(t *testing.T) { }, expected: OpenTelemetryCollector{ ObjectMeta: metav1.ObjectMeta{ - Labels: map[string]string{ - "app.kubernetes.io/managed-by": "opentelemetry-operator", - }, + Labels: map[string]string{}, }, Spec: OpenTelemetryCollectorSpec{ Mode: ModeDeployment, @@ -247,9 +239,7 @@ func TestCollectorDefaultingWebhook(t *testing.T) { }, expected: OpenTelemetryCollector{ ObjectMeta: metav1.ObjectMeta{ - Labels: map[string]string{ - "app.kubernetes.io/managed-by": "opentelemetry-operator", - }, + Labels: map[string]string{}, }, Spec: OpenTelemetryCollectorSpec{ Mode: ModeDeployment, @@ -290,9 +280,7 @@ func TestCollectorDefaultingWebhook(t *testing.T) { }, expected: OpenTelemetryCollector{ ObjectMeta: metav1.ObjectMeta{ - Labels: map[string]string{ - "app.kubernetes.io/managed-by": "opentelemetry-operator", - }, + Labels: map[string]string{}, }, Spec: OpenTelemetryCollectorSpec{ Mode: ModeDeployment, @@ -329,9 +317,7 @@ func TestCollectorDefaultingWebhook(t *testing.T) { }, expected: OpenTelemetryCollector{ ObjectMeta: metav1.ObjectMeta{ - Labels: map[string]string{ - "app.kubernetes.io/managed-by": "opentelemetry-operator", - }, + Labels: map[string]string{}, }, Spec: OpenTelemetryCollectorSpec{ Mode: ModeDeployment, @@ -379,9 +365,7 @@ func TestCollectorDefaultingWebhook(t *testing.T) { }, expected: OpenTelemetryCollector{ ObjectMeta: metav1.ObjectMeta{ - Labels: map[string]string{ - "app.kubernetes.io/managed-by": "opentelemetry-operator", - }, + Labels: map[string]string{}, }, Spec: OpenTelemetryCollectorSpec{ Mode: ModeDeployment, @@ -424,9 +408,7 @@ func TestCollectorDefaultingWebhook(t *testing.T) { }, expected: OpenTelemetryCollector{ ObjectMeta: metav1.ObjectMeta{ - Labels: map[string]string{ - "app.kubernetes.io/managed-by": "opentelemetry-operator", - }, + Labels: map[string]string{}, }, Spec: OpenTelemetryCollectorSpec{ Mode: ModeDeployment, @@ -468,9 +450,7 @@ func TestCollectorDefaultingWebhook(t *testing.T) { }, expected: OpenTelemetryCollector{ ObjectMeta: metav1.ObjectMeta{ - Labels: map[string]string{ - "app.kubernetes.io/managed-by": "opentelemetry-operator", - }, + Labels: map[string]string{}, }, Spec: OpenTelemetryCollectorSpec{ Mode: ModeDeployment, diff --git a/apis/v1beta1/metrics.go b/apis/v1beta1/metrics.go index 395306059d..53675277f8 100644 --- a/apis/v1beta1/metrics.go +++ b/apis/v1beta1/metrics.go @@ -121,13 +121,8 @@ func NewMetrics(prv metric.MeterProvider, ctx context.Context, cl client.Reader) // Init metrics from the first time the operator starts. func (m *Metrics) init(ctx context.Context, cl client.Reader) error { - opts := []client.ListOption{ - client.MatchingLabels(map[string]string{ - "app.kubernetes.io/managed-by": "opentelemetry-operator", - }), - } list := &OpenTelemetryCollectorList{} - if err := cl.List(ctx, list, opts...); err != nil { + if err := cl.List(ctx, list); err != nil { return fmt.Errorf("failed to list: %w", err) } diff --git a/pkg/collector/upgrade/upgrade.go b/pkg/collector/upgrade/upgrade.go index 971689169d..72735700a4 100644 --- a/pkg/collector/upgrade/upgrade.go +++ b/pkg/collector/upgrade/upgrade.go @@ -42,14 +42,8 @@ const RecordBufferSize int = 10 // ManagedInstances finds all the otelcol instances for the current operator and upgrades them, if necessary. func (u VersionUpgrade) ManagedInstances(ctx context.Context) error { u.Log.Info("looking for managed instances to upgrade") - - opts := []client.ListOption{ - client.MatchingLabels(map[string]string{ - "app.kubernetes.io/managed-by": "opentelemetry-operator", - }), - } list := &v1beta1.OpenTelemetryCollectorList{} - if err := u.Client.List(ctx, list, opts...); err != nil { + if err := u.Client.List(ctx, list); err != nil { return fmt.Errorf("failed to list: %w", err) } diff --git a/pkg/instrumentation/upgrade/upgrade.go b/pkg/instrumentation/upgrade/upgrade.go index 948a8e7288..f4b963f1c9 100644 --- a/pkg/instrumentation/upgrade/upgrade.go +++ b/pkg/instrumentation/upgrade/upgrade.go @@ -79,14 +79,8 @@ func NewInstrumentationUpgrade(client client.Client, logger logr.Logger, recorde // ManagedInstances upgrades managed instances by the opentelemetry-operator. func (u *InstrumentationUpgrade) ManagedInstances(ctx context.Context) error { u.Logger.Info("looking for managed Instrumentation instances to upgrade") - - opts := []client.ListOption{ - client.MatchingLabels(map[string]string{ - "app.kubernetes.io/managed-by": "opentelemetry-operator", - }), - } list := &v1alpha1.InstrumentationList{} - if err := u.Client.List(ctx, list, opts...); err != nil { + if err := u.Client.List(ctx, list); err != nil { return fmt.Errorf("failed to list: %w", err) } From 74cf1e1dae6b2ecf613966a11ad41e5f18666fb8 Mon Sep 17 00:00:00 2001 From: Jacob Aronoff Date: Tue, 25 Jun 2024 15:23:05 -0400 Subject: [PATCH 02/11] chlog --- .chloggen/fix-managed-by-gross.yaml | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) create mode 100755 .chloggen/fix-managed-by-gross.yaml diff --git a/.chloggen/fix-managed-by-gross.yaml b/.chloggen/fix-managed-by-gross.yaml new file mode 100755 index 0000000000..5a89247d87 --- /dev/null +++ b/.chloggen/fix-managed-by-gross.yaml @@ -0,0 +1,19 @@ +# One of 'breaking', 'deprecation', 'new_component', 'enhancement', 'bug_fix' +change_type: breaking + +# 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, auto-instrumentation, opamp + +# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`). +note: This removes the static label app.kubernetes.io/managed-by from user applied CRs (collector, instrumentation, bridge) + +# One or more tracking issues related to the change +issues: [3014] + +# (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: | + The existence of app.kubernetes.io/managed-by=opentelemetry-operator was preventing the helm managed-by + labels from persisting correctly. this resulted in unnecessary conflicts while also resulting in an + operator failure to pull the correct images for auto-instrumentation. From e4f9c863d3f191e6bbe81bf88ab5d4d45bf9d3b4 Mon Sep 17 00:00:00 2001 From: Jacob Aronoff Date: Fri, 23 Aug 2024 17:17:43 -0400 Subject: [PATCH 03/11] fix upgrade issue --- main.go | 3 +-- pkg/collector/upgrade/upgrade.go | 26 ++++++++++++++++++++------ 2 files changed, 21 insertions(+), 8 deletions(-) diff --git a/main.go b/main.go index 8868b1ff03..fbfca13ffe 100644 --- a/main.go +++ b/main.go @@ -36,7 +36,6 @@ import ( "k8s.io/client-go/kubernetes" clientgoscheme "k8s.io/client-go/kubernetes/scheme" _ "k8s.io/client-go/plugin/pkg/client/auth/gcp" - "k8s.io/client-go/tools/record" k8sapiflag "k8s.io/component-base/cli/flag" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/cache" @@ -454,7 +453,7 @@ func addDependencies(_ context.Context, mgr ctrl.Manager, cfg config.Config, v v Log: ctrl.Log.WithName("collector-upgrade"), Version: v, Client: mgr.GetClient(), - Recorder: record.NewFakeRecorder(collectorupgrade.RecordBufferSize), + Recorder: mgr.GetEventRecorderFor("opentelemetry-operator"), } return up.ManagedInstances(c) })) diff --git a/pkg/collector/upgrade/upgrade.go b/pkg/collector/upgrade/upgrade.go index c7416fbfe0..64cedcaf87 100644 --- a/pkg/collector/upgrade/upgrade.go +++ b/pkg/collector/upgrade/upgrade.go @@ -23,6 +23,7 @@ import ( semver "github.com/Masterminds/semver/v3" "github.com/go-logr/logr" "k8s.io/client-go/tools/record" + "k8s.io/client-go/util/retry" "sigs.k8s.io/controller-runtime/pkg/client" "github.com/open-telemetry/opentelemetry-operator/apis/v1alpha1" @@ -67,23 +68,36 @@ func (u VersionUpgrade) ManagedInstances(ctx context.Context) error { u.Recorder.Event(&original, "Error", "Upgrade", msg) continue } - if !reflect.DeepEqual(upgraded, list.Items[i]) { // the resource update overrides the status, so, keep it so that we can reset it later st := upgraded.Status - patch := client.MergeFrom(&original) - if err := u.Client.Patch(ctx, &upgraded, patch); err != nil { + err := retry.RetryOnConflict(retry.DefaultRetry, func() error { + col := v1beta1.OpenTelemetryCollector{} + err = u.Client.Get(ctx, client.ObjectKeyFromObject(&original), &col) + if err != nil { + return err + } + var upgradedErr error + upgraded, upgradedErr = u.ManagedInstance(ctx, col) + if upgradedErr != nil { + return upgradedErr + } + if err := u.Client.Update(ctx, &upgraded); err != nil { + return err + } + return nil + }) + if err != nil { itemLogger.Error(err, "failed to apply changes to instance") - continue + return err } // the status object requires its own update upgraded.Status = st - if err := u.Client.Status().Patch(ctx, &upgraded, patch); err != nil { + if err := u.Client.Status().Patch(ctx, &upgraded, client.MergeFrom(&original)); err != nil { itemLogger.Error(err, "failed to apply changes to instance's status object") continue } - itemLogger.Info("instance upgraded", "version", upgraded.Status.Version) } } From 347090185ba085aa9b00bdf830f55962332cbed6 Mon Sep 17 00:00:00 2001 From: Jacob Aronoff Date: Fri, 23 Aug 2024 17:23:05 -0400 Subject: [PATCH 04/11] Fix lint --- pkg/collector/upgrade/upgrade.go | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/pkg/collector/upgrade/upgrade.go b/pkg/collector/upgrade/upgrade.go index 64cedcaf87..aea5fcfb01 100644 --- a/pkg/collector/upgrade/upgrade.go +++ b/pkg/collector/upgrade/upgrade.go @@ -71,25 +71,25 @@ func (u VersionUpgrade) ManagedInstances(ctx context.Context) error { if !reflect.DeepEqual(upgraded, list.Items[i]) { // the resource update overrides the status, so, keep it so that we can reset it later st := upgraded.Status - err := retry.RetryOnConflict(retry.DefaultRetry, func() error { + upgradeErr := retry.RetryOnConflict(retry.DefaultRetry, func() error { col := v1beta1.OpenTelemetryCollector{} err = u.Client.Get(ctx, client.ObjectKeyFromObject(&original), &col) if err != nil { return err } - var upgradedErr error - upgraded, upgradedErr = u.ManagedInstance(ctx, col) - if upgradedErr != nil { - return upgradedErr + var versionUpgradeErr error + upgraded, versionUpgradeErr = u.ManagedInstance(ctx, col) + if versionUpgradeErr != nil { + return versionUpgradeErr } if err := u.Client.Update(ctx, &upgraded); err != nil { return err } return nil }) - if err != nil { - itemLogger.Error(err, "failed to apply changes to instance") - return err + if upgradeErr != nil { + itemLogger.Error(upgradeErr, "failed to apply changes to instance") + return upgradeErr } // the status object requires its own update From 90df7fb9531b2a69ed5b4be50cd99039e4cb2c90 Mon Sep 17 00:00:00 2001 From: Jacob Aronoff Date: Fri, 23 Aug 2024 17:29:35 -0400 Subject: [PATCH 05/11] Fix test --- pkg/collector/upgrade/upgrade.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/pkg/collector/upgrade/upgrade.go b/pkg/collector/upgrade/upgrade.go index aea5fcfb01..3835c711a0 100644 --- a/pkg/collector/upgrade/upgrade.go +++ b/pkg/collector/upgrade/upgrade.go @@ -72,13 +72,13 @@ func (u VersionUpgrade) ManagedInstances(ctx context.Context) error { // the resource update overrides the status, so, keep it so that we can reset it later st := upgraded.Status upgradeErr := retry.RetryOnConflict(retry.DefaultRetry, func() error { - col := v1beta1.OpenTelemetryCollector{} - err = u.Client.Get(ctx, client.ObjectKeyFromObject(&original), &col) + objKey := client.ObjectKeyFromObject(&original) + err = u.Client.Get(ctx, objKey, &original) if err != nil { return err } var versionUpgradeErr error - upgraded, versionUpgradeErr = u.ManagedInstance(ctx, col) + upgraded, versionUpgradeErr = u.ManagedInstance(ctx, original) if versionUpgradeErr != nil { return versionUpgradeErr } From 7bd9e9f5d4703b28c1dda4891c7a1609c06725e7 Mon Sep 17 00:00:00 2001 From: Jacob Aronoff Date: Tue, 27 Aug 2024 11:12:30 -0400 Subject: [PATCH 06/11] repro --- pkg/collector/upgrade/upgrade.go | 5 +- pkg/collector/upgrade/upgrade_test.go | 79 +++++++++++++++++++++++++++ 2 files changed, 82 insertions(+), 2 deletions(-) diff --git a/pkg/collector/upgrade/upgrade.go b/pkg/collector/upgrade/upgrade.go index 3835c711a0..3cdcad0389 100644 --- a/pkg/collector/upgrade/upgrade.go +++ b/pkg/collector/upgrade/upgrade.go @@ -82,8 +82,9 @@ func (u VersionUpgrade) ManagedInstances(ctx context.Context) error { if versionUpgradeErr != nil { return versionUpgradeErr } - if err := u.Client.Update(ctx, &upgraded); err != nil { - return err + patch := client.MergeFrom(&original) + if patchErr := u.Client.Patch(ctx, &upgraded, patch); patchErr != nil { + return patchErr } return nil }) diff --git a/pkg/collector/upgrade/upgrade_test.go b/pkg/collector/upgrade/upgrade_test.go index 640ccb923a..d74c22e77c 100644 --- a/pkg/collector/upgrade/upgrade_test.go +++ b/pkg/collector/upgrade/upgrade_test.go @@ -84,6 +84,85 @@ func TestShouldUpgradeAllToLatestBasedOnUpgradeStrategy(t *testing.T) { } } +func TestEnvVarUpdates(t *testing.T) { + nsn := types.NamespacedName{Name: "my-instance", Namespace: "default"} + collectorInstance := v1beta1.OpenTelemetryCollector{ + TypeMeta: metav1.TypeMeta{ + Kind: "OpenTelemetryCollector", + APIVersion: "v1beta1", + }, + ObjectMeta: metav1.ObjectMeta{ + Name: nsn.Name, + Namespace: nsn.Namespace, + }, + Status: v1beta1.OpenTelemetryCollectorStatus{ + Version: "0.104.0", + }, + Spec: v1beta1.OpenTelemetryCollectorSpec{ + OpenTelemetryCommonFields: v1beta1.OpenTelemetryCommonFields{ + Args: map[string]string{ + "foo": "bar", + "feature-gates": "+baz,-confmap.unifyEnvVarExpansion", + }, + }, + Config: v1beta1.Config{ + Receivers: v1beta1.AnyConfig{ + Object: map[string]interface{}{ + "prometheus": []interface{}{}, + }, + }, + Exporters: v1beta1.AnyConfig{ + Object: map[string]interface{}{ + "debug": []interface{}{}, + }, + }, + Service: v1beta1.Service{ + Pipelines: map[string]*v1beta1.Pipeline{ + "metrics": { + Exporters: []string{"debug"}, + Processors: nil, + Receivers: []string{"prometheus"}, + }, + }, + }, + }, + }, + } + err := k8sClient.Create(context.Background(), &collectorInstance) + require.NoError(t, err) + + collectorInstance.Status.Version = "0.104.0" + err = k8sClient.Status().Update(context.Background(), &collectorInstance) + require.NoError(t, err) + // sanity check + persisted := &v1beta1.OpenTelemetryCollector{} + err = k8sClient.Get(context.Background(), nsn, persisted) + require.NoError(t, err) + require.Equal(t, collectorInstance.Status.Version, persisted.Status.Version) + + currentV := version.Get() + currentV.OpenTelemetryCollector = "0.105.0" + up := &upgrade.VersionUpgrade{ + Log: logger, + Version: currentV, + Client: k8sClient, + Recorder: record.NewFakeRecorder(upgrade.RecordBufferSize), + } + + // test + err = up.ManagedInstances(context.Background()) + assert.NoError(t, err) + + // verify + err = k8sClient.Get(context.Background(), nsn, persisted) + assert.NoError(t, err) + assert.Equal(t, upgrade.Latest.String(), persisted.Status.Version) + assert.NotContainsf(t, persisted.Spec.OpenTelemetryCommonFields.Args["feature-gates"], "-confmap.unifyEnvVarExpansion", "still has env var") + + // cleanup + assert.NoError(t, k8sClient.Delete(context.Background(), &collectorInstance)) +} + func TestUpgradeUpToLatestKnownVersion(t *testing.T) { for _, tt := range []struct { desc string From 680fb0dbd1ea07c2ddfd1164eca86a40b26fb8c9 Mon Sep 17 00:00:00 2001 From: Jacob Aronoff Date: Tue, 27 Aug 2024 11:47:24 -0400 Subject: [PATCH 07/11] fix upgrade --- pkg/collector/upgrade/upgrade.go | 64 ++++++++++----------------- pkg/collector/upgrade/upgrade_test.go | 2 +- pkg/collector/upgrade/v0_105_0.go | 2 +- 3 files changed, 26 insertions(+), 42 deletions(-) diff --git a/pkg/collector/upgrade/upgrade.go b/pkg/collector/upgrade/upgrade.go index 3cdcad0389..55d7ccf214 100644 --- a/pkg/collector/upgrade/upgrade.go +++ b/pkg/collector/upgrade/upgrade.go @@ -23,7 +23,6 @@ import ( semver "github.com/Masterminds/semver/v3" "github.com/go-logr/logr" "k8s.io/client-go/tools/record" - "k8s.io/client-go/util/retry" "sigs.k8s.io/controller-runtime/pkg/client" "github.com/open-telemetry/opentelemetry-operator/apis/v1alpha1" @@ -71,24 +70,8 @@ func (u VersionUpgrade) ManagedInstances(ctx context.Context) error { if !reflect.DeepEqual(upgraded, list.Items[i]) { // the resource update overrides the status, so, keep it so that we can reset it later st := upgraded.Status - upgradeErr := retry.RetryOnConflict(retry.DefaultRetry, func() error { - objKey := client.ObjectKeyFromObject(&original) - err = u.Client.Get(ctx, objKey, &original) - if err != nil { - return err - } - var versionUpgradeErr error - upgraded, versionUpgradeErr = u.ManagedInstance(ctx, original) - if versionUpgradeErr != nil { - return versionUpgradeErr - } - patch := client.MergeFrom(&original) - if patchErr := u.Client.Patch(ctx, &upgraded, patch); patchErr != nil { - return patchErr - } - return nil - }) - if upgradeErr != nil { + patch := client.MergeFrom(&original) + if upgradeErr := u.Client.Patch(ctx, &upgraded, patch); upgradeErr != nil { itemLogger.Error(upgradeErr, "failed to apply changes to instance") return upgradeErr } @@ -123,60 +106,61 @@ func (u VersionUpgrade) ManagedInstance(_ context.Context, otelcol v1beta1.OpenT return otelcol, err } + updated := *(otelcol.DeepCopy()) if instanceV.GreaterThan(&Latest.Version) { // Update with the latest known version, which is what we have from versions.txt - u.Log.V(4).Info("no upgrade routines are needed for the OpenTelemetry instance", "name", otelcol.Name, "namespace", otelcol.Namespace, "version", otelcol.Status.Version, "latest", Latest.Version.String()) + u.Log.V(4).Info("no upgrade routines are needed for the OpenTelemetry instance", "name", updated.Name, "namespace", updated.Namespace, "version", updated.Status.Version, "latest", Latest.Version.String()) otelColV, err := semver.NewVersion(u.Version.OpenTelemetryCollector) if err != nil { - return otelcol, err + return updated, err } if instanceV.LessThan(otelColV) { - u.Log.Info("upgraded OpenTelemetry Collector version", "name", otelcol.Name, "namespace", otelcol.Namespace, "version", otelcol.Status.Version) - otelcol.Status.Version = u.Version.OpenTelemetryCollector + u.Log.Info("upgraded OpenTelemetry Collector version", "name", updated.Name, "namespace", updated.Namespace, "version", updated.Status.Version) + updated.Status.Version = u.Version.OpenTelemetryCollector } else { - u.Log.V(4).Info("skipping upgrade for OpenTelemetry Collector instance", "name", otelcol.Name, "namespace", otelcol.Namespace) + u.Log.V(4).Info("skipping upgrade for OpenTelemetry Collector instance", "name", updated.Name, "namespace", updated.Namespace) } - return otelcol, nil + return updated, nil } for _, available := range versions { if available.GreaterThan(instanceV) { if available.upgrade != nil { otelcolV1alpha1 := &v1alpha1.OpenTelemetryCollector{} - if err := otelcolV1alpha1.ConvertFrom(&otelcol); err != nil { - return otelcol, err + if err := otelcolV1alpha1.ConvertFrom(&updated); err != nil { + return updated, err } upgradedV1alpha1, err := available.upgrade(u, otelcolV1alpha1) if err != nil { - u.Log.Error(err, "failed to upgrade managed otelcol instances", "name", otelcol.Name, "namespace", otelcol.Namespace) - return otelcol, err + u.Log.Error(err, "failed to upgrade managed otelcol instances", "name", updated.Name, "namespace", updated.Namespace) + return updated, err } upgradedV1alpha1.Status.Version = available.String() - if err := upgradedV1alpha1.ConvertTo(&otelcol); err != nil { - return otelcol, err + if err := upgradedV1alpha1.ConvertTo(&updated); err != nil { + return updated, err } - u.Log.V(1).Info("step upgrade", "name", otelcol.Name, "namespace", otelcol.Namespace, "version", available.String()) + u.Log.V(1).Info("step upgrade", "name", updated.Name, "namespace", updated.Namespace, "version", available.String()) } else { - upgraded, err := available.upgradeV1beta1(u, &otelcol) //available.upgrade(params., &otelcol) + upgraded, err := available.upgradeV1beta1(u, &updated) //available.upgrade(params., &updated) if err != nil { - u.Log.Error(err, "failed to upgrade managed otelcol instances", "name", otelcol.Name, "namespace", otelcol.Namespace) - return otelcol, err + u.Log.Error(err, "failed to upgrade managed otelcol instances", "name", updated.Name, "namespace", updated.Namespace) + return updated, err } - u.Log.V(1).Info("step upgrade", "name", otelcol.Name, "namespace", otelcol.Namespace, "version", available.String()) + u.Log.V(1).Info("step upgrade", "name", updated.Name, "namespace", updated.Namespace, "version", available.String()) upgraded.Status.Version = available.String() - otelcol = *upgraded + updated = *upgraded } } } // Update with the latest known version, which is what we have from versions.txt - otelcol.Status.Version = u.Version.OpenTelemetryCollector + updated.Status.Version = u.Version.OpenTelemetryCollector - u.Log.V(1).Info("final version", "name", otelcol.Name, "namespace", otelcol.Namespace, "version", otelcol.Status.Version) - return otelcol, nil + u.Log.V(1).Info("final version", "name", updated.Name, "namespace", updated.Namespace, "version", updated.Status.Version) + return updated, nil } diff --git a/pkg/collector/upgrade/upgrade_test.go b/pkg/collector/upgrade/upgrade_test.go index d74c22e77c..40e6282f8e 100644 --- a/pkg/collector/upgrade/upgrade_test.go +++ b/pkg/collector/upgrade/upgrade_test.go @@ -157,7 +157,7 @@ func TestEnvVarUpdates(t *testing.T) { err = k8sClient.Get(context.Background(), nsn, persisted) assert.NoError(t, err) assert.Equal(t, upgrade.Latest.String(), persisted.Status.Version) - assert.NotContainsf(t, persisted.Spec.OpenTelemetryCommonFields.Args["feature-gates"], "-confmap.unifyEnvVarExpansion", "still has env var") + assert.NotContainsf(t, persisted.Spec.Args["feature-gates"], "-confmap.unifyEnvVarExpansion", "still has env var") // cleanup assert.NoError(t, k8sClient.Delete(context.Background(), &collectorInstance)) diff --git a/pkg/collector/upgrade/v0_105_0.go b/pkg/collector/upgrade/v0_105_0.go index 3f497ce6a7..daa5795590 100644 --- a/pkg/collector/upgrade/v0_105_0.go +++ b/pkg/collector/upgrade/v0_105_0.go @@ -34,7 +34,7 @@ func upgrade0_105_0(_ VersionUpgrade, otelcol *v1beta1.OpenTelemetryCollector) ( } envVarExpansionFeatureFlag := "-confmap.unifyEnvVarExpansion" - otelcol.Spec.Args = RemoveFeatureGate(otelcol.Spec.Args, envVarExpansionFeatureFlag) + otelcol.Spec.OpenTelemetryCommonFields.Args = RemoveFeatureGate(otelcol.Spec.OpenTelemetryCommonFields.Args, envVarExpansionFeatureFlag) return otelcol, nil } From 19f662961ee4372dceea58293d1eeb2711fc01b6 Mon Sep 17 00:00:00 2001 From: Jacob Aronoff Date: Tue, 27 Aug 2024 11:49:45 -0400 Subject: [PATCH 08/11] duplication --- pkg/collector/upgrade/upgrade.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/collector/upgrade/upgrade.go b/pkg/collector/upgrade/upgrade.go index 55d7ccf214..9ef5ff1928 100644 --- a/pkg/collector/upgrade/upgrade.go +++ b/pkg/collector/upgrade/upgrade.go @@ -78,7 +78,7 @@ func (u VersionUpgrade) ManagedInstances(ctx context.Context) error { // the status object requires its own update upgraded.Status = st - if err := u.Client.Status().Patch(ctx, &upgraded, client.MergeFrom(&original)); err != nil { + if err := u.Client.Status().Patch(ctx, &upgraded, patch); err != nil { itemLogger.Error(err, "failed to apply changes to instance's status object") continue } From 6669a284cceb13d7a223e32b73a4cd7798d37ca2 Mon Sep 17 00:00:00 2001 From: Jacob Aronoff Date: Tue, 27 Aug 2024 11:55:20 -0400 Subject: [PATCH 09/11] revert change --- pkg/collector/upgrade/upgrade.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/pkg/collector/upgrade/upgrade.go b/pkg/collector/upgrade/upgrade.go index 9ef5ff1928..9a1f426747 100644 --- a/pkg/collector/upgrade/upgrade.go +++ b/pkg/collector/upgrade/upgrade.go @@ -71,9 +71,9 @@ func (u VersionUpgrade) ManagedInstances(ctx context.Context) error { // the resource update overrides the status, so, keep it so that we can reset it later st := upgraded.Status patch := client.MergeFrom(&original) - if upgradeErr := u.Client.Patch(ctx, &upgraded, patch); upgradeErr != nil { - itemLogger.Error(upgradeErr, "failed to apply changes to instance") - return upgradeErr + if err := u.Client.Patch(ctx, &upgraded, patch); err != nil { + itemLogger.Error(err, "failed to apply changes to instance") + continue } // the status object requires its own update From b91b5762e12a546a5e066569de2c6c72249aa8cd Mon Sep 17 00:00:00 2001 From: Jacob Aronoff Date: Tue, 27 Aug 2024 12:06:56 -0400 Subject: [PATCH 10/11] chlog entry --- .chloggen/fix-managed-by-gross.yaml | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/.chloggen/fix-managed-by-gross.yaml b/.chloggen/fix-managed-by-gross.yaml index 5a89247d87..d733674d9d 100755 --- a/.chloggen/fix-managed-by-gross.yaml +++ b/.chloggen/fix-managed-by-gross.yaml @@ -1,19 +1,18 @@ # One of 'breaking', 'deprecation', 'new_component', 'enhancement', 'bug_fix' -change_type: breaking +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, auto-instrumentation, opamp +component: collector # A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`). -note: This removes the static label app.kubernetes.io/managed-by from user applied CRs (collector, instrumentation, bridge) +note: Fixes a bug that was preventing upgrade patches from reliably applying. # One or more tracking issues related to the change -issues: [3014] +issues: [3074] # (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: | - The existence of app.kubernetes.io/managed-by=opentelemetry-operator was preventing the helm managed-by - labels from persisting correctly. this resulted in unnecessary conflicts while also resulting in an - operator failure to pull the correct images for auto-instrumentation. + A bug was discovered in the process of testing the PR that was failing to remove the environment + variables introduced in the 0.104.0 upgrade. The fix was to take a deepcopy of the object and update that. From 2a17e98eb06f2f0f711cdc927676c38d9280307b Mon Sep 17 00:00:00 2001 From: Jacob Aronoff Date: Wed, 28 Aug 2024 13:52:04 -0400 Subject: [PATCH 11/11] another chlog entry --- .chloggen/fix-managed-by-gross-2.yaml | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) create mode 100755 .chloggen/fix-managed-by-gross-2.yaml diff --git a/.chloggen/fix-managed-by-gross-2.yaml b/.chloggen/fix-managed-by-gross-2.yaml new file mode 100755 index 0000000000..b06808183e --- /dev/null +++ b/.chloggen/fix-managed-by-gross-2.yaml @@ -0,0 +1,20 @@ +# 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: auto-instrumentation + +# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`). +note: Fixes a bug that was preventing auto instrumentation from getting correct images. + +# One or more tracking issues related to the change +issues: [3014] + +# (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 PR removes the restriction on the operator to only upgrade manually applied CRDs. This meant + that resources applied by helm were not upgraded at all. The solution was to remove the restriction + we had on querying the label app.kubernetes.io/managed-by=opentelemetry-operator, thereby upgrading + ALL CRDs in the cluster.