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

chore(opentelemetry-operator): upgrade opentelemetry-operator subchart to 0.13.0 #2561

Merged
merged 23 commits into from
Oct 18, 2022

Conversation

mat-rumian
Copy link
Contributor

@mat-rumian mat-rumian commented Oct 12, 2022

Description

This PR:

  • updates OpenTelemetry Operator subchart to latest version
  • moves Instrumentation CR generation to confs (config map created for this purpose)
  • adds Job which applies Instrumentation CRs to specified namespaces
  • disables by default creation of Instrumentation resource - opentelemetry-operator.createDefaultInstrumentation=false
  • adds opentelemetry-operator.instrumentationNamespaces parameter for Instrumentation resource creation

Checklist
  • Changelog updated
Testing performed
  • Java instrumentation with version 1.16.0
  • Python instrumentation with version 0.28b0
  • NodeJS instrumentation with version 0.27.0
  • .NET instrumentation with version 0.3.1-beta.1
  • self-signed certificate creation
  • Instrumentation CR creation (new Job)

@mat-rumian mat-rumian marked this pull request as ready for review October 14, 2022 08:05
@mat-rumian mat-rumian requested a review from a team as a code owner October 14, 2022 08:05
Comment on lines 1544 to 1549
---
apiVersion: admissionregistration.k8s.io/v1
kind: MutatingWebhookConfiguration
apiVersion: opentelemetry.io/v1alpha1
kind: Instrumentation
metadata:
annotations:
cert-manager.io/inject-ca-from: {{ printf "%s/%s" .Release.Namespace ( include "opentelemetry-operator.controller.manager.service.cert.name" . ) }}
namespace: {{ $ns }}
name: {{ template "sumologic.metadata.name.opentelemetry.operator.instrumentation" $ctx }}
Copy link
Contributor

Choose a reason for hiding this comment

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

I would put it in separate file

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've moved Instrumentation CR generation to separated file located in confs/opentelemetry-operator

@mat-rumian mat-rumian changed the title WIP: Update OpenTelemetry Operator subchart Update OpenTelemetry Operator subchart Oct 14, 2022
Copy link
Contributor

@sumo-drosiek sumo-drosiek left a comment

Choose a reason for hiding this comment

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

Generally LGTM, but I would put instrumentation template in separate file

@mat-rumian mat-rumian changed the title Update OpenTelemetry Operator subchart chore(opentelemetry-operator): upgrade opentelemetry-operator subchart to 0.13.0 Oct 17, 2022
@github-actions github-actions bot added the documentation documentation label Oct 17, 2022
{{- $ctx := . -}}
{{ $watchNamespace := index .Values "opentelemetry-operator" "manager" "env" }}
{{- if eq ( get $watchNamespace "WATCH_NAMESPACE" ) "" }}
{{ fail "It is mandatory to set value for \"opentelemetry-operator.manager.env.WATCH_NAMESPACE\" when opentelemetry-operator is enabled. Value is comma separated namespaces e.g. \"ns1\\,ns2\"" }}

Choose a reason for hiding this comment

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

Why is this mandatory, actually? A user may not want to use Instrumentation at all and just have the operator manage OT in all namespaces.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If WATCH_NAMESPACE is not provided then in theory Operator should watch for all namespaces but it doesn't. At least in case of Instrumentation.

Logs say that Instrumentation resource exists and became default

{"level":"info","ts":1666018961.0456557,"logger":"instrumentation-resource","msg":"default","name":"collection-sumologic-ot-operator-instr"}
{"level":"info","ts":1666018961.0561447,"logger":"instrumentation-resource","msg":"validate update","name":"collection-sumologic-ot-operator-instr"}

But when I add annotation to the e.g. Deployment I'm getting this kind of error. Doesn't matter if annotation value is true or name-of-the-instrumentation-resource.

