[bitname/redis] Redis sentinel master service

[anthosz]

Description of the change

Add a sentinel managed redis master service.

Benefits

Clients do not need sentinel to use redis failover.

Useful for clients outside of the Kubernetes Cluster and inside if sentinel connector didn't exist.

Possible drawbacks

Sentinel pod has kubectl installed. Role has list and patch pod permissions.

Applicable issues

• fixes #4082

Additional information

• Forked from PR https://github.com/bitnami/charts/pull/5622 (that was closed) & adapted/tested for the current master branch

Checklist

• [X] Chart version bumped in Chart.yaml according to semver. This is not necessary when the changes only affect README.md files.
• [X] Variables are documented in the values.yaml and added to the README.md using readme-generator-for-helm
• [X] Title of the pull request follows this pattern [bitnami/<name_of_the_chart>] Descriptive title

[carrodher]

Thank you for initiating this pull request. We appreciate your effort. Just a friendly reminder that it's important to sign your commits. Adding your signature certifies that you either authored the patch or have the necessary rights to contribute the changes. You can find detailed information on how to do this in the “Sign your work” section of our contributing guidelines.

Feel free to reach out if you have any questions or need assistance with the signing process.

[anthosz]

Hi @carrodher

I just do a rebase of all previous commits with this signed line...

[carrodher]

Unfortunately, the signature is still failing, see details at https://github.com/bitnami/charts/pull/21913/checks?check_run_id=20763583241

Apart from that, there is another issue because the version parameter is duplicated in the Chart.yaml

[anthosz]

@carrodher all fixed :)

[anthosz]

Please note that this MR cannot be tested until https://github.com/bitnami/charts/issues/22541 is fixed

[carrodher]

It seems there are more issues with the DCO, see https://github.com/bitnami/charts/pull/21913/checks?check_run_id=21015827654. Could you please take a look?

[anthosz]

I guess it's due to #22541 Seems the last release was not tested with sentinel so it never spawn (probable a rollback of master can help for redis)

[david-sykora]

@anthosz Thank you for the pull request, it looks like a much more efficient solution than what I've been using (HAProxy setup - relatively resource demanding). I tried to test the pull request using the workaround from issue #22541 and it looks like no isMaster label is added to any pod in the statefulset. The sentinel container logs should record that the sentinel hook was triggered after the master change. The logs should say something like this at this point:

32711:X 18 Oct 16:06:42.615 # +failover-state-reconf-slaves master mymaster 10.2.0.6 6379
32711:X 18 Oct 16:06:42.671 * +slave-reconf-sent slave 10.2.0.8:6379 10.2.0.8 6379 @ mymaster 10.2.0.6 6379
32711:X 18 Oct 16:06:42.671 . +script-child 397
32711:X 18 Oct 16:06:42.813 . -script-child 397 0 0

But there is nothing like this in the logs. Did you manage to test the isRedis label functionality in any way?

[anthosz]

@david-sykora strange, since #22541, all is broken on my side so unable to test it. You can try to rollback the code before the alignment with master.

Concerning isMaster, it is supposed to show something like "pod edited"

[david-sykora]

I used the workaround mentioned in the issue, the normal service works, but the master doesn't match any pods because the isRedis label is not assigned. I am attaching the POC:

preparation of repository with your changes applied:

# Step 1: Clone the Bitnami Charts Repository with Depth 1
git clone --depth 1 https://github.com/bitnami/charts.git
cd charts

# Step 2: Checkout to branch with pull request applied
git fetch origin pull/21913/head:redis-master-service
git checkout redis-master-service

values.yaml with workaround values from #22541:

master:
  serviceAccount:
    name: redis
replica:
  replicaCount: 3
  serviceAccount:
    name: redis
    create: false
sentinel:
  enabled: true
  service:
    type: LoadBalancer
    createMaster: true

installing the chart:

helm install --values values.yaml redis ./bitnami/redis/

maybe I'm doing something wrong, maybe this will help you in your testing 🤷‍♂️

[anthosz]

@david-sykora if I try with my last commit: 12611e1d1636953a897e56af2f0c532bb496b9cb, all is ok but rbac need to be created:

rbac:
  create: true

The pod need this role to extract some metadatas to be able to put the isMaster label.

Moreover, the label used is isMaster & not isRedis.

If I check on the last commits, yep completelly broken. Probably due to the fact serviceaccount is disabled so sentinel is lost.

[anthosz]

Doc updated

[juan131]

