From d826821877b5abcacd038532d5ce7dfe1aecf303 Mon Sep 17 00:00:00 2001 From: Todd Yan Date: Sun, 11 Feb 2024 18:47:38 -0800 Subject: [PATCH 1/3] feat: Add tests and add feature to update daemonset to have selector --- internal/status/collector/collector.go | 10 +- internal/status/collector/collector_test.go | 177 ++++++++++++++++++++ 2 files changed, 182 insertions(+), 5 deletions(-) create mode 100644 internal/status/collector/collector_test.go diff --git a/internal/status/collector/collector.go b/internal/status/collector/collector.go index cdebae02d0..116fe5481f 100644 --- a/internal/status/collector/collector.go +++ b/internal/status/collector/collector.go @@ -32,11 +32,12 @@ import ( func UpdateCollectorStatus(ctx context.Context, cli client.Client, changed *v1alpha1.OpenTelemetryCollector) error { if changed.Status.Version == "" { - // a version is not set, otherwise let the upgrade mechanism take care of it! changed.Status.Version = version.OpenTelemetryCollector() } + mode := changed.Spec.Mode - if mode != v1alpha1.ModeDeployment && mode != v1alpha1.ModeStatefulSet { + + if mode != v1alpha1.ModeDeployment && mode != v1alpha1.ModeStatefulSet && mode != v1alpha1.ModeDaemonSet { changed.Status.Scale.Replicas = 0 changed.Status.Scale.Selector = "" return nil @@ -44,7 +45,6 @@ func UpdateCollectorStatus(ctx context.Context, cli client.Client, changed *v1al name := naming.Collector(changed.Name) - // Set the scale selector labels := manifestutils.Labels(changed.ObjectMeta, name, changed.Spec.Image, collector.ComponentOpenTelemetryCollector, []string{}) selector, err := metav1.LabelSelectorAsSelector(&metav1.LabelSelector{MatchLabels: labels}) if err != nil { @@ -52,7 +52,6 @@ func UpdateCollectorStatus(ctx context.Context, cli client.Client, changed *v1al } changed.Status.Scale.Selector = selector.String() - // Set the scale replicas objKey := client.ObjectKey{ Namespace: changed.GetNamespace(), Name: naming.Collector(changed.Name), @@ -63,7 +62,7 @@ func UpdateCollectorStatus(ctx context.Context, cli client.Client, changed *v1al var statusReplicas string var statusImage string - switch mode { // nolint:exhaustive + switch mode { case v1alpha1.ModeDeployment: obj := &appsv1.Deployment{} if err := cli.Get(ctx, objKey, obj); err != nil { @@ -91,6 +90,7 @@ func UpdateCollectorStatus(ctx context.Context, cli client.Client, changed *v1al } statusImage = obj.Spec.Template.Spec.Containers[0].Image } + changed.Status.Scale.Replicas = replicas changed.Status.Image = statusImage changed.Status.Scale.StatusReplicas = statusReplicas diff --git a/internal/status/collector/collector_test.go b/internal/status/collector/collector_test.go new file mode 100644 index 0000000000..ae961c58a5 --- /dev/null +++ b/internal/status/collector/collector_test.go @@ -0,0 +1,177 @@ +package collector + +import ( + "context" + "testing" + + "github.com/open-telemetry/opentelemetry-operator/apis/v1alpha1" + "github.com/stretchr/testify/assert" + appsv1 "k8s.io/api/apps/v1" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/fake" +) + +func TestUpdateCollectorStatusUnsupported(t *testing.T) { + ctx := context.TODO() + cli := client.Client(fake.NewFakeClient()) + + changed := &v1alpha1.OpenTelemetryCollector{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-sidecar", + Namespace: "default", + }, + Spec: v1alpha1.OpenTelemetryCollectorSpec{ + Mode: v1alpha1.ModeSidecar, + }, + } + + err := UpdateCollectorStatus(ctx, cli, changed) + assert.NoError(t, err) + + assert.Equal(t, int32(0), changed.Status.Scale.Replicas, "expected replicas to be 0") + assert.Equal(t, "", changed.Status.Scale.Selector, "expected selector to be empty") +} + +func createMockKubernetesClientDeployment() client.Client { + deployment := &appsv1.Deployment{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-deployment-collector", + Namespace: "default", + }, + Status: appsv1.DeploymentStatus{ + Replicas: 1, + ReadyReplicas: 1, + }, + Spec: appsv1.DeploymentSpec{ + Template: corev1.PodTemplateSpec{ + Spec: corev1.PodSpec{ + Containers: []corev1.Container{ + { + Name: "app", + Image: "app:latest", + }, + }, + }, + }, + }, + } + return fake.NewClientBuilder().WithObjects(deployment).Build() +} + +func TestUpdateCollectorStatusDeploymentMode(t *testing.T) { + ctx := context.TODO() + cli := createMockKubernetesClientDeployment() + + changed := &v1alpha1.OpenTelemetryCollector{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-deployment", + Namespace: "default", + }, + Spec: v1alpha1.OpenTelemetryCollectorSpec{ + Mode: v1alpha1.ModeDeployment, + }, + } + + err := UpdateCollectorStatus(ctx, cli, changed) + assert.NoError(t, err) + + assert.Equal(t, int32(1), changed.Status.Scale.Replicas, "expected replicas to be 1") + assert.Equal(t, "1/1", changed.Status.Scale.StatusReplicas, "expected status replicas to be 1/1") + assert.Equal(t, "app:latest", changed.Status.Image, "expected image to be app:latest") +} + +func createMockKubernetesClientStatefulset() client.Client { + statefulset := &appsv1.StatefulSet{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-statefulset-collector", + Namespace: "default", + }, + Status: appsv1.StatefulSetStatus{ + Replicas: 1, + ReadyReplicas: 1, + }, + Spec: appsv1.StatefulSetSpec{ + Template: corev1.PodTemplateSpec{ + Spec: corev1.PodSpec{ + Containers: []corev1.Container{ + { + Name: "app", + Image: "app:latest", + }, + }, + }, + }, + }, + } + return fake.NewClientBuilder().WithObjects(statefulset).Build() +} + +func TestUpdateCollectorStatusStatefulset(t *testing.T) { + ctx := context.TODO() + cli := createMockKubernetesClientStatefulset() + + changed := &v1alpha1.OpenTelemetryCollector{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-statefulset", + Namespace: "default", + }, + Spec: v1alpha1.OpenTelemetryCollectorSpec{ + Mode: v1alpha1.ModeStatefulSet, + }, + } + + err := UpdateCollectorStatus(ctx, cli, changed) + assert.NoError(t, err) + + assert.Equal(t, int32(1), changed.Status.Scale.Replicas, "expected replicas to be 1") + assert.Equal(t, "1/1", changed.Status.Scale.StatusReplicas, "expected status replicas to be 1/1") + assert.Equal(t, "app:latest", changed.Status.Image, "expected image to be app:latest") +} + +func createMockKubernetesClientDaemonset() client.Client { + daemonset := &appsv1.DaemonSet{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-daemonset-collector", + Namespace: "default", + }, + Spec: appsv1.DaemonSetSpec{ + Template: corev1.PodTemplateSpec{ + Spec: corev1.PodSpec{ + Containers: []corev1.Container{ + { + Name: "app", + Image: "app:latest", + }, + }, + }, + }, + }, + } + return fake.NewClientBuilder().WithObjects(daemonset).Build() +} + +func TestUpdateCollectorStatusDaemonsetMode(t *testing.T) { + ctx := context.TODO() + cli := createMockKubernetesClientDaemonset() + + changed := &v1alpha1.OpenTelemetryCollector{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-daemonset", + Namespace: "default", + Labels: map[string]string{ + "customLabel": "customValue", + }, + }, + Spec: v1alpha1.OpenTelemetryCollectorSpec{ + Mode: v1alpha1.ModeDaemonSet, + }, + } + + err := UpdateCollectorStatus(ctx, cli, changed) + assert.NoError(t, err) + + assert.Contains(t, changed.Status.Scale.Selector, "customLabel=customValue", "expected selector to contain customlabel=customValue") + assert.Equal(t, "app:latest", changed.Status.Image, "expected image to be app:latest") +} From 61ee8cee6a03a009065a1090eed02350bf44d51f Mon Sep 17 00:00:00 2001 From: Todd Yan Date: Sat, 24 Feb 2024 17:05:25 -0800 Subject: [PATCH 2/3] bugfix: fixing diableprometheusannotation casing --- apis/v1alpha1/opentelemetrycollector_types.go | 2 +- .../manifests/opentelemetry.io_opentelemetrycollectors.yaml | 4 ++-- .../crd/bases/opentelemetry.io_opentelemetrycollectors.yaml | 4 ++-- docs/api.md | 4 ++-- 4 files changed, 7 insertions(+), 7 deletions(-) diff --git a/apis/v1alpha1/opentelemetrycollector_types.go b/apis/v1alpha1/opentelemetrycollector_types.go index 1647ca54a9..8467806c59 100644 --- a/apis/v1alpha1/opentelemetrycollector_types.go +++ b/apis/v1alpha1/opentelemetrycollector_types.go @@ -527,7 +527,7 @@ type MetricsConfigSpec struct { // // +optional // +kubebuilder:validation:Optional - DisablePrometheusAnnotations bool `json:"DisablePrometheusAnnotations,omitempty"` + DisablePrometheusAnnotations bool `json:"disablePrometheusAnnotations,omitempty"` } // ObservabilitySpec defines how telemetry data gets handled. diff --git a/bundle/manifests/opentelemetry.io_opentelemetrycollectors.yaml b/bundle/manifests/opentelemetry.io_opentelemetrycollectors.yaml index cf15404595..ab2ddd3026 100644 --- a/bundle/manifests/opentelemetry.io_opentelemetrycollectors.yaml +++ b/bundle/manifests/opentelemetry.io_opentelemetrycollectors.yaml @@ -3867,7 +3867,7 @@ spec: metrics: description: Metrics defines the metrics configuration for operands. properties: - DisablePrometheusAnnotations: + disablePrometheusAnnotations: description: DisablePrometheusAnnotations controls the automatic addition of default Prometheus annotations ('prometheus.io/scrape', 'prometheus.io/port', and 'prometheus.io/path') @@ -5211,7 +5211,7 @@ spec: description: Metrics defines the metrics configuration for operands. properties: - DisablePrometheusAnnotations: + disablePrometheusAnnotations: description: DisablePrometheusAnnotations controls the automatic addition of default Prometheus annotations ('prometheus.io/scrape', 'prometheus.io/port', and 'prometheus.io/path') diff --git a/config/crd/bases/opentelemetry.io_opentelemetrycollectors.yaml b/config/crd/bases/opentelemetry.io_opentelemetrycollectors.yaml index 5ed95c6145..ea7f6db9bf 100644 --- a/config/crd/bases/opentelemetry.io_opentelemetrycollectors.yaml +++ b/config/crd/bases/opentelemetry.io_opentelemetrycollectors.yaml @@ -3864,7 +3864,7 @@ spec: metrics: description: Metrics defines the metrics configuration for operands. properties: - DisablePrometheusAnnotations: + disablePrometheusAnnotations: description: DisablePrometheusAnnotations controls the automatic addition of default Prometheus annotations ('prometheus.io/scrape', 'prometheus.io/port', and 'prometheus.io/path') @@ -5208,7 +5208,7 @@ spec: description: Metrics defines the metrics configuration for operands. properties: - DisablePrometheusAnnotations: + disablePrometheusAnnotations: description: DisablePrometheusAnnotations controls the automatic addition of default Prometheus annotations ('prometheus.io/scrape', 'prometheus.io/port', and 'prometheus.io/path') diff --git a/docs/api.md b/docs/api.md index a586f024e8..0981bbaed4 100644 --- a/docs/api.md +++ b/docs/api.md @@ -17812,7 +17812,7 @@ Metrics defines the metrics configuration for operands. - DisablePrometheusAnnotations + disablePrometheusAnnotations boolean DisablePrometheusAnnotations controls the automatic addition of default Prometheus annotations ('prometheus.io/scrape', 'prometheus.io/port', and 'prometheus.io/path')
@@ -20322,7 +20322,7 @@ Metrics defines the metrics configuration for operands. - DisablePrometheusAnnotations + disablePrometheusAnnotations boolean DisablePrometheusAnnotations controls the automatic addition of default Prometheus annotations ('prometheus.io/scrape', 'prometheus.io/port', and 'prometheus.io/path')
From b41debda37b758ddb11cce9876ab4148dc05dc34 Mon Sep 17 00:00:00 2001 From: Todd Yan Date: Sat, 24 Feb 2024 17:06:39 -0800 Subject: [PATCH 3/3] Adding change log entry --- .chloggen/daemonset-vpa.yaml | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) create mode 100755 .chloggen/daemonset-vpa.yaml diff --git a/.chloggen/daemonset-vpa.yaml b/.chloggen/daemonset-vpa.yaml new file mode 100755 index 0000000000..0fc87d3be5 --- /dev/null +++ b/.chloggen/daemonset-vpa.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. operator, target allocator, github action) +component: operator + +# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`). +note: Updates the Daemonset to allow for label selectors && fix casing for DisablePrometheusAnnotations + +# One or more tracking issues related to the change +issues: [2605, 2608] + +# (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: