Helm Chart Enhancements: Cluster Mode Deployment, Adding, Resharding, and Deleting Clusters #534
Conversation
…re already clustered before attempting to
|
Can folks in the community review this PR (we do not use Helm much ourselves)? @babykart @AlexCaston |
charts/garnet/templates/cmd-pod.yaml
Outdated
| spec: | ||
| containers: | ||
| - name: cmd | ||
| image: redis:latest |
There was a problem hiding this comment.
consider allowing to override this - similar to the init-job configuration ("{{ .Values.cluster.initJob.image.registry }}/{{ .Values.cluster.initJob.image.repository }}:{{ .Values.cluster.initJob.image.tag | default "latest" }}")
There was a problem hiding this comment.
How was this addressed, I don't see a new commit. Thanks.
There was a problem hiding this comment.
consider splitting into two separate files (ie. cluster-sts.yaml & standalone-sts.yaml) so that it is more easily readable and maintainable.
charts/garnet/templates/cmd-pod.yaml
Outdated
| spec: | ||
| containers: | ||
| - name: cmd | ||
| image: redis:latest |
There was a problem hiding this comment.
Why does this say redis-latest? We have nothing to do with that image.
There was a problem hiding this comment.
that pod is used for testing. So, i'll remove it from the pr.
There was a problem hiding this comment.
Also to use the redis-cli, don't we have to use the redis image?
There was a problem hiding this comment.
Isn't redis-tools sufficient for this?
| command: ["/bin/sh", "-c"] | ||
| args: | ||
| - | | ||
| garnet_host="{{ include "garnet.fullname" . }}-0.{{ include "garnet.fullname" . }}-headless.{{ .Release.Namespace }}.svc.cluster.local" |
There was a problem hiding this comment.
What happens when pod-0 is down or unknown state ?
With reshard-delete-job.yaml, if pod0 restarts how will it be able to re-shard
| command: ["/bin/sh", "-c"] | ||
| args: | ||
| - | | ||
| garnet_host="{{ include "garnet.fullname" . }}-0.{{ include "garnet.fullname" . }}-headless.{{ .Release.Namespace }}.svc.cluster.local" |
There was a problem hiding this comment.
This contains quite some logic for resharding. Is there a way we can unit test this 🤔
| args: | ||
| - | | ||
| garnet_host="{{ include "garnet.fullname" . }}-0.{{ include "garnet.fullname" . }}-headless.{{ .Release.Namespace }}.svc.cluster.local" | ||
| garnet_port="{{ .Values.containers.port }}" |
There was a problem hiding this comment.
garnet host, garnet port being used across multiple files. Wondering if this can be centrally managed along with other configurations like retries, sleep intervals..
There was a problem hiding this comment.
This file must be removed because is duplicated with the standalone-sts.yaml.
| affinity: {} | ||
|
|
||
| # Default values | ||
| statefulSet: |
There was a problem hiding this comment.
This key should be removed and the sub key annotations should be put in cluster.statefulSet & standalone.statefulSet .
There was a problem hiding this comment.
Don't remove the comments (# -- ...) that wil be used by helm-docs to generate the README.md
Furthermore, I invite you to add this type of comment to provide a description per key, briefly describing its use for users.
|
@njnicko - could you address the comments and push commits when you get a chance, thanks again for your PR. |
|
Closing as outdated. We have a proper Helm chart package now thanks to @babykart. Would be great to see a cluster version contributed over that version, at some point. Thank you! |
This PR introduces several enhancements to the Helm chart for deploying Redis clusters in cluster mode. The key features include:
Cluster Mode Deployment
Node Addition
Node Removal
Resharding
This PR aims to simplify the management of Redis clusters and enhance scalability and reliability.