feat(vm-alert): add notifier config support #572

hakuno · 2023-07-06T00:36:40Z

Notifier also supports configuration via file specified with flag -notifier.config.

But, no way to do that in current Helm chart.

So, how could I place discover notifiers?

*** EDIT ***

I see it can be better with config and configPath.

config: we can set the config with a map like the alert rules;
configPath: or we can set a file path from a volume mount if desired.

Scenarios

Scenario 1 with `server.notifier.config`

helm template --dry-run --validate . -f custom-config.yaml

server:
  notifier:
    config:
      dns_sd_configs:
        - names:
            - alertmanager-headless.ns.svc.cluster.local
          type: 'A'
          port: 9093

---
apiVersion: v1
kind: ConfigMap
metadata:
  name: release-name-victoria-metrics-alert-server-notifier-config
  namespace: prometheus
  labels:
    app: server
    app.kubernetes.io/name: victoria-metrics-alert
    app.kubernetes.io/instance: release-name
    helm.sh/chart: victoria-metrics-alert-0.6.4
    app.kubernetes.io/managed-by: Helm
data:
  notifier.yaml: |
    dns_sd_configs:
    - names:
      - alertmanager-headless.ns.svc.cluster.local
      port: 9093
      type: A

---
    ...
    spec:
      containers:
        - name: victoria-metrics-alert-server
          image: "victoriametrics/vmalert:v1.91.3"
          args:
            - -rule=/config/alert-rules.yaml
            - -datasource.url=http://localhost
            - -notifier.config=/config/notifier.yaml
            - -remoteRead.url=
            - -remoteWrite.url=
            - -envflag.enable=true
            - -envflag.prefix=VM_
            - -loggerFormat=json
          volumeMounts:
            - name: alerts-config
              mountPath: /config
      volumes:
        - name: alerts-config
          projected:
            sources:
              - configMap:
                  name: release-name-victoria-metrics-alert-server-alert-rules-config
              - configMap:
                  name: release-name-victoria-metrics-alert-server-notifier-config

Scenario #2 with `server.notifier.configPath`

helm template --dry-run --validate . -f custom-configpath.yaml

server:
  notifier:
    configPath: "/custom-path/random.yaml"

---
    ...
    spec:
      containers:
        - name: victoria-metrics-alert-server
          image: "victoriametrics/vmalert:v1.91.3"
          args:
            - -rule=/config/alert-rules.yaml
            - -datasource.url=http://localhost
            - -notifier.config=/custom-path/random.yaml
            - -remoteRead.url=
            - -remoteWrite.url=
            - -envflag.enable=true
            - -envflag.prefix=VM_
            - -loggerFormat=json

Please, check it out.

hakuno · 2023-07-06T00:39:02Z

Also relates to #468

hakuno · 2023-07-06T01:03:43Z

The Alertmanager in HA mode has a headless service. So the DNS discover could account that. Is it?

dns_sd_configs:
  - names:
      - alertmanager-headless.ns.svc.cluster.local
    type: 'A'
    port: 9093

charts/victoria-metrics-alert/values.yaml

Haleygo

Thanks!
/LGTM
cc @f41gh7

hakuno · 2023-07-11T01:47:40Z

@Haleygo I made a big improvement. Please, can you review again? I see it can be better. I'll update the PR description.

Haleygo · 2023-07-11T03:23:14Z

charts/victoria-metrics-alert/values.yaml

@@ -119,7 +119,8 @@ server:
        token: ""
        # -- Token Auth file with Bearer token. You can use one of token or tokenFile
        tokenFile: ""
-    config: ""
+    config: {}
+    configPath: ""


I assume the configPath will use configmap mostly, maybe we can change it to "configMap", use cm when specified or create cm when config is using, and mount it like "alerts-config" here.

helm-charts/charts/victoria-metrics-alert/values.yaml

Lines 249 to 254 in 1b6009a

# vmalert alert rules configuration configuration:

# use existing configmap if specified

# otherwise .config values will be used

configMap: ""

config:

alerts:

helm-charts/charts/victoria-metrics-alert/templates/_helpers.tpl

Lines 75 to 81 in 1b6009a

{{- define "vmalert.server.configname" -}}

{{- if .Values.server.configMap -}}

{{- .Values.server.configMap -}}

{{- else -}}

{{- include "vmalert.server.fullname" . -}}-alert-rules-config

{{- end -}}

{{- end -}}

helm-charts/charts/victoria-metrics-alert/templates/server-deployment.yaml

Lines 170 to 173 in 1b6009a

volumes:

- name: alerts-config

configMap:

name: {{ include "vmalert.server.configname" . }}

WDYT?

@Haleygo Okay. Also I added support for secret.

Haleygo · 2023-07-11T03:23:45Z

charts/victoria-metrics-alert/README.md

