api: add volumeGroupAttributes field to VGRContent API

[iPraveenParihar]

This commit introduces volumeGroupAttributes field to VolumeGroupReplicationContent API.

The field is intended to store key-value pairs derived from the VolumeGroupContext returned by CSI drivers in the CreateVolumeGroupResponse.

[iPraveenParihar]

What kind of attributes are these, and where do they come from? Would be good to see something about them in the documentation and/or protocol specification.

fyi: https://github.com/ceph/ceph-csi/issues/5220 These attributes will be added from VolumeGroupReplicationClass. Sure, will try to add about them in appropriate docs.

[nixpanic]

fyi: ceph/ceph-csi#5220

Thanks for the reference. That is just one implementation of a csi-driver with csi-addons support. Please included a description/design for the need of attributes, and what attributes are somewhere in this csi-driver-independent project.

[iPraveenParihar]

fyi: ceph/ceph-csi#5220

Thanks for the reference. That is just one implementation of a csi-driver with csi-addons support. Please included a description/design for the need of attributes, and what attributes are somewhere in this csi-driver-independent project.

Instead of specifying what parameters to add or adding all, I'd like to do for volume‑groups what CSI already does for individual volumes, when we create a PV, the external provisioner takes the key‑value pairs returned by the CSI driver in CreateVolumeResponse.Volume.VolumeContext and stuffs them into pv.spec.csi.volumeAttributes.

Can we do the same thing for a VolumeGroup? IOW, have the CSI drivers return its own attributes in CreateVolumeGroupResponse.VolumeGroup.VolumeGroupContext, and then let the VolumeGroupReplicationContent controller copy those entries into VolumeGroupReplicationContent.spec.volumeGroupAttributes?

WDYT? @Madhu-1 @Nikhil-Ladha

[Madhu-1]

fyi: ceph/ceph-csi#5220

Thanks for the reference. That is just one implementation of a csi-driver with csi-addons support. Please included a description/design for the need of attributes, and what attributes are somewhere in this csi-driver-independent project.

Instead of specifying what parameters to add or adding all, I'd like to do for volume‑groups what CSI already does for individual volumes, when we create a PV, the external provisioner takes the key‑value pairs returned by the CSI driver in CreateVolumeResponse.Volume.VolumeContext and stuffs them into pv.spec.csi.volumeAttributes.

Can we do the same thing for a VolumeGroup? IOW, have the CSI drivers return its own attributes in CreateVolumeGroupResponse.VolumeGroup.VolumeGroupContext, and then let the VolumeGroupReplicationContent controller copy those entries into VolumeGroupReplicationContent.spec.volumeGroupAttributes?

WDYT? @Madhu-1 @Nikhil-Ladha

Yes thats what we need to do, its the context parameters returned by csi driver during create operation

[nixpanic]

Instead of specifying what parameters to add or adding all, I'd like to do for volume‑groups what CSI already does for individual volumes, when we create a PV, the external provisioner takes the key‑value pairs returned by the CSI driver in CreateVolumeResponse.Volume.VolumeContext and stuffs them into pv.spec.csi.volumeAttributes.

Sure. that is a reasonable explanation. As CreateVolumeGroupResponse.VolumeGroup.VolumeGroupContext already exists, it might even have been an oversight and this could be marked as a bug.

[Nikhil-Ladha]

Instead of specifying what parameters to add or adding all, I'd like to do for volume‑groups what CSI already does for individual volumes, when we create a PV, the external provisioner takes the key‑value pairs returned by the CSI driver in CreateVolumeResponse.Volume.VolumeContext and stuffs them into pv.spec.csi.volumeAttributes.

Can we do the same thing for a VolumeGroup? IOW, have the CSI drivers return its own attributes in CreateVolumeGroupResponse.VolumeGroup.VolumeGroupContext, and then let the VolumeGroupReplicationContent controller copy those entries into VolumeGroupReplicationContent.spec.volumeGroupAttributes?

WDYT? @Madhu-1 @Nikhil-Ladha

That is a better approach. But, in that case we would still have to depend on VolumeGroupReplicationClass in the omap generator to fetch the secrets, as we won't be storing them in VGRContent. The secrets won't be returned in those attributes, right? We have spec.csi.controllerExpandSecretRef in PV that contains the secrets, so are we planning to have something like that in VGRContent as well?

[Madhu-1]

But, in that case we would still have to depend on VolumeGroupReplicationClass in the omap generator to fetch the secrets, as we won't be storing them in VGRContent. The secrets won't be returned in those attributes, right? We have spec.csi.controllerExpandSecretRef in PV that contains the secrets, so are we planning to have something like that in VGRContent as well?