Hi @anthosz It seems sth went wrong while merging latest changes from main branch in your fork's branch. Please try to fix all these conflicts and update the PR

[anthosz]

Hi @juan131 ,

ready for review and aligned with master :)

[anthosz]

README updated

Ready for new review

[anthosz]

Hi @juan131 ,

Can we expect a to have this merged this month? It can be wonderfull

[anthosz]

Thanks for you patience! The changes now LGTM

I've been trying your changes and I found a situation where the solution may not work. It can be workarounded but it's not easy to identify the problem.

This situation occurs on clusters with RBAC enabled since "kubectl" is unable to contact the K8s API:

E0308 10:30:25.088684      21 memcache.go:265] couldn't get current server API group list: Get "http://localhost:8080/api?timeout=32s": dial tcp [::1]:8080: connect: connection refused

On these cases, the following parameters are mandatory for the solution to work:

* `rbac.create` must be set to `true` (by default is `false`).

* `replica.automountServiceAccountToken` must be set to `true` (by default is `false`).

* `serviceAccount.create` must be set to `true` (by default is `true`).

It'd be nice to include some warning in the chart installation NOTES if these parameters aren't properly set when enabling this feature (check https://github.com/bitnami/charts/blob/main/bitnami/redis/templates/_helpers.tpl#L255-L313)

In the meantime, I'll start preparing our internal systems to automatically update kubectl image tag on future releases.

Thank you!

Done!

Do I need to remove this note also https://github.com/bitnami/charts/pull/21913/files#diff-124e7200c9157afc7c5dcfbf90f04932771624080855e564889d2b21ef64ee3fR761-R766 ?

[juan131]

Yes please! Remove the references from the README

[anthosz]

Yes please! Remove the references from the README

done!

[anthosz]

Thank you very much for your feedbacks @juan131 !

[juan131]

Thank you for the great contribution! It was a pleasure to review it.

[jasaltvik]

@anthosz / @juan131 Thanks for this awesome feature. I've been testing it recently to add a Redis HA setup to a service that doesn't support Sentinel out-of-the-box. I realised that the master service only exposes the main Redis port, and not the sentinel port. And then I read https://github.com/bitnami/charts/tree/main/bitnami/redis#master-replicas-with-sentinel, which mentions

In addition to this, only one service is exposed:

    Redis® service: Exposes port 6379 for Redis® read-only operations and port 26379 for accessing Redis® Sentinel.

For read-only operations, access the service using port 6379. For write operations, it's necessary to access the Redis® Sentinel cluster and query the current master using the command below (using redis-cli or similar):

According to metrics, no keys were written to the new Redis HA setup, so I was just wondering if the sentinel port must be exposed as well to be able to write to the cluster via the master service. I might have misunderstood something regarding this setup as well.

Thanks for any input on this!

UPDATE! It works using the standard port after all 💯 We were testing it the wrong way. But, I was a bit confused about the read-only bit in https://github.com/bitnami/charts/tree/main/bitnami/redis#master-replicas-with-sentinel, so this might need some clarification (as it seems write is possible via 6379)?

[nachomateos]

@anthosz and @juan131 Thank you also for this awesome feature. I still have an issue in an installation in gke. I am using internal Load Balancers for the services, which means that in my vpc redis-master and redis services are exposed to external services to the cluster. Unfortunately, the service type and loadbalancerIP used for the master service are the same as the used for redis in the sentinels/service.yaml template:

{{- if and (eq .Values.sentinel.service.type "LoadBalancer") (not (empty .Values.sentinel.service.loadBalancerIP)) }}

GKE allows shared LBs, but in this case, the redis port is also exposed by both services using the same value:

{{ .Values.sentinel.service.ports.redis }}

Is there a possibility to change the template to support a different loadbalancerIP/serviceType for the master service? Or am I missing something here to work around it? Thank for any inputs/recommendations!

[anthosz]

@nachomateos I guess you can add a PR/ticket to add this feature.

Unfortunately, on my side, I don't have any env that expose Redis externally

[juan131]

Hi @nachomateos

Thank you for bringing this issue to our attention. We appreciate your involvement! If you're interested in contributing a solution, we welcome you to create a pull request. The Bitnami team is excited to review your submission and offer feedback. You can find the contributing guidelines here.

Your contribution will greatly benefit the community. Feel free to reach out if you have any questions or need assistance.

[nachomateos]

@juan131 , @anthosz, it will be my first PR in this community but I will definitely do that. Thanks!