Skip to content

Conversation

@naimadswdn
Copy link
Contributor

@naimadswdn naimadswdn commented Oct 9, 2025

Description

New feature added: support for volumes and volumeMounts for the all of the Redis deployment types.

Type of change

  • New feature (non-breaking change which adds functionality)

Checklist

  • Tests have been added/modified and all tests pass.
  • Functionality/bugs have been confirmed to be unchanged or fixed.
  • I have performed a self-review of my own code.
  • Documentation has been updated or added where necessary.

Additional Context

I need the volume and volumeMounts support to mount the external ACL file.

I tested the solution on my own cluster (AKS) and it works as expected:
image
image

@naimadswdn naimadswdn force-pushed the feat/add-volumes branch 3 times, most recently from 715eaa1 to 5b40387 Compare October 13, 2025 07:25
@codecov
Copy link

codecov bot commented Oct 14, 2025

Codecov Report

❌ Patch coverage is 90.90909% with 2 lines in your changes missing coverage. Please review.
⚠️ Please upload report for BASE (main@66eaa2d). Learn more about missing BASE report.

Files with missing lines Patch % Lines
internal/k8sutils/statefulset.go 50.00% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1557   +/-   ##
=======================================
  Coverage        ?   32.44%           
=======================================
  Files           ?       79           
  Lines           ?     5866           
  Branches        ?        0           
=======================================
  Hits            ?     1903           
  Misses          ?     3770           
  Partials        ?      193           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@naimadswdn naimadswdn changed the title Add support for volumes and volumeMounts feat: Add support for volumes and volumeMounts Oct 14, 2025
@naimadswdn
Copy link
Contributor Author

naimadswdn commented Oct 16, 2025

@shubham-cmyk @drivebyer @iamabhishek-dubey can I ask one of you to approve the workflow? I fixed all the things that failed recently.


Update: I fixed the examples and run the E2E tests locally. It should be good finally 👌
@shubham-cmyk @drivebyer @iamabhishek-dubey

@naimadswdn naimadswdn force-pushed the feat/add-volumes branch 2 times, most recently from a2b0b24 to b02983a Compare October 17, 2025 10:12
@drivebyer
Copy link
Collaborator

I think this pr should split into seprate pr. one for feature, another one for bugfix :)

@naimadswdn
Copy link
Contributor Author

naimadswdn commented Oct 20, 2025

@drivebyer right, I just spitted the PR.
There is a new one for the PDB fix: #1563

@naimadswdn
Copy link
Contributor Author

@shubham-cmyk @drivebyer @iamabhishek-dubey sorry for direct ping, but can you please approve the workflow again?

@naimadswdn
Copy link
Contributor Author

naimadswdn commented Oct 22, 2025

Semantic check fail, after so many pushes already.. What a shame 😅 Just fixed, sorry guys for that.
I saw all the CI steps passed, besides the semantic one. We should be finally good now.

@naimadswdn
Copy link
Contributor Author

@shubham-cmyk @drivebyer @iamabhishek-dubey Hi guys, the PR passed all the CI steps, can we merge it? :)

@naimadswdn naimadswdn force-pushed the feat/add-volumes branch 2 times, most recently from 8b051e5 to 8dd9da6 Compare October 23, 2025 12:09
@naimadswdn
Copy link
Contributor Author

Seems like E2E tests are failing due to previously merged PR. I allowed myself to fix these in a separate PR: #1569

@drivebyer
Copy link
Collaborator

If you’d like to add ACL configuration to your Redis cluster, you can refer to the examples here: v1beta2/acl_config

@naimadswdn
Copy link
Contributor Author

If you’d like to add ACL configuration to your Redis cluster, you can refer to the examples here: v1beta2/acl_config

You are right, but I have a hard requirement to persist my ACL between Redis installation and k8s secret is not the best to store such data.
That is why I am provisioning a separate StorageClass with the Retain policy and mount the PVC to the Redis Cluster components.
This way I have a single source of truth, that persist between installations.

@drivebyer
Copy link
Collaborator

This way I have a single source of truth, that persist between installations.

The ACL data is stored in a dedicated Secret resource, making it independent from the Redis cluster lifecycle.

@naimadswdn
Copy link
Contributor Author

In my setup, I need to support the multitenancy solution with the single Redis Cluster. That means, there is a lot of ACL modifications and relying the k8s secret for that is not the best practice.
Using the separate PVC indicates better performance and allows to track the changes.

