feat(redis): add affinity configuration - #2665

Claude Code Review

This pull request is from a fork — automated review is disabled. A repository maintainer can comment @claude review to run a one-time review.

Code Review

This pull request introduces Kubernetes affinity configuration for Redis and Sentinel pods by updating the Go API types, Helm templates, JSON schema, and documentation. The review feedback suggests marking the affinity parameter as optional in the values.yaml documentation to ensure consistency and proper Go type generation. Additionally, it is recommended to refine the Helm templates to avoid rendering empty affinity blocks in the resulting manifests, as the current default values in values.yaml cause the templates to always evaluate as true.

Same feature as your rabbitmq PR and the same verified good news: the affinity does reach the pods (RedisFailover.spec.redis.affinity and spec.sentinel.affinity both exist in the vendored operator CRD, and rendering the branch puts the values at those paths), and the generated artifacts genuinely regenerate byte for byte, so the bot comments claiming they were hand-edited are wrong.

Three things to fix here that do not apply to the rabbitmq one.

The template crashes on an explicit null. helm template --set affinity=null fails with nil pointer evaluating interface {}.sentinel, because packages/apps/redis/templates/redisfailover.yaml:29 does {{- with .Values.affinity.sentinel }} and dereferences the parent before testing it. The rabbitmq chart is immune because its with sits on the parent itself. (.Values.affinity).sentinel fixes it. I agree this is hard to reach through the Application path, since the CRD's own typing should reject an explicit null, but it is one character of parentheses and the failure mode is a render crash.

The fields should be *RawExtension rather than the value form, which is what your rabbitmq PR already does. omitempty is a no-op on a struct, so as written this always emits affinity with two nulls inside instead of omitting it. Your other PR has the right spelling; matching it here also lines this up with the third affinity PR, where I have asked for the same change.

api/apps/v1alpha1/redis/types.go:50 exports a type named Affinity that is not the Kubernetes corev1.Affinity but a two-field wrapper holding the redis and sentinel rules. In a module that gets published and imported by tenants, that name will be read as the Kubernetes one every time. Something like AffinitySpec, or a name mentioning the split, would avoid the collision.

As on the other PR: no Signed-off-by on the commits, which is a hard merge gate, and a mechanical conflict with main that a rebase plus the package's make generate should clear. The field descriptions would also benefit from naming the expected shape, since preserve-unknown-fields gives the dashboard nothing else to show.

On ordering, so you are not blocked by the queue: several other open PRs touch this chart's values.yaml, but every overlap is in artifacts that regenerate deterministically, so it is merge ordering rather than a conflict of intent. Whichever lands first, the rest rebase and regenerate.

And the same note as the other one: lifecycle/stale puts this on a close timer, so it needs a nudge if you still want it.

One more thing worth knowing about this field, which I checked in the operator's source and which belongs in the field description rather than in the code.

The operator does not merge what you pass with its own scheduling rules. Its helper returns the user-supplied affinity verbatim when one is set, and only when none is set does it synthesize a soft pod anti-affinity, weight 100 on kubernetes.io/hostname, that spreads the replicas across nodes.

So a user who sets this field only to express a node preference, which is the most common reason to reach for affinity at all, silently loses replica spreading on what is meant to be a highly available service. Nothing warns them, and the symptom only appears when a node goes down and takes more than one replica with it.

Documenting that in the field description is enough, ideally with the concrete advice that anyone setting this should include a podAntiAffinity themselves unless they specifically want replicas co-located. The rabbitmq PR has no equivalent trap, since that operator passes a nil straight through and adds no default of its own, which is another reason the two field descriptions should not simply be copies of each other.

Kostiantyn Astakhov (@kastakhov) NOT LGTM. Everything from my July reviews is fixed. What is left is a rebase and a test.

The conflict with main is only in the generated packages/system/redis-rd/cozyrds/redis.yaml. Rebase and rerun make generate in packages/apps/redis with cozyvalues-gen v1.7.0, which is what CI uses. On my trial merge that regenerates the RD with affinity in the schema and in keysOrder, and the other generated files come out identical to what git merges on its own. The render is right: spec.redis.affinity and spec.sentinel.affinity get the values, and the default, affinity: null and affinity.redis: null render no affinity key, so the operator keeps its own default spreading.

Since your last push, main added helm-unittest tests for this chart in packages/apps/redis/tests/, and none of them covers affinity. I pointed the redis block at (.Values.affinity).sentinel and all 48 tests still passed, so swapped or dropped wiring would ship unnoticed. One small test file is enough: defaults render no affinity under either spec.redis or spec.sentinel, and each value lands only under its own component.

CI on this head tells nothing yet. The August runs failed before any job started because the branch could not be merged, and E2E Tests never ran. They will run after the rebase.

Kostiantyn Astakhov (@kastakhov) LGTM. The rebase and affinity tests address my previous change request. I ran the 71 chart tests and checked separate Redis and Sentinel settings, including null values and TLS. Swapping either component's input makes the new test fail.

Functional CI has not run on this head; the passing checks only cover metadata.

Please link a Terraform provider follow-up in the downstream checklist; its schema and mappings do not expose the new affinity fields yet.