-| server.notifier | object | `{"alertmanager":{"basicAuth":{"password":"","username":""},"bearer":{"token":"","tokenFile":""},"url":""},"config":""}` | Notifier to use for alerts. Multiple notifiers can be enabled by using `notifiers` section |
-| server.notifier.config | object | `""` | Path to configuration file for notifiers |
+| server.notifier | object | `{"alertmanager":{"basicAuth":{"password":"","username":""},"bearer":{"token":"","tokenFile":""},"url":""},"config":{},"configPath":""}` | Notifier to use for alerts. Multiple notifiers can be enabled by using `notifiers` section |
+| server.notifier.config | object | `{}` | Path to configuration file for notifiers |


Can you update the comment here?

Changed. Please, give a look at.

hakuno · 2023-07-11T16:29:02Z

I made changes. Now it can support 3 ways:

Set the config on values;
Set a config map (with custom key name if desired);
Set a secret (with custom key name if desired);

The default values are:

...
notifier:
    config: {}
    configMap: ""
    configMapKey: "notifier.yaml"
    existingSecret: ""
    existingSecretKey: "notifier.yaml"

The existingSecret has precedence over configMap; the configMap has precedence over config; the config has precedence over url list.

Haleygo · 2023-07-12T02:27:31Z

I made changes. Now it can support 3 ways:

Set the config on values;

Set a config map (with custom key name if desired);

Set a secret (with custom key name if desired);

The default values are:
...
notifier:
    config: {}
    configMap: ""
    configMapKey: "notifier.yaml"
    existingSecret: ""
    existingSecretKey: "notifier.yaml"
The existingSecret has precedence over configMap; the configMap has precedence over config; the config has precedence over url list.

Oh, so there will be secret using cases. But adding another two secret fileds and the precedences looks overdone to me, maybe we should just come back to the simple configpath and let user mount file however they like.
Any suggestion here) @f41gh7 @Amper

hakuno · 2023-07-12T03:51:13Z

Other charts like Prometheus community or Apache Airflow do something like this. So it seems regular. But I have no problem to follow the prefer design. Just let me know. Em ter., 11 de jul. de 2023 23:27, Haleygo ***@***.***> escreveu:

…

I made changes. Now it can support 3 ways: 1. Set the config on values; 2. Set a config map (with custom key name if desired); 3. Set a secret (with custom key name if desired); The default values are: ... notifier: config: {} configMap: "" configMapKey: "notifier.yaml" existingSecret: "" existingSecretKey: "notifier.yaml" The existingSecret has precedence over configMap; the configMap has precedence over config; the config has precedence over url list. Oh, so there will be secret using cases. But adding another two secret fileds and the precedences looks overdone to me, maybe we should just come back to the simple configpath and let user mount file however they like. Any suggestion here) @f41gh7 <https://github.com/f41gh7> @Amper <https://github.com/Amper> — Reply to this email directly, view it on GitHub <#572 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AEJMSGCIAPESXAXCZ4DYVNDXPYDR7ANCNFSM6AAAAAAZ7UPDLQ> . You are receiving this because you authored the thread.Message ID: ***@***.***>

f41gh7 · 2023-07-13T15:37:38Z

I made changes. Now it can support 3 ways:

Set the config on values;

Set a config map (with custom key name if desired);

Set a secret (with custom key name if desired);

The default values are:
...
notifier:
    config: {}
    configMap: ""
    configMapKey: "notifier.yaml"
    existingSecret: ""
    existingSecretKey: "notifier.yaml"
The existingSecret has precedence over configMap; the configMap has precedence over config; the config has precedence over url list.
Oh, so there will be secret using cases. But adding another two secret fileds and the precedences looks overdone to me, maybe we should just come back to the simple configpath and let user mount file however they like. Any suggestion here) @f41gh7 @Amper

Agreed, I can propose a simple solution:
allow adding configuration only from external secret.

Such secret can be defined at values or externally

# values.yaml
# -- Add extra specs dynamically to this chart
extraObjects:
- kind: Secret
  metadata:
     name: notifier-configuration
  stringData:
    notifier_config.yaml |
     some-config

usage

server:
  notifier:
     config:
       secretName: notifier-configuration
       secretKey: notifier_config.yaml

Note, notifier config could be used with notifiers defined at server.notifiers[] section.

hakuno · 2023-07-13T17:38:02Z

Note, notifier config could be used with notifiers defined at server.notifiers[] section.

@f41gh7 The docs don't explain it.

The docs show that urls can be appended to notifier.url.

So, also does config support? Like below

-notifier.config=/config/first.yaml,/config/second.yaml,/config/third.yaml

or

-notifier.config=/config/first.yaml,/config/notifier.yaml
-notifier.config=/config/first.yaml,/config/other_notifier.yaml

Please, let me know.

Haleygo · 2023-07-18T15:52:29Z

