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.

[nixpanic]

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.

[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: ceph/ceph-csi#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?

[Nikhil-Ladha]

lgtm