@Nikhil-Ladha IMO we dont need to introduce a new fields for it. we could store it as annotation in the VGR not at the VGRC. we pass the parameters from the VGRC always for all the Group RPC calls. we can revisit the secrets later

[Nikhil-Ladha]

But, in that case we would still have to depend on VolumeGroupReplicationClass in the omap generator to fetch the secrets, as we won't be storing them in VGRContent. The secrets won't be returned in those attributes, right? We have spec.csi.controllerExpandSecretRef in PV that contains the secrets, so are we planning to have something like that in VGRContent as well?

@Nikhil-Ladha IMO we dont need to introduce a new fields for it. we could store it as annotation in the VGR not at the VGRC. we pass the parameters from the VGRC always for all the Group RPC calls. we can revisit the secrets later

But, isn't that was the initial purpose of the issue? That is to avoid dependency on VGRClass for omap generation?

RPC calls are fine, we filter the secret params and pass it to the RPC. But, even after the proposed change we would continue to use VGRClass in omap generator. So, I am not sure how are we planning to address that?

[Madhu-1]

RPC calls are fine, we filter the secret params and pass it to the RPC. But, even after the proposed change we would continue to use VGRClass in omap generator. So, I am not sure how are we planning to address that?

Yes we need to address it in different PR's. as i mentioned earlier we can use annotation for it.

[Nikhil-Ladha]

RPC calls are fine, we filter the secret params and pass it to the RPC. But, even after the proposed change we would continue to use VGRClass in omap generator. So, I am not sure how are we planning to address that?

Yes we need to address it in different PR's. as i mentioned earlier we can use annotation for it.

Ack. Yeah, in case of annotation it would be VGRContent then instead of VGR, as that is referred for omap generation.

[iPraveenParihar]

Sure. that is a reasonable explanation. As CreateVolumeGroupResponse.VolumeGroup.VolumeGroupContext already exists, it might even have been an oversight and this could be marked as a bug.

we should be good with changes in this PR?

[Madhu-1]

RPC calls are fine, we filter the secret params and pass it to the RPC. But, even after the proposed change we would continue to use VGRClass in omap generator. So, I am not sure how are we planning to address that?

Yes we need to address it in different PR's. as i mentioned earlier we can use annotation for it.

Ack. Yeah, in case of annotation it would be VGRContent then instead of VGR, as that is referred for omap generation.

i would store the secrets on the VGR not on VGRC (like we store the provisioner secret on the PVC) secrets are optional and not a mandatory fields .

[Nikhil-Ladha]

i would store the secrets on the VGR not on VGRC (like we store the provisioner secret on the PVC) secrets are optional and not a mandatory fields .

In that case, we should update the issue description to reflect the same for awareness.

[iPraveenParihar]

@Madhu-1 @Nikhil-Ladha PTAL If there is nothing more to be done here, we can merge it?

[nixpanic]

Yes thats what we need to do, its the context parameters returned by csi driver during create operation

This is still something that needs to be set in the sidecar/controller, right? It would be good to include that in this PR as well. Otherwise the API suggests it is available, but drivers returning a VolumeGroupContext won't find it in the VGRContent.

[Nikhil-Ladha]

@iPraveenParihar can you please address Niels's last comment?

[iPraveenParihar]

@iPraveenParihar can you please address Niels's last comment?

Yes, I was working on that. Will update the PR soon.

[iPraveenParihar]

@Nikhil-Ladha, Currently we are not passing the secretName & secretNamespace for CreateVolumeGroupRequest. So, the CSI drivers can't add these details to the CreateVolumeGroupResponse.Volume.VolumeContext.

One way am thinking is to add secret details as annotation in VolumeGroupReplicationContent CR

replication.storage.openshift.io/group-replication-secret-name: <secret-name>
replication.storage.openshift.io/group-replication-secret-namespace: <secret-namespace>

this should be okay?

[Nikhil-Ladha]

@Nikhil-Ladha, Currently we are not passing the secretName & secretNamespace for CreateVolumeGroupRequest. So, the CSI drivers can't add these details to the CreateVolumeGroupResponse.Volume.VolumeContext.

One way am thinking is to add secret details as annotation in VolumeGroupReplicationContent CR

replication.storage.openshift.io/group-replication-secret-name: <secret-name>
replication.storage.openshift.io/group-replication-secret-namespace: <secret-namespace>

this should be okay?

We are passing these details to the CreateVolumeGroup RPC call, ref: https://github.com/csi-addons/kubernetes-csi-addons/blob/main/internal/controller/replication.storage/volumegroupreplicationcontent_controller.go#L184

[Nikhil-Ladha]

i would store the secrets on the VGR not on VGRC (like we store the provisioner secret on the PVC) secrets are optional and not a mandatory fields .

