internal/manifests/collector: switch internally to v1alpha2 #2532

frzifus · 2024-01-18T00:50:54Z

Description:

This PR replaces the internal dependency in internal/manifests/collector on v1alpha1 in the course of the migration to v1alpha2.

To convert the v1alpha1 CR fetched and passed from the controller it adds a internal/api/convert.v1alpha1to2() function to upgrade to v1alpha2.

The next step is to migrate opampbridge, targetallocator and manifests. It should then be possible to perform the final conversion from v1alpha1 to v1alpha2 and back in the reconcile loop.

Link to tracking Issue:

Testing:

Documentation:

iblancasa · 2024-01-18T17:11:17Z

Could you add some description/link to the issue to explain what are you implementing here?

frzifus · 2024-01-18T17:49:57Z

@iblancasa ive added a description. Does it make sense to you?

internal/manifests/collector/annotations.go

pkg/sidecar/pod.go

internal/api/convert/v1alpha.go

internal/manifests/collector/container_test.go

jaronoff97

A few thoughts, but it does feel like we're on a good path here!

internal/api/convert/v1alpha.go

jaronoff97 · 2024-01-18T22:25:03Z

internal/api/convert/v1alpha.go

+				Pods: m.Pods,
+			}
+		}
+		out.Spec.OpenTelemetryCommonFields.Autoscaler = &v1alpha2.AutoscalerSpec{


do we need to worry about the deprecated min and max replica fields?

I did say no. Since you are still able to set min/max in the AutoscalerSpec.

Since they are deprecated and this is a version bump... I would say this is the perfect moment to remove them

I would prefer to keep the change as minimal as possible. We can add/remove/change behavior afterwards.

internal/api/convert/v1alpha.go

internal/manifests/collector/container.go

internal/manifests/collector/daemonset.go

internal/manifests/collector/horizontalpodautoscaler.go

internal/manifests/collector/rbac.go

internal/api/convert/v1alpha_test.go

internal/manifests/collector/rbac.go

pkg/sidecar/pod.go

frzifus · 2024-01-23T17:58:53Z

Seems there are still some tests failing that compare strings.
e.g.:

- "collector.yaml": "receivers:\n  examplereceiver:\n    endpoint: \"0.0.0.0:12345\"\nexporters:\n  logging:\nservice:\n  pipelines:\n    metrics:\n      receivers: [examplereceiver]\n      exporters: [logging]\n",
+ "collector.yaml": "exporters:\n    logging: null\nreceivers:\n    prometheus:\n        config: {}\n        target_allocator:\n            collector_id: ${POD_NAME}\n            endpoint: http://test-targetallocator:80\n            interval: 30s\nservice:\n    pipelines:\n        metrics:\n            exporters:\n                - logging\n            receivers:\n                - prometheus\n",

pavolloffay

LGTM I would add require statements for returned error that was added to many manifest files

internal/manifests/collector/daemonset_test.go

frzifus · 2024-01-24T01:08:48Z

Well, now the formatting in the e2e tests seems to be slightly different. 🦝

internal/api/convert/v1alpha.go

iblancasa · 2024-01-24T11:55:01Z

internal/api/convert/v1alpha.go

+				Pods: m.Pods,
+			}
+		}
+		out.Spec.OpenTelemetryCommonFields.Autoscaler = &v1alpha2.AutoscalerSpec{


Since they are deprecated and this is a version bump... I would say this is the perfect moment to remove them

internal/api/convert/v1alpha.go

internal/api/convert/v1alpha_test.go

internal/manifests/collector/annotations.go

internal/manifests/collector/config_replace.go

internal/manifests/collector/configmap.go

internal/manifests/collector/container_test.go

internal/manifests/collector/horizontalpodautoscaler_test.go

internal/manifests/collector/horizontalpodautoscaler.go

Signed-off-by: Benedikt Bongartz <bongartz@klimlive.de>

jaronoff97 · 2024-02-01T16:41:51Z

internal/manifests/collector/annotations.go

 }

 // PodAnnotations return the spec annotations for OpenTelemetryCollector pod.