Note, notifier config could be used with notifiers defined at server.notifiers[] section.

I suppose @f41gh7 means that what we do here for .server.notifier should also be available for .server.notifiers
https://github.com/VictoriaMetrics/helm-charts/blob/master/charts/victoria-metrics-alert/values.yaml#L136-146

So, also does config support? Like below
-notifier.config=/config/first.yaml,/config/second.yaml,/config/third.yaml

No, notifier.config is only support one file.

tenmozes · 2023-10-17T13:06:50Z

@Haleygo @zekker6 could you plaese take a look and accept(or not) this PR, if this requires any follow up please cover it, thank you

hakuno · 2023-10-18T14:18:16Z

Hey, friends!
I've noticed it got extraObjects already. I'm unable to look at notifier.config right now... So I'll close this.

Thanks in advance!

Haleygo · 2023-11-24T08:57:43Z

I've noticed it got extraObjects already.

Yeah, thanks for the work!
Now, users who want to use -notifier.config can use extraVolumes mount configmap or secret.

feat(vm-alert): add notifier config support

0eeea04

hakuno requested review from f41gh7, k1rk and tenmozes as code owners July 6, 2023 00:36

hakuno mentioned this pull request Jul 6, 2023

vmalert to support notifier config #468

Closed

hagen1778 requested review from Amper and Haleygo July 6, 2023 09:40

Haleygo reviewed Jul 9, 2023

View reviewed changes

charts/victoria-metrics-alert/values.yaml Outdated Show resolved Hide resolved

Seiji added 2 commits July 10, 2023 09:51

refactor

af4011a

refactor

6adbb85

Haleygo previously approved these changes Jul 10, 2023

View reviewed changes

refactor: config mapping

2ff0b42

hakuno dismissed Haleygo’s stale review via 2ff0b42 July 11, 2023 01:45

Haleygo reviewed Jul 11, 2023

View reviewed changes

secret support

1a09580

put it back

13f8450

hakuno requested a review from Haleygo July 11, 2023 18:23

hakuno closed this Oct 18, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(vm-alert): add notifier config support #572

feat(vm-alert): add notifier config support #572

hakuno commented Jul 6, 2023 •

edited

Loading

hakuno commented Jul 6, 2023

hakuno commented Jul 6, 2023

Haleygo left a comment

hakuno commented Jul 11, 2023

Haleygo Jul 11, 2023

hakuno Jul 11, 2023

Haleygo Jul 11, 2023

hakuno Jul 11, 2023

hakuno commented Jul 11, 2023 •

edited

Loading

Haleygo commented Jul 12, 2023

hakuno commented Jul 12, 2023 via email

f41gh7 commented Jul 13, 2023

hakuno commented Jul 13, 2023

Haleygo commented Jul 18, 2023

tenmozes commented Oct 17, 2023

hakuno commented Oct 18, 2023

Haleygo commented Nov 24, 2023

	# vmalert alert rules configuration configuration:
	# use existing configmap if specified
	# otherwise .config values will be used
	configMap: ""
	config:
	alerts:

	{{- define "vmalert.server.configname" -}}
	{{- if .Values.server.configMap -}}
	{{- .Values.server.configMap -}}
	{{- else -}}
	{{- include "vmalert.server.fullname" . -}}-alert-rules-config
	{{- end -}}
	{{- end -}}

	volumes:
	- name: alerts-config
	configMap:
	name: {{ include "vmalert.server.configname" . }}

feat(vm-alert): add notifier config support #572

feat(vm-alert): add notifier config support #572

Conversation

hakuno commented Jul 6, 2023 • edited Loading

Scenarios

Scenario 1 with server.notifier.config

Scenario #2 with server.notifier.configPath

hakuno commented Jul 6, 2023

hakuno commented Jul 6, 2023

Haleygo left a comment

Choose a reason for hiding this comment

hakuno commented Jul 11, 2023

Haleygo Jul 11, 2023

Choose a reason for hiding this comment

hakuno Jul 11, 2023

Choose a reason for hiding this comment

Haleygo Jul 11, 2023

Choose a reason for hiding this comment

hakuno Jul 11, 2023

Choose a reason for hiding this comment

hakuno commented Jul 11, 2023 • edited Loading

Haleygo commented Jul 12, 2023

hakuno commented Jul 12, 2023 via email

f41gh7 commented Jul 13, 2023

hakuno commented Jul 13, 2023

Haleygo commented Jul 18, 2023

tenmozes commented Oct 17, 2023

hakuno commented Oct 18, 2023

Haleygo commented Nov 24, 2023

hakuno commented Jul 6, 2023 •

edited

Loading

Scenario 1 with `server.notifier.config`

Scenario #2 with `server.notifier.configPath`

hakuno commented Jul 11, 2023 •

edited

Loading