-
Notifications
You must be signed in to change notification settings - Fork 521
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
Helm Chart Enhancements: Cluster Mode Deployment, Adding, Resharding, and Deleting Clusters #534
base: main
Are you sure you want to change the base?
Conversation
…re already clustered before attempting to
@njnicko please read the following Contributor License Agreement(CLA). If you agree with the CLA, please reply with the following information.
Contributor License AgreementContribution License AgreementThis Contribution License Agreement (“Agreement”) is agreed to by the party signing below (“You”),
|
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How was this addressed, I don't see a new commit. Thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does this say redis-latest? We have nothing to do with that image.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that pod is used for testing. So, i'll remove it from the pr.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also to use the redis-cli, don't we have to use the redis image?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file must be removed because is duplicated with the standalone-sts.yaml.
affinity: {} | ||
|
||
# Default values | ||
statefulSet: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. |
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.