-func PodAnnotations(instance v1alpha1.OpenTelemetryCollector) map[string]string {
+func PodAnnotations(instance v1alpha2.OpenTelemetryCollector) (map[string]string, error) {


note: in the future, i think we should make the getConfigMapSHA a method on the collector itself that returns no error. Note worth doing in this PR, but doing so in the future would help us clean up a lot of the error checking this introduces.

I made an issue for this:

v1alpha2: make getConfigMapSHA a method of the new CRD #2591

jaronoff97

a few thoughts/questions. But overall i think this LGTM. I do think we should merge this after #2567 so that we don't incur a major performance hit from all this marshalling business we're doing now.

jaronoff97 · 2024-02-01T16:44:12Z

internal/manifests/collector/deployment.go

-func Deployment(params manifests.Params) *appsv1.Deployment {
-	name := naming.Collector(params.OtelCol.Name)
-	labels := manifestutils.Labels(params.OtelCol.ObjectMeta, name, params.OtelCol.Spec.Image, ComponentOpenTelemetryCollector, params.Config.LabelsFilter())
+func Deployment(params manifests.Params) (*appsv1.Deployment, error) {


I think after this change and #2516 go in, i will modify these signatures to not take in the whole manifests.Params object, but only the object they need (i.e. collector in this case)

v1alpha2: Minimise signatures of object generation functions #2592

jaronoff97 · 2024-02-01T16:46:11Z

internal/manifests/collector/poddisruptionbudget.go

 	"github.com/open-telemetry/opentelemetry-operator/internal/manifests"
 	"github.com/open-telemetry/opentelemetry-operator/internal/manifests/manifestutils"
 	"github.com/open-telemetry/opentelemetry-operator/internal/naming"
 )

-func PodDisruptionBudget(params manifests.Params) *policyV1.PodDisruptionBudget {
+func PodDisruptionBudget(params manifests.Params) (*policyV1.PodDisruptionBudget, error) {
+	otelCol, err := convert.V1Alpha1to2(params.OtelCol)


can you open an issue for cleaning this up after this is merged?

v1alpha2: Minimise signatures of object generation functions #2592

jaronoff97 · 2024-02-01T16:48:26Z

tests/e2e/targetallocator-prometheuscr/00-assert.yaml

-          collector_id: ${POD_NAME}
-          endpoint: http://prometheus-cr-targetallocator:80
-          interval: 30s
+        prometheus:


is there a reason we need to change the whitespacing for these now? I saw the indent call above in the serialization and wasn't sure why we need that.

y, thats a bit weird. This one works fine. But as soon as we use yaml.Marshal it falls back to ident = 4.

The default yaml.Marshal creates a newEncoder() which is initliazed with ident = 4.

https://github.com/go-yaml/yaml/blob/f6f7691b1fdeb513f56608cd2c32c51f8194bf51/encode.go#L56-L67

pavolloffay

Great progress 🎉

Just one comment, I would prefer avoiding type config -> yaml -> type config translation.

pavolloffay · 2024-02-01T17:04:46Z

internal/manifests/collector/rbac.go

+		return nil, err
+	}
+
+	confStr, err := otelCol.Spec.Config.Yaml()


The typed config is serialized to yaml and then back unserialized to map[interface{}]interface{}

The typed config could be easily converted to map[interface{}]interface{} which is accepted in most signatures

Could we add another toMap() method and extend the scope of #2591 for it?

please book a follow up ticket for this

frzifus · 2024-02-01T18:03:29Z

As discussed on the sig call, we will wait until the next release is published.

Release the operator v0.93.0 #2567

…emetry#2532) * internal/manifests/collector: switch internally to v1alphav2 Signed-off-by: Benedikt Bongartz <bongartz@klimlive.de> * internal/manifests/collector: change svc account Signed-off-by: Benedikt Bongartz <bongartz@klimlive.de> * internal/api/convert: propagate ver upgrade error Signed-off-by: Benedikt Bongartz <bongartz@klimlive.de> * intern/{api,manifests}: directly call yaml unmarshal Signed-off-by: Benedikt Bongartz <bongartz@klimlive.de> * internal/api/convert: map DaemonSetUpdateStrategy Signed-off-by: Benedikt Bongartz <bongartz@klimlive.de> * internal/manifests/collector: migrate configmap Signed-off-by: Benedikt Bongartz <bongartz@klimlive.de> * internal/api/convert: use go-yaml instead of sig-yaml Signed-off-by: Benedikt Bongartz <bongartz@klimlive.de> * pkg/sidecar: upgrade internal use from v1alpha2 to v1alpha2 Signed-off-by: Benedikt Bongartz <bongartz@klimlive.de> * internal/manifests/collector: adapt cm, svc, ingress & route Signed-off-by: Benedikt Bongartz <bongartz@klimlive.de> * internal/api/convert: fix v1alpha1 to v1alpha2 Signed-off-by: Benedikt Bongartz <bongartz@klimlive.de> * internal/manifests/collector: evaluate returned error value in tests Signed-off-by: Benedikt Bongartz <bongartz@klimlive.de> * apis/v1alpha2: config yaml method Signed-off-by: Benedikt Bongartz <bongartz@klimlive.de> * internal/manifests/collector: use v1alpha2.Config yaml method Signed-off-by: Benedikt Bongartz <bongartz@klimlive.de> * controllers/builder: adapt ident in unit test Signed-off-by: Benedikt Bongartz <bongartz@klimlive.de> * internal/manifests/collector: fix test Signed-off-by: Benedikt Bongartz <bongartz@klimlive.de> * tests/e2e: align to new indentation Signed-off-by: Benedikt Bongartz <bongartz@klimlive.de> * internal/manifests/collector: cleanup Signed-off-by: Benedikt Bongartz <bongartz@klimlive.de> --------- Signed-off-by: Benedikt Bongartz <bongartz@klimlive.de>

frzifus force-pushed the v1alphav2 branch 2 times, most recently from 4499392 to ffea150 Compare January 18, 2024 16:10

frzifus added the Skip Changelog PRs that do not require a CHANGELOG.md entry label Jan 18, 2024

frzifus force-pushed the v1alphav2 branch 2 times, most recently from 0eec778 to e5040f6 Compare January 18, 2024 16:58

frzifus marked this pull request as ready for review January 18, 2024 17:00

frzifus requested a review from a team January 18, 2024 17:00

pavolloffay reviewed Jan 18, 2024

View reviewed changes

internal/manifests/collector/annotations.go Outdated Show resolved Hide resolved

pkg/sidecar/pod.go Outdated Show resolved Hide resolved

frzifus force-pushed the v1alphav2 branch from e5040f6 to adcc1f7 Compare January 18, 2024 21:34

frzifus requested review from pavolloffay, jaronoff97 and swiatekm January 18, 2024 21:41

frzifus commented Jan 18, 2024

View reviewed changes

internal/api/convert/v1alpha.go Outdated Show resolved Hide resolved

internal/manifests/collector/container_test.go Show resolved Hide resolved

jaronoff97 reviewed Jan 18, 2024

View reviewed changes

frzifus force-pushed the v1alphav2 branch 4 times, most recently from 778c94d to ee38503 Compare January 23, 2024 14:16

jaronoff97 reviewed Jan 23, 2024

View reviewed changes

internal/api/convert/v1alpha_test.go Outdated Show resolved Hide resolved

internal/api/convert/v1alpha_test.go Outdated Show resolved Hide resolved

internal/manifests/collector/rbac.go Outdated Show resolved Hide resolved

pkg/sidecar/pod.go Outdated Show resolved Hide resolved

frzifus force-pushed the v1alphav2 branch from ee38503 to e468ffd Compare January 23, 2024 17:48

pavolloffay reviewed Jan 23, 2024

View reviewed changes

internal/manifests/collector/daemonset_test.go Outdated Show resolved Hide resolved

internal/manifests/collector/daemonset_test.go Outdated Show resolved Hide resolved

frzifus requested review from pavolloffay and jaronoff97 January 23, 2024 23:07

frzifus force-pushed the v1alphav2 branch from cfd6ee0 to 74fa022 Compare January 24, 2024 00:37

iblancasa reviewed Jan 24, 2024

View reviewed changes

swiatekm mentioned this pull request Jan 25, 2024

Generate TargetAllocator CR from Collector CR #2565

Merged

3 tasks

frzifus added 9 commits February 1, 2024 10:39

internal/api/convert: use go-yaml instead of sig-yaml

ec3cc4c

Signed-off-by: Benedikt Bongartz <bongartz@klimlive.de>

pkg/sidecar: upgrade internal use from v1alpha2 to v1alpha2

fdcd167

Signed-off-by: Benedikt Bongartz <bongartz@klimlive.de>

internal/manifests/collector: adapt cm, svc, ingress & route

16d84f6

Signed-off-by: Benedikt Bongartz <bongartz@klimlive.de>

internal/api/convert: fix v1alpha1 to v1alpha2

ddc747d

Signed-off-by: Benedikt Bongartz <bongartz@klimlive.de>

internal/manifests/collector: evaluate returned error value in tests

10f3ed0

Signed-off-by: Benedikt Bongartz <bongartz@klimlive.de>

apis/v1alpha2: config yaml method

df8df4b

Signed-off-by: Benedikt Bongartz <bongartz@klimlive.de>

internal/manifests/collector: use v1alpha2.Config yaml method

8c73026

Signed-off-by: Benedikt Bongartz <bongartz@klimlive.de>

controllers/builder: adapt ident in unit test

f7c7006

Signed-off-by: Benedikt Bongartz <bongartz@klimlive.de>

internal/manifests/collector: fix test

feaa1e1

Signed-off-by: Benedikt Bongartz <bongartz@klimlive.de>

frzifus force-pushed the v1alphav2 branch from 74fa022 to feaa1e1 Compare February 1, 2024 09:55

tests/e2e: align to new indentation

d6d01bd

Signed-off-by: Benedikt Bongartz <bongartz@klimlive.de>

frzifus force-pushed the v1alphav2 branch from f671e12 to d6d01bd Compare February 1, 2024 10:37

internal/manifests/collector: cleanup

414d68c

Signed-off-by: Benedikt Bongartz <bongartz@klimlive.de>

jaronoff97 reviewed Feb 1, 2024

View reviewed changes

jaronoff97 approved these changes Feb 1, 2024

View reviewed changes

frzifus mentioned this pull request Feb 1, 2024

v1alpha2: make getConfigMapSHA a method of the new CRD #2591

Closed

pavolloffay changed the title ~~internal/manifests/collector: switch internally to v1alphav2~~ internal/manifests/collector: switch internally to v1alpha2 Feb 1, 2024

frzifus mentioned this pull request Feb 1, 2024

v1alpha2: Minimise signatures of object generation functions #2592

Closed

pavolloffay reviewed Feb 1, 2024

View reviewed changes

Merge branch 'main' into v1alphav2

da09025

frzifus requested a review from pavolloffay February 1, 2024 17:30

jaronoff97 mentioned this pull request Feb 1, 2024

Release the operator v0.93.0 #2567

Closed

pavolloffay mentioned this pull request Feb 3, 2024

Remove unnecessary config marshalling in v1beta1 #2603

Open

pavolloffay approved these changes Feb 3, 2024

View reviewed changes

Merge branch 'main' into v1alphav2

99f782c

pavolloffay merged commit 769c984 into open-telemetry:main Feb 5, 2024
27 checks passed

frzifus deleted the v1alphav2 branch February 5, 2024 15:16

swiatekm mentioned this pull request Mar 6, 2024

Generate target allocator manifests from TargetAllocator CR #2725

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

internal/manifests/collector: switch internally to v1alpha2 #2532

internal/manifests/collector: switch internally to v1alpha2 #2532

frzifus commented Jan 18, 2024 •

edited

Loading

iblancasa commented Jan 18, 2024

frzifus commented Jan 18, 2024

jaronoff97 left a comment

jaronoff97 Jan 18, 2024

frzifus Jan 19, 2024

iblancasa Jan 24, 2024

frzifus Feb 1, 2024 •

edited

Loading

frzifus commented Jan 23, 2024

pavolloffay left a comment

frzifus commented Jan 24, 2024

iblancasa Jan 24, 2024

jaronoff97 Feb 1, 2024

frzifus Feb 1, 2024

jaronoff97 left a comment

jaronoff97 Feb 1, 2024

frzifus Feb 1, 2024

jaronoff97 Feb 1, 2024

frzifus Feb 1, 2024

jaronoff97 Feb 1, 2024

frzifus Feb 1, 2024

frzifus Feb 1, 2024

pavolloffay left a comment

pavolloffay Feb 1, 2024

frzifus Feb 1, 2024

pavolloffay Feb 3, 2024

pavolloffay Feb 3, 2024

frzifus commented Feb 1, 2024

internal/manifests/collector: switch internally to v1alpha2 #2532

internal/manifests/collector: switch internally to v1alpha2 #2532

Conversation

frzifus commented Jan 18, 2024 • edited Loading

iblancasa commented Jan 18, 2024

frzifus commented Jan 18, 2024

jaronoff97 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frzifus Feb 1, 2024 • edited Loading

Choose a reason for hiding this comment

frzifus commented Jan 23, 2024

pavolloffay left a comment

Choose a reason for hiding this comment

frzifus commented Jan 24, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaronoff97 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pavolloffay left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frzifus commented Feb 1, 2024

frzifus commented Jan 18, 2024 •

edited

Loading

frzifus Feb 1, 2024 •

edited

Loading