Add support for volumeAttributesClassName on persistent storage #11210

venkatesh2090 · 2025-03-01T11:44:37Z

Type of change

Enhancement / new feature

Description

Currently, it is not possible to set the volumeAttributesClassName in the generated PVC using the persistent storage type. This enables that feature by adding a volumeAttributesClass field to the persistent storage that maps to the PVC.

Changes to the StorageDiff was required because it fails when a field is changed other than size and a few others.

Fixes #10462

Checklist

Please go through this checklist and make sure all applicable tasks have been done

Write tests
Make sure all tests pass
Update documentation
Check RBAC rights for Kubernetes / OpenShift roles
Try your changes from Pod inside your Kubernetes and OpenShift cluster, not just locally
Reference relevant issue(s) and close them after merging
Update CHANGELOG.md
Supply screenshots for visual changes, such as Grafana dashboards

im-konge

Thanks for the PR, I have one question.

cluster-operator/src/main/java/io/strimzi/operator/cluster/model/StorageDiff.java

im-konge · 2025-03-07T21:21:27Z

cluster-operator/src/main/java/io/strimzi/operator/cluster/model/StorageDiff.java

+                    if (pathValue.endsWith("/volumeAttributesClass") && desired.getType().equals(current.getType()) &&
+                        (isNull(persistentCurrent.getVolumeAttributesClass()) || isNull(persistentDesired.getVolumeAttributesClass()) ||
+                            !persistentCurrent.getVolumeAttributesClass().equals(persistentDesired.getVolumeAttributesClass()))) {


Thanks for the change, however from my point of view this is not much readable at the first sight.
I would keep the if (pathValue.endsWith("/volumeAttributesClass") && desired.getType().equals(current.getType())) which you had there before and then checking if those two are equal or not.

Instead of these checks for null and then comparing, you can use Objects.equals() as it can work with nulls and it will not throw NPEs.

What do you think about moving the desired.getType().equals(current.getType()) to the first if? It is common in both volumeAttributeClass and size.

Like this

-if (current instanceof PersistentClaimStorage persistentCurrent && desired instanceof PersistentClaimStorage persistentDesired) { +if (current instanceof PersistentClaimStorage persistentCurrent && desired instanceof PersistentClaimStorage persistentDesired && + desired.getType().equals(current.getType())) {

Instead of these checks for null and then comparing, you can use Objects.equals() as it can work with nulls and it will not throw NPEs.

Thanks for that. Didn't know this existed.

What do you think about moving the desired.getType().equals(current.getType()) to the first if? It is common in both volumeAttributeClass and size.

Yeah I guess that should be fine, good point.

ppatierno · 2025-03-10T09:25:42Z

Thanks for the PR can you elaborate a little bit more about this ...

Changes to the StorageDiff was required because it fails when a field is changed other than size and a few others.

fvaleri

Hi @venkatesh2090, LGTM but I can't easily test the changes. Is there a way to test it locally? Are you using some AWS enabled instance? Thanks.

venkatesh2090 · 2025-03-10T11:27:58Z

Thanks for the PR can you elaborate a little bit more about this ...

Changes to the StorageDiff was required because it fails when a field is changed other than size and a few others.

By that I meant, StorageDiff would consider everything in the Storage a change except if it is the size, overrides, deleteClaim and something with kraftMetadata that I couldn't wrap my head around. So I had to explicitly make it ignore a change in the volumeAttributesClass since it would treat that as immutable, when it shouldn't be.

venkatesh2090 · 2025-03-10T11:33:00Z

Hi @venkatesh2090, LGTM but I can't easily test the changes. Is there a way to test it locally? Are you using some AWS enabled instance? Thanks.

I used a feature gate in my kind setup. This is the cluster definition. But the CSI driver itself doesn't support VACs, so it wouldn't reconcile it. You would be able to see the change in the PVC though. I also tested it in AWS with rancher since EKS doesn't have that version of k8s.

kind: Cluster
apiVersion: kind.x-k8s.io/v1alpha4
name: strimzi
containerdConfigPatches:
- |
  [plugins."io.containerd.grpc.v1.cri".registry]
    config_path = "/etc/containerd/certs.d"
featureGates:
  VolumeAttributesClass: true # <-- this
runtimeConfig:
  storage.k8s.io/v1beta1: "true"
nodes:
- role: control-plane
- role: worker
- role: worker

I had to follow some article to setup a local container registry as well. Can't remember off the top of my head.

ppatierno · 2025-03-10T13:21:19Z

@venkatesh2090 @im-konge so what's the plan around STs for this taking into account that even local test seems to be not so easy?

im-konge · 2025-03-10T13:26:31Z

@venkatesh2090 @im-konge so what's the plan around STs for this taking into account that even local test seems to be not so easy?

We have more things that are not so easy to actually write STs. I haven't got time to look and test it locally yet, however if it's something that will need for example some AWS stuff, we cannot simply test it in STs. We would need to manually verify it.

scholzj · 2025-03-10T16:36:05Z

cluster-operator/src/main/java/io/strimzi/operator/cluster/model/StorageDiff.java

        boolean volumesAddedOrRemoved = false;
        boolean tooManyKRaftMetadataVolumes = false;
        boolean duplicateVolumeIds = false;
+        boolean volumeAttributesClassChanged = false;


Do we really need to know it changed? Isn't it sufficient to ignore the change? We do not seem to use this anywhere in the production code.

Yeah. It is only being used in tests. Can remove it and the related tests. I'll just add some KafkaAssemblyOperator tests as well

I think the tests for the StorageDiff can just test that it does not report any issues when the volume class atrributes change, or?

Oh right... Didn't think this through before replying. Thanks

scholzj · 2025-03-10T16:37:23Z

My general expectation for this was that it should be covered by unit tests. This is anyway more or less Kubernetes / infrastructure thing. So I do not think there is much for us to do in system tests. That said, I would expect some KafkaAssemblyOperator unit test for this to check that the PVCs are actually changed.

venkatesh2090 · 2025-03-15T14:03:03Z

My general expectation for this was that it should be covered by unit tests. This is anyway more or less Kubernetes / infrastructure thing. So I do not think there is much for us to do in system tests. That said, I would expect some KafkaAssemblyOperator unit test for this to check that the PVCs are actually changed.

Trying to figure out how to test at the KafkaAssemblyOperator level. The only change I could think of is adding another storage with VAC in the parameterized data for KafkaAssemblyOperatorTest. I will push that change so you can see what I mean. I will change it if I misunderstood.

scholzj · 2025-03-15T14:08:10Z

Trying to figure out how to test at the KafkaAssemblyOperator level. The only change I could think of is adding another storage with VAC in the parameterized data for KafkaAssemblyOperatorTest. I will push that change so you can see what I mean. I will change it if I misunderstood.

KafkaAssemblyOperatorTest is probably nto the best class as it is parametrized and does nto test actual changes. Maybe you can use one of the Mock based classes for it (they should have Mock in the name). In there, create one test that for example:

Create the cluster / PVCs without the attribute class
Add the attribute class and check it changes in next reconciliation
Changes the attributes class and check it changes again

venkatesh2090 · 2025-03-17T21:47:22Z

So I'm writing my tests in JbodStorageMockTest. I have a few non-trivial things I would like to modify and understand.

I would like to add --feature-gates=VolumeAttributesClass=true to the command line argument to MockKube3s kube-apiserver container. I have changes locally like so in the constructor

       this.apiServer = new ApiServerContainer<>().withEtcdImage(DockerImageName.parse(ETCD_IMAGE))
        .withCreateContainerCmdModifier(cmd -> {
            String[] cmdParts = cmd.getCmd();
            String[] newCmdParts = Arrays.copyOf(cmdParts, cmdParts.length + 1);
            newCmdParts[cmdParts.length] = "--feature-gates=VolumeAttributesClass=true";
            cmd.withCmd(newCmdParts);
        });

I would like to modify MockKube3 to have a cleaner API, say adding an withExtraCmdArgs method or similar. Would appreciate more opinions.

Since I'm updating the PVCs programmatically, the updates happen very quickly. When I update the PVC with VAC before it is Bound I get
```
Forbidden: spec is immutable after creation except resources.requests and volumeAttributesClassName for bound claims
```
How do I wait for the PVC to be bound in the tests?

I can push my current changes if you'd like to get the full picture.

scholzj · 2025-03-17T22:23:44Z

Hmm, I did not realize this would need to have the flag enabled :-/.

venkatesh2090 · 2025-03-26T21:12:34Z

Is this something I should keep looking at or should it be paused for now? Maybe until VolumeAttributesClass is out of beta and doesn't require a feature flag?

katheris · 2025-03-31T09:25:58Z

Is this something I should keep looking at or should it be paused for now? Maybe until VolumeAttributesClass is out of beta and doesn't require a feature flag?

Hi @venkatesh2090 just to check my understanding here. The changes you're suggesting here, if the user hasn't enabled VolumeAttributesClass in their Kubernetes (or they are using an older version that doesn't have it) it won't break anything right? The field will just be ignored by Kubernetes?

But to test this feature we need to enable a feature flag in MockKube3?

venkatesh2090 · 2025-03-31T14:58:20Z

Yes, that is right. I would have to enable that feature flag in MockKube to test this feature.

katheris · 2025-04-02T09:52:19Z

I don't think there is an issue going ahead, since MockKube is just used for testing. However, I wonder whether many people would use this feature before it is GA in Kubernetes. Is it something you were hoping to use right away, or are you happy to wait for it to become GA @venkatesh2090 ?

I'm afraid I'm not familiar enough with the MockKube3 tests to answer your other question.

Tagging @strimzi/maintainers to see what others think.

venkatesh2090 · 2025-04-02T10:17:30Z

Is it something you were hoping to use right away, or are you happy to wait for it to become GA @venkatesh2090 ?

No that's fine. I don't need this feature. I just raised a PR because I saw an open issue 🙂.

Feel free to tag me or take over this PR when the feature goes GA.

katheris · 2025-04-03T08:30:59Z

No that's fine. I don't need this feature. I just raised a PR because I saw an open issue 🙂.

That's great, we really value your contribution.

Ok so I think we have two options. If you want to continue working on this you could switch up the tests to use Mockito instead of MockKube3. That way we don't need to worry about the feature flag. Or you can close this PR and we will reopen it and tag you once the feature is GA.

If you are keen to pick up another task feel free to grab one, or you can drop a message in the the strimzi-dev channel in the CNCF Slack org if you want help selecting one :)

venkatesh2090 · 2025-04-04T17:58:53Z

Thanks. I'll close it for now and do it properly when it is GA

andyspiers · 2025-10-08T15:57:07Z

VolumeAttributesClass has gone GA since Kubernetes 1.34 and this is a feature that we'd really like so that we could codify higher IOPS/throughput for our Kafka volumes rather than having to make manual modifications to them. It would be really appreciated @venkatesh2090 if you had some time to revive this PR 🙏

venkatesh2090 · 2025-10-15T20:43:10Z

Sure @andyspiers. I will take a look over the weekend.

venkatesh2090 · 2025-10-25T16:25:44Z

So I'm writing my tests in JbodStorageMockTest. I have a few non-trivial things I would like to modify and understand.

I would like to add --feature-gates=VolumeAttributesClass=true to the command line argument to MockKube3s kube-apiserver container. I have changes locally like so in the constructor
java this.apiServer = new ApiServerContainer<>().withEtcdImage(DockerImageName.parse(ETCD_IMAGE)) .withCreateContainerCmdModifier(cmd -> { String[] cmdParts = cmd.getCmd(); String[] newCmdParts = Arrays.copyOf(cmdParts, cmdParts.length + 1); newCmdParts[cmdParts.length] = "--feature-gates=VolumeAttributesClass=true"; cmd.withCmd(newCmdParts); });

I would like to modify MockKube3 to have a cleaner API, say adding an withExtraCmdArgs method or similar. Would appreciate more opinions.

Since I'm updating the PVCs programmatically, the updates happen very quickly. When I update the PVC with VAC before it is Bound I get
Forbidden: spec is immutable after creation except resources.requests and volumeAttributesClassName for bound claims
How do I wait for the PVC to be bound in the tests?

I can push my current changes if you'd like to get the full picture.

While the first point I brought up isn't relevant anymore, I am still looking for solution to test this. I can see that MockKube3 has mocks for deployments, services etc. but not for PVCs. I need one for PVC because I would have to change the status to "Bound" because of that error mentioned. Would like to hear more opinions / comments before I go ahead and make changes to MockKube3.

Signed-off-by: Venkatesh Kannan <[email protected]>

codecov · 2025-10-25T20:31:53Z

Codecov Report

❌ Patch coverage is 85.18519% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 74.80%. Comparing base (8f39d33) to head (bdc5a6a).
⚠️ Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
.../test/mockkube3/controllers/MockPvcController.java	83.33%	3 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main   #11210      +/-   ##
============================================
+ Coverage     74.78%   74.80%   +0.01%     
- Complexity     6615     6620       +5     
============================================
  Files           376      377       +1     
  Lines         25325    25352      +27     
  Branches       3389     3392       +3     
============================================
+ Hits          18939    18964      +25     
- Misses         5000     5001       +1     
- Partials       1386     1387       +1

Files with missing lines	Coverage Δ
...ator/cluster/model/PersistentVolumeClaimUtils.java	`94.87% <100.00%> (+0.13%)`	⬆️
...io/strimzi/operator/cluster/model/StorageDiff.java	`90.66% <ø> (ø)`
...main/java/io/strimzi/test/mockkube3/MockKube3.java	`53.53% <100.00%> (+0.95%)`	⬆️
.../test/mockkube3/controllers/MockPvcController.java	`83.33% <83.33%> (ø)`

... and 1 file with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

scholzj

Thanks for re-opening the PR. I left one nit. There are now CRDs used for testing in the api module resources as well, which you do not seem to have updated here. You might need to rebase and rebuild at least the api module to have them updated.

api/src/main/java/io/strimzi/api/kafka/model/kafka/PersistentClaimStorage.java

Signed-off-by: Venkatesh Kannan <[email protected]>

scholzj

I think the code looks good. I will run the system tests once the build completes. Maybe you can add some simple test for the MockPvcController to MockKube3ControllersMockTest?

Signed-off-by: Venkatesh Kannan <[email protected]>

scholzj · 2025-10-27T00:22:25Z

/azp run regression

azure-pipelines · 2025-10-27T00:22:36Z

Azure Pipelines successfully started running 1 pipeline(s).

venkatesh2090 mentioned this pull request Mar 1, 2025

Add support for volumeAttributesClassName in storage configuration #10462

Open

im-konge reviewed Mar 6, 2025

View reviewed changes

cluster-operator/src/main/java/io/strimzi/operator/cluster/model/StorageDiff.java Outdated Show resolved Hide resolved

im-konge requested a review from a team March 6, 2025 11:39

im-konge added the needs review label Mar 6, 2025

im-konge added this to the 0.46.0 milestone Mar 6, 2025

venkatesh2090 force-pushed the add-support-for-volumeattributeclassname branch from 1b96399 to 9882466 Compare March 7, 2025 17:44

im-konge reviewed Mar 7, 2025

View reviewed changes

fvaleri reviewed Mar 10, 2025

View reviewed changes

scholzj reviewed Mar 10, 2025

View reviewed changes

venkatesh2090 closed this Apr 4, 2025

venkatesh2090 reopened this Oct 25, 2025

venkatesh2090 force-pushed the add-support-for-volumeattributeclassname branch from 4edae04 to aba8369 Compare October 25, 2025 16:27

venkatesh2090 added 8 commits October 25, 2025 17:29

Add volumeAttributesClass and map to PVC

429ffbd

Signed-off-by: Venkatesh Kannan <[email protected]>

ignore volume attributes on storage diff

5c9f1ae

Signed-off-by: Venkatesh Kannan <[email protected]>

fix javadoc

08b0995

Signed-off-by: Venkatesh Kannan <[email protected]>

Check if VACs have changed in diff

38d4817

Signed-off-by: Venkatesh Kannan <[email protected]>

Null check before equals

807ca79

Signed-off-by: Venkatesh Kannan <[email protected]>

Address comments

4c63775

Signed-off-by: Venkatesh Kannan <[email protected]>

Remove volumeAttributesClassChanged and add the field to ignorable paths

b716987

Signed-off-by: Venkatesh Kannan <[email protected]>

Add storage with VAC to test param

27816f1

Signed-off-by: Venkatesh Kannan <[email protected]>

venkatesh2090 force-pushed the add-support-for-volumeattributeclassname branch from aba8369 to 27816f1 Compare October 25, 2025 16:30

venkatesh2090 added 2 commits October 25, 2025 17:39

Fix compilation after updating against main

301fa65

Signed-off-by: Venkatesh Kannan <[email protected]>

re-render crds and helm charts

a3d9e59

Signed-off-by: Venkatesh Kannan <[email protected]>

scholzj reviewed Oct 26, 2025

View reviewed changes

api/src/main/java/io/strimzi/api/kafka/model/kafka/PersistentClaimStorage.java Outdated Show resolved Hide resolved

venkatesh2090 added 2 commits October 26, 2025 20:10

Update doc and re-render CRD

4c26887

Signed-off-by: Venkatesh Kannan <[email protected]>

Test to verify if VAC is updated with changes to PersistentStorage.

3c73c6f

Signed-off-by: Venkatesh Kannan <[email protected]>

venkatesh2090 force-pushed the add-support-for-volumeattributeclassname branch from c39c746 to 3c73c6f Compare October 26, 2025 21:07

scholzj reviewed Oct 26, 2025

View reviewed changes

Test MockPvcController

bdc5a6a

Signed-off-by: Venkatesh Kannan <[email protected]>

scholzj requested review from katheris and ppatierno October 27, 2025 00:22

Add support for volumeAttributesClassName on persistent storage #11210

Are you sure you want to change the base?

Add support for volumeAttributesClassName on persistent storage #11210

Conversation

venkatesh2090 commented Mar 1, 2025

Type of change

Description

Checklist

Uh oh!

im-konge left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

im-konge Mar 7, 2025

Choose a reason for hiding this comment

Uh oh!

venkatesh2090 Mar 7, 2025

Choose a reason for hiding this comment

Uh oh!

im-konge Mar 7, 2025

Choose a reason for hiding this comment

Uh oh!

ppatierno commented Mar 10, 2025

Uh oh!

fvaleri left a comment

Choose a reason for hiding this comment

Uh oh!

venkatesh2090 commented Mar 10, 2025

Uh oh!

venkatesh2090 commented Mar 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ppatierno commented Mar 10, 2025

Uh oh!

im-konge commented Mar 10, 2025

Uh oh!

scholzj Mar 10, 2025

Choose a reason for hiding this comment

Uh oh!

venkatesh2090 Mar 11, 2025

Choose a reason for hiding this comment

Uh oh!

scholzj Mar 11, 2025

Choose a reason for hiding this comment

Uh oh!

venkatesh2090 Mar 11, 2025

Choose a reason for hiding this comment

Uh oh!

scholzj commented Mar 10, 2025

Uh oh!

venkatesh2090 commented Mar 15, 2025

Uh oh!

scholzj commented Mar 15, 2025

Uh oh!

venkatesh2090 commented Mar 17, 2025

Uh oh!

scholzj commented Mar 17, 2025

Uh oh!

venkatesh2090 commented Mar 26, 2025

Uh oh!

katheris commented Mar 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

venkatesh2090 commented Mar 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

katheris commented Apr 2, 2025

Uh oh!

venkatesh2090 commented Apr 2, 2025

Uh oh!

katheris commented Apr 3, 2025

Uh oh!

venkatesh2090 commented Apr 4, 2025

Uh oh!

andyspiers commented Oct 8, 2025

Uh oh!

venkatesh2090 commented Oct 15, 2025

Uh oh!

venkatesh2090 commented Oct 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

venkatesh2090 commented Mar 10, 2025 •

edited

Loading

katheris commented Mar 31, 2025 •

edited

Loading

venkatesh2090 commented Mar 31, 2025 •

edited

Loading

venkatesh2090 commented Oct 25, 2025 •

edited

Loading

codecov bot commented Oct 25, 2025 •

edited

Loading