fix: add HA templates and modify the code #1284

omrozowicz-splunk · 2025-11-17T14:16:00Z

Description

This PR adds HA Sentinel option to Redis component. Some of the main changes:

Initialise all redis env variables in construct-redis-url.sh
Add replication option to redis.mode
Add readiness and liveness probe
Add annotations to retrigger all pods whenever secrets change

Fixes # (issue)

Type of change

Please delete options that are not relevant.

New feature
Refactor/improvement

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

Checklist

My commit message is conventional
I have run pre-commit on all files before creating the PR
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
I have added tests that prove my fix is effective or that my feature works
New and existing unit tests pass locally with my changes
I have checked my code and corrected any misspellings

Kawron · 2025-11-18T12:56:32Z

construct-redis-url.sh

+
+    # RedBeat uses redis-sentinel:// with master_name query
+    : "${REDIS_URL:=${REDBEAT_SCHEME}/${REDIS_DB}#master_name=${REDIS_MASTER_NAME}}"
+    SENTINEL_CHECK="redis://${REDIS_SENTINEL_SERVICE}:${REDIS_SENTINEL_PORT}"


shouldn't it be also in the if statment above in case REDIS_PASSWORD is set?

this is what I thought too, but when I tested that it didn't work at all with password 😅 I can check it once again just to be sure

ok, I know why — sed -i "s/\${REDIS_PASSWORD}/$REDIS_PASSWORD/g" /data/sentinel.conf I have in config is handling Redis password (to access redis instances and know their roles) and not the sentinel's one. Sentinel instances are running without password, and from what I read it is standard practice for internal setups. It's enough to have redis passwrod protected. I'll test again if we need password in SENTINEL_SCHEME and REDBEAT_SCHEME

Kawron · 2025-11-18T14:57:50Z

charts/splunk-connect-for-snmp/templates/redis/redis-ha-statefulset.yaml

+            - -c
+            - |
+              {{- if .Values.redis.auth.enabled }}
+              redis-cli -a "$REDIS_PASSWORD" ping


This is an overkill, but you can use set REDISCLI_AUTH env to authenticate without passing the password in the command, so it won't be visible in history or process list. See: https://stackoverflow.com/a/64372000

For this case I think this is okay, because the password is also visible in the config files, but I've just wanted to share this nice feautre : )

I use env to authenticate anyway, this is being rendered to redis-cli -a "$REDIS_PASSWORD" ping, $REDIS_PASSWORD is not becoming visible from the k8s perspective

Kawron · 2025-11-18T15:14:15Z

charts/splunk-connect-for-snmp/templates/redis/sentinel/redis-sentinel-config.yaml

+    {{- if .Values.redis.auth.enabled }}
+    sentinel auth-pass mymaster ${REDIS_PASSWORD}
+    {{- end }}
+    sentinel down-after-milliseconds mymaster 5000


Maybe we could parametrize this parameters? Would be there any benefit from fine-tuning those values for customer? If not, I would leave it as it

not sure about the parameters, but for sure I can use that env variable for mymaster

Kawron · 2025-11-18T15:16:24Z

charts/splunk-connect-for-snmp/templates/redis/sentinel/redis-sentinel-statefulset.yaml

+    app.kubernetes.io/component: sentinel
+spec:
+  serviceName: {{ .Release.Name }}-redis-sentinel
+  replicas: {{ .Values.redis.sentinel.replicas | default 3 }}


I wonder if we need "default" here? It is already set to 3 in values.yaml

this is what we generally do in this project as a fallback, not sure if it is necessary, but I know sometimes users edit our values.yaml directly in the local package instead of creating their own 🙈

Kawron · 2025-11-18T15:21:23Z

charts/splunk-connect-for-snmp/templates/ui/role-binding.yaml

 metadata:
  name: job-robot
-  namespace: sc4snmp
+  namespace: {{ .Release.Namespace | quote }}


when searching for: "namespace: sc4snmp" in IDE I still found some occurances. Shouldn't it be cleaned as well?

Yes, this is an issue I found while looking at those UI files. I think I deleted it from all the templates but didn't run make render after that so you probably see it in rendered manifests. Let me update it.

Kawron · 2025-11-18T15:22:55Z

charts/splunk-connect-for-snmp/templates/worker/_helpers.tpl

