Jaeger v2 with v1 #613

hellspawn679 · 2024-10-26T18:20:35Z

What this PR does

Which issue this PR fixes

(optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format,
will close that issue when PR gets merged)

fixes #

Checklist

DCO signed
Commits are GPG signed
Chart Version bumped
Title of the PR starts with chart name ([jaeger] or [jaeger-operator])
README.md has been updated to match version/contain new values

Signed-off-by: Reddysekhar Gaduputi <gsekhar73@gmail.com> upgrade jaeger-operator to latest 1.61.0 (jaegertracing#605) Signed-off-by: Blair Bowden <blair@radiusmethod.com> added all-in-one deployment and configmap for jaeger-v2 Signed-off-by: Mehul <mehulsharma4786@gmail.com> lint fix Signed-off-by: Mehul <mehulsharma4786@gmail.com> fixed Signed-off-by: Mehul <mehulsharma4786@gmail.com> lint fix Signed-off-by: Mehul <mehulsharma4786@gmail.com> lint fix Signed-off-by: Mehul <mehulsharma4786@gmail.com> fixed using pre-hook Signed-off-by: Mehul <mehulsharma4786@gmail.com> fixed --config flag is not been passed Signed-off-by: mehul <mehulsharam4786@gmail.com> release ns for config-map.yaml Signed-off-by: mehul <mehulsharam4786@gmail.com> testing ci Signed-off-by: mehul <mehulsharam4786@gmail.com> testing ci Signed-off-by: mehul <mehulsharam4786@gmail.com> fixed ns Signed-off-by: mehul <mehulsharam4786@gmail.com> fixed ns Signed-off-by: mehul <mehulsharam4786@gmail.com> fixed template Signed-off-by: mehul <mehulsharam4786@gmail.com> removed sampling Signed-off-by: mehul <mehulsharam4786@gmail.com> removed adaptive sampling from processors Signed-off-by: mehul <mehulsharam4786@gmail.com> Revert "Jaeger v2 test2" Signed-off-by: mehul <mehulsharam4786@gmail.com> attempt to create v2 chart in v1 Signed-off-by: mehul <mehulsharam4786@gmail.com> enabled collector query and agent Signed-off-by: mehul <mehulsharam4786@gmail.com> version bump Signed-off-by: mehul <mehulsharam4786@gmail.com> testing-v2-ci Signed-off-by: mehul <mehulsharam4786@gmail.com> testing-v2-ci Signed-off-by: mehul <mehulsharam4786@gmail.com> added --helm-extra-set-args flag Signed-off-by: mehul <mehulsharam4786@gmail.com> fixed healthcheck port-v2 Signed-off-by: mehul <mehulsharam4786@gmail.com> Fix health check path Signed-off-by: Yuri Shkuro <github@ysh.us> minor changes Signed-off-by: mehul <mehulsharam4786@gmail.com>

Signed-off-by: mehul gautam <mehulsharma4786@gmail.com>

Signed-off-by: mehul <mehulsharam4786@gmail.com>

…elm-charts into jaeger-v2-with-v1

Signed-off-by: mehul <mehulsharam4786@gmail.com>

yurishkuro · 2024-10-27T18:52:20Z

charts/jaeger/values.yaml

@@ -96,6 +81,75 @@ allInOne:
    fsGroup: 10001
  securityContext: {}

+service:


I don't like how you are mixing the OTEL config sections with chart sections like provisionDataStore. Is it possible to wrap the config under parent config: element?

charts/jaeger/values.yaml

yurishkuro · 2024-10-27T18:53:33Z

.github/workflows/lint-test.yaml

@@ -69,4 +69,5 @@ jobs:
          cmctl check api --wait=5m

      - name: Run chart-testing (install)
-        run: ct install --config ct.yaml
+        run: |
+            ct install --config ct.yaml --helm-extra-set-args "--set v2.enabled=true --set provisionDataStore.cassandra=false --set storage.type=memory --set allInOne.enabled=true --set agent.enabled=false --set collector.enabled=false --set query.enabled=false" 


--set v2.enabled=true left over?

oh i changes v2 to jaeger will fix it now

yurishkuro · 2024-10-27T18:55:26Z

charts/jaeger/templates/_helpers.tpl

+
+
+
+{{- define "jaeger-v2.name" -}}


we already discussed this - all these settings are already present in the chart without the v2 name, so please use the existing ones. I don't think there should be any properties / templates that have v2 in the name

yurishkuro · 2024-10-27T18:56:07Z

charts/jaeger/templates/_helpers.tpl

+{{- define "jaeger-v2.extensionsConfig" -}}
+{{ toYaml .Values.extensions | nindent 6 }}
+{{- end }}
+
+{{- define "jaeger-v2.receiversConfig" -}}
+{{ toYaml .Values.receivers | nindent 6 }}
+{{- end }}
+
+{{- define "jaeger-v2.processorsConfig" -}}
+{{ toYaml .Values.processors | nindent 6 }}
+{{- end }}
+
+{{- define "jaeger-v2.exportersConfig" -}}
+{{ toYaml .Values.exporters | nindent 6 }}
+{{- end }}


these can be inlined directly in the template, they provide no value

yurishkuro · 2024-10-27T18:57:32Z

charts/jaeger/templates/allinone-sa.yaml

@@ -1,14 +0,0 @@
-{{- if .Values.allInOne.enabled -}}


why is this deleted? all-in-one is still a valid deployment and will need a ServiceAccount

yurishkuro · 2024-10-27T18:58:13Z

charts/jaeger/templates/jaeger-v2-agent-svc.yaml

@@ -1,4 +1,4 @@
-{{- if .Values.allInOne.enabled -}}


why do we need to rename all these files? Please keep the same names - there should be no files with v2 in the name.

Signed-off-by: mehul <mehulsharam4786@gmail.com>

yurishkuro · 2024-10-30T02:55:27Z

charts/jaeger/Chart.yaml

 description: A Jaeger Helm chart for Kubernetes
 name: jaeger
 type: application
-version: 3.3.1
+version: 3.3.2


maybe go to 4.x? It's not backwards compatible with chart-3.x which only works with jaeger-v1

yurishkuro · 2024-10-30T02:56:55Z

charts/jaeger/templates/allInOne-config.yaml

+  name: jaeger-configmap
+  namespace: {{ include "jaeger.namespace" . }}
+  labels:
+        {{- include "jaeger.labels" . | nindent 4 }}


is this much whitespace on the left expected? Could we use normal indentation

labels: {{- include "jaeger.labels" . | nindent 4 }}

could you explain what do yo mean by "normal indentation" i used this format because that is how labels are indented thought-out the chart

I showed example

oh got it will fix it

charts/jaeger/templates/allInOne-config.yaml

Signed-off-by: mehul <mehulsharam4786@gmail.com>

yurishkuro · 2024-11-01T20:04:53Z

charts/jaeger/templates/allInOne-config.yaml

@@ -0,0 +1,12 @@
+{{ if .Values.userconfig }}


I think this file should be called user-config.yaml, it's not specific to all-in-one.

i used that only in the start but I got this error
Error: INSTALLATION FAILED: parse error at (jaeger/templates/allInOne-config.yaml:1): bad character U+002D '-'
the problem is helm chart don't allow '-' there is workaround we can use
{{index .Values.user-config }}
but with if or any other condition statements it throw this error
Error: INSTALLATION FAILED: parse error at (jaeger/templates/allInOne-config.yaml:1): unexpected <if> in operand

I made this change to the template

diff --git a/charts/jaeger/templates/allInOne-config.yaml b/charts/jaeger/templates/allInOne-config.yaml index eca21f2..e473618 100644 --- a/charts/jaeger/templates/allInOne-config.yaml +++ b/charts/jaeger/templates/allInOne-config.yaml @@ -8,5 +8,5 @@ metadata: {{- include "jaeger.labels" . | nindent 4 }} data: config.yaml: | - {{ .Values.userconfig | nindent 8 }} + {{- .Values.userconfig | nindent 4 }} {{- end }}

created mock config file

$ cat myconfig.yaml hello: world: true

and ran a test

$ helm template --set-file userconfig=myconfig.yaml charts/jaeger > test-output.txt

which resulted in reasonably looking resource file:

# Source: jaeger/templates/allInOne-config.yaml apiVersion: v1 kind: ConfigMap metadata: name: jaeger-configmap namespace: default labels: helm.sh/chart: jaeger-4.0.0 app.kubernetes.io/name: jaeger app.kubernetes.io/instance: release-name app.kubernetes.io/version: "2.0.0-rc2" app.kubernetes.io/managed-by: Helm data: config.yaml: | hello: world: true

charts/jaeger/values.yaml

Signed-off-by: mehul <mehulsharam4786@gmail.com>

yurishkuro · 2024-11-02T17:31:48Z

charts/jaeger/templates/allinone-deploy.yaml

+         - name: jaeger-config
+           configMap:
+            name: jaeger-configmap


please clean-up all the naming. Not jaeger-config, not jaeger-configmap, not allInOne-config.yaml, not config.yaml - everything should be using a single name user-config

done should i convert it to PR

Signed-off-by: mehul <mehulsharam4786@gmail.com>

yurishkuro · 2024-11-02T22:41:13Z

I merged changes from main into v2, let's resolve conflicts

Signed-off-by: mehul gautam <mehulsharma4786@gmail.com>

yurishkuro · 2024-11-03T05:21:52Z

.github/workflows/lint-test.yaml

-            fetch-depth: 0
-
-        - uses: ./.github/actions/prepare-k8s
+            ct install --config ct.yaml --helm-extra-set-args "--set provisionDataStore.cassandra=false --set storage.type=memory --set allInOne.enabled=true --set agent.enabled=false --set collector.enabled=false --set query.enabled=false" 


this line is hard to read, please use the same formatting as in the deleted sections, one flag per line

yurishkuro

please delete empty es.txt

yurishkuro · 2024-11-03T05:22:52Z

.github/workflows/lint-test.yaml

-      - uses: ./.github/actions/prepare-k8s
-
-      - name: Run cassandra-chart-testing (install)
-        run: ct install --config ct.yaml
  test-with-allInOne:


Suggested change

test-with-allInOne:

test-with-all-in-one:

yurishkuro

couple small things, ready to merge otherwise

Signed-off-by: mehul <mehulsharma4786@gmail.com>

charts/jaeger/templates/user-config.yaml

Signed-off-by: Yuri Shkuro <yurishkuro@users.noreply.github.com>

rgaduput and others added 7 commits October 21, 2024 02:04

Merge branch 'main' into jaeger-v2-with-v1

7212b86

Signed-off-by: mehul gautam <mehulsharma4786@gmail.com>

minor changes

9e73b35

Signed-off-by: mehul <mehulsharam4786@gmail.com>

Merge branch 'jaeger-v2-with-v1' of https://github.com/hellspawn679/h…

0dd3924

…elm-charts into jaeger-v2-with-v1

added image changes

f7db758

Signed-off-by: mehul <mehulsharam4786@gmail.com>

fix lint

b980560

Signed-off-by: mehul <mehulsharam4786@gmail.com>

removed v1 allInOne and added jaeger-v2

d9aa942

Signed-off-by: mehul <mehulsharam4786@gmail.com>

yurishkuro reviewed Oct 27, 2024

View reviewed changes

mehul added 3 commits October 28, 2024 21:39

removed v2 references

01d341b

Signed-off-by: mehul <mehulsharam4786@gmail.com>

added allInOne again

ee12ee4

Signed-off-by: mehul <mehulsharam4786@gmail.com>

minor fix

3012649

Signed-off-by: mehul <mehulsharam4786@gmail.com>

hellspawn679 requested a review from yurishkuro October 29, 2024 18:27

yurishkuro reviewed Oct 30, 2024

View reviewed changes

mehul added 3 commits October 30, 2024 21:37

fixed how the config is being passed

1ea257d

Signed-off-by: mehul <mehulsharam4786@gmail.com>

fixed mirror template change

42c30a0

Signed-off-by: mehul <mehulsharam4786@gmail.com>

use --set-file for user config

f247a95

Signed-off-by: mehul <mehulsharam4786@gmail.com>

yurishkuro reviewed Nov 1, 2024

View reviewed changes

charts/jaeger/values.yaml Show resolved Hide resolved

removed config-test and updated configmap template

647f7fa

Signed-off-by: mehul <mehulsharam4786@gmail.com>

yurishkuro reviewed Nov 2, 2024

View reviewed changes

fixed

8069914

Signed-off-by: mehul <mehulsharam4786@gmail.com>

Merge branch 'v2' into jaeger-v2-with-v1

742febb

Signed-off-by: mehul gautam <mehulsharma4786@gmail.com>

yurishkuro reviewed Nov 3, 2024

View reviewed changes

fixed minor naming

b6bbc4b

Signed-off-by: mehul <mehulsharma4786@gmail.com>

hellspawn679 requested a review from yurishkuro November 3, 2024 09:20

yurishkuro reviewed Nov 3, 2024

View reviewed changes

charts/jaeger/templates/user-config.yaml Outdated Show resolved Hide resolved

indentation

7a24558

Signed-off-by: Yuri Shkuro <yurishkuro@users.noreply.github.com>

yurishkuro approved these changes Nov 3, 2024

View reviewed changes

yurishkuro marked this pull request as ready for review November 3, 2024 16:53

yurishkuro requested review from davidvonthenen, naseemkullah and pavelnikolov as code owners November 3, 2024 16:53

yurishkuro merged commit 6b4a59c into jaegertracing:v2 Nov 3, 2024
3 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Jaeger v2 with v1 #613

Jaeger v2 with v1 #613

hellspawn679 commented Oct 26, 2024

yurishkuro Oct 27, 2024

yurishkuro Oct 27, 2024

hellspawn679 Oct 28, 2024

yurishkuro Oct 27, 2024

yurishkuro Oct 27, 2024

yurishkuro Oct 27, 2024

yurishkuro Oct 27, 2024

yurishkuro Oct 30, 2024

yurishkuro Oct 30, 2024

hellspawn679 Oct 30, 2024

yurishkuro Oct 30, 2024

hellspawn679 Oct 30, 2024

yurishkuro Nov 1, 2024

hellspawn679 Nov 1, 2024

yurishkuro Nov 2, 2024

yurishkuro Nov 2, 2024

hellspawn679 Nov 2, 2024

yurishkuro commented Nov 2, 2024

yurishkuro Nov 3, 2024

yurishkuro left a comment

yurishkuro Nov 3, 2024

yurishkuro left a comment

Jaeger v2 with v1 #613

Jaeger v2 with v1 #613

Conversation

hellspawn679 commented Oct 26, 2024

What this PR does

Which issue this PR fixes

Checklist

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

yurishkuro commented Nov 2, 2024

Choose a reason for hiding this comment

yurishkuro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yurishkuro left a comment

Choose a reason for hiding this comment