Skip to content

feat(helm): add cluster mode values for Dragonfly chart#7123

Open
VedantMadane wants to merge 3 commits intodragonflydb:mainfrom
VedantMadane:feat/helm-cluster-mode-3861
Open

feat(helm): add cluster mode values for Dragonfly chart#7123
VedantMadane wants to merge 3 commits intodragonflydb:mainfrom
VedantMadane:feat/helm-cluster-mode-3861

Conversation

@VedantMadane
Copy link
Copy Markdown

@VedantMadane VedantMadane commented Apr 12, 2026

Summary

Adds predefined Helm values for Redis-compatible cluster mode so users can enable Dragonfly cluster flags without hand-writing extraArgs.

Changes

  • values.yaml: new cluster.mode (maps to --cluster_mode) and cluster.experimentalShardBySlot (maps to --experimental_cluster_shard_by_slot=true when mode is set)
  • templates/_pod.tpl: append the corresponding CLI args when cluster.mode is non-empty

Notes

Valid cluster.mode values match the Dragonfly flag: emulated, yes, or empty to leave cluster mode disabled. See server cluster_support.cc for flag semantics.

Fixes #3861

Copilot AI review requested due to automatic review settings April 12, 2026 11:49
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds first-class Helm values for enabling Dragonfly’s Redis-compatible cluster mode, so users don’t need to manually supply extraArgs for common cluster flags.

Changes:

  • Introduces cluster.mode and cluster.experimentalShardBySlot in chart values.yaml.
  • Updates the pod template to append --cluster_mode=... and optionally --experimental_cluster_shard_by_slot=true based on those values.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
contrib/charts/dragonfly/values.yaml Adds documented cluster.* values for cluster mode and slot-based sharding.
contrib/charts/dragonfly/templates/_pod.tpl Renders the corresponding Dragonfly CLI flags when cluster.mode is set.

Comment thread contrib/charts/dragonfly/values.yaml Outdated
extraArgs: []

cluster:
# -- Redis cluster protocol mode passed to Dragonfly as --cluster_mode. Use "emulated", "yes", or leave empty to disable cluster mode.
Copy link

Copilot AI Apr 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

.Values.cluster.mode is documented as accepting "yes", but in YAML users often write mode: yes (unquoted), which parses as a boolean and can either render --cluster_mode=true or even break the ne comparison during templating. Consider updating the docs here to explicitly require quoting (e.g. mode: "yes") and/or changing the chart to coerce the value to string before comparing/printing.

Suggested change
# -- Redis cluster protocol mode passed to Dragonfly as --cluster_mode. Use "emulated", "yes", or leave empty to disable cluster mode.
# -- Redis cluster protocol mode passed to Dragonfly as --cluster_mode.
# -- This value must be quoted because YAML may parse unquoted `yes` as a boolean.
# -- Use `mode: "emulated"` or `mode: "yes"`, or leave empty to disable cluster mode.

Copilot uses AI. Check for mistakes.
Comment on lines +90 to +93
{{- if ne (.mode | default "") "" }}
- "--cluster_mode={{ .mode }}"
{{- end }}
{{- if and (ne (.mode | default "") "") .experimentalShardBySlot }}
Copy link

Copilot AI Apr 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The ne (.mode | default "") "" check and --cluster_mode={{ .mode }} assume .mode is a string; if a user sets cluster.mode: yes without quotes, Helm may parse it as boolean and this comparison can fail (or render --cluster_mode=true). Coerce .mode to a string (and ideally validate allowed values) before comparing/using it.

Suggested change
{{- if ne (.mode | default "") "" }}
- "--cluster_mode={{ .mode }}"
{{- end }}
{{- if and (ne (.mode | default "") "") .experimentalShardBySlot }}
{{- $clusterMode := printf "%v" (.mode | default "") }}
{{- if ne $clusterMode "" }}
- "--cluster_mode={{ $clusterMode }}"
{{- end }}
{{- if and (ne $clusterMode "") .experimentalShardBySlot }}

Copilot uses AI. Check for mistakes.
@romange
Copy link
Copy Markdown
Collaborator

romange commented Apr 12, 2026

augment review

Copy link
Copy Markdown
Contributor

@Abhra303 Abhra303 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR, see some comments.

