Reconcile v1beta1 collector CRD #2703

pavolloffay · 2024-03-04T15:23:56Z

Description:

Do not merge this. the PR will be split into smaller PRs.

Link to tracking Issue:
Resolves: #2620

This can be merged after #2708

Follow ups:

Testing:

Documentation:

internal/manifests/collector/horizontalpodautoscaler.go

pavolloffay · 2024-03-06T16:03:34Z

pkg/collector/upgrade/versions.go

-	upgrade upgradeFunc
+	// deprecated use upgradeV1beta1.
+	upgrade        upgradeFunc
+	upgradeV1beta1 upgradeFuncV1beta1


I didn't want to rewrite old upgrade procedures hence I am adding this API and just adopting tests to work (they need a valid collector spec because v1beta1 is a minimal doing validation)

i wonder if we should also include some deprecations around these collector version upgrades. like do we need to keep around something that upgrades collector from 0.30.0 to 0.31.0?

swiatekm

This looks like a good time to upgrade kubebuilder and other similar dependencies. WDYT?

pavolloffay · 2024-03-06T17:44:39Z

This looks like a good time to upgrade kubebuilder and other similar dependencies. WDYT?

Could you please be more specific about why it is related to this PR?

swiatekm · 2024-03-06T18:06:32Z

This looks like a good time to upgrade kubebuilder and other similar dependencies. WDYT?

Could you please be more specific about why it is related to this PR?

There's some differences in output, so it would be easier if we switch before officially publishing v1beta1 CRDs. But you're right, that doesn't need to happen in this PR.

swiatekm · 2024-03-07T16:11:37Z

Makefile

@@ -39,7 +39,8 @@ BUNDLE_DEFAULT_CHANNEL := --default-channel=$(DEFAULT_CHANNEL)
 endif
 BUNDLE_METADATA_OPTS ?= $(BUNDLE_CHANNELS) $(BUNDLE_DEFAULT_CHANNEL)

-CRD_OPTIONS ?= "crd:generateEmbeddedObjectMeta=true,maxDescLen=200"
+# kubectl apply does not work on large CRDs.
+CRD_OPTIONS ?= "crd:generateEmbeddedObjectMeta=true,maxDescLen=0"


I don't think we want to publish CRDs with no descriptions on fields?

