feat(helm): add memcached port support to Service#6608
feat(helm): add memcached port support to Service#6608badal773 wants to merge 8 commits intodragonflydb:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds comprehensive memcached protocol support to the Dragonfly Helm chart, addressing issue #6567 where users could not properly expose the Memcached port (11211) via Kubernetes Services. The implementation provides a clean, declarative way to enable memcached support through values.yaml configuration.
Changes:
- Added
memcached.enabledandmemcached.portconfiguration options with sensible defaults - Added
service.targetPortconfiguration to allow custom port routing - Automatic configuration when memcached is enabled: container port exposure, Service port mapping, and
--memcached_portargument injection - Comprehensive test coverage with golden test files
- Updated documentation in README.md
- Chart version bump from v1.36.0 to v1.36.1
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| contrib/charts/dragonfly/values.yaml | Added memcached configuration section and service.targetPort parameter |
| contrib/charts/dragonfly/templates/service.yaml | Added conditional memcached port to Service and templated targetPort |
| contrib/charts/dragonfly/templates/_pod.tpl | Added conditional memcached container port and automatic --memcached_port argument |
| contrib/charts/dragonfly/ci/memcached-values.yaml | Test configuration for memcached feature |
| contrib/charts/dragonfly/ci/memcached-values.golden.yaml | Expected golden output for memcached test |
| contrib/charts/dragonfly/ci/tls-values.golden.yaml | Updated checksum due to template changes |
| contrib/charts/dragonfly/README.md | Added documentation for new memcached parameters |
| contrib/charts/dragonfly/Chart.yaml | Bumped chart version to v1.36.1 |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Badal Kumar <130441461+badal773@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Badal Kumar <130441461+badal773@users.noreply.github.com>
| {{- if and .Values.memcached .Values.memcached.enabled }} | ||
| - "--memcached_port={{ .Values.memcached.port }}" | ||
| {{- end }} | ||
| {{- with .Values.extraArgs }} | ||
| {{- toYaml . | trim | nindent 6 }} | ||
| {{- end }} |
There was a problem hiding this comment.
The --memcached_port argument is placed before extraArgs (line 92), while TLS arguments are placed after extraArgs (lines 97-100). This inconsistency in argument ordering could lead to confusion. More critically, placing memcached args before extraArgs allows users to override them, potentially creating a mismatch between memcached.port and the actual --memcached_port flag. Consider moving the memcached args to after extraArgs for consistency with TLS configuration and to prevent accidental overrides.
| {{- if and .Values.memcached .Values.memcached.enabled }} | |
| - "--memcached_port={{ .Values.memcached.port }}" | |
| {{- end }} | |
| {{- with .Values.extraArgs }} | |
| {{- toYaml . | trim | nindent 6 }} | |
| {{- end }} | |
| {{- with .Values.extraArgs }} | |
| {{- toYaml . | trim | nindent 6 }} | |
| {{- end }} | |
| {{- if and .Values.memcached .Values.memcached.enabled }} | |
| - "--memcached_port={{ .Values.memcached.port }}" | |
| {{- end }} |
| {{- end }} | ||
| {{- end }} | ||
| {{- end }} |
There was a problem hiding this comment.
The validation only checks if memcached.port equals service.port, but it doesn't check if memcached.port equals 6379 (the hardcoded Redis port). If a user sets memcached.port to 6379, Dragonfly will fail to start because both protocols would try to bind to the same port. The validation should also check: if eq (int .Values.memcached.port) 6379
| {{- end }} | |
| {{- end }} | |
| {{- end }} | |
| {{- end }} | |
| {{- if eq (int .Values.memcached.port) 6379 }} | |
| {{- fail "memcached.port must not be 6379 (Redis/Dragonfly default port). Please use a different port for memcached." }} | |
| {{- end }} | |
| {{- end }} | |
| {{- end }} |
| @@ -1,3 +1,4 @@ | |||
| {{- include "dragonfly.validateMemcached" . }} | |||
There was a problem hiding this comment.
The validation is only called from service.yaml, which means if someone renders deployment.yaml or statefulset.yaml in isolation (e.g., helm template with --show-only), they won't get the validation error. Consider also adding the validation to deployment.yaml and statefulset.yaml to ensure it always runs when memcached is enabled.
| # -- Dragonfly target port on the pod. Can be a port number or a named port (dragonfly or memcached) | ||
| targetPort: dragonfly |
There was a problem hiding this comment.
There's no validation to ensure that service.targetPort is a valid value. If a user sets service.targetPort to an arbitrary number or name that doesn't match any containerPort, the Service will be created but traffic won't route correctly. Consider adding validation to ensure targetPort is either "dragonfly", "memcached", 6379, or (when memcached is enabled) the memcached.port value.
| # -- Dragonfly target port on the pod. Can be a port number or a named port (dragonfly or memcached) | |
| targetPort: dragonfly | |
| # -- Dragonfly target port on the pod. Must match a containerPort and should be one of: | |
| # -- 6379, "dragonfly", "memcached", or (when memcached is enabled) the configured memcached.port. | |
| targetPort: 6379 |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Badal Kumar <130441461+badal773@users.noreply.github.com>
| {{- if eq (int .Values.memcached.port) (int .Values.service.port) }} | ||
| {{- fail "memcached.port must not be the same as service.port (default 6379). Please use a different port for memcached." }} |
There was a problem hiding this comment.
The validation logic checks if memcached.port equals service.port, but this is incorrect. According to the issue description and Dragonfly's behavior, memcached.port must not equal 6379 (the fixed Redis/Dragonfly container port), not service.port. The service.port is just the Service's external port and can be changed independently. The validation should be: if eq (int .Values.memcached.port) 6379, fail with error message "memcached.port must not be 6379 (the Redis/Dragonfly port). Please use a different port for memcached."
| {{- if eq (int .Values.memcached.port) (int .Values.service.port) }} | |
| {{- fail "memcached.port must not be the same as service.port (default 6379). Please use a different port for memcached." }} | |
| {{- if eq (int .Values.memcached.port) 6379 }} | |
| {{- fail "memcached.port must not be 6379 (the Redis/Dragonfly port). Please use a different port for memcached." }} |
|
@Abhra303 can you PTAL? |
Signed-off-by: Roman Gershman <roman@dragonflydb.io>
|
Stale PR |
Fixes #6567
Summary
Add memcached support to the Helm chart. When
memcached.enabled=true, the chart automatically configures everything needed.Changes
memcached.enabledandmemcached.portvalues--memcached_portarg when memcached is enabledservice.targetPortfor custom routingUsage