-  value: "0"
 - name: SC4SNMP_VERSION
  value: {{ .Chart.Version | default "0.0.0" }}
+{{- include "splunk-connect-for-snmp.redis-env" . | nindent 0 }}


I think that nident with 0 does not do anything

nindent 0 adds a newline before the content and indents all lines by 0 spaces. In this case, alternatively I could use it like:

{{ include "splunk-connect-for-snmp.redis-env" . }}

Kawron · 2025-11-18T15:26:20Z

charts/splunk-connect-for-snmp/templates/redis/sentinel/redis-sentinel-statefulset.yaml

+            command:
+            - sh
+            - -c
+            - redis-cli -p 26379 ping


Will it work if redis is password protected? Password is not passed by -a or REDISCLI_AUTH env

check: #1284 (comment) -> sentinel does not have own authentication

Kawron · 2025-11-18T15:26:48Z

charts/splunk-connect-for-snmp/templates/redis/sentinel/redis-sentinel-statefulset.yaml

+            command:
+            - sh
+            - -c
+            - redis-cli -p 26379 sentinel get-master-addr-by-name mymaster | grep snmp-redis-ha-0


same as above

also here we hardcoded release name as snmp whereas in otherplaces it is templated {{ .Release.Name }}

I deleted it completely as after first failover it doesn't work (as other replica is a master)

ajasnosz · 2025-11-25T11:19:52Z

charts/splunk-connect-for-snmp/templates/_helpers.tpl

+- name: REDIS_SENTINEL_PORT
+  value: "26379"
+- name: REDIS_MASTER_NAME
+  value: mymaster


can we set different default name?

Any name would work, the only thing is that it must be the same everywhere. From what I learned it is better not to give user option to change this name, as it is easy to break this way.

ajasnosz · 2025-11-25T12:19:41Z

docs/microk8s/configuration/redis-configuration.md

+!!!note
+    The `storageClassName` must point to a `StorageClass` that supports network-attached volumes with `ReadWriteMany` (or `ReadWriteOnce` with cross-node support). Examples: NFS, Ceph, EBS, GCP Persistent Disk, Azure Disk.
+
+For MicrokK8s on Ubuntu, you can enable NFS this way:


typo in microk8s

ajasnosz · 2025-11-25T12:23:13Z

docs/microk8s/configuration/redis-configuration.md

+
+For MicrokK8s on Ubuntu, you can enable NFS this way:
+
+1. Enable NFS client on ALL nodes:


maybe we want to emphasize more that this is just an example, not solution we recommend and support while configuring?

I added EXAMPLE: before the section

fix: add HA templates and modify the code

ca637ae

omrozowicz-splunk requested review from Kawron, cwadhwani-splunk and gmaiyani-crest November 17, 2025 14:20

omrozowicz-splunk added 6 commits November 17, 2025 15:21

test: test [run-int-tests]

e68b756

fix: namespace problems in UI

7277375

doc: update documentation with storage considerations

25cb653

test: test [run-ui-tests]

afe522a

test: test [run-ui-tests]

8b4eca3

test: adjust free disk space on ui tests [run-ui-tests]

3b6fe40

Kawron requested changes Nov 18, 2025

View reviewed changes

omrozowicz-splunk added 7 commits November 19, 2025 09:48

fix: run make render

b86bee9

fix: liveness probe, blank lines in templates

d21cc14

fix: run make render

16524e0

fix: add redis env variables to redis

2eace2b

test: test [run-int-tests]

123cd42

fix: set default resources to {} and add documentation

047dd85

fix: check both databases on redis ha [run-int-tests]

ddd6246

omrozowicz-splunk requested a review from Kawron November 20, 2025 10:17

Kawron approved these changes Nov 20, 2025

View reviewed changes

omrozowicz-splunk requested a review from ajasnosz November 24, 2025 14:00

ajasnosz reviewed Nov 25, 2025

View reviewed changes

fix: docs

2c9539d


		For MicrokK8s on Ubuntu, you can enable NFS this way:

		1. Enable NFS client on ALL nodes:

fix: add HA templates and modify the code #1284

Are you sure you want to change the base?

fix: add HA templates and modify the code #1284

Conversation

omrozowicz-splunk commented Nov 17, 2025

Description

Type of change

How Has This Been Tested?

Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Kawron Nov 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Kawron Nov 18, 2025 •

edited

Loading