Unfortunately the kubectl apply on the CRD will not work (https://github.com/open-telemetry/opentelemetry-operator?tab=readme-ov-file#getting-started).

It will break installations, maybe helm, gitops and e2e tests?

I can take a look at enabling descriptions in a follow-up PR. There we will better see what the implications are. Please book a follow-up ticket if you think we should have descriptions.

I think a server side apply actually does work for these

apis/v1alpha1/opentelemetrycollector_types.go

apis/v1beta1/collector_webhook.go

swiatekm

I don't see any problems. We should track the issue with descriptions and v1beta1 webhook though.

pavolloffay · 2024-03-08T14:24:11Z

thanks for the approval. I have created follow issues and linked them to the PR description.

@jaronoff97 I think this PR deserves your review as well and please merge it once 0.96.0 operator is released so we have some time to test it in the main.

jaronoff97

mostly questions to confirm my understanding of what's happening here. This looks pretty good overall though.

jaronoff97 · 2024-03-08T14:31:59Z

.chloggen/v1beta1-webhook.yaml

+subtext: | 
+  Users are expected to migrate to `otelcol.v1beta1.opentelemetry.io`. 
+  The support for `otelcol.v1alpha1.opentelemetry.io` will be removed in the future.
+  Follow https://kubernetes.io/docs/tasks/extend-kubernetes/custom-resources/custom-resource-definition-versioning/#upgrade-existing-objects-to-a-new-stored-version for version migration.


i wonder if we should include our own documentation for how to do this. Specifically calling out the deprecated fields and the things to migrate. Especially when it comes to migrating the TA to the new TA crd as well.

I think what we have might be good enough for now. However we should definitely talk about until when v1alpha1 will be served.

jaronoff97 · 2024-03-08T14:34:38Z

Makefile

@@ -39,7 +39,8 @@ BUNDLE_DEFAULT_CHANNEL := --default-channel=$(DEFAULT_CHANNEL)
 endif
 BUNDLE_METADATA_OPTS ?= $(BUNDLE_CHANNELS) $(BUNDLE_DEFAULT_CHANNEL)

-CRD_OPTIONS ?= "crd:generateEmbeddedObjectMeta=true,maxDescLen=200"
+# kubectl apply does not work on large CRDs.
+CRD_OPTIONS ?= "crd:generateEmbeddedObjectMeta=true,maxDescLen=0"


I think a server side apply actually does work for these

apis/v1beta1/collector_webhook.go

jaronoff97 · 2024-03-08T14:37:56Z

bundle/manifests/opentelemetry-operator.clusterserviceversion.yaml

+      kind: OpenTelemetryCollector
+      name: opentelemetrycollectors.opentelemetry.io
+      resources:
+      - kind: ConfigMaps


Is this autogenerated? Aren't we missing some resources like HPA, PDB, ingress, etc?

yes it is, the annotation is on the collector type. This is the same as for v1alpha1 and it's just a hint for users to understand which objects are managed.

i see. should we update it with the rest of the resources so its accurate going forward? Maybe a good separate PR...

jaronoff97 · 2024-03-08T14:38:32Z

bundle/manifests/opentelemetry-operator.clusterserviceversion.yaml

    type: OwnNamespace
-  - supported: true
+  - supported: false


what do these flags do?

IIRC It's for the OLM to allow watching of single or multiple namespaces. This was required for introducing the new version.

hm in practice this makes me nervous, doesn't this imply that the operator can no longer watch any namespaces? can you link me the doc?

https://olm.operatorframework.io/docs/tasks/install-operator-with-olm/

https://docs.openshift.com/container-platform/4.15/operators/understanding/olm/olm-understanding-operatorgroups.html#olm-operatorgroups-membership_olm-understanding-operatorgroups

https://docs.openshift.com/container-platform/4.15/operators/operator_sdk/osdk-generating-csvs.html

I have noted this in the release notes

jaronoff97 · 2024-03-08T14:40:08Z

config/crd/patches/webhook_in_opentelemetrycollectors.yaml

@@ -2,15 +2,16 @@
 apiVersion: apiextensions.k8s.io/v1
 kind: CustomResourceDefinition
 metadata:
-  name: opampbridges.opentelemetry.io
+  name: opentelemetrycollectors.opentelemetry.io


why does it think its changing the opampbridge CRD here?

I changed this manually. The file was not used anyways.

jaronoff97 · 2024-03-08T14:44:12Z

pkg/collector/upgrade/upgrade.go

-			if err != nil {
-				u.Log.Error(err, "failed to upgrade managed otelcol instances", "name", otelcol.Name, "namespace", otelcol.Namespace)
-				return otelcol, err
+			if available.upgrade != nil {


i wonder if we should be recording some metrics about these upgrades:

how often they succeed / fail

reason for failure

I think this is out of the scope of this PR. We should probably book a separate ticket for this if necessary.

jaronoff97 · 2024-03-08T14:47:12Z

pkg/collector/upgrade/versions.go

-	upgrade upgradeFunc
+	// deprecated use upgradeV1beta1.
+	upgrade        upgradeFunc
+	upgradeV1beta1 upgradeFuncV1beta1


i wonder if we should also include some deprecations around these collector version upgrades. like do we need to keep around something that upgrades collector from 0.30.0 to 0.31.0?

jaronoff97

two questions. One other thing i thought of which would be a good followup to book would be disabling the reconciliation of v1alpha1 resources behind a featuregate

jaronoff97 · 2024-03-11T18:48:06Z

bundle/manifests/opentelemetry-operator.clusterserviceversion.yaml

    type: OwnNamespace
-  - supported: true
+  - supported: false


hm in practice this makes me nervous, doesn't this imply that the operator can no longer watch any namespaces? can you link me the doc?

internal/status/collector/handle.go

internal/status/collector/collector.go

pavolloffay · 2024-03-12T09:07:46Z

One other thing i thought of which would be a good followup to book would be disabling the reconciliation of v1alpha1 resources behind a featuregate

This PR removes v1alpha1 reconciliation. The remaining v1alpha1 pieces will be: CRD and webhooks. Both of these require manifests at the installation time so they cannot be controlled via the operator feature flag.

jaronoff97 · 2024-03-12T18:14:58Z

@pavolloffay let's wait to merge this until after #2756 is done

Signed-off-by: Pavol Loffay <p.loffay@gmail.com>

…2703)" This reverts commit 9710eed

This reverts commit 9710eed

…lemetry#2703)" (open-telemetry#2800)" This reverts commit 57024fc.

#2813) * Revert "Revert" Add reconciliation for Collector v1beta1 CRD (#2703)" (#2800)" This reverts commit 57024fc. * Fix image Signed-off-by: Pavol Loffay <p.loffay@gmail.com> * Clarify hostport Signed-off-by: Pavol Loffay <p.loffay@gmail.com> * Fix issue after israel Signed-off-by: Pavol Loffay <p.loffay@gmail.com> * Fix Signed-off-by: Pavol Loffay <p.loffay@gmail.com> --------- Signed-off-by: Pavol Loffay <p.loffay@gmail.com>

Signed-off-by: Pavol Loffay <p.loffay@gmail.com>

…2703)" (open-telemetry#2800) This reverts commit 9710eed

…lemetry#2703)"… (open-telemetry#2813) * Revert "Revert" Add reconciliation for Collector v1beta1 CRD (open-telemetry#2703)" (open-telemetry#2800)" This reverts commit 57024fc. * Fix image Signed-off-by: Pavol Loffay <p.loffay@gmail.com> * Clarify hostport Signed-off-by: Pavol Loffay <p.loffay@gmail.com> * Fix issue after israel Signed-off-by: Pavol Loffay <p.loffay@gmail.com> * Fix Signed-off-by: Pavol Loffay <p.loffay@gmail.com> --------- Signed-off-by: Pavol Loffay <p.loffay@gmail.com>

pavolloffay mentioned this pull request Mar 4, 2024

POC: Reconcile v1alpha2 crd #2637

Closed

pavolloffay force-pushed the v1beta1-webhook branch from dca4839 to eecca66 Compare March 6, 2024 10:16

pavolloffay commented Mar 6, 2024

View reviewed changes

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

pavolloffay commented Mar 6, 2024

View reviewed changes

pavolloffay force-pushed the v1beta1-webhook branch from d175ceb to a8a3798 Compare March 6, 2024 16:17

pavolloffay changed the title ~~V1beta1 webhookPOC: Reconcile v1beta1 crd~~ Reconcile v1beta1 collector CRD Mar 6, 2024

pavolloffay force-pushed the v1beta1-webhook branch 2 times, most recently from 3636c19 to e92d64b Compare March 6, 2024 17:19

swiatekm reviewed Mar 6, 2024

View reviewed changes

pavolloffay marked this pull request as ready for review March 7, 2024 09:13

pavolloffay requested a review from a team March 7, 2024 09:13

pavolloffay force-pushed the v1beta1-webhook branch from b207213 to cc3ab94 Compare March 7, 2024 12:59

swiatekm reviewed Mar 7, 2024

View reviewed changes

swiatekm approved these changes Mar 8, 2024

View reviewed changes

pavolloffay mentioned this pull request Mar 8, 2024

Migrate v1alpha1 webhook to v1beta1 #2736

Closed

pavolloffay mentioned this pull request Mar 8, 2024

Add CRD descriptions #2737

Open

jaronoff97 reviewed Mar 8, 2024

View reviewed changes

pavolloffay mentioned this pull request Mar 11, 2024

Updated optional resources manged by the operator #2746

Closed

pavolloffay force-pushed the v1beta1-webhook branch from b4a473c to 523f7ce Compare March 11, 2024 13:32

jaronoff97 reviewed Mar 11, 2024

View reviewed changes

pavolloffay force-pushed the v1beta1-webhook branch 6 times, most recently from 9bda806 to 1c69e22 Compare March 12, 2024 14:03

jaronoff97 approved these changes Mar 12, 2024

View reviewed changes

Add reconciliation for Collector v1beta1 CRD

857ea86

Signed-off-by: Pavol Loffay <p.loffay@gmail.com>

pavolloffay force-pushed the v1beta1-webhook branch from 1c69e22 to 857ea86 Compare March 14, 2024 10:05

pavolloffay merged commit 9710eed into open-telemetry:main Mar 14, 2024
31 checks passed

swiatekm added a commit to swiatekm/opentelemetry-operator that referenced this pull request Mar 29, 2024

Revert "Add reconciliation for Collector v1beta1 CRD (open-telemetry#…

6ae6b38

…2703)" This reverts commit 9710eed

