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

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

im-konge

Thanks for the PR, I have one question.

im-konge · 2025-03-06T11:39:09Z

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

+                    }
+
+                    if (pathValue.endsWith("/volumeAttributesClass") && desired.getType().equals(current.getType())) {
+                        volumeAttributesClassChanged = true;


Shouldn't here be check that it's really different in comparison current/desired?

You're right. I didn't think about that. Will change

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

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.

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

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

@@ -70,6 +72,7 @@ private StorageDiff(Reconciliation reconciliation, Storage current, Storage desi
        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.

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

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.

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

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

venkatesh2090 added 3 commits March 1, 2025 11:37

Add volumeAttributesClass and map to PVC

cf46f33

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

ignore volume attributes on storage diff

6ea010f

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

fix javadoc

f2956c6

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

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

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

Check if VACs have changed in diff

9882466

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

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

Null check before equals

db66f4a

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

im-konge reviewed Mar 7, 2025

View reviewed changes

Address comments

56e1763

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

fvaleri reviewed Mar 10, 2025

View reviewed changes

scholzj reviewed Mar 10, 2025

View reviewed changes

Remove volumeAttributesClassChanged and add the field to ignorable paths

ed4f241

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

Add storage with VAC to test param

4edae04

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

venkatesh2090 closed this Apr 4, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for volumeAttributesClassName on persistent storage #11210

Add support for volumeAttributesClassName on persistent storage #11210

venkatesh2090 commented Mar 1, 2025

im-konge left a comment

im-konge Mar 6, 2025

venkatesh2090 Mar 7, 2025

im-konge Mar 7, 2025

venkatesh2090 Mar 7, 2025

im-konge Mar 7, 2025

ppatierno commented Mar 10, 2025

fvaleri left a comment

venkatesh2090 commented Mar 10, 2025

venkatesh2090 commented Mar 10, 2025 •

edited

Loading

ppatierno commented Mar 10, 2025

im-konge commented Mar 10, 2025

scholzj Mar 10, 2025

venkatesh2090 Mar 11, 2025

scholzj Mar 11, 2025

venkatesh2090 Mar 11, 2025

scholzj commented Mar 10, 2025

venkatesh2090 commented Mar 15, 2025

scholzj commented Mar 15, 2025

venkatesh2090 commented Mar 17, 2025

scholzj commented Mar 17, 2025

venkatesh2090 commented Mar 26, 2025

katheris commented Mar 31, 2025 •

edited

Loading

venkatesh2090 commented Mar 31, 2025 •

edited

Loading

katheris commented Apr 2, 2025

venkatesh2090 commented Apr 2, 2025

katheris commented Apr 3, 2025

venkatesh2090 commented Apr 4, 2025

Add support for volumeAttributesClassName on persistent storage #11210

Add support for volumeAttributesClassName on persistent storage #11210

Conversation

venkatesh2090 commented Mar 1, 2025

Type of change

Description

Checklist

im-konge left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ppatierno commented Mar 10, 2025

fvaleri left a comment

Choose a reason for hiding this comment

venkatesh2090 commented Mar 10, 2025

venkatesh2090 commented Mar 10, 2025 • edited Loading

ppatierno commented Mar 10, 2025

im-konge commented Mar 10, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scholzj commented Mar 10, 2025

venkatesh2090 commented Mar 15, 2025

scholzj commented Mar 15, 2025

venkatesh2090 commented Mar 17, 2025

scholzj commented Mar 17, 2025

venkatesh2090 commented Mar 26, 2025

katheris commented Mar 31, 2025 • edited Loading

venkatesh2090 commented Mar 31, 2025 • edited Loading

katheris commented Apr 2, 2025

venkatesh2090 commented Apr 2, 2025

katheris commented Apr 3, 2025

venkatesh2090 commented Apr 4, 2025

venkatesh2090 commented Mar 10, 2025 •

edited

Loading

katheris commented Mar 31, 2025 •

edited

Loading

venkatesh2090 commented Mar 31, 2025 •

edited

Loading