We have the deletion secrets in PV and not in PVC.

volume.kubernetes.io/provisioner-deletion-secret-name: rook-csi-rbd-provisioner
volume.kubernetes.io/provisioner-deletion-secret-namespace: rook-ceph

I don't see any secret related annotations on PVC 🤔

[iPraveenParihar]

LGTM, @iPraveenParihar is this tested and working as expected?

1. VolumeGroupReplicationClass with key-value pair

pm@dhcp53-176:~/vg-examples$ k get volumegroupreplicationclasses.replication.storage.openshift.io vgrc-sample -oyaml
apiVersion: replication.storage.openshift.io/v1alpha1
kind: VolumeGroupReplicationClass
metadata:
  creationTimestamp: "2025-06-05T08:24:15Z"
  generation: 1
  name: vgrc-sample
  resourceVersion: "2273725"
  uid: 3e7edc50-ab5f-46cd-b7f2-697ae9c52f0b
spec:
  parameters:
    clusterID: rook-ceph
    customKey1: customValue1
    customKey2: customValue2
    pool: replicapool
    replication.storage.openshift.io/group-replication-secret-name: rook-csi-rbd-provisioner
    replication.storage.openshift.io/group-replication-secret-namespace: rook-ceph
  provisioner: rbd.csi.ceph.com

2. VolumeGroupReplication CR

pm@dhcp53-176:~/vg-examples$ k get volumegroupreplication vg -oyaml
apiVersion: replication.storage.openshift.io/v1alpha1
kind: VolumeGroupReplication
metadata:
  annotations:
    pvcSelector: group=test
    replication.storage.openshift.io/volume-group-replication-content-name: vgrcontent-85a6caf4-d6c5-4d52-aa80-7579afc71b22
  creationTimestamp: "2025-06-05T08:24:20Z"
  finalizers:
  - replication.storage.openshift.io/vgr-protection
  generation: 3
  name: vg
  namespace: default
  resourceVersion: "2273758"
  uid: 85a6caf4-d6c5-4d52-aa80-7579afc71b22
spec:
  autoResync: false
  replicationState: primary
  source:
    selector:
      matchLabels:
        group: test
  volumeGroupReplicationClassName: vgrc-sample
  volumeGroupReplicationContentName: vgrcontent-85a6caf4-d6c5-4d52-aa80-7579afc71b22
  volumeReplicationClassName: vrc-sample
  volumeReplicationName: vr-85a6caf4-d6c5-4d52-aa80-7579afc71b22
status:
  observedGeneration: 3
  persistentVolumeClaimsRefList:
  - name: rbd-pvc

3. VolumeGroupReplicationContent CR with volumeGroupAttributes field updated as expected and secret details updated in annotations.

pm@dhcp53-176:~/vg-examples$ k get volumegroupreplicationcontents.replication.storage.openshift.io vgrcontent-85a6caf4-d6c5-4d52-aa80-7579afc71b22 -oyaml
apiVersion: replication.storage.openshift.io/v1alpha1
kind: VolumeGroupReplicationContent
metadata:
  annotations:
    replication.storage.openshift.io/group-replication-secret-name: rook-csi-rbd-provisioner
    replication.storage.openshift.io/group-replication-secret-namespace: rook-ceph
    replication.storage.openshift.io/volumegroupref: default/vg
  creationTimestamp: "2025-06-05T08:24:20Z"
  finalizers:
  - replication.storage.openshift.io/vgr-protection
  generation: 2
  name: vgrcontent-85a6caf4-d6c5-4d52-aa80-7579afc71b22
  resourceVersion: "2273754"
  uid: 1a44a217-16ed-40f1-858b-f9167db9d094
spec:
  provisioner: rbd.csi.ceph.com
  source:
    volumeHandles:
    - 0001-0009-rook-ceph-0000000000000001-76a4cdf4-1e7e-4aa5-8d37-a4299c48b049
  volumeGroupAttributes:
    clusterID: rook-ceph
    customKey1: customValue1
    customKey2: customValue2
    pool: replicapool
  volumeGroupReplicationClassName: vgrc-sample
  volumeGroupReplicationHandle: 0001-0009-rook-ceph-0000000000000001-3894a957-9780-4674-bb42-c87a27052a5e
  volumeGroupReplicationRef:
    apiVersion: replication.storage.openshift.io/v1alpha1
    kind: VolumeGroupReplication
    name: vg
    namespace: default
    uid: 85a6caf4-d6c5-4d52-aa80-7579afc71b22
status:
  persistentVolumeRefList:
  - name: pvc-22a36e64-996a-4bd8-b84c-cc2d2c455230

[Madhu-1]

@nixpanic you have requested changes, PTAL