I am not saying storing ACL in a secret is bad idea - it is good, but not for all situations.
I need a multitenancy ACL support that dynamically updates and persist even if the Redis Helm chart was uninstalled and installed again.

Basically this additional volumes can be used also for other configuration items in the future. ACL is just an example that I am relying on my project.

@drivebyer
Copy link
Collaborator

Got it. That might make the setup a bit more complicated for users. Since we already support spec.acl, what do you think about adding the ACL volume configuration there?

@naimadswdn
Copy link
Contributor Author

I could but I wonder if that is not a step back from the current implementation.
Right now, you can use the additionalVolumes that are part of this PR to attach not only the external ACL, but also for example:

  • persistent volume for Redis, when you want to kept the data (AOF, RDB snapshots)
  • attach custom scripts from configMap,
  • attach cutom secrets from k8s secrets,
  • use some epheral volumes for caching or some dev/test environments,
  • use CSI volume like Azure Disk to store some data (ACL for example)
  • volume sharing between all Redis leaders or followers.

Etc etc

So maybe instead of limiting the additionalVolumes to spec.acl only, maybe I can add a separate example that will run Redis with ACL hosted on the additional volume?
This way we will have both ways documented.

@drivebyer
Copy link
Collaborator

I could but I wonder if that is not a step back from the current implementation. Right now, you can use the additionalVolumes that are part of this PR to attach not only the external ACL, but also for example:

  • persistent volume for Redis, when you want to kept the data (AOF, RDB snapshots)
  • attach custom scripts from configMap,
  • attach cutom secrets from k8s secrets,
  • use some epheral volumes for caching or some dev/test environments,
  • use CSI volume like Azure Disk to store some data (ACL for example)
  • volume sharing between all Redis leaders or followers.

This PR definitely adds flexibility — but it also increases complexity for users, since they need to understand how to configure volume mounts.
For example, the ACL volume path is currently hardcoded as /etc/redis/user.acl.

That’s why I’d suggest keeping it simple and focusing on solving your specific requirement directly.
Maybe something like:

spec:
  acl:
    volumeSource:
      ...

Then the operator could automatically handle the volume mount for the Redis cluster, which would make it more user-friendly overall.

@naimadswdn
Copy link
Contributor Author

I could but I wonder if that is not a step back from the current implementation. Right now, you can use the additionalVolumes that are part of this PR to attach not only the external ACL, but also for example:

  • persistent volume for Redis, when you want to kept the data (AOF, RDB snapshots)
  • attach custom scripts from configMap,
  • attach cutom secrets from k8s secrets,
  • use some epheral volumes for caching or some dev/test environments,
  • use CSI volume like Azure Disk to store some data (ACL for example)
  • volume sharing between all Redis leaders or followers.

This PR definitely adds flexibility — but it also increases complexity for users, since they need to understand how to configure volume mounts. For example, the ACL volume path is currently hardcoded as /etc/redis/user.acl.

That’s why I’d suggest keeping it simple and focusing on solving your specific requirement directly. Maybe something like:

spec:
  acl:
    volumeSource:
      ...

Then the operator could automatically handle the volume mount for the Redis cluster, which would make it more user-friendly overall.

Ok, got it. I will work in the refactor probably today or tomorrow. Thanks for the guidlines and the discussion.

@naimadswdn naimadswdn force-pushed the feat/add-volumes branch 4 times, most recently from 8d9b334 to 9069d50 Compare October 28, 2025 14:47
@naimadswdn
Copy link
Contributor Author

naimadswdn commented Oct 29, 2025

@drivebyer I implemented the changes to ACL as discussed, but I did it on top of the extra volumes.
So user can specify both right now:

  • the extra volumes for whatever purpose
  • the ACL using the pre-populated PVC

I added some E2E tests to cover the ACL from PVC too, but there is this flaky tests with RedisCluster setup failing.
I took a look and I raised a separate PR to improve how RedisCluster status is evaluated: #1575
Let's merge 1575, then rebase this one and see 😄

@naimadswdn naimadswdn force-pushed the feat/add-volumes branch 2 times, most recently from 4f3bbde to 00940cb Compare October 29, 2025 10:10
Add support for volumes and volumeMounts.
Cover the changes with the tests.
Update helm charts with the Volumes and VolumeMounts options.
Add missing helm chart support for persistentVolumeClaimRetentionPolicy.

Signed-off-by: Damian Seredyn <[email protected]>
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.

2 participants