Support volume name generate interface

[sanjid133]

There are some cloud provider like Linode, has some constraints on volume (like volume id is integer number). So, It will be helpful to add a VolumeNameGenerate interface{}, then pass it to the controller.

[sanjid133]

cc: @displague @tamalsaha

pharmer#1

[tamalsaha]

Linode's csi driver https://github.com/displague/csi-linode

[tamalsaha]

@@saad-ali, do you mind taking a look?

[saad-ali]

Sorry I don't follow. Can you clarify what behavior you want from what component? What will VolumeNameGenerate interface{} do?

[saad-ali]

Spoke offline. Seems like we may need some way to control this. +@lpabon

[j-griffith]

Just a thought, maybe add this to the spec via a capabilities field? Then we can just auto-detect it in the tests for those that need it and do the right thing based on capabilities? That way it's in the spec and it's handled without custom testing interfaces. I suspect there will be other cases like this that come up over time and capabilities seems like a good way to mange that.

[lpabon]

I think we can add this as a switch to csi-sanity. This switch would then notify csi-sanity to use a specific name type. This is a good suggestion and an easy fix.

[displague]

@lpabon If you can provide a little more detail on how you would have this implemented, I'd love to send the PR. (Do commits on this GH org count towards kubernetes/org contributions?)

To restate the problem with csi-test here, in the Linode CSI, the VolumeId is a string number "123",
so the Linode CSI expects it will be able to apply strconv.Atoi against the CO supplied VolumeId.

This fails when the csi-test tests use VolumeId fixtures of reallyfakevolumeid and some-vol-id.

This problem also comes up with NodeId fixtures of some-node-id and some-fake-node-id.

[lpabon]

i think adding an implementation for volume generation would be good, but needs to be part of csi-test repo so that it can be used from csi-sanity tool.

[lpabon]

This does feel weird, that the spec says it is a string but really they are numbers. I think this really should go to CSI spec to modify the documentation and explain that the CO should create names using string-numbers only.

[tamalsaha]

@lpabon / @displague, any updates on this one?

[displague]

I worked around this problem with a hack.

https://github.com/linode/linode-blockstorage-csi-driver/blob/e312ce00cf75ed243c4e545d73403c395ff1489d/pkg/common/idhelpers.go#L25-L47

I would still like to see a proper solution so I could include early validation around the requested ids.
Actual Linode users specifying "afakeid" will find that the translated integer value is invalid 99.9999% of the time 🎲, leading to API responses that the VolumeID/NodeID specified was not found (which is also the response for ids not associated with the account), which is effectively true.

[fejta-bot]

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

[fejta-bot]

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle rotten

[davidz627]

/remove-lifecycle rotten