Support access points on the same file system #185

2uasimojo · 2020-06-16T22:12:47Z

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

Expands the supported volumeHandle formats to include a three-field
version: {fsid}:{subpath}:{apid}. This addresses the limitation
originally described in #100 whereby k8s relies solely on the
volumeHandle to uniquely distinguish one PV from another.

As part of this fix, specifying accesspoint=fsap-... in mountOptions
is deprecated.

For more details, see the related issue (#167).

What testing is done?

The following scenarios were tested in a live environment:

Conflicting access point in volumeHandle and mountOptions

volumeHandle: fs::ap1
mountOptions: ['tls', 'accesspoint=ap2']
expect: fail
actual: fail with Warning FailedMount 1s (x4 over 4s) kubelet, ip-10-0-137-122.ec2.internal MountVolume.SetUp failed for volume "pv-aptest-1" : kubernetes.io/csi: mounter.SetupAt failed: rpc error: code = InvalidArgument desc = Found conflicting access point IDs in mountOptions (fsap-04d3307beebd04739) and volumeHandle (fsap-057e9a9b209ec9531)
result: ✔

Same access point in volumeHandle and mountOptions

volumeHandle: fs::ap1
mountOptions: ['tls', 'accesspoint=ap1']
expect: success
result: ✔

Two access points on the same file system

(This is the main case for this fix.)

Also makes sure we populate tls and accesspoint in mountOptions

mountOptions: [] (for both)
PV1:
- volumeHandle: fs1::ap1
PV2:
- volumeHandle: fs1::ap2
expect: success, both mounts accessible and distinct
result: ✔

Subpaths with access points

mountOptions: [] (for all)
PV1:
- volumeHandle: fs1::ap1 (root -- should be able to see /foo and bar)
PV2:
- volumeHandle: fs1:/foo/bar:ap1 (absolute path)
PV3:
- volumeHandle: fs1:foo/bar:ap1 (relative path)
expect: success
actual: success (had to create $absolutemountpoint/foo/bar in the fs first, as expected)
result: ✔

Is this a bug fix or adding new feature?

Fixes: #167

Signed-off-by: Eric Fried [email protected]

k8s-ci-robot · 2020-06-16T22:12:55Z

Hi @2uasimojo. 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.

cblecker · 2020-06-16T22:29:53Z

/ok-to-test

wongma7 · 2020-06-17T19:16:45Z

lgtm in current state pending docs/unittests. thanks!

2uasimojo · 2020-06-18T13:27:03Z

UT and docs are done.

I'll start working on the e2e tests now. We can merge this as is and put the e2e in a separate PR, or wait and I'll add it here -- up to you.

2uasimojo · 2020-06-18T13:39:08Z

/test pull-aws-efs-csi-driver-unit

jharrington22 · 2020-06-18T14:28:35Z

/lgtm

k8s-ci-robot · 2020-06-18T14:28:42Z

@jharrington22: changing LGTM is restricted to collaborators

In response to this:

/lgtm

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.

2uasimojo · 2020-06-18T14:36:12Z

BTW, this reverts the doc change in #168.

examples/kubernetes/access_points/specs/example.yaml

wongma7 · 2020-06-18T20:01:07Z

examples/kubernetes/access_points/README.md

@@ -6,8 +6,7 @@ In this case, the separation is managed on the EFS side rather than the kubernet

 ### Create Access Points (in EFS)
 Following [this doc](https://docs.aws.amazon.com/efs/latest/ug/create-access-point.html), create a separate access point for each independent data store you wish to expose in your cluster, tailoring the ownership and permissions as desired.
-
-**Note**: Until [issue #167](https://github.com/kubernetes-sigs/aws-efs-csi-driver/issues/167) is resolved, you must use a separate file system for each access point if they are to be mounted in the same pod.
+There is no need to use different EFS volumes.


this sentence seems out of place now, i would remove

Well, this is actually the crux of the change. Previously, if you wanted multiple shared stores in the same pod, you had to use separate volumes; and if you didn't, things broke in mysterious ways. Now this is possible, and it may be quite desirable for consumers to avoid the extra overhead.

Is there some way I can rephrase to make that clearer, or somewhere else I can put it so the context is better?

wongma7 · 2020-06-18T20:03:58Z

lgtm aside from the example nit.

regarding the e2e tests, i am trying to get #173 merged so you can reuse/build upon some of the createEFSPVCPV and such functions. You can rebase your tests on top of that PR or you dont have to use it at all, up to you!

2uasimojo · 2020-06-18T20:08:00Z

lgtm aside from the example nit.

Thank you for the review. I'll fix up the docs ASAP.

regarding the e2e tests, i am trying to get #173 merged so you can reuse/build upon some of the createEFSPVCPV and such functions. You can rebase your tests on top of that PR or you dont have to use it at all, up to you!

Ah, I hadn't noticed that #173 is still open. I'm sure I would have once I started trying to build on it -- which I actually haven't started on yet. That being the case, I'd prefer to get this merged (once fixed up) and work on the e2e tests in (a) separate PR(s).

2uasimojo · 2020-06-18T20:41:08Z

Question on docs @wongma7: Do I need to put in a CHANGELOG entry like this one as part of this PR, or do we roll all those up at release time?

Expands the supported `volumeHandle` formats to include a three-field version: `{fsid}:{subpath}:{apid}`. This addresses the limitation originally described in kubernetes-sigs#100 whereby k8s relies solely on the `volumeHandle` to uniquely distinguish one PV from another. As part of this fix, specifying `accesspoint=fsap-...` in `mountOptions` is deprecated. For more details, see the related issue (kubernetes-sigs#167). The following scenarios were tested in a live environment: **Conflicting access point in volumeHandle and mountOptions** - `volumeHandle: fs::ap1` - `mountOptions: ['tls', 'accesspoint=ap2']` - expect: fail - actual: fail with `Warning FailedMount 1s (x4 over 4s) kubelet, ip-10-0-137-122.ec2.internal MountVolume.SetUp failed for volume "pv-aptest-1" : kubernetes.io/csi: mounter.SetupAt failed: rpc error: code = InvalidArgument desc = Found conflicting access point IDs in mountOptions (fsap-04d3307beebd04739) and volumeHandle (fsap-057e9a9b209ec9531)` - result: ✔ **Same access point in volumeHandle and mountOptions** - `volumeHandle: fs::ap1` - `mountOptions: ['tls', 'accesspoint=ap1']` - expect: success - result: ✔ **Two access points on the same file system** Also makes sure we populate tls and accesspoint in mountOptions - `mountOptions: []` (for both) - PV1: - `volumeHandle: fs1::ap1` - PV2: - `volumeHandle: fs1::ap2` - expect: success, both mounts accessible and distinct - result: ✔ **Subpaths with access points** - `mountOptions: []` (for all) - PV1: - `volumeHandle: fs1::ap1` (root -- should be able to see /foo and bar) - PV2: - `volumeHandle: fs1:/foo/bar:ap1` (absolute path) - PV3: - `volumeHandle: fs1:foo/bar:ap1` (relative path) - expect: success - actual: success (had to create `$absolutemountpoint/foo/bar` in the fs first, as expected) - result: ✔ Signed-off-by: Eric Fried <[email protected]>

wongma7 · 2020-06-18T21:02:21Z

/lgtm
/approve

Let's wait until release time, if you want to write the release note/action required we can mimic the k/k convention and put it in the PR body so that whoever compiles the changelog will see and use it

k8s-ci-robot · 2020-06-18T21:02:29Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 2uasimojo, wongma7

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~~ [wongma7]

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

2uasimojo · 2020-06-18T21:06:11Z

/test pull-aws-efs-csi-driver-e2e

Since the [fix](kubernetes-sigs/aws-efs-csi-driver#185) for [issue #167](kubernetes-sigs/aws-efs-csi-driver#167) merged, the AWS EFS CSI driver overwrote the [`latest` image tag](sha256-962619a5deb34e1c4257f2120dd941ab026fc96adde003e27f70b65023af5a07?context=explore) to include it. For starters, this means we can update this operator to use the [new method of specifying access points](https://github.com/kubernetes-sigs/aws-efs-csi-driver/tree/0ae998c5a95fe6dbee7f43c182997e64872695e6/examples/kubernetes/access_points#edit-persistent-volume-spec) via a colon-delimited `volumeHandle` as opposed to in `mountOptions`. However, the same update to `latest` also brought in a [commit](kubernetes-sigs/aws-efs-csi-driver@b3baff8) that requires an additional mount in the `efs-plugin` container of the DaemonSet. So we need to update our YAML for that resource at the same time, or everything is broken (this might be upstream [issue #192](kubernetes-sigs/aws-efs-csi-driver#192). This update to the DaemonSet YAML also syncs with [upstream](https://github.com/kubernetes-sigs/aws-efs-csi-driver/blob/0ae998c5a95fe6dbee7f43c182997e64872695e6/deploy/kubernetes/base/node.yaml) by bumping the image versions for the other two containers (csi-node-driver-registrar: v1.1.0 => v1.3.0; and livenessprobe: v1.1.0 => livenessprobe:v2.0.0).

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 Jun 16, 2020

k8s-ci-robot requested review from justinsb and wongma7 June 16, 2020 22:12

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jun 16, 2020

2uasimojo force-pushed the issues/167 branch from 8c336b1 to c3f6cdd Compare June 16, 2020 22:14

k8s-ci-robot removed the do-not-merge/invalid-commit-message Indicates that a PR should not merge because it has an invalid commit message. label Jun 16, 2020

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 Jun 16, 2020

2uasimojo force-pushed the issues/167 branch from c3f6cdd to f99c4b3 Compare June 18, 2020 13:23

2uasimojo changed the title ~~WIP: Support access points on the same file system~~ Support access points on the same file system Jun 18, 2020

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 Jun 18, 2020

2uasimojo mentioned this pull request Jun 18, 2020

Doc: Access Points: Require separate file systems #168

Merged

wongma7 reviewed Jun 18, 2020

View reviewed changes

examples/kubernetes/access_points/specs/example.yaml Outdated Show resolved Hide resolved

wongma7 reviewed Jun 18, 2020

View reviewed changes

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 18, 2020

2uasimojo force-pushed the issues/167 branch from f99c4b3 to 69c3d51 Compare June 18, 2020 20:49

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 18, 2020

k8s-ci-robot assigned wongma7 Jun 18, 2020

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 18, 2020

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 18, 2020

k8s-ci-robot merged commit 0ae998c into kubernetes-sigs:master Jun 18, 2020

2uasimojo deleted the issues/167 branch June 18, 2020 22:53

ossareh mentioned this pull request Jun 23, 2020

MountVolume.SetUp failed for volume "efs-pv" #192

Closed

2uasimojo mentioned this pull request Jun 23, 2020

Please tag a new version #152

Closed

2uasimojo mentioned this pull request Jun 23, 2020

Update for latest CSI driver image post issues/167 openshift/aws-efs-operator#17

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support access points on the same file system #185

Support access points on the same file system #185

2uasimojo commented Jun 16, 2020 •

edited

Loading

k8s-ci-robot commented Jun 16, 2020

cblecker commented Jun 16, 2020

wongma7 commented Jun 17, 2020

2uasimojo commented Jun 18, 2020

2uasimojo commented Jun 18, 2020

jharrington22 commented Jun 18, 2020

k8s-ci-robot commented Jun 18, 2020

2uasimojo commented Jun 18, 2020

wongma7 Jun 18, 2020

2uasimojo Jun 18, 2020

wongma7 commented Jun 18, 2020

2uasimojo commented Jun 18, 2020

2uasimojo commented Jun 18, 2020

wongma7 commented Jun 18, 2020

k8s-ci-robot commented Jun 18, 2020

2uasimojo commented Jun 18, 2020

Support access points on the same file system #185

Support access points on the same file system #185

Conversation

2uasimojo commented Jun 16, 2020 • edited Loading

k8s-ci-robot commented Jun 16, 2020

cblecker commented Jun 16, 2020

wongma7 commented Jun 17, 2020

2uasimojo commented Jun 18, 2020

2uasimojo commented Jun 18, 2020

jharrington22 commented Jun 18, 2020

k8s-ci-robot commented Jun 18, 2020

2uasimojo commented Jun 18, 2020

wongma7 Jun 18, 2020

Choose a reason for hiding this comment

2uasimojo Jun 18, 2020

Choose a reason for hiding this comment

wongma7 commented Jun 18, 2020

2uasimojo commented Jun 18, 2020

2uasimojo commented Jun 18, 2020

wongma7 commented Jun 18, 2020

k8s-ci-robot commented Jun 18, 2020

2uasimojo commented Jun 18, 2020

2uasimojo commented Jun 16, 2020 •

edited

Loading