Unable to mount two access points on same EFS volume

[2uasimojo]

/kind bug

This is just like #100, but for access points.

What happened?
Pod fails to come up, eventually timing out waiting for mounts. Logs show only one mount was attempted.

What you expected to happen?
All file systems mounted; pod comes up.

How to reproduce it (as minimally and precisely as possible)?

• Create two PVs using access points. Crucially, use access points on the same [FileSystemId].
• Create claims for the above PVs
• Create a pod using both claims. It will fail to start, showing symptoms identical to those described in Unable to mount two folders on same EFS volume #100.

Anything else we need to know?:

• The document referred to above states that this setup is supported. (That's my bad.) It needs to be fixed until/unless this bug is resolved. I'll put up a PR for that in the next few minutes.
• If you use access points on different file systems, it works fine.
• The solution (Update to read subpath from volume handle #102) for Unable to mount two folders on same EFS volume #100 was to glom the subdirectory into the volumeHandle, separated by a colon. It is not out of the question that a similar fix could be implemented here. For example, support any of:
  • volumeHandle: fs-XXX
  • volumeHandle: fs-XXX:/path
  • volumeHandle: fs-XXX:fsap-YYY <== new
    The access point would be fed into the mount options by the driver, rather than having to specify it explicitly under mountOptions in the PV spec. (Note that I don't think fs-XXX:fsap-YYY:/path makes sense because the access point is already subpathed, but it could be looked at.)

Environment

• Kubernetes version (use kubectl version):

Client Version: openshift-clients-4.3-31-gc25fb9c7
Server Version: 4.3.19
Kubernetes Version: v1.16.2

• Driver version:
  latest (as of 20200520)

[2uasimojo]

• The document referred to above states that this setup is supported. (That's my bad.) It needs to be fixed until/unless this bug is resolved. I'll put up a PR for that in the next few minutes.

#168 ⬆️

[leakingtapan]

Yes, the access point ID has to be part of volumeHandle to make kubelet aware of there is a different volume to be mounted instead of the mounting the same volume twice (which will be ignored by kubelet).

We need to define a proper volumeHandle semantic for both use cases:

• just the filesystem ID
• filesystem ID + subpath
• filesystem ID + access point

[wongma7]

(Note that I don't think fs-XXX:fsap-YYY:/path makes sense because the access point is already subpathed, but it could be looked at.)

I think it makes sense. An accesspoint should be treated like its own "root." If I have an accesspoint fsap-AAA at directory /a in my EFS then I want to be able to do mount fs-XXX:/b -o accesspoint=fsap-AAA, equivalent to doing mount fs-asdf:/a/b.

It's equivalent also to mount fs-XXX:b -o accesspoint=fsap-AAA...i.e. the path doesn't need to have a leading slash.

So I think volumeHandle is going to need another colon delimited token/field :/.

(TBH I think the proper long-term fix would be on the CSI/kubernetes side...I just read #100 (comment) and it seems wrong that CSI drivers have lost the ability to implement GetVolumeName compared to in-tree drivers, forcing CSI drivers to stuff fields into volumeHandle rather than build out volumeAttributes.

[2uasimojo]

(Note that I don't think fs-XXX:fsap-YYY:/path makes sense because the access point is already subpathed, but it could be looked at.)

I think it makes sense. An accesspoint should be treated like its own "root." If I have an accesspoint fsap-AAA at directory /a in my EFS then I want to be able to do mount fs-XXX:/b -o accesspoint=fsap-AAA, equivalent to doing mount fs-asdf:/a/b.

It's equivalent also to mount fs-XXX:b -o accesspoint=fsap-AAA...i.e. the path doesn't need to have a leading slash.

I was afraid of that :( It's going to make it harder to make this backward compatible with the existing two-field syntax, because when we see a two-field input, we wouldn't know whether the second field is a path or an access point ID.

I suppose this means we have to require all three fields if you want to use access points, even if the path component is empty (as I suspect it would be most of the time).

So how about this for a scratch design:

• One field is always interpreted as {fsid}. E.g. fs-abc123def.
• Two fields is always interpreted as {fsid}:{path}. E.g.
  • fs-abc123def:/foo (path has explicit leading slash)
  • fs-abc123def:foo (no leading slash) (is this legitimate?)
  • fs-abc123def: (empty path), treated same as single field fs-abc123def (do we allow this?)
• Three fields is always interpreted as {fsid}:{apid}:{path}. E.g.
  • fs-abc123def:fsap-001122334455: (common access point case, no subpath)
  • fs-abc123def:fsap-001122334455:/foo (subpath with explicit slash)
  • fs-abc123def:fsap-001122334455:foo (subpath with no slash)
  • fs-abc123def:: (no access point or path, same as single field fs-abc123def)
  • fs-abc123def::/foo (no access point, same as two-field fs-abc123def:/foo, path can have slashes or not)
• The only case we'll pre-check and fail on is if the {fsid} is empty. That goes for the one, two, or three field syntax:
  • "" (empty string)
  • :/foo
  • ::/foo
  • :fsap-001122334455:
  • :fsap-001122334455:/foo
  • etc.
    Otherwise we'll just let the mount command fail.

(TBH I think the proper long-term fix would be on the CSI/kubernetes side...I just read #100 (comment) and it seems wrong that CSI drivers have lost the ability to implement GetVolumeName compared to in-tree drivers, forcing CSI drivers to stuff fields into volumeHandle rather than build out volumeAttributes.

I agree wholeheartedly.

[wongma7]

I like your design, one simplification could be to keep {path} always the second field, i.e. be super strict about path and apid as "positional args", so whether it's two fields or three we can always assume we will get some variation of {fsid}:{path}:{apid} where everything past {fsid} is optional.

[2uasimojo]

Yup, I like that. I was trying to avoid the common access point case having the empty field in the middle ({fsid}::{apid}) but I think the simplicity of the ordering strictness is worth the tradeoff.

[2uasimojo]

I'll start attacking this today.