{{- end }}
{{- end }}
{{- end }}
{{- end }} No newline at end of file
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add back newline at the end of the file

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, restored the trailing newline.

Comment thread contrib/charts/dragonfly/values.yaml Outdated
Comment thread contrib/charts/dragonfly/values.yaml Outdated

cluster:
# -- Redis cluster protocol mode passed to Dragonfly as --cluster_mode.
# -- Use "emulated" or "yes" (must be quoted in YAML to avoid boolean coercion).
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the user passes unquoted yes, the template won't validate this and and df will fail. I suggest to either validate the input and fail if the input is not string or support boolean yes as well. As you're adding a cluster specific fields, it makes sense to support boolean yes as well.

for boolean yes support :

  {{- with .Values.cluster }}                                                                                                                                                                              
  {{- $modeRaw := .mode | default "" }}
  {{- $clusterMode := "" }}
  {{- if kindIs "bool" $modeRaw }}
    {{- if $modeRaw }}{{- $clusterMode = "yes" }}{{- end }}                                                                                                                                                
  {{- else }}
    {{- $clusterMode = $modeRaw | toString }}                                                                                                                                                              
  {{- end }}                                                                                                                                                                                               
  {{- if ne $clusterMode "" }}
    - "--cluster_mode={{ $clusterMode }}"                                                                                                                                                                  
  {{- end }}  

Or, if you want to validate:

  {{- with .Values.cluster }}
  {{- $modeRaw := .mode | default "" }}                                                                                                                                                                    
  {{- if kindIs "bool" $modeRaw }}
    {{- fail "cluster.mode must be a quoted string — use '\"yes\"' or '\"emulated\"' (unquoted 'yes' is parsed as a YAML boolean)" }}
  {{- end }}                                                                                                                                                                                               
  {{- $clusterMode := $modeRaw | toString }}

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good call, went with the boolean coercion approach. If .mode is a bool (unquoted yes in YAML), it now maps to the string "yes". Updated the values.yaml comment to document that behavior too.

@Abhra303
Copy link
Copy Markdown
Contributor

Also wondering we have extraArgs which should be enough to add cluster mode values. Why do you think it is not viable to set cluster mode via extraArgs?

@romange romange requested a review from Abhra303 April 13, 2026 13:06
Copy link
Copy Markdown
Contributor

@Abhra303 Abhra303 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good.

@romange
Copy link
Copy Markdown
Collaborator

romange commented Apr 13, 2026

@VedantMadane please squash and sign your commits. See this for details:

### Signing commits with GPG
We require all commits to be signed with a GPG key. To set this up:
```sh
# List your GPG keys
gpg --list-secret-keys --keyid-format=long
# If you don't have a key, create one
gpg --full-generate-key
# Configure git to use your GPG key for signing (replace KEY_ID with your actual key)
git config --global user.signingkey KEY_ID
# Enable automatic signing for all commits
git config --global commit.gpgsign true
```
All commits must be signed (this is enforced by the pre-commit hooks). When you commit, you'll be prompted for your GPG passphrase if it's not cached.

@VedantMadane
Copy link
Copy Markdown
Author

You raise a fair point about extraArgs. The difference is that dedicated cluster.mode values let the chart validate input (the boolean coercion fix), set sensible defaults for related settings (like replica count), and keep the values.yaml self-documenting for users who helm show values the chart. extraArgs would work but pushes validation onto the user.

That said, if you prefer the extraArgs-only approach, I am happy to simplify the PR to just the documentation changes. Let me know which direction you would like.

Regarding squash and sign-off: will do that now.

@VedantMadane VedantMadane force-pushed the feat/helm-cluster-mode-3861 branch from 2817e38 to da26108 Compare April 15, 2026 15:01
Vedant Madane and others added 3 commits April 15, 2026 15:30
Add dedicated cluster.mode and cluster.node_count values to the Helm
chart so users can enable cluster mode without resorting to extraArgs.
The template coerces unquoted YAML booleans (yes/no) to the string
Dragonfly expects, preventing silent misconfiguration.

Fixes dragonflydb#3861

Signed-off-by: Vedant Madane <vedant.madane@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

No predefined configuration for cluster mode in helm chart

4 participants