[csi-plugins] add support for PVC annotations

[kayrus]

What this PR does / why we need it:

This PR adds an ability to use PVC annotations to create persistent volumes with extra options.

For manila-csi-plugin a new groupID option is added allowing the share to be created in a specific group.

Which issue this PR fixes(if applicable):
fixes #2685

Special notes for reviewers:

This is a fraft, since it's depends on:

• [manila] add scheduler_hints to the shares CreateOpts gophercloud/gophercloud#3211
• [manila] add share_group_id to share's CreateOpts gophercloud/gophercloud#3205

Release note:

[csi-plugins] add support for PVC annotations
[manila-csi-plugin] add support for share group id

[k8s-ci-robot]

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

[kayrus]

@zetaab @dulek @jichenjc ready for review

[kayrus]

I just discovered that var *volumes.SchedulerHintOpts is not nil, when the func receives the interface type as an arg.

if schedulerHints != nil {
   schedulerHints.ToSchedulerHintsMap()
}

this will cause a panic: value method openstack/blockstorage/v3/volumes.SchedulerHintOpts.ToSchedulerHintsMap called using nil *SchedulerHintOpts pointer

According to go, the passed var *volumes.SchedulerHintOpts is (*volumes.SchedulerHintOpts)(nil), and passed var volumes.SchedulerHintOptsBuilder interface is <nil>

So I had to push this:

diff --git a/pkg/csi/cinder/controllerserver.go b/pkg/csi/cinder/controllerserver.go
index a96b817a..dd630ba8 100644
--- a/pkg/csi/cinder/controllerserver.go
+++ b/pkg/csi/cinder/controllerserver.go
@@ -198,7 +198,7 @@ func (cs *controllerServer) CreateVolume(ctx context.Context, req *csi.CreateVol
        }
 
        // Set scheduler hints if affinity or anti-affinity is set in PVC annotations
-       var schedulerHints *volumes.SchedulerHintOpts
+       var schedulerHints volumes.SchedulerHintOptsBuilder
        var volCtx map[string]string
        affinity := util.SplitTrimJoin(pvcAnnotations[affinityKey], ',')
        antiAffinity := util.SplitTrimJoin(pvcAnnotations[antiAffinityKey], ',')
diff --git a/pkg/csi/cinder/openstack/openstack.go b/pkg/csi/cinder/openstack/openstack.go
index cc733690..46116ecb 100644
--- a/pkg/csi/cinder/openstack/openstack.go
+++ b/pkg/csi/cinder/openstack/openstack.go
@@ -45,7 +45,7 @@ func AddExtraFlags(fs *pflag.FlagSet) {
 }
 
 type IOpenStack interface {
-       CreateVolume(*volumes.CreateOpts, *volumes.SchedulerHintOpts) (*volumes.Volume, error)
+       CreateVolume(*volumes.CreateOpts, volumes.SchedulerHintOptsBuilder) (*volumes.Volume, error)
        DeleteVolume(volumeID string) error
        AttachVolume(instanceID, volumeID string) (string, error)
        ListVolumes(limit int, startingToken string) ([]volumes.Volume, string, error)
diff --git a/pkg/csi/cinder/openstack/openstack_mock.go b/pkg/csi/cinder/openstack/openstack_mock.go
index b35d923c..89f0d3dc 100644
--- a/pkg/csi/cinder/openstack/openstack_mock.go
+++ b/pkg/csi/cinder/openstack/openstack_mock.go
@@ -85,7 +85,7 @@ func (_m *OpenStackMock) AttachVolume(instanceID string, volumeID string) (strin
 }
 
 // CreateVolume provides a mock function with given fields: name, size, vtype, availability, tags
-func (_m *OpenStackMock) CreateVolume(opts *volumes.CreateOpts, _ *volumes.SchedulerHintOpts) (*volumes.Volume, error) {
+func (_m *OpenStackMock) CreateVolume(opts *volumes.CreateOpts, _ volumes.SchedulerHintOptsBuilder) (*volumes.Volume, error) {
        name := opts.Name
        size := opts.Size
        vtype := opts.VolumeType
diff --git a/pkg/csi/cinder/openstack/openstack_volumes.go b/pkg/csi/cinder/openstack/openstack_volumes.go
index f7f6be95..b3d90230 100644
--- a/pkg/csi/cinder/openstack/openstack_volumes.go
+++ b/pkg/csi/cinder/openstack/openstack_volumes.go
@@ -51,7 +51,7 @@ const (
 var volumeErrorStates = [...]string{"error", "error_extending", "error_deleting"}
 
 // CreateVolume creates a volume of given size
-func (os *OpenStack) CreateVolume(opts *volumes.CreateOpts, schedulerHints *volumes.SchedulerHintOpts) (*volumes.Volume, error) {
+func (os *OpenStack) CreateVolume(opts *volumes.CreateOpts, schedulerHints volumes.SchedulerHintOptsBuilder) (*volumes.Volume, error) {
        blockstorageClient, err := openstack.NewBlockStorageV3(os.blockstorage.ProviderClient, os.epOpts)
        if err != nil {
                return nil, err
diff --git a/tests/sanity/cinder/fakecloud.go b/tests/sanity/cinder/fakecloud.go
index a02a8e77..56ee04ad 100644
--- a/tests/sanity/cinder/fakecloud.go
+++ b/tests/sanity/cinder/fakecloud.go
@@ -34,7 +34,7 @@ func getfakecloud() *cloud {
 var _ openstack.IOpenStack = &cloud{}
 
 // Fake Cloud
-func (cloud *cloud) CreateVolume(opts *volumes.CreateOpts, _ *volumes.SchedulerHintOpts) (*volumes.Volume, error) {
+func (cloud *cloud) CreateVolume(opts *volumes.CreateOpts, _ volumes.SchedulerHintOptsBuilder) (*volumes.Volume, error) {
        vol := &volumes.Volume{
                ID:               randString(10),
                Name:             opts.Name,

to fix the:

panic: value method github.com/gophercloud/gophercloud/v2/openstack/blockstorage/v3/volumes.SchedulerHintOpts.ToSchedulerHintsMap called using nil *SchedulerHintOpts pointer

goroutine 62 [running]:
github.com/gophercloud/gophercloud/v2/openstack/blockstorage/v3/volumes.(*SchedulerHintOpts).ToSchedulerHintsMap(0xd?)
	<autogenerated>:1 +0x8c
github.com/gophercloud/gophercloud/v2/openstack/blockstorage/v3/volumes.Create({0x22647c8, 0x3383fc0}, 0xc000233180, {0x2241820, 0xc00002e370}, {0x2241840, 0x0})
	github.com/gophercloud/gophercloud/[email protected]/openstack/blockstorage/v3/volumes/requests.go:154 +0xf7
k8s.io/cloud-provider-openstack/pkg/csi/cinder/openstack.(*OpenStack).CreateVolume(0xc0001a6c80, 0xc00002e370, 0x0)
	k8s.io/cloud-provider-openstack/pkg/csi/cinder/openstack/openstack_volumes.go:68 +0x245
k8s.io/cloud-provider-openstack/pkg/csi/cinder.(*controllerServer).CreateVolume(0xc00051a030, {0x1ec1a20?, 0x20?}, 0xc0000c6800)
	k8s.io/cloud-provider-openstack/pkg/csi/cinder/controllerserver.go:215 +0x1caf
github.com/container-storage-interface/spec/lib/go/csi._Controller_CreateVolume_Handler.func1({0x2264800?, 0xc0003c3500?}, {0x1e83680?, 0xc0000c6800?})
	github.com/container-storage-interface/[email protected]/lib/go/csi/csi.pb.go:6587 +0xcb
k8s.io/cloud-provider-openstack/pkg/csi/cinder.logGRPC({0x2264800, 0xc0003c3500}, {0x1e83680, 0xc0000c6800}, 0xc000307240, 0xc000415c38)
	k8s.io/cloud-provider-openstack/pkg/csi/cinder/utils.go:97 +0x2f5
github.com/container-storage-interface/spec/lib/go/csi._Controller_CreateVolume_Handler({0x1ec1a20, 0xc00051a030}, {0x2264800, 0xc0003c3500}, 0xc0000c6700, 0x2089b60)
	github.com/container-storage-interface/[email protected]/lib/go/csi/csi.pb.go:6589 +0x143
google.golang.org/grpc.(*Server).processUnaryRPC(0xc0001a3000, {0x2264800, 0xc0003c3470}, {0x2270ae0, 0xc0004e9680}, 0xc00017b200, 0xc000502b40, 0x3306020, 0x0)
	google.golang.org/[email protected]/server.go:1379 +0xdf8
google.golang.org/grpc.(*Server).handleStream(0xc0001a3000, {0x2270ae0, 0xc0004e9680}, 0xc00017b200)
	google.golang.org/[email protected]/server.go:1790 +0xe8b
google.golang.org/grpc.(*Server).serveStreams.func2.1()
	google.golang.org/[email protected]/server.go:1029 +0x8b
created by google.golang.org/grpc.(*Server).serveStreams.func2 in goroutine 60
	google.golang.org/[email protected]/server.go:104

[kayrus]

@zetaab @dulek @jichenjc kindly ping

[chuan137]

@freeformz @gnufied @ncdc @dulek @jichenjc
Hey guys, could you have a look at this PR?
I am going to discuss adding affinity feature to share groups in the coming openstack PTG meeting with upstream. This PR allows us to provision group of PVCs with affinity relationships. It would be nice to have your feedback.
Cheers,
Chuan

[dulek]

Do I correctly understand that this requires knowing the volume or share ID upfront before creating a PVC? This is totally not a K8s pattern, as to e.g. deploy three volumes with anti-affinity you need to create one, wait for ID to be populated, create second, wait for its ID and only then create the third, right?

I can see that for Manila you can specify share names too? When looking at the filters code I don't exactly see that support, but nevertheless:

With scheduler hints, you can optionally specify the affinity and anti-affinity rules in relation to other shares. The scheduler will enforce these rules when determining where to create the share. Possible keys are same_host and different_host, and the value must be the share name or id.

Source: https://docs.openstack.org/manila/latest/user/create-and-manage-shares.html#create-a-share

Does this work? Name's easier to use, so I think it would be great to implement and document support for that (by implement I mean - do we need to do some names manipulations to map K8s objects to Manila shares? If so users should specify annotations using K8s names).

[dulek]

scheduler

[kayrus]

@dulek

Do I correctly understand that this requires knowing the volume or share ID upfront before creating a PVC? This is totally not a K8s pattern, as to e.g. deploy three volumes with anti-affinity you need to create one, wait for ID to be populated, create second, wait for its ID and only then create the third, right?

Right. Unfortunately I don't see any other way to distribute shares across multiple backends. There will be a feature, based on the share group id, but I suppose it will be implemented mid next year, that's why I introduced a group ID support in advance.

the value must be the share name or id

Interesting finding, but it doesn't work with API. I tested this locally and got: {"badRequest": {"code": 400, "message": "123a is not a valid uuid."}}. Do you think it's worth adding the resolving share ID by name like it's done in user CLI tool?

[dulek]

Alright, so we get what we can here.

/lgtm
/approve
/hold for @kayrus to decide when this is ready.

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dulek

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [dulek]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[kayrus]

@dulek I decided to add name resolving.

[kayrus]

@dulek waiting for a second lgtm

[kayrus]

@dulek @zetaab kindly ping

[dulek]

/lgtm

[kayrus]

/unhold

[dulek]

/lgtm

[kayrus]

/test openstack-cloud-csi-cinder-e2e-test

[kayrus]

/test openstack-cloud-csi-manila-e2e-test