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

change alertmanager-svc-headless from http to grpc port + expose 9094 TCP and UDP #435

Open
wants to merge 14 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

* [FEATURE] add autoscaler for the ruler #430
* [ENHANCEMENT] Add annotations and labels to memberlist service #433
* [CHANGE] change alertmanager-svc-headless from http to grpc port #420

# 2.0.1 / 2023-01-06

Expand Down
9 changes: 9 additions & 0 deletions templates/alertmanager/alertmanager-statefulset.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -175,6 +175,15 @@ spec:
- name: gossip
containerPort: {{ .Values.config.memberlist.bind_port }}
protocol: TCP
- name: grpc
containerPort: {{ .Values.config.server.grpc_listen_port }}
protocol: TCP
- containerPort: 9094
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm conflicted on this. The magic number 9094 bothers me. It would be nice to be able to support the configuration and only default to 9094, but that's awkward because the port number is embedded at the end of the listen string. Something like this (but this is kind of ugly):

Suggested change
- containerPort: 9094
- containerPort: {{ (.Values.config.alertmanager.cluster.listen_address | default "0.0.0.0:9094" | split ":").1 }}

Copy link
Collaborator

Choose a reason for hiding this comment

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

What about introducing an extra variable with a docstring in values.yaml?

Copy link
Contributor Author

@AlexandreRoux AlexandreRoux Mar 15, 2023

Choose a reason for hiding this comment

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

I haven't manage to figure out a way rather than the above suggestion of @kd7lxl to make 9094 configurable since it will always come from config.alertmanager.cluster.listen_address and the need to split the port at the end.

I will push that for now (to progress on the PL) but suggestion are welcome.

name: gossip-clu-tcp
protocol: TCP
- containerPort: 9094
name: gossip-clu-udp
protocol: UDP
nschad marked this conversation as resolved.
Show resolved Hide resolved
nschad marked this conversation as resolved.
Show resolved Hide resolved
startupProbe:
{{- toYaml .Values.alertmanager.startupProbe | nindent 12 }}
livenessProbe:
Expand Down
6 changes: 3 additions & 3 deletions templates/alertmanager/alertmanager-svc-headless.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,10 @@ spec:
clusterIP: None
publishNotReadyAddresses: true
ports:
- port: {{ .Values.config.server.http_listen_port }}
- port: {{ .Values.config.server.grpc_listen_port }}
protocol: TCP
name: http-metrics
targetPort: http-metrics
name: grpc
targetPort: grpc
nschad marked this conversation as resolved.
Show resolved Hide resolved
selector:
{{- include "cortex.alertmanagerSelectorLabels" . | nindent 4 }}
{{- end -}}
Expand Down