Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

[csi-plugins] add support for PVC annotations #2687

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Nov 14, 2024

Conversation

kayrus
Copy link
Contributor

@kayrus kayrus commented Oct 14, 2024

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:

Release note:

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

@k8s-ci-robot
Copy link
Contributor

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

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Oct 14, 2024
@k8s-ci-robot k8s-ci-robot requested review from gman0 and zetaab October 14, 2024 09:00
@k8s-ci-robot k8s-ci-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Oct 14, 2024
@kayrus kayrus force-pushed the csi-pvc-annotations branch 4 times, most recently from 0688187 to e325a5e Compare October 16, 2024 14:18
@kayrus kayrus marked this pull request as ready for review October 16, 2024 14:18
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 16, 2024
@k8s-ci-robot k8s-ci-robot requested a review from Fedosin October 16, 2024 14:19
@kayrus kayrus force-pushed the csi-pvc-annotations branch 4 times, most recently from 5b89207 to 41d0b46 Compare October 16, 2024 14:46
@kayrus
Copy link
Contributor Author

kayrus commented Oct 16, 2024

@zetaab @dulek @jichenjc ready for review

@kayrus kayrus requested review from dulek and jichenjc October 16, 2024 15:29
@kayrus kayrus force-pushed the csi-pvc-annotations branch from 694a897 to daf4c35 Compare October 16, 2024 22:49
@kayrus
Copy link
Contributor Author

kayrus commented Oct 16, 2024

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>

mind-blown-mind

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/v2@v2.1.2-0.20241016132526-1c4dd03733fe/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/spec@v1.9.0/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/spec@v1.9.0/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/grpc@v1.65.0/server.go:1379 +0xdf8
google.golang.org/grpc.(*Server).handleStream(0xc0001a3000, {0x2270ae0, 0xc0004e9680}, 0xc00017b200)
	google.golang.org/grpc@v1.65.0/server.go:1790 +0xe8b
google.golang.org/grpc.(*Server).serveStreams.func2.1()
	google.golang.org/grpc@v1.65.0/server.go:1029 +0x8b
created by google.golang.org/grpc.(*Server).serveStreams.func2 in goroutine 60
	google.golang.org/grpc@v1.65.0/server.go:104

@kayrus kayrus force-pushed the csi-pvc-annotations branch 2 times, most recently from b175db6 to 8a1c265 Compare October 16, 2024 23:04
@kayrus
Copy link
Contributor Author

kayrus commented Oct 21, 2024

@zetaab @dulek @jichenjc kindly ping

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 22, 2024
@kayrus kayrus force-pushed the csi-pvc-annotations branch from 8a1c265 to 4600776 Compare October 22, 2024 11:12
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 22, 2024
@chuan137
Copy link

@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
Copy link
Contributor

dulek commented Nov 6, 2024

Alright, so we get what we can here.

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

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 6, 2024
@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 6, 2024
@k8s-ci-robot
Copy link
Contributor

[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:

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 6, 2024
@kayrus kayrus force-pushed the csi-pvc-annotations branch from 4600776 to 9185d4e Compare November 6, 2024 11:35
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 6, 2024
@kayrus
Copy link
Contributor Author

kayrus commented Nov 6, 2024

@dulek I decided to add name resolving.

@kayrus kayrus force-pushed the csi-pvc-annotations branch 2 times, most recently from ddb1c1a to cc55d15 Compare November 6, 2024 11:41
@kayrus
Copy link
Contributor Author

kayrus commented Nov 6, 2024

@dulek waiting for a second lgtm

@kayrus
Copy link
Contributor Author

kayrus commented Nov 11, 2024

@dulek @zetaab kindly ping

@dulek
Copy link
Contributor

dulek commented Nov 13, 2024

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 13, 2024
@kayrus
Copy link
Contributor Author

kayrus commented Nov 13, 2024

/unhold

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 13, 2024
@kayrus kayrus force-pushed the csi-pvc-annotations branch from cc55d15 to f9b5c9b Compare November 13, 2024 15:30
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 13, 2024
@dulek
Copy link
Contributor

dulek commented Nov 13, 2024

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 13, 2024
@kayrus
Copy link
Contributor Author

kayrus commented Nov 13, 2024

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

@kayrus
Copy link
Contributor Author

kayrus commented Nov 13, 2024

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

@k8s-ci-robot k8s-ci-robot merged commit 7bfb2eb into kubernetes:master Nov 14, 2024
12 checks passed
@kayrus kayrus deleted the csi-pvc-annotations branch November 14, 2024 00:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[manila-csi-plugin] add support for share group and affinity annotations
4 participants
Morty Proxy This is a proxified and sanitized view of the page, visit original site.