{"level":"error","ts":1666018765.0062091,"msg":"failed to select an OpenTelemetry Instrumentation instance for this pod","namespace":"","name":"","error":"no OpenTelemetry Instrumentation instances available","stacktrace":"github.com/open-telemetry/opentelemetry-operator/pkg/instrumentation.(*instPodMutator).Mutate\n\t/workspace/pkg/instrumentation/podmutator.go:102\ngithub.com/open-telemetry/opentelemetry-operator/internal/webhookhandler.(*podSidecarInjector).Handle\n\t/workspace/internal/webhookhandler/webhookhandler.go:92\nsigs.k8s.io/controller-runtime/pkg/webhook/admission.(*Webhook).Handle\n\t/go/pkg/mod/sigs.k8s.io/controller-runtime@v0.13.0/pkg/webhook/admission/webhook.go:169\nsigs.k8s.io/controller-runtime/pkg/webhook/admission.(*Webhook).ServeHTTP\n\t/go/pkg/mod/sigs.k8s.io/controller-runtime@v0.13.0/pkg/webhook/admission/http.go:98\ngithub.com/prometheus/client_golang/prometheus/promhttp.InstrumentHandlerInFlight.func1\n\t/go/pkg/mod/github.com/prometheus/client_golang@v1.12.2/prometheus/promhttp/instrument_server.go:40\nnet/http.HandlerFunc.ServeHTTP\n\t/usr/local/go/src/net/http/server.go:2109\ngithub.com/prometheus/client_golang/prometheus/promhttp.InstrumentHandlerCounter.func1\n\t/go/pkg/mod/github.com/prometheus/client_golang@v1.12.2/prometheus/promhttp/instrument_server.go:117\nnet/http.HandlerFunc.ServeHTTP\n\t/usr/local/go/src/net/http/server.go:2109\ngithub.com/prometheus/client_golang/prometheus/promhttp.InstrumentHandlerDuration.func2\n\t/go/pkg/mod/github.com/prometheus/client_golang@v1.12.2/prometheus/promhttp/instrument_server.go:84\nnet/http.HandlerFunc.ServeHTTP\n\t/usr/local/go/src/net/http/server.go:2109\nnet/http.(*ServeMux).ServeHTTP\n\t/usr/local/go/src/net/http/server.go:2487\nnet/http.serverHandler.ServeHTTP\n\t/usr/local/go/src/net/http/server.go:2947\nnet/http.(*conn).serve\n\t/usr/local/go/src/net/http/server.go:1991"}

I believe this is something to improve in the upstream.

Choose a reason for hiding this comment

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

Hm, but what if you don't want instrumentation at all? I think we used to have a setting to disable default instrumentation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're getting instrumentation out of the box. It's just a matter of creation of Instrumentation resource in the specific namespace.

@swiatekm swiatekm self-requested a review October 18, 2022 12:27
@mat-rumian mat-rumian merged commit d4d7599 into main Oct 18, 2022
@mat-rumian mat-rumian deleted the update-opentelemetry-operator-chart branch October 18, 2022 12:49
@sumo-backporter
Copy link
Contributor

The backport to release-v2 failed:

The process '/usr/bin/git' failed with exit code 1

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-release-v2 release-v2
# Navigate to the new working tree
cd .worktrees/backport-release-v2
# Create a new branch
git switch --create backport-2561-to-release-v2
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 d4d75994a1c31f9daabca8b25281b81ca9a301f8
# Push it to GitHub
git push --set-upstream origin backport-2561-to-release-v2
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-release-v2

Then, create a pull request where the base branch is release-v2 and the compare/head branch is backport-2561-to-release-v2.

mat-rumian added a commit that referenced this pull request Oct 19, 2022
…t to 0.13.0 (#2561)

* chore(chart): update version

* chore(tls): remove certificate creation, supported by operator chart

* chore(clusterole): add opentelemetry-operator api group

* chore(instrumentation): add instrumentation configmap and job

* chore(instrumentatio-cr): move instrumentation cr to confs

* chore(tests): update tests

* chore(opentelemetry-operator): split instrumentation resource creation

* chore(readme): update parameters list

(cherry picked from commit d4d7599)
mat-rumian added a commit that referenced this pull request Oct 20, 2022
#2577)

* chore(opentelemetry-operator): upgrade opentelemetry-operator subchart to 0.13.0 (#2561)

* chore(tls): remove certificate creation, supported by operator chart

* chore(clusterole): add opentelemetry-operator api group

* chore(instrumentation): add instrumentation configmap and job

* chore(instrumentatio-cr): move instrumentation cr to confs

* chore(tests): update tests

* chore(opentelemetry-operator): split instrumentation resource creation

* chore(readme): update parameters list

(cherry picked from commit d4d7599)

* docs(changelog): update
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants