feat: area/helm: Modify helm template to use http_listen_port and grpc_listen_port instead of hardcoded value #11646

Sheikh-Abubaker · 2024-01-10T21:42:23Z

What this PR does / why we need it:
This PR basically modifies helm template to use parameters http_listen_port and grpc_listen_port instead of hardcoded value to handle the cases where user assigns some other port other than default.
Which issue(s) this PR fixes:
Fixes #11641

Special notes for your reviewer:

Checklist

Reviewed the CONTRIBUTING.md guide (required)
Documentation added
Tests updated
CHANGELOG.md updated
- If the change is worth mentioning in the release notes, add add-to-release-notes label
Changes that require user attention or interaction to upgrade are documented in docs/sources/setup/upgrade/_index.md
For Helm chart changes bump the Helm chart version in production/helm/loki/Chart.yaml and update production/helm/loki/CHANGELOG.md and production/helm/loki/README.md. Example PR
If the change is deprecating or removing a configuration option, update the deprecated-config.yaml and deleted-config.yaml files respectively in the tools/deprecated-config-checker directory. Example PR

…ten_port Signed-off-by: Sheikh-Abubaker <[email protected]>

Signed-off-by: Sheikh-Abubaker <[email protected]>

bebosudo

Is it working for you?
My loki-read pod is stuck in an endless list of errors:

level=warn ts=2024-01-11T08:43:57.902636597Z caller=scheduler_processor.go:98 msg="error contacting scheduler" err="rpc error: code = Unavailable desc = connection error: desc = \"transport: Error while dialing: dial tcp 172.28.12.169:9095: connect: connection refused\"" addr=172.28.12.169:9095
level=warn ts=2024-01-11T08:43:57.985055672Z caller=scheduler_processor.go:98 msg="error contacting scheduler" err="rpc error: code = Unavailable desc = connection error: desc = \"transport: Error while dialing: dial tcp 172.28.12.169:9095: connect: connection refused\"" addr=172.28.12.169:9095
level=error ts=2024-01-11T08:43:58.268012236Z caller=frontend_scheduler_worker.go:237 msg="error contacting scheduler" err="rpc error: code = Unavailable desc = connection error: desc = \"transport: Error while dialing: dial tcp 172.28.12.169:9095: connect: connection refused\"" addr=172.28.12.169:9095
level=error ts=2024-01-11T08:43:58.619265229Z caller=frontend_scheduler_worker.go:237 msg="error contacting scheduler" err="rpc error: code = Unavailable desc = connection error: desc = \"transport: Error while dialing: dial tcp 172.28.12.169:9095: connect: connection refused\"" addr=172.28.12.169:9095
level=warn ts=2024-01-11T08:43:59.015424721Z caller=scheduler_processor.go:98 msg="error contacting scheduler" err="rpc error: code = Unavailable desc = connection error: desc = \"transport: Error while dialing: dial tcp 172.28.12.169:9095: connect: connection refused\"" addr=172.28.12.169:9095
level=warn ts=2024-01-11T08:43:59.171403506Z caller=scheduler_processor.go:98 msg="error contacting scheduler" err="rpc error: code = Unavailable desc = connection error: desc = \"transport: Error while dialing: dial tcp 172.28.12.169:9095: connect: connection refused\"" addr=172.28.12.169:9095
level=warn ts=2024-01-11T08:43:59.206568576Z caller=scheduler_processor.go:98 msg="error contacting scheduler" err="rpc error: code = Unavailable desc = connection error: desc = \"transport: Error while dialing: dial tcp 172.28.12.169:9095: connect: connection refused\"" addr=172.28.12.169:9095

The 172.28.12.169 address is the backend pod address.

There are many more places where we need to edit hardcoded values. I'll open a PR to your personal branch.

Edit: PR ready here: Sheikh-Abubaker#1. Already tested to be working fine.

The memberlist port is still hardcoded to 7946 (grep for it in the helm templates) though.

bebosudo · 2024-01-11T08:45:43Z

production/helm/loki/CHANGELOG.md

@@ -13,6 +13,10 @@ Entries should include a reference to the pull request that introduced the chang

 [//]: # (<AUTOMATED_UPDATES_LOCATOR> : do not remove this line. This locator is used by the CI pipeline to automatically create a changelog entry for each new Loki release. Add other chart versions and respective changelog entries bellow this line.)

+## 5.41.6
+
+- [ENHANCEMENT] Modified helm template to use parameters http_listen_port and grpc_listen_port instead of hardcoded value of 3100 and 9095.


Suggested change

- [ENHANCEMENT] Modified helm template to use parameters http_listen_port and grpc_listen_port instead of hardcoded value of 3100 and 9095.

- [ENHANCEMENT] Modified helm template to use parameters http_listen_port and grpc_listen_port instead of hardcoded values.

bebosudo · 2024-01-11T08:49:18Z

production/helm/loki/templates/backend/query-scheduler-discovery.yaml

@@ -14,11 +14,11 @@ spec:
  publishNotReadyAddresses: true
  ports:
    - name: http-metrics
-      port: 3100
+      port: {{ .Values.loki.server.http_listen_port | default 3100 }}


Suggested change

port: {{ .Values.loki.server.http_listen_port | default 3100 }}

port: {{ .Values.loki.server.http_listen_port }}

No need to set defaults here too, if the user didn't specify the value, helm will use the default from the values.yaml provided by the helm chart, so that resolves to {{ 3100 | default 3100 }}.
This means that there are less "magic numbers" defined across the helm chart, and if the default values will need to be changed in the future they could be changed only in the values.yaml and it would be updated everywhere immediately.

bebosudo · 2024-01-11T08:49:49Z

production/helm/loki/templates/backend/query-scheduler-discovery.yaml

      targetPort: http-metrics
      protocol: TCP
    - name: grpc
-      port: 9095
+      port: {{ .Values.loki.server.grpc_listen_port | default 9095 }}


Suggested change

port: {{ .Values.loki.server.grpc_listen_port | default 9095 }}

port: {{ .Values.loki.server.grpc_listen_port }}

bebosudo · 2024-01-11T08:49:59Z

production/helm/loki/templates/backend/service-backend-headless.yaml

@@ -28,11 +28,11 @@ spec:
  clusterIP: None
  ports:
    - name: http-metrics
-      port: 3100
+      port: {{ .Values.loki.server.http_listen_port | default 3100 }}


Suggested change

port: {{ .Values.loki.server.http_listen_port | default 3100 }}

port: {{ .Values.loki.server.http_listen_port }}

bebosudo · 2024-01-11T08:50:06Z

production/helm/loki/templates/backend/service-backend-headless.yaml

      targetPort: http-metrics
      protocol: TCP
    - name: grpc
-      port: 9095
+      port: {{ .Values.loki.server.grpc_listen_port | default 9095 }}


Suggested change

port: {{ .Values.loki.server.grpc_listen_port | default 9095 }}

port: {{ .Values.loki.server.grpc_listen_port }}

…ki_server_ports [helm] Replace all hardcoded loki server ports

…into http-grpc-pr

Signed-off-by: Sheikh-Abubaker <[email protected]>

…fana-loki into http-grpc-pr

bebosudo · 2024-01-11T18:01:26Z

The memberlist port is still hardcoded to 7946 (grep for it in the helm templates) though.

May I substitute those harcdoded value with varibles ? as we did it for http and grpc ?

The problem with that port number is that there is currently no variable in the helm values that defines the port, so the default port is hardcoded everywhere: if you grep for the port number 7946 you'll only find occurrences in service.yaml files, but not in config files! So if we search-and-replace it everywhere, loki will spawn the process on the default 7946, but the services would be listening on another port number.

From the loki docs it seems it can be set with the following variable:

# Gossip port to advertise to other members in the cluster. Used for NAT
# traversal.
# CLI flag: -memberlist.advertise-port
[advertise_port: <int> | default = 7946]

In the values file we can define values to be set in the memberlist section of the config. Therefore I added to my values the following:

loki:
  extraMemberlistConfig:  
    advertise_port: 7498

And the value shows up correctly in the configmap:

[alberto@fedora]$ k get cm/loki -o yaml
apiVersion: v1
data:
  config.yaml: |2
    ...
    memberlist:
      advertise_port: 7498
      join_members:
      - loki-memberlist

And loki seems to read it without any issue:

$ curl loki-backend:3101/config

target: backend
http_prefix: ""
...
memberlist:
  node_name: ""
  ...
  advertise_addr: ""
  advertise_port: 7498

But the pods are not obeying it somehow:

[alberto@fedora]$ k exec -it pod/loki-backend-0 -c loki -- sh
/ $ netstat -tuplen
Active Internet connections (only servers)
Proto Recv-Q Send-Q Local Address           Foreign Address         State       PID/Program name    
tcp        0      0 :::3101                 :::*                    LISTEN      1/loki
tcp        0      0 :::9096                 :::*                    LISTEN      1/loki
tcp        0      0 :::7946                 :::*                    LISTEN      1/loki

For this reason I'd open a separate issue. If you find a solution ping me in the new PR and I'll happily test it!

Sheikh-Abubaker · 2024-01-11T18:49:30Z

But the pods are not obeying it somehow:

[alberto@fedora]$ k exec -it pod/loki-backend-0 -c loki -- sh
/ $ netstat -tuplen
Active Internet connections (only servers)
Proto Recv-Q Send-Q Local Address           Foreign Address         State       PID/Program name    
tcp        0      0 :::3101                 :::*                    LISTEN      1/loki
tcp        0      0 :::9096                 :::*                    LISTEN      1/loki
tcp        0      0 :::7946                 :::*                    LISTEN      1/loki

For this reason I'd open a separate issue. If you find a solution ping me in the new PR and I'll happily test it!

@bebosudo I'm onto completing the above memberlist port task

bebosudo · 2024-01-11T19:02:16Z

I'd discourage putting the memberlist port task change in this PR: these are different changes and it'd be better fixing them in different PRs.
1 PR, 1 simple fix.

Sheikh-Abubaker · 2024-01-11T19:09:13Z

I'd discourage putting the memberlist port task change in this PR: these are different changes and it'd be better fixing them in different PRs. 1 PR, 1 simple fix.

@bebosudo okk, is there anything I'm supposed to do for this PR ?

bebosudo · 2024-01-11T19:14:03Z

@bebosudo okk, is there anything I'm supposed to do for this PR ?

Nope, it's ready for review by a maintainer :-)

Signed-off-by: Sheikh-Abubaker <[email protected]>

…into http-grpc-pr

Sheikh-Abubaker · 2024-01-11T19:23:36Z

Nope, it's ready for review by a maintainer :-)

It was great working with you, looking forward to connecting. resolving issues and opening more PR's together :-)

…into http-grpc-pr

Signed-off-by: Sheikh-Abubaker <[email protected]>

…into http-grpc-pr

Signed-off-by: Sheikh-Abubaker <[email protected]>

…into http-grpc-pr

Signed-off-by: Sheikh-Abubaker <[email protected]>

…into http-grpc-pr

Signed-off-by: Sheikh-Abubaker <[email protected]>

…into http-grpc-pr

Signed-off-by: Sheikh-Abubaker <[email protected]>

MaxThom · 2024-03-17T22:43:57Z

Hey guys! I think this PR introduced an issue on the templates/monitoring/_helpers-monitoring.tpl.
So this PR changed from hardcoded to the templated values, but in the printf function, its using %s instead of %d which causes this error in the agent.

grafana-agent 2024/03/17 22:40:27 error loading config file /var/lib/grafana-agent/config/agent.yml: parse "http://hummus-loki.max-dev.svc.cluster.local:%!s(float64=3100)/loki/api/v1/push": invalid port ":%!s(float64=3100)" after host

If I overwrite the http_listen_port to "3100" (with quotes), then it works, but then the loki core fails with same error: want int instead of string.

I manage to get it to work by using %v instead of %s. If using:

%d: same error
%f: works, but then print 3100.0000 which then fails
%v: work beautifully

Ive been trying the single binary installation, here's my values:

  loki:
    commonConfig:
      replication_factor: 1
    storage:
      type: "filesystem"
  singleBinary:
    replicas: 1

MichelHollands · 2024-03-18T10:59:07Z

Hey guys! I think this PR introduced an issue on the templates/monitoring/_helpers-monitoring.tpl. So this PR changed from hardcoded to the templated values, but in the printf function, its using %s instead of %d which causes this error in the agent.

@MaxThom Apologies, that did indeed cause a problem. The fix is here: #12242

…rpc_listen_port instead of hardcoded value (grafana#11646) Signed-off-by: Sheikh-Abubaker <[email protected]> Co-authored-by: Alberto Chiusole <[email protected]> Co-authored-by: Michel Hollands <[email protected]>

Modified helm template to use variables http_listen_port and grpc_lis…

c36271d

…ten_port Signed-off-by: Sheikh-Abubaker <[email protected]>

Sheikh-Abubaker requested a review from a team as a code owner January 10, 2024 21:42

pull-request-size bot added the size/S label Jan 10, 2024

github-actions bot added the area/helm label Jan 10, 2024

Updated CHANGELOG.md

37f1d91

Signed-off-by: Sheikh-Abubaker <[email protected]>

bebosudo reviewed Jan 11, 2024

View reviewed changes

bebosudo added 4 commits January 11, 2024 13:01

Remove defaults from service objects, already in values.yaml

c598943

Replace all occurrences of port 9095 with value from values.yaml

65531b2

Replace all occurrences of port 3100 with value from values.yaml

c540201

Format port numbers to string

3a89bf7

bebosudo mentioned this pull request Jan 11, 2024

[helm] Replace all hardcoded loki server ports Sheikh-Abubaker/grafana-loki#1

Merged

Merge pull request #1 from bebosudo/chiusole/replace_all_hardcoded_lo…

4b2d13d

…ki_server_ports [helm] Replace all hardcoded loki server ports

pull-request-size bot added size/M and removed size/S labels Jan 11, 2024

Sheikh-Abubaker added 3 commits January 11, 2024 21:01

Merge branch 'main' of https://github.com/Sheikh-Abubaker/grafana-loki …

7c4154a

…into http-grpc-pr

Updated branch and Modified CHANGELOG.md

a381054

Signed-off-by: Sheikh-Abubaker <[email protected]>

Merge branch 'http-grpc-pr' of https://github.com/Sheikh-Abubaker/gra…

b01c17a

…fana-loki into http-grpc-pr

Sheikh-Abubaker added 2 commits January 12, 2024 00:47

Fixed Linting error

7d5fafc

Signed-off-by: Sheikh-Abubaker <[email protected]>

Merge branch 'main' of https://github.com/Sheikh-Abubaker/grafana-loki …

8f6f367

…into http-grpc-pr

Sheikh-Abubaker added 5 commits January 13, 2024 23:30

Merge branch 'main' of https://github.com/Sheikh-Abubaker/grafana-loki …

8759798

…into http-grpc-pr

Resolved merge conflicts

cf12e4c

Signed-off-by: Sheikh-Abubaker <[email protected]>

Merge branch 'main' of https://github.com/Sheikh-Abubaker/grafana-loki …

d86817f

…into http-grpc-pr

Resolved conflicts

4bbb6bb

Signed-off-by: Sheikh-Abubaker <[email protected]>

Merge branch 'main' of https://github.com/Sheikh-Abubaker/grafana-loki …

951e574

…into http-grpc-pr

Sheikh-Abubaker added 5 commits January 19, 2024 21:16

Merge branch 'main' of https://github.com/Sheikh-Abubaker/grafana-loki …

36d50c9

…into http-grpc-pr

Merge branch 'main' of https://github.com/Sheikh-Abubaker/grafana-loki …

cb2bbd5

…into http-grpc-pr

Merge branch 'main' of https://github.com/Sheikh-Abubaker/grafana-loki …

4ab4dc4

…into http-grpc-pr

Resolved Conflicts

af929f1

Signed-off-by: Sheikh-Abubaker <[email protected]>

Resolved conflicts

99c3794

Signed-off-by: Sheikh-Abubaker <[email protected]>

JStickler requested a review from MichelHollands February 8, 2024 21:59

Sheikh-Abubaker added 2 commits February 13, 2024 14:33

Merge branch 'main' of https://github.com/Sheikh-Abubaker/grafana-loki …

91b26ad

…into http-grpc-pr

Resolved conflicts

aec0e88

Signed-off-by: Sheikh-Abubaker <[email protected]>

Sheikh-Abubaker changed the title ~~area/helm: Modified helm template to use http_listen_port and grpc_listen_port instead of hardcoded value~~ feat:area/helm: Modified helm template to use http_listen_port and grpc_listen_port instead of hardcoded value Feb 22, 2024

Resolved conflicts

e36fa1c

Signed-off-by: Sheikh-Abubaker <[email protected]>

Sheikh-Abubaker changed the title ~~feat:area/helm: Modified helm template to use http_listen_port and grpc_listen_port instead of hardcoded value~~ feat: area/helm: Modified helm template to use http_listen_port and grpc_listen_port instead of hardcoded value Feb 22, 2024

Sheikh-Abubaker and others added 7 commits February 22, 2024 15:18

Updated CHANGELOG.md

73b64f1

Signed-off-by: Sheikh-Abubaker <[email protected]>

Merge branch 'main' of https://github.com/Sheikh-Abubaker/grafana-loki …

5757e90

…into http-grpc-pr

Merge branch 'main' of https://github.com/Sheikh-Abubaker/grafana-loki …

483ceb3

…into http-grpc-pr

Merge branch 'main' of https://github.com/Sheikh-Abubaker/grafana-loki …

4376ae6

…into http-grpc-pr

Resolved conflicts

c754aa6

Signed-off-by: Sheikh-Abubaker <[email protected]>

Merge branch 'main' into http-grpc-pr

ebbe299

Merge branch 'main' into http-grpc-pr

fb81edf

MichelHollands approved these changes Mar 15, 2024

View reviewed changes

MichelHollands merged commit 347fd4d into grafana:main Mar 15, 2024
13 checks passed

MichelHollands mentioned this pull request Mar 18, 2024

fix: Helm fix usage of http_listen_port and grpc_listen_port fields in templates #12242

Merged

8 tasks

loki-gh-app bot mentioned this pull request Mar 27, 2024

chore(add-major-release-workflow): release 3.0.0-rc.1 #12380

Closed

Sheikh-Abubaker changed the title ~~feat: area/helm: Modified helm template to use http_listen_port and grpc_listen_port instead of hardcoded value~~ feat: area/helm: Modify helm template to use http_listen_port and grpc_listen_port instead of hardcoded value May 31, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: area/helm: Modify helm template to use http_listen_port and grpc_listen_port instead of hardcoded value #11646

feat: area/helm: Modify helm template to use http_listen_port and grpc_listen_port instead of hardcoded value #11646

Sheikh-Abubaker commented Jan 10, 2024 •

edited

Loading

bebosudo left a comment •

edited

Loading

bebosudo Jan 11, 2024

bebosudo Jan 11, 2024

bebosudo Jan 11, 2024

bebosudo Jan 11, 2024

bebosudo Jan 11, 2024

bebosudo commented Jan 11, 2024

Sheikh-Abubaker commented Jan 11, 2024 •

edited

Loading

bebosudo commented Jan 11, 2024

Sheikh-Abubaker commented Jan 11, 2024

bebosudo commented Jan 11, 2024

Sheikh-Abubaker commented Jan 11, 2024 •

edited

Loading

MaxThom commented Mar 17, 2024 •

edited

Loading

MichelHollands commented Mar 18, 2024

	- [ENHANCEMENT] Modified helm template to use parameters http_listen_port and grpc_listen_port instead of hardcoded value of 3100 and 9095.
	- [ENHANCEMENT] Modified helm template to use parameters http_listen_port and grpc_listen_port instead of hardcoded values.

	port: {{ .Values.loki.server.http_listen_port \| default 3100 }}
	port: {{ .Values.loki.server.http_listen_port }}

	port: {{ .Values.loki.server.grpc_listen_port \| default 9095 }}
	port: {{ .Values.loki.server.grpc_listen_port }}

feat: area/helm: Modify helm template to use http_listen_port and grpc_listen_port instead of hardcoded value #11646

feat: area/helm: Modify helm template to use http_listen_port and grpc_listen_port instead of hardcoded value #11646

Conversation

Sheikh-Abubaker commented Jan 10, 2024 • edited Loading

bebosudo left a comment • edited Loading

Choose a reason for hiding this comment

bebosudo Jan 11, 2024

Choose a reason for hiding this comment

bebosudo Jan 11, 2024

Choose a reason for hiding this comment

bebosudo Jan 11, 2024

Choose a reason for hiding this comment

bebosudo Jan 11, 2024

Choose a reason for hiding this comment

bebosudo Jan 11, 2024

Choose a reason for hiding this comment

bebosudo commented Jan 11, 2024

Sheikh-Abubaker commented Jan 11, 2024 • edited Loading

bebosudo commented Jan 11, 2024

Sheikh-Abubaker commented Jan 11, 2024

bebosudo commented Jan 11, 2024

Sheikh-Abubaker commented Jan 11, 2024 • edited Loading

MaxThom commented Mar 17, 2024 • edited Loading

MichelHollands commented Mar 18, 2024

Sheikh-Abubaker commented Jan 10, 2024 •

edited

Loading

bebosudo left a comment •

edited

Loading

Sheikh-Abubaker commented Jan 11, 2024 •

edited

Loading

Sheikh-Abubaker commented Jan 11, 2024 •

edited

Loading

MaxThom commented Mar 17, 2024 •

edited

Loading