feat(valkey): add managed Valkey service - #3380
Conversation
📝 WalkthroughWalkthroughAdds managed Valkey support across CRD types, Helm deployment templates, versioned Redis-compatible images, platform registration, permissions, documentation, build automation, and Chainsaw end-to-end validation. ChangesValkey API and chart runtime
Build and platform integration
Validation and documentation
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a managed Valkey service to the Cozystack platform. By leveraging the existing redis-operator through a lightweight compatibility layer, this change provides a robust, BSD-3-Clause licensed alternative to Redis. The implementation includes all necessary Helm charts, API definitions, monitoring resources, and automated versioning scripts to integrate Valkey seamlessly into the current infrastructure. Highlights
New Features🧠 You can now enable Memory (public preview) to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Ignored Files
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on Gemini (@gemini-code-assist) comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request introduces a new managed Valkey service to Cozystack, adding API types, a Helm chart with a thin Redis-compatibility layer, E2E tests, and platform integration. The review feedback highlights a bug in the service scrape configuration where target_label is incorrectly written in snake_case, and points out potential resource over-provisioning for Sentinel pods. Additionally, improvements are suggested for the version update script to preserve file permissions and ensure proper cleanup, alongside minor cleanups of redundant template conditionals and empty keys.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| - metricRelabelConfigs: | ||
| relabelConfigs: | ||
| - replacement: valkey | ||
| targetLabel: job | ||
| - sourceLabels: [__meta_kubernetes_pod_node_name] | ||
| targetLabel: node | ||
| - replacement: cluster | ||
| targetLabel: tier | ||
| - target_label: service | ||
| replacement: {{ .Release.Name }} | ||
| port: metrics |
There was a problem hiding this comment.
There are two issues in this endpoint definition:
- Bug: On line 22,
target_labelis written in snake_case. Kubernetes CRDs (likeVMServiceScrapeandServiceMonitor) use camelCase (targetLabel). Usingtarget_labelwill cause the operator to ignore this relabeling rule, and theservicelabel will not be set on the metrics. - Redundancy: The empty
metricRelabelConfigs:key on line 14 is unnecessary. We can clean this up by movingport: metricsto the top of the endpoint block and removing the empty key.
- port: metrics
relabelConfigs:
- replacement: valkey
targetLabel: job
- sourceLabels: [__meta_kubernetes_pod_node_name]
targetLabel: node
- replacement: cluster
targetLabel: tier
- targetLabel: service
replacement: {{ .Release.Name }}| sentinel: | ||
| replicas: 3 | ||
| resources: {{- include "cozy-lib.resources.defaultingSanitize" (list .Values.resourcesPreset .Values.resources $) | nindent 6 }} |
There was a problem hiding this comment.
Sentinel is a lightweight monitoring and failover coordinator that does not store data or serve database traffic. Assigning it the same resources and resourcesPreset as the main Valkey database pods can lead to massive resource over-provisioning (e.g., if a user selects a large preset like m1.large for the database, the 3 Sentinel pods will also request m1.large resources, wasting a significant amount of CPU and memory).
Consider using a fixed small preset (like nano) for Sentinel, or providing a separate configuration for Sentinel resources.
sentinel:
replicas: 3
resources: {{- include "cozy-lib.resources.defaultingSanitize" (list "nano" dict $) | nindent 6 }}| ' "$VALUES_FILE" > "$TEMP_FILE.tmp" | ||
| mv "$TEMP_FILE.tmp" "$VALUES_FILE" |
There was a problem hiding this comment.
Using mv with a file created by mktemp can cause the target file (values.yaml) to lose its original permissions (since mktemp creates files with 0600 permissions, which mv preserves). Additionally, writing to $TEMP_FILE.tmp bypasses the trap cleanup if the script fails mid-execution.
To fix both issues, write directly to $TEMP_FILE (which is tracked by the trap) and copy its contents back using cat to preserve the original file permissions of values.yaml.
| ' "$VALUES_FILE" > "$TEMP_FILE.tmp" | |
| mv "$TEMP_FILE.tmp" "$VALUES_FILE" | |
| ' "$VALUES_FILE" > "$TEMP_FILE" | |
| cat "$TEMP_FILE" > "$VALUES_FILE" |
| ' "$VALUES_FILE" > "$TEMP_FILE.tmp" | ||
| mv "$TEMP_FILE.tmp" "$VALUES_FILE" |
There was a problem hiding this comment.
Similarly, write directly to $TEMP_FILE and use cat to overwrite values.yaml to ensure proper cleanup and preserve file permissions.
| ' "$VALUES_FILE" > "$TEMP_FILE.tmp" | |
| mv "$TEMP_FILE.tmp" "$VALUES_FILE" | |
| ' "$VALUES_FILE" > "$TEMP_FILE" | |
| cat "$TEMP_FILE" > "$VALUES_FILE" |
| app: {{ .Release.Name }}-metrics | ||
| app.kubernetes.io/instance: {{ .Release.Name }} | ||
| app.kubernetes.io/managed-by: {{ .Release.Service }} | ||
| annotations: |
There was a problem hiding this comment.
The annotations: key is empty and redundant. It should be removed to keep the manifest clean.
| type: {{ ternary "LoadBalancer" "ClusterIP" .Values.external }} | ||
| {{- if .Values.external }} | ||
| externalTrafficPolicy: Local | ||
| {{- if (include "cozy-lib.network.disableLoadBalancerNodePorts" $ | fromYaml) }} | ||
| allocateLoadBalancerNodePorts: false | ||
| {{- end }} | ||
| {{- end }} |
There was a problem hiding this comment.
Since the entire Service manifest is already wrapped in {{- if .Values.external }} (line 1), .Values.external is guaranteed to be true here. We can simplify the type field and remove the redundant inner if check.
type: LoadBalancer
externalTrafficPolicy: Local
{{- if (include "cozy-lib.network.disableLoadBalancerNodePorts" $ | fromYaml) }}
allocateLoadBalancerNodePorts: false
{{- end }}There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/apps/valkey/templates/service.yaml (1)
11-17: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSimplify redundant conditionals for the external service.
Since the entire file is wrapped in
{{- if .Values.external }}(line 1), the service will only be created whenexternalis truthy. Therefore,typewill always evaluate to"LoadBalancer"and the nested{{- if .Values.external }}check is redundant.Consider simplifying the code to remove the redundancy.
♻️ Proposed refactor
- type: {{ ternary "LoadBalancer" "ClusterIP" .Values.external }} - {{- if .Values.external }} - externalTrafficPolicy: Local - {{- if (include "cozy-lib.network.disableLoadBalancerNodePorts" $ | fromYaml) }} - allocateLoadBalancerNodePorts: false - {{- end }} - {{- end }} + type: LoadBalancer + externalTrafficPolicy: Local + {{- if (include "cozy-lib.network.disableLoadBalancerNodePorts" $ | fromYaml) }} + allocateLoadBalancerNodePorts: false + {{- end }}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/apps/valkey/templates/service.yaml` around lines 11 - 17, In the service template, simplify the external-service block by setting the service type directly to LoadBalancer and removing the redundant .Values.external conditional around externalTrafficPolicy and allocateLoadBalancerNodePorts. Preserve the existing disableLoadBalancerNodePorts condition and its behavior.packages/apps/valkey/templates/servicescrape.yaml (1)
22-23: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueFix inconsistent field naming to prevent potential CRD pruning.
The
relabelConfigsfield usestargetLabel(camelCase) for the preceding entries, but switches totarget_label(snake_case) here. Kubernetes CRDs typically enforce camelCase fields, andtarget_labelmight be silently pruned by the API server depending on the VictoriaMetrics CRD schema definition.Use
targetLabelconsistently.♻️ Proposed refactor
- - target_label: service - replacement: {{ .Release.Name }} + - targetLabel: service + replacement: {{ .Release.Name }}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/apps/valkey/templates/servicescrape.yaml` around lines 22 - 23, Update the final relabelConfigs entry in the servicescrape template to use the camelCase targetLabel field instead of target_label, matching the preceding entries and the CRD schema.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/apps/valkey/README.md`:
- Around line 15-38: Regenerate the ## Parameters section in README.md using
cozyvalues-gen by running make generate from packages/apps/valkey or the
repository root. Commit the generated table alignment and spacing changes,
without manually editing the generated output.
---
Nitpick comments:
In `@packages/apps/valkey/templates/service.yaml`:
- Around line 11-17: In the service template, simplify the external-service
block by setting the service type directly to LoadBalancer and removing the
redundant .Values.external conditional around externalTrafficPolicy and
allocateLoadBalancerNodePorts. Preserve the existing
disableLoadBalancerNodePorts condition and its behavior.
In `@packages/apps/valkey/templates/servicescrape.yaml`:
- Around line 22-23: Update the final relabelConfigs entry in the servicescrape
template to use the camelCase targetLabel field instead of target_label,
matching the preceding entries and the CRD schema.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: fd3252c8-c822-4cc7-9f27-864c317bd2a5
⛔ Files ignored due to path filters (1)
packages/apps/valkey/logos/valkey.svgis excluded by!**/*.svg
📒 Files selected for processing (33)
.github/workflows/pr-labeler.yamlMakefileapi/apps/v1alpha1/valkey/types.goapi/apps/v1alpha1/valkey/zz_generated.deepcopy.godocs/storage-immutability.mdhack/e2e-chainsaw/valkey/chainsaw-test.yamlhack/e2e-chainsaw/valkey/valkey.yamlpackages/apps/valkey/.helmignorepackages/apps/valkey/Chart.yamlpackages/apps/valkey/Makefilepackages/apps/valkey/README.mdpackages/apps/valkey/charts/cozy-libpackages/apps/valkey/files/versions.yamlpackages/apps/valkey/hack/update-versions.shpackages/apps/valkey/images/valkey-7.2.13.tagpackages/apps/valkey/images/valkey-8.1.8.tagpackages/apps/valkey/images/valkey/Dockerfilepackages/apps/valkey/templates/_resources.tplpackages/apps/valkey/templates/_versions.tplpackages/apps/valkey/templates/dashboard-resourcemap.yamlpackages/apps/valkey/templates/service.yamlpackages/apps/valkey/templates/servicescrape.yamlpackages/apps/valkey/templates/valkeyfailover.yamlpackages/apps/valkey/values.schema.jsonpackages/apps/valkey/values.yamlpackages/core/platform/sources/valkey-application.yamlpackages/core/platform/templates/bundles/paas.yamlpackages/system/cozystack-basics/templates/clusterroles.yamlpackages/system/valkey-rd/Chart.yamlpackages/system/valkey-rd/Makefilepackages/system/valkey-rd/cozyrds/valkey.yamlpackages/system/valkey-rd/templates/cozyrd.yamlpackages/system/valkey-rd/values.yaml
|
Before we proceed, I think we need an explicit maintainer position on the operator lifecycle.
My preference is to move away from the Spotahome operator to an actively maintained alternative with comparable capabilities—Sentinel-style HA/failover, persistence, authentication, metrics, and external access. Maintaining our own fork does not look like a good long-term tradeoff: this operator is a small, non-core part of Cozystack, and owning its maintenance would consume disproportionate time and resources. Could the author/maintainers please explicitly acknowledge the upstream archival and state the intended position?
I would prefer not to add another consumer until this direction is clear. |
There was a problem hiding this comment.
Code Review
This pull request introduces a new managed Valkey application to Cozystack, integrating it as a drop-in Redis-compatible engine. It includes the API types, Helm chart templates, a thin compatibility Dockerfile, version update scripts, and E2E chainsaw tests. The review feedback highlights several improvement opportunities: optimizing Sentinel resource allocations to prevent waste (and updating the WorkloadMonitor label accordingly), ensuring proper cleanup in the version update script by writing directly to the tracked temporary file, and removing redundant checks and empty blocks in the Helm templates to improve readability.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| sentinel: | ||
| replicas: 3 | ||
| resources: {{- include "cozy-lib.resources.defaultingSanitize" (list .Values.resourcesPreset .Values.resources $) | nindent 6 }} |
There was a problem hiding this comment.
Sentinel is a lightweight coordinator and does not require the same resource allocation as the main Valkey database. Sharing .Values.resources and .Values.resourcesPreset with Sentinel can lead to significant resource waste when scaling up the database. Consider hardcoding Sentinel to a minimal preset like nano (or exposing a separate configuration for it).
sentinel:
replicas: 3
resources: {{- include "cozy-lib.resources.defaultingSanitize" (list "nano" dict $) | nindent 6 }}| name: {{ $.Release.Name }}-sentinel | ||
| namespace: {{ $.Release.Namespace }} | ||
| labels: | ||
| workloads.cozystack.io/resource-preset: {{ .Values.resourcesPreset | quote }} |
There was a problem hiding this comment.
If Sentinel is configured with a fixed lightweight preset (like nano), the resource-preset label on its WorkloadMonitor should also be updated to match, preventing the controller from applying the database's larger preset to Sentinel pods.
workloads.cozystack.io/resource-preset: "nano"| awk -v new_section="$NEW_VERSION_SECTION" ' | ||
| /^## @enum {string} Version/ { | ||
| in_section = 1 | ||
| print new_section | ||
| next | ||
| } | ||
| in_section && /^version: / { | ||
| in_section = 0 | ||
| next | ||
| } | ||
| in_section { | ||
| next | ||
| } | ||
| { print } | ||
| ' "$VALUES_FILE" > "$TEMP_FILE.tmp" | ||
| mv "$TEMP_FILE.tmp" "$VALUES_FILE" |
There was a problem hiding this comment.
Writing to a .tmp file outside of the tracked TEMP_FILE path bypasses the cleanup trap if the script is interrupted or fails during the awk execution. Writing directly to TEMP_FILE ensures it is properly cleaned up on exit.
| awk -v new_section="$NEW_VERSION_SECTION" ' | |
| /^## @enum {string} Version/ { | |
| in_section = 1 | |
| print new_section | |
| next | |
| } | |
| in_section && /^version: / { | |
| in_section = 0 | |
| next | |
| } | |
| in_section { | |
| next | |
| } | |
| { print } | |
| ' "$VALUES_FILE" > "$TEMP_FILE.tmp" | |
| mv "$TEMP_FILE.tmp" "$VALUES_FILE" | |
| awk -v new_section="$NEW_VERSION_SECTION" ' | |
| /^## @enum {string} Version/ { | |
| in_section = 1 | |
| print new_section | |
| next | |
| } | |
| in_section && /^version: / { | |
| in_section = 0 | |
| next | |
| } | |
| in_section { | |
| next | |
| } | |
| { print } | |
| ' "$VALUES_FILE" > "$TEMP_FILE" | |
| mv "$TEMP_FILE" "$VALUES_FILE" |
| awk -v new_section="$NEW_VERSION_SECTION" ' | ||
| /^## @section Application-specific parameters/ { | ||
| print new_section | ||
| print "" | ||
| } | ||
| { print } | ||
| ' "$VALUES_FILE" > "$TEMP_FILE.tmp" | ||
| mv "$TEMP_FILE.tmp" "$VALUES_FILE" |
There was a problem hiding this comment.
Similarly, write directly to TEMP_FILE instead of TEMP_FILE.tmp to ensure proper cleanup by the exit trap.
| awk -v new_section="$NEW_VERSION_SECTION" ' | |
| /^## @section Application-specific parameters/ { | |
| print new_section | |
| print "" | |
| } | |
| { print } | |
| ' "$VALUES_FILE" > "$TEMP_FILE.tmp" | |
| mv "$TEMP_FILE.tmp" "$VALUES_FILE" | |
| awk -v new_section="$NEW_VERSION_SECTION" ' | |
| /^## @section Application-specific parameters/ { | |
| print new_section | |
| print "" | |
| } | |
| { print } | |
| ' "$VALUES_FILE" > "$TEMP_FILE" | |
| mv "$TEMP_FILE" "$VALUES_FILE" |
| type: {{ ternary "LoadBalancer" "ClusterIP" .Values.external }} | ||
| {{- if .Values.external }} | ||
| externalTrafficPolicy: Local | ||
| {{- if (include "cozy-lib.network.disableLoadBalancerNodePorts" $ | fromYaml) }} | ||
| allocateLoadBalancerNodePorts: false | ||
| {{- end }} | ||
| {{- end }} |
There was a problem hiding this comment.
Since the entire file is already wrapped in {{- if .Values.external }} on line 1, the nested {{- if .Values.external }} check and the ternary function are redundant. Simplifying this block improves readability.
type: LoadBalancer
externalTrafficPolicy: Local
{{- if (include "cozy-lib.network.disableLoadBalancerNodePorts" $ | fromYaml) }}
allocateLoadBalancerNodePorts: false
{{- end }}| metadata: | ||
| name: {{ .Release.Name }}-metrics | ||
| labels: | ||
| app: {{ .Release.Name }}-metrics | ||
| app.kubernetes.io/instance: {{ .Release.Name }} | ||
| app.kubernetes.io/managed-by: {{ .Release.Service }} | ||
| annotations: | ||
| spec: |
There was a problem hiding this comment.
The empty annotations: block is redundant and can be removed to keep the manifest clean.
metadata:
name: {{ .Release.Name }}-metrics
labels:
app: {{ .Release.Name }}-metrics
app.kubernetes.io/instance: {{ .Release.Name }}
app.kubernetes.io/managed-by: {{ .Release.Service }}
spec:6c0bf89 to
00d1a4e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/apps/valkey/templates/valkeyfailover.yaml`:
- Around line 26-28: Update the sentinel.resources configuration in the
valkeyfailover template to use minimal explicit CPU and memory requests/limits
instead of the database-level resourcesPreset/resources values, while leaving
the Valkey data-node resource configuration unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f983917e-bf29-4246-bf78-ca05f315f3b1
⛔ Files ignored due to path filters (1)
packages/apps/valkey/logos/valkey.svgis excluded by!**/*.svg
📒 Files selected for processing (33)
.github/workflows/pr-labeler.yamlMakefileapi/apps/v1alpha1/valkey/types.goapi/apps/v1alpha1/valkey/zz_generated.deepcopy.godocs/storage-immutability.mdhack/e2e-chainsaw/valkey/chainsaw-test.yamlhack/e2e-chainsaw/valkey/valkey.yamlpackages/apps/valkey/.helmignorepackages/apps/valkey/Chart.yamlpackages/apps/valkey/Makefilepackages/apps/valkey/README.mdpackages/apps/valkey/charts/cozy-libpackages/apps/valkey/files/versions.yamlpackages/apps/valkey/hack/update-versions.shpackages/apps/valkey/images/valkey-7.2.13.tagpackages/apps/valkey/images/valkey-8.1.8.tagpackages/apps/valkey/images/valkey/Dockerfilepackages/apps/valkey/templates/_resources.tplpackages/apps/valkey/templates/_versions.tplpackages/apps/valkey/templates/dashboard-resourcemap.yamlpackages/apps/valkey/templates/service.yamlpackages/apps/valkey/templates/servicescrape.yamlpackages/apps/valkey/templates/valkeyfailover.yamlpackages/apps/valkey/values.schema.jsonpackages/apps/valkey/values.yamlpackages/core/platform/sources/valkey-application.yamlpackages/core/platform/templates/bundles/paas.yamlpackages/system/cozystack-basics/templates/clusterroles.yamlpackages/system/valkey-rd/Chart.yamlpackages/system/valkey-rd/Makefilepackages/system/valkey-rd/cozyrds/valkey.yamlpackages/system/valkey-rd/templates/cozyrd.yamlpackages/system/valkey-rd/values.yaml
🚧 Files skipped from review as they are similar to previous changes (24)
- packages/system/valkey-rd/Makefile
- packages/apps/valkey/templates/_versions.tpl
- packages/core/platform/templates/bundles/paas.yaml
- packages/apps/valkey/Chart.yaml
- packages/apps/valkey/.helmignore
- packages/system/valkey-rd/values.yaml
- packages/apps/valkey/images/valkey/Dockerfile
- packages/apps/valkey/images/valkey-8.1.8.tag
- packages/apps/valkey/images/valkey-7.2.13.tag
- packages/core/platform/sources/valkey-application.yaml
- hack/e2e-chainsaw/valkey/valkey.yaml
- packages/system/valkey-rd/Chart.yaml
- packages/apps/valkey/files/versions.yaml
- packages/apps/valkey/charts/cozy-lib
- .github/workflows/pr-labeler.yaml
- docs/storage-immutability.md
- packages/system/cozystack-basics/templates/clusterroles.yaml
- packages/system/valkey-rd/cozyrds/valkey.yaml
- packages/apps/valkey/values.yaml
- packages/apps/valkey/values.schema.json
- api/apps/v1alpha1/valkey/types.go
- hack/e2e-chainsaw/valkey/chainsaw-test.yaml
- packages/apps/valkey/templates/_resources.tpl
- api/apps/v1alpha1/valkey/zz_generated.deepcopy.go
| sentinel: | ||
| replicas: 3 | ||
| resources: {{- include "cozy-lib.resources.defaultingSanitize" (list .Values.resourcesPreset .Values.resources $) | nindent 6 }} |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
Avoid allocating full database resources to Sentinel nodes.
Currently, the Sentinel nodes are assigned the exact same resource preset as the Valkey data nodes. Sentinel is a lightweight quorum manager; allocating it database-sized resources (e.g., large CPU/RAM instances) will lead to severe resource waste and potential scheduling issues.
Consider providing minimal explicit resource requests/limits for Sentinels.
⚡ Proposed fix
sentinel:
replicas: 3
- resources: {{- include "cozy-lib.resources.defaultingSanitize" (list .Values.resourcesPreset .Values.resources $) | nindent 6 }}
+ resources:
+ requests:
+ cpu: 50m
+ memory: 64Mi
+ limits:
+ memory: 128Mi📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| sentinel: | |
| replicas: 3 | |
| resources: {{- include "cozy-lib.resources.defaultingSanitize" (list .Values.resourcesPreset .Values.resources $) | nindent 6 }} | |
| sentinel: | |
| replicas: 3 | |
| resources: | |
| requests: | |
| cpu: 50m | |
| memory: 64Mi | |
| limits: | |
| memory: 128Mi |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/apps/valkey/templates/valkeyfailover.yaml` around lines 26 - 28,
Update the sentinel.resources configuration in the valkeyfailover template to
use minimal explicit CPU and memory requests/limits instead of the
database-level resourcesPreset/resources values, while leaving the Valkey
data-node resource configuration unchanged.
00d1a4e to
b97499f
Compare
|
Thanks — you're right, and here's the explicit position after digging into the alternatives. Acknowledged: Direction: migrate the operator to freshworks-oss/redis-operator — an actively-maintained (v3.3.5, commits through July 2026), Apache-2.0 fork that keeps the exact Answers to your questions:
Operator migration is a separate PR (#3406); this one now stacks on it. Also reaching out about upstreaming the cozystack/redis-operator TLS work into freshworks-oss so it lands in the shared fork. |
b97499f to
4180343
Compare
IvanHunters
left a comment
There was a problem hiding this comment.
Reviewed with the cozy-review methodology. Verdict: LGTM with non-blocking notes. No blocking findings. A clean additive mirror of the existing managed Redis app onto the same operator.
- Upgrade (Phase 5b): the diff is purely additive: a new app package, a new PackageSource plus bundle entry, a new ApplicationDefinition, and one
valkeysresource added to thecozy:tenant:admin:baseClusterRole. No migration, no default changes, no RBAC contraction, no CRD transition. - Fresh install:
sources/valkey-application.yamlis wired intobundles/paas.yaml:26;dependsOnis identical to redis; cozyrdsresourceNamesmatch the dashboard Role underrelease.prefix: valkey-;make generatehas no drift. - The Go API module builds and
go vetis clean. - Render corners (
external=true,version=v7,authEnabled=false) are valid and admittable;version=v9andsize=""are correctly rejected byvalues.schema.json.
Minor (non-blocking)
packages/apps/valkey/templates/valkeyfailover.yaml:119,145: container images (valkey/valkey:..., oliver006/redis_exporter:v1.55.0-alpine) are not digest-pinned and do not go through cozy-lib.image, so air-gapped / mirrored-registry installs fail to pull. This is verbatim parity with the existing packages/apps/redis (same two unpinned lines), i.e. inherited operator-chart debt, not a regression introduced by this PR. If fixed, add the new image lines to the Renovate managerFilePatterns.
Notes
- This PR is stacked on #3406 (base branch
feat/redis-operator-freshworks), andspec.engine: Valkeyis served by the CRD from #3406. #3406 should be merged first. - Follow-up (non-blocking): the chainsaw suite only covers the default corner; the
version=v7,external=true,authEnabled=falsebranches have no automated test (parity with redis).
…ss fork spotahome/redis-operator was archived 2026-06-11 (read-only). Switch the operator to the actively-maintained freshworks-oss/redis-operator fork, which keeps the same RedisFailover CRD and API group (databases.spotahome.com) and adds a native Valkey engine (spec.engine: Redis|Valkey) — so managed Redis is unaffected and managed Valkey no longer needs a Redis-compat shim image. - re-vendor the chart from oci://ghcr.io/freshworks-oss/charts/redis-operator - build the operator image from freshworks-oss v3.3.5 source - re-target the label/annotation-preservation patch onto the freshworks module The operator image is built out-of-band (make -C packages/system/redis-operator image) and its digest pinned in values.yaml; a maintainer must rebuild and pin before merge (tag is a placeholder here). Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Andrei Kvapil <andrei.kvapil@aenix.io>
Build the freshworks-oss v3.3.5 operator image (with our label-preservation patch) and pin its digest in values.yaml, replacing the placeholder tag. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Andrei Kvapil <andrei.kvapil@aenix.io>
7d03ed6 to
ba310b8
Compare
Add a managed Valkey service mirroring the managed Redis app. Valkey is the BSD-3-Clause, Linux Foundation-governed fork of Redis 7.2 and a drop-in replacement for it, offered as a governance-clean alternative to the now source-available (AGPLv3/SSPL/RSALv2) Redis 8.x. Valkey runs on the same Spotahome redis-operator: the operator hardcodes the redis-server command and redis-cli in its (non-overridable) probe scripts, so the chart deploys a thin compat image (FROM valkey/valkey:<ver>) that adds the redis-* symlinks over the official upstream binaries — no Valkey code rebuilt. Versions offered: v8 (8.1.8, default) and v7 (7.2.13, the BSD Redis 7.2 line). Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Andrei Kvapil <andrei.kvapil@aenix.io>
Wire the managed Valkey app into the platform: add the valkey-rd package carrying the Valkey ApplicationDefinition, the valkey-application PackageSource (reusing the existing redis-operator), enable it in the paas bundle, and grant tenant roles access to the valkeys resource. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Andrei Kvapil <andrei.kvapil@aenix.io>
Add the Valkey e2e bats test (mirrors redis), build the compat image in the top-level build target, map the valkey scope to area/database in the PR labeler, and list valkey among charts with an immutable storageClass. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Andrei Kvapil <andrei.kvapil@aenix.io>
The app e2e harness on main is chainsaw (hack/e2e-chainsaw/<app>/), not the older bats layout. Add hack/e2e-chainsaw/valkey/ mirroring the redis suite so select-e2e.sh maps the valkey-application source to the valkey suite, and drop the misplaced hack/e2e-apps/valkey.bats. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Andrei Kvapil <andrei.kvapil@aenix.io>
- servicescrape: fix target_label -> targetLabel (snake_case was ignored by the VM operator), drop the empty metricRelabelConfigs and annotations keys - service: drop the redundant inner external conditional (the whole manifest is already gated on .Values.external) - update-versions.sh: write through the trap-tracked temp file and cat back to preserve values.yaml permissions - regenerate README.md parameter table with the pinned cozyvalues-gen v1.6.0 (column widths differed from the committed version) Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Andrei Kvapil <andrei.kvapil@aenix.io>
With the operator on freshworks-oss/redis-operator, RedisFailover supports spec.engine: Valkey — the operator runs valkey-server/valkey-cli natively. Drop the thin Redis-compat wrapper image entirely and point the chart at the official valkey/valkey image directly: - RedisFailover: set spec.engine: Valkey, image valkey/valkey:<version> - remove images/ (Dockerfile + per-version tags), the image build target, the root build wiring and the images helmignore entry Depends on the freshworks-oss operator migration. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Andrei Kvapil <andrei.kvapil@aenix.io>
The valkey-application PackageSource ships system/valkey-rd (an ApplicationDefinition); it must dependOn cozystack.cozystack-engine so the ApplicationDefinition CRD is registered before the -rd HelmRelease reconciles. Mirrors redis-application.yaml and satisfies check-applicationdefinition-crd-ordering. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Andrei Kvapil <andrei.kvapil@aenix.io>
38b3396 to
2f5a3b1
Compare
IvanHunters
left a comment
There was a problem hiding this comment.
LGTM. Two independent static reviews (no cluster, no kubectl) both confirm:
- Faithful mirror of managed Redis:
values.yaml/values.schema.json/types.gobyte-identical to the redis equivalents modulo redis→valkey rename;valkeyfailover.yamldiffers only byspec.engine: Valkey, the valkey image, and monitor labels. - Base-branch CRD (#3406) actually supports the switch:
engineenum [Redis,Valkey] under databases.spotahome.com/v1 RedisFailover — verified in the base tree, not taken on faith. - Renders cleanly across the config matrix: v8/v7, external=true (LoadBalancer), authEnabled=false (drops Secret + auth.secretPath), size=null (ephemeral: appendonly no / save "").
- All in-repo registries updated: tenant ClusterRole (
valkeys), paas bundle, PackageSource, ApplicationDefinition, pr-labeler, storage-immutability doc. Generated artifacts (cozyvalues-gen, schema, README, deepcopy) reproduce with zero git diff;go build/vetclean. - Purely additive: new app, new Package CR on upgrade, no existing resource touched — no regression, no migration-state change.
- Chainsaw e2e is non-vacuous (PVCs Bound, Deployment Available, StatefulSet readyReplicas: 2), matches redis suite.
Non-blocking:
hack/e2e-chainsaw/README.mdsuite list omitsvalkey(docs-only; select-e2e.sh discovers dynamically) — add the entry.update-versions.sh: SC2064 (trap expands at set-time, harmless) and MAJORS=(8 7) hardcode diverges from redis's auto-detect — DRY drift to watch (dev-time script only).- Placeholder logo with non-official brand colors — cosmetic, acknowledged in PR.
- Stacked on #3406 (targets feat/redis-operator-freshworks) — correctly declared; #3406 must land first.
- Optional follow-up: run cozystack-pr-test on a disposable dev cluster to confirm the freshworks operator schedules valkey/valkey pods for engine: Valkey end-to-end — the one link inherited on trust from #3406.
|
Timofei Larkin (@lllamnyp) requesting your review, as an API owner, thank you |
a95fa0e to
41d1153
Compare
a76852c to
b3064e1
Compare
d0ec44b to
3197c48
Compare
The base branch was changed.
What this PR does
Adds a managed Valkey service to the catalog, alongside managed Redis.
Why
Redis relicensed at 8.x to a source-available family (AGPLv3 / SSPL / RSALv2), and the managed Redis app currently ships
v8: 8.4.0with no BSD-licensed option ≤ 7.2. That is a licensing risk for CNCF Incubation. Valkey is the BSD-3-Clause fork of Redis 7.2.4, governed by the Linux Foundation, and a drop-in replacement for Redis. This is the same move Argo CD made —cncf/foundation#750was closed with "no need for exception" after they migrated to Valkey.How it works
Valkey runs on the same operator as managed Redis. #3406 migrates that operator from the archived
spotahome/redis-operatorto the actively-maintained freshworks-oss/redis-operator fork, which adds a nativespec.engine: Valkeyswitch. With it, the operator runsvalkey-server/valkey-clinatively (in the launch command and in its liveness / readiness / shutdown probes), so the chart uses the officialvalkey/valkeyimage directly — no Redis-compatibility shim image.The app mirrors managed Redis: same shape/values (
replicas,resources/resourcesPreset,size,storageClass,external,version,authEnabled), Sentinel-based HA, metrics exporter,WorkloadMonitors, dashboard resource map, andApplicationDefinition(kind: Valkey).Versions offered:
v8 → 8.1.8(default) andv7 → 7.2.13(the BSD-3-Clause Redis 7.2 line, most conservative drop-in).How it was verified
helm templaterenders cleanly forv8,v7, andexternal: true; theRedisFailovercarriesspec.engine: Valkeyandimage: valkey/valkey:<version>.helm lintshows only the pre-existing icon-URL /cozy-libwarnings that managed Redis emits too.helm templateofvalkey-rdrenders a validApplicationDefinition(kind: Valkey,plural: valkeys,category: PaaS).values.schema.json,README.md,types.go,zz_generated.deepcopy.go, the ApplicationDefinitionopenAPISchema) reproduce with the pinnedcozyvalues-genv1.6.0 andcontroller-genv0.16.4 — rootmake generateand per-app generate both produce no drift;go build ./valkey/...passes.hack/e2e-chainsaw/valkey/(mirroring the redis suite) is included andhack/select-e2e.shmaps thevalkey-applicationsource to it, so it runs in CI once the freshworks operator image is built.Notes
packages/apps/valkey/logos/valkey.svg) is a placeholder in Valkey's brand colors, pending the official brand asset.db/valkeydashboard was intentionally left out of this initial PoC; it can follow.Screenshots
N/A — no UI changes.
Downstream repositories
This adds a new managed-app kind, which likely warrants follow-ups in
cozystack/website(user-facing docs for the new service) and possiblycozystack/terraform-provider-cozystack. No follow-up PRs are opened yet — leaving these for a maintainer to decide, per the template guidance.Release note