Introduce Controller with Create and Delete Volume capability. Adds support for Dynamic Provisioning via access points #274

kbasv · 2020-11-01T02:55:33Z

Is this a bug fix or adding new feature?
New Feature

What is this PR about? / Why do we need it?

Adds support for Dynamic Provisioning via access points. Provisions a new volume by creating an access point.
Fix for the following issues:
Fixes Implement dynamic provision support #6
Fixes Auto creation of Accesspoints #269
[EKS] [request]: EFS CSI driver automatically provision EFS directory or access point aws/containers-roadmap#891
Sample Storage class:

kind: StorageClass
apiVersion: storage.k8s.io/v1
metadata:
  name: efs-sc
provisioner: efs.csi.aws.com
parameters:
  provisioningMode: efs-ap
  fileSystemId: fs-01234567
  gidRangeStart: "1000"  //Optional
  gidRangeEnd: "2000"   //Optional
  directoryPerms: "777"
  basePath: "/data"  //Optional

Adds an opt-in flag to delete access virtual point directory and it's contents. By default, deleting a access point will not delete its virtual root directory or its content.

What testing is done?

Tested Manually on my EKS cluster.
E2E integration test for dynamic provisioning

k8s-ci-robot · 2020-11-01T02:55:40Z

Hi @kbasv. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

pkg/driver/controller.go

wongma7 · 2020-11-02T17:51:50Z

/ok-to-test

pkg/driver/node.go

chrishenzie

Thank you for your work on this! One other nit: Can you run goimports on the files you modified here?

go get golang.org/x/tools/cmd/goimports
find . -name \*.go -not -path './vendor/*' -exec goimports -w {} \;

Makefile

pkg/cloud/cloud.go

pkg/cloud/fakes.go

pkg/driver/controller.go

pkg/driver/node.go

pkg/driver/efs_watch_dog.go

jqmichael

I assume you are working on e2e tests at least for the happy path?

deploy/kubernetes/base/clusterrole-provisioner.yaml

deploy/kubernetes/base/controller.yaml

pkg/cloud/cloud.go

pkg/driver/controller.go

pkg/driver/node.go

vendor/github.com/kubernetes-csi/csi-test/pkg/sanity/controller.go

jqmichael · 2020-11-12T19:21:46Z

pkg/driver/controller.go

+			//Mount File System at it root and delete access point root directory
+			mountOptions := []string{"tls"}
+			target := TempMountPathPrefix + "/" + accessPointId
+			if err := d.mounter.MakeDir(target); err != nil {
+				return nil, status.Errorf(codes.Internal, "Could not create dir %q: %v", target, err)
+			}
+			if err := d.mounter.Mount(fileSystemId, target, "efs", mountOptions); err != nil {
+				os.Remove(target)
+				return nil, status.Errorf(codes.Internal, "Could not mount %q at %q: %v", fileSystemId, target, err)
+			}
+			os.RemoveAll(target + accessPoint.AccessPointRootDir)
+			err = d.mounter.Unmount(target)


This is saying on an arbitrary node (where the controller pod is assigned with), mount the EFS volume, delete the content, and unmount after.

I think this is OK but would like @wongma7 to double check.

It looks OK to me as long as the behavior is behind a flag and well documented.

IMO deleteAccessPointRootDir should default true to match kubernetes expectations where Delete means data is gone.

pkg/driver/controller.go

deploy/kubernetes/overlays/dev/master_image.yaml

jqmichael

Can we update example how to run dynamic provisioning with efs driver?

jqmichael · 2021-01-05T20:38:34Z

test/e2e/e2e.go

@@ -102,6 +103,26 @@ func (e *efsDriver) GetPersistentVolumeSource(readOnly bool, fsType string, volu
 	return &pvSource, nil
 }

+func (e *efsDriver) GetDynamicProvisionStorageClass(config *testsuites.PerTestConfig, fsType string) *storagev1.StorageClass {


Is implementing GetDynamicProvisionStorageClass how to enable upstream dynamic provision tests?

Yeah, this allows the test suites to detect and run dynamic provision tests

kbasv · 2021-01-05T20:44:23Z

@jqmichael I will be creating adding examples and documentation in a follow up PR

wangycc · 2021-01-06T03:05:04Z

kubectl apply -f serviceaccount-csi-controller.yaml node.yaml csidriver.yaml controller.yaml clusterrole-provisioner.yaml clusterrolebinding-provisioner.yaml

After all apply, the controller component fails to start

controller logs:

I0106 03:02:23.058741       1 connection.go:186] GRPC error: rpc error: code = Unimplemented desc = unknown service csi.v1.Controller
F0106 03:02:23.058758       1 csi-provisioner.go:188] Error getting CSI driver capabilities: rpc error: code = Unimplemented desc = unknown service csi.v1.Controller

wongma7 · 2021-01-06T05:41:11Z

@wangycc what is the image in the controller.yaml? it should be amazon/aws-efs-csi-driver:master since no release contains this commit yet

wangycc · 2021-01-06T07:45:52Z

@wongma7 amazon/aws-efs-csi-driver:latest,When do you plan to push the controller image? We started to try.

Most application charts don't usually create `StorageClass`es. They create PVCs. This is extending the work done in kubernetes-sigs#274. The chart itself is kind of broken unless `image.tag` is set to "master". kubernetes-sigs#291 will probably address that.

Skaronator · 2021-02-28T23:58:39Z

I'm a bit confused. This is not part of the 1.1 release, right? Will there be a new release any time soon?

wongma7 · 2021-03-01T19:39:30Z

@Skaronator right, this was merged into master branch but excluded from 1.1 release. I don't have ETA for when this will be released

Eternal21 · 2022-09-06T00:16:04Z

I'm following the link to this issue from this document. Is it now part of the official release?

rnishtala-sumo · 2022-11-03T23:18:05Z

it would help to have an update on when support for dynamic provisioning will be released. Creating multiple accesspoints/PVs manually seems like a large overhead.

ncgisudo · 2022-12-05T14:23:29Z

Is this actually available? The feature was merged in Jan 2021 and it's now December 2022 (23 months later) and there's no information about it.

amej · 2023-02-21T03:43:23Z

Is this actually available? The feature was merged in Jan 2021 and it's now December 2022 (23 months later) and there's no information about it.

As a customer of this efs csi driver, I confirm that Dynamic provisioning via access point is available. Delete volume capability does not fully work as mentioned in the README.

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Nov 1, 2020

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Nov 1, 2020

k8s-ci-robot requested review from nckturner and wongma7 November 1, 2020 02:55

k8s-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Nov 1, 2020

kbasv force-pushed the controller branch from 2db2cc7 to c11c4b1 Compare November 1, 2020 03:02

wongma7 reviewed Nov 2, 2020

View reviewed changes

pkg/driver/controller.go Show resolved Hide resolved

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Nov 2, 2020

kbasv force-pushed the controller branch 3 times, most recently from d7e43c8 to 22ccd19 Compare November 3, 2020 06:00

wongma7 reviewed Nov 3, 2020

View reviewed changes

pkg/driver/node.go Outdated Show resolved Hide resolved

chrishenzie suggested changes Nov 3, 2020

View reviewed changes

kbasv commented Nov 3, 2020

View reviewed changes

pkg/driver/efs_watch_dog.go Outdated Show resolved Hide resolved

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 4, 2020

jqmichael reviewed Nov 5, 2020

View reviewed changes

kbasv force-pushed the controller branch from 22ccd19 to fee90b1 Compare November 7, 2020 05:48

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 7, 2020

kbasv force-pushed the controller branch 3 times, most recently from a00050c to 4faa95e Compare November 10, 2020 14:40

chrishenzie mentioned this pull request Nov 11, 2020

REQUEST: New membership for chrishenzie kubernetes/org#2327

Closed

6 tasks

kbasv force-pushed the controller branch from 4faa95e to 2122cda Compare November 12, 2020 18:08

jqmichael reviewed Nov 12, 2020

View reviewed changes

kbasv force-pushed the controller branch from 2122cda to c396275 Compare November 17, 2020 16:37

wongma7 reviewed Dec 1, 2020

View reviewed changes

pkg/driver/controller.go Show resolved Hide resolved

wongma7 reviewed Dec 1, 2020

View reviewed changes

deploy/kubernetes/overlays/dev/master_image.yaml Outdated Show resolved Hide resolved

k8s-ci-robot merged commit aad2f30 into kubernetes-sigs:master Jan 5, 2021

jqmichael reviewed Jan 5, 2021

View reviewed changes

wongma7 mentioned this pull request Jan 6, 2021

change config dir location #286

Merged

This was referenced Jan 6, 2021

Add psp rbac helm #279

Closed

chore: Release new version of chart #295

Closed

kbasv deleted the controller branch January 7, 2021 23:07

kbasv mentioned this pull request Jan 8, 2021

Add documentation and examples for dynamic provisioning #297

Merged

gazal-k mentioned this pull request Jan 10, 2021

feat: Add option to provision StorageClasses #298

Merged

gazal-k mentioned this pull request Jan 11, 2021

chown/chgrp on dynamic provisioned pvc fails #300

Closed

wochanda mentioned this pull request Jan 22, 2021

Dynamically create file systems #310

Closed

sdwerwed mentioned this pull request Mar 14, 2021

Cannot deploy EFS CSI in cn-north-1 and cn-northwest-1 #348

Closed

lwj5 mentioned this pull request Mar 29, 2021

[SOLR] EKS Fargate Support PreferredAI/helm-charts#27

Closed

wongma7 mentioned this pull request Apr 14, 2021

Files not getting deleted on the EFS when PVCs & Access Points are Deleted #411

Closed

kbasv mentioned this pull request Apr 23, 2021

REQUEST: New membership for kbasv kubernetes/org#2656

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce Controller with Create and Delete Volume capability. Adds support for Dynamic Provisioning via access points #274

Introduce Controller with Create and Delete Volume capability. Adds support for Dynamic Provisioning via access points #274

kbasv commented Nov 1, 2020 •

edited

Loading

k8s-ci-robot commented Nov 1, 2020

wongma7 commented Nov 2, 2020

chrishenzie left a comment •

edited

Loading

jqmichael left a comment

jqmichael Nov 12, 2020

wongma7 Dec 1, 2020

wongma7 Dec 1, 2020

jqmichael left a comment

jqmichael Jan 5, 2021

kbasv Jan 5, 2021

kbasv commented Jan 5, 2021

wangycc commented Jan 6, 2021 •

edited

Loading

wongma7 commented Jan 6, 2021

wangycc commented Jan 6, 2021

Skaronator commented Feb 28, 2021

wongma7 commented Mar 1, 2021

Eternal21 commented Sep 6, 2022

rnishtala-sumo commented Nov 3, 2022

ncgisudo commented Dec 5, 2022

amej commented Feb 21, 2023

Introduce Controller with Create and Delete Volume capability. Adds support for Dynamic Provisioning via access points #274

Introduce Controller with Create and Delete Volume capability. Adds support for Dynamic Provisioning via access points #274

Conversation

kbasv commented Nov 1, 2020 • edited Loading

k8s-ci-robot commented Nov 1, 2020

wongma7 commented Nov 2, 2020

chrishenzie left a comment • edited Loading

Choose a reason for hiding this comment

jqmichael left a comment

Choose a reason for hiding this comment

jqmichael Nov 12, 2020

Choose a reason for hiding this comment

wongma7 Dec 1, 2020

Choose a reason for hiding this comment

wongma7 Dec 1, 2020

Choose a reason for hiding this comment

jqmichael left a comment

Choose a reason for hiding this comment

jqmichael Jan 5, 2021

Choose a reason for hiding this comment

kbasv Jan 5, 2021

Choose a reason for hiding this comment

kbasv commented Jan 5, 2021

wangycc commented Jan 6, 2021 • edited Loading

wongma7 commented Jan 6, 2021

wangycc commented Jan 6, 2021

Skaronator commented Feb 28, 2021

wongma7 commented Mar 1, 2021

Eternal21 commented Sep 6, 2022

rnishtala-sumo commented Nov 3, 2022

ncgisudo commented Dec 5, 2022

amej commented Feb 21, 2023

kbasv commented Nov 1, 2020 •

edited

Loading

chrishenzie left a comment •

edited

Loading

wangycc commented Jan 6, 2021 •

edited

Loading