swiatekm added a commit to swiatekm/opentelemetry-operator that referenced this pull request Apr 2, 2024

Revert "Add reconciliation for Collector v1beta1 CRD (open-telemetry#…

89de713

…2703)" This reverts commit 9710eed

pavolloffay pushed a commit that referenced this pull request Apr 3, 2024

Revert "Add reconciliation for Collector v1beta1 CRD (#2703)" (#2800)

57024fc

This reverts commit 9710eed

pavolloffay added a commit to pavolloffay/opentelemetry-operator that referenced this pull request Apr 5, 2024

Revert "Revert" Add reconciliation for Collector v1beta1 CRD (open-te…

cb4b9f4

…lemetry#2703)" (open-telemetry#2800)" This reverts commit 57024fc.

pavolloffay mentioned this pull request Apr 5, 2024

Revert "Revert" Add reconciliation for Collector v1beta1 CRD (#2703)"… #2813

Merged

pavolloffay added a commit to pavolloffay/opentelemetry-operator that referenced this pull request Apr 12, 2024

Revert "Revert" Add reconciliation for Collector v1beta1 CRD (open-te…

8dd6972

…lemetry#2703)" (open-telemetry#2800)" This reverts commit 57024fc.

ItielOlenick pushed a commit to ItielOlenick/opentelemetry-operator that referenced this pull request May 1, 2024

Add reconciliation for Collector v1beta1 CRD (open-telemetry#2703)

1741863

Signed-off-by: Pavol Loffay <p.loffay@gmail.com>

ItielOlenick pushed a commit to ItielOlenick/opentelemetry-operator that referenced this pull request May 1, 2024

Revert "Add reconciliation for Collector v1beta1 CRD (open-telemetry#…

478c3c2

…2703)" (open-telemetry#2800) This reverts commit 9710eed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reconcile v1beta1 collector CRD #2703

Reconcile v1beta1 collector CRD #2703

pavolloffay commented Mar 4, 2024 •

edited

Loading

pavolloffay Mar 6, 2024

jaronoff97 Mar 8, 2024

swiatekm left a comment

pavolloffay commented Mar 6, 2024

swiatekm commented Mar 6, 2024

swiatekm Mar 7, 2024

pavolloffay Mar 8, 2024

jaronoff97 Mar 8, 2024

swiatekm left a comment

pavolloffay commented Mar 8, 2024 •

edited

Loading

jaronoff97 left a comment

jaronoff97 Mar 8, 2024

pavolloffay Mar 8, 2024

jaronoff97 Mar 8, 2024

jaronoff97 Mar 8, 2024

pavolloffay Mar 8, 2024

jaronoff97 Mar 8, 2024

pavolloffay Mar 11, 2024

jaronoff97 Mar 8, 2024

pavolloffay Mar 8, 2024

jaronoff97 Mar 11, 2024

pavolloffay Mar 12, 2024

pavolloffay Mar 12, 2024

jaronoff97 Mar 8, 2024

pavolloffay Mar 8, 2024

jaronoff97 Mar 8, 2024

pavolloffay Mar 8, 2024

jaronoff97 Mar 8, 2024

jaronoff97 left a comment

jaronoff97 Mar 11, 2024

pavolloffay commented Mar 12, 2024

jaronoff97 commented Mar 12, 2024

Reconcile v1beta1 collector CRD #2703

Reconcile v1beta1 collector CRD #2703

Conversation

pavolloffay commented Mar 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

swiatekm left a comment

Choose a reason for hiding this comment

pavolloffay commented Mar 6, 2024

swiatekm commented Mar 6, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

swiatekm left a comment

Choose a reason for hiding this comment

pavolloffay commented Mar 8, 2024 • edited Loading

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

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

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

pavolloffay commented Mar 12, 2024

jaronoff97 commented Mar 12, 2024

pavolloffay commented Mar 4, 2024 •

edited

Loading

pavolloffay commented Mar 8, 2024 •

edited

Loading