Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update docs and values.shema.json for operatorcrds migration #1619

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

jvoravong
Copy link
Contributor

Description:

  • Add breaking change and migration notes
  • Update values.schema.json to prevent misusage

@jvoravong jvoravong requested review from a team as code owners January 16, 2025 16:35
Copy link
Contributor

@crobert-1 crobert-1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just some documentation nits, thanks for adding so much information with the change!

I'm not familiar enough with functionality to confidently sign off, but nothing looks off from what I understand.

.chloggen/move-operator-crd-install-method.yaml Outdated Show resolved Hide resolved
docs/auto-instrumentation-install.md Outdated Show resolved Hide resolved
docs/auto-instrumentation-install.md Outdated Show resolved Hide resolved

#### Alternative Approach: Manual CRD Deployment

If you prefer to manage CRD deployment manually, apply the CRDs using the commands below before installing the Helm chart:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How likely is it that these commands get outdated? Is there any way to make this into a test to ensure this doesn't go stale?

This is not a blocker for this PR, just something to consider.


#### CRD Cleanup

When uninstalling this chart, the OpenTelemetry CRDs are not removed automatically. To delete them manually, use the following commands:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as previous comment, not blocking: Is there any way to ensure these commands stay up to date and work properly?

curl -sL https://raw.githubusercontent.com/open-telemetry/opentelemetry-operator/main/config/crd/bases/opentelemetry.io_opampbridges.yaml | kubectl apply -f -
curl -sL https://raw.githubusercontent.com/open-telemetry/opentelemetry-operator/main/config/crd/bases/opentelemetry.io_instrumentations.yaml | kubectl apply -f -
```

Copy link
Collaborator

@jinja2 jinja2 Jan 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
You can also use below helm template command to get the CRD yamls from the helm chart. This method can be helpful in keeping CRDs in-sync with the version bundled with our helm chart.
```bash
helm template helm-charts/splunk-otel-collector/ --include-crds \
--set="splunkObservability.realm=us0,splunkObservability.accessToken=xxxxxx,clusterName=my-cluster,operatorcrds.install=true" \
| yq e '. | select(.kind == "CustomResourceDefinition")' \
| kubectl apply -f -

We can direct users to leverage the our chart to get the crds directly, esp since they can have their CD pipelines just get the correct CRD version from the chart version they want to install

kubectl delete crd opampbridges.opentelemetry.io
kubectl delete crd instrumentations.opentelemetry.io
```

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
You can use below combination of helm and kubectl command to delete CRDs.
```bash
helm template helm-charts/splunk-otel-collector/ --include-crds \
--set="splunkObservability.realm=us0,splunkObservability.accessToken=xxxxxx,clusterName=my-cluster,operatorcrds.install=true" \
| yq e '. | select(.kind == "CustomResourceDefinition")' \
| kubectl delete --dry-run=client -f -

Comment on lines +13 to +15
- Users enabling the operator (`.Values.operator.enabled=true`) must now set `operatorcrds.install=true` in Helm values or [manually manage CRD installation](https://github.com/signalfx/splunk-otel-collector-chart/blob/main/docs/auto-instrumentation-install.md#crd-management).
- Previously, CRDs were installed using templates (`operator.crds.create=true`), which could cause race conditions and installation failures.
- CRD installation is now handled via Helm's native `crds/` directory for better stability, using a [localized subchart](https://github.com/signalfx/splunk-otel-collector-chart/tree/main/helm-charts/splunk-otel-collector/charts/opentelemetry-operator-crds).
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
- Users enabling the operator (`.Values.operator.enabled=true`) must now set `operatorcrds.install=true` in Helm values or [manually manage CRD installation](https://github.com/signalfx/splunk-otel-collector-chart/blob/main/docs/auto-instrumentation-install.md#crd-management).
- Previously, CRDs were installed using templates (`operator.crds.create=true`), which could cause race conditions and installation failures.
- CRD installation is now handled via Helm's native `crds/` directory for better stability, using a [localized subchart](https://github.com/signalfx/splunk-otel-collector-chart/tree/main/helm-charts/splunk-otel-collector/charts/opentelemetry-operator-crds).
- Users must set `operatorcrds.install=true` along with the existing option `operator.enabled=true` to install CRDs when deploying the opentelemetry-operator from this chart version onwards.
- CRD installation is now handled via Helm's native `crds/` directory for better stability, using a [localized subchart](https://github.com/signalfx/splunk-otel-collector-chart/tree/main/helm-charts/splunk-otel-collector/charts/opentelemetry-operator-crds). Refer to the [helm guide](https://helm.sh/docs/chart_best_practices/custom_resource_definitions/#method-1-let-helm-do-it-for-you) for further information about the new approach.
- Users trying to upgrade to this chart version with the operator already enabled (`operator.enabled=true`) can keep the installed CRDs by setting the arg `operator.crds.create=true`. The chart sets this to `false` by default in this version. This deprecated approach installs CRDs using templates which could cause race conditions and installation failures. Users are encouraged to reinstall with the new CRDs installation approach.

Maybe something like this will work. Assuming we are removing the schema change for operator.crds.create.


#### Recommended Approach: Automated CRD Deployment

Set the Helm chart value `operatorcrds.install=true` to allow the chart to handle CRD deployment automatically.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Set the Helm chart value `operatorcrds.install=true` to allow the chart to handle CRD deployment automatically.
Set the Helm chart value `operatorcrds.install=true` to allow the chart to handle CRD installation automatically.

clarifies it is only useful on an install

#### Recommended Approach: Automated CRD Deployment

Set the Helm chart value `operatorcrds.install=true` to allow the chart to handle CRD deployment automatically.
_This option deploys the CRDs using a local subchart, available at [opentelemetry-operator-crds](https://github.com/signalfx/splunk-otel-collector-chart/tree/main/helm-charts/splunk-otel-collector/charts/opentelemetry-operator-crds)._
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
_This option deploys the CRDs using a local subchart, available at [opentelemetry-operator-crds](https://github.com/signalfx/splunk-otel-collector-chart/tree/main/helm-charts/splunk-otel-collector/charts/opentelemetry-operator-crds)._
_This option deploys the CRDs using a local subchart, available at [opentelemetry-operator-crds](https://github.com/signalfx/splunk-otel-collector-chart/tree/main/helm-charts/splunk-otel-collector/charts/opentelemetry-operator-crds)._ _Please note, helm will not update or delete these CRDs after initial install as noted in their [documentation](https://helm.sh/docs/chart_best_practices/custom_resource_definitions/#some-caveats-and-explanations)._

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants