Change size limitation for all file paths and node_id

[Jiawei0227]

Size limitation default for CSI spec is 128 bytes.
This has been brought up several times in different issues[1][2] that we should increase the limitation.

I think there are two types of length now need to increase:

1. node_id
2. file_paths including(target_path, volume_path, staging_target_path)

In this PR, suggest to increase node_id length limitation to 192 bytes.
For all other paths related field, remove the limitation of 128 and instead put a minimal support size of 128 bytes

[1] #350
[2] kubernetes-sigs/gcp-compute-persistent-disk-csi-driver#581

[saad-ali]

Discussed this at community meeting. See notes here.

[Jiawei0227]

/assign @saad-ali
/cc @msau42 @gnufied

[saad-ali]

@jdef @jieyu PTAL

Also let's get input from @gnufied @jsafrane - Per Hemant, even existing 128 length limit for paths may be a problem for systemd.

[jdef]

Why is the new limit for node id 253 instead of 256? Something k8s specific? Otherwise I'm fine w/ Saad's suggestions.

…

On Fri, Jan 8, 2021 at 4:46 PM Saad Ali ***@***.***> wrote: ***@***.**** commented on this pull request. @jdef <https://github.com/jdef> @jieyu <https://github.com/jieyu> PTAL Also let's get input from @gnufied <https://github.com/gnufied> @jsafrane <https://github.com/jsafrane> - Per Hemant, even existing 128 length limit for paths may be a problem for systemd. ------------------------------ In spec.md <#464 (comment)> : > @@ -2073,6 +2075,7 @@ message NodeStageVolumeRequest { // CO SHALL be responsible for creating the directory if it does not // exist. // This is a REQUIRED field. + // The size limitation for this field is 256 bytes. ⬇️ Suggested change - // The size limitation for this field is 256 bytes. + // This field overrides the general CSI size limit. + // The size of this field SHALL NOT exceed 256 bytes. + // The general CSI size limit, 128 byte, is RECOMMENDED for best backwards compatibility. Do the same for similar comments below. ------------------------------ In spec.md <#464 (comment)> : > @@ -436,6 +436,8 @@ These limits apply for messages generated by both COs and plugins. | 128 bytes | string | | 4 KiB | map<string, string> | +Please note that for any length of paths or id, 128 bytes will still be the preferrable size for backward compatibility. Remove this and just include it in each override as specified below. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#464 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAR5KLFPEVXUYP7KA7FH7N3SY54J5ANCNFSM4TRMNXTA> .

-- James DeFelice 585.241.9488 (voice) 650.649.6071 (fax)

[saad-ali]

Why is the new limit for node id 253 instead of 256? Something k8s specific?

Yep, k8s object name length limit is 253 and these id's end up being used there.

[gnufied]

I was checking the systemd issue again and 256 indeed is too big to be a unit name for mount point to be managed by systemd. More details are here - https://github.com/systemd/systemd/pull/15911/files#diff-cba2cdf3ac7ac3975d69ed999252a138fed94763e46312c7e48500744eb8028aR452 . The issue itself is discussed in more detail - systemd/systemd#14294

The underlying reasoning is - on Linux FILENAME_MAX is 256 chars and hence you can't make a file whose name is bigger than 256 character. Now each mount point typically when managed via systemd gets converted to a unit name. This conversion happens on whole path and hence if it is bigger than 256 chars, systemd fails to manage it.

It should also be noted that, there is another variable called PATH_MAX which defines the maximum length of whole path (i.e not just the last component) and according to https://docs.microsoft.com/en-us/windows/win32/fileio/maximum-file-path-limitation the value is 260 characters on windows and hence setting this to 256 char will be pushing it awfully close to the upper limit.

So I think that 256 char might actually be too close to the upper limit of maximum path lengths and we might have to reduce the length. I don't know what good default will be but something smaller I think.

[Jiawei0227]

Hmm, what about 192? Seems like a magic number to use

[Jiawei0227]

I am putting 192 as the reduced length for both node_id and path. So it could be less error-prone.

We can talk more in the next CSI meeting to confirm this tho.

[Jiawei0227]

Changed as per discussion in the CSI community meeting

Notes:

256 can be a little too close. So to make it less error-prone, suggest change to 192 for both node_id and path
Systemd has a 256 limit
Windows has a 260 limit
Includes prefix “/var/lib/kubelet/…” some distros use different dir.
Nothing at gRPC level enforces this, SP should enforce it. Some may do it, some may not. Change to new limit will take time.
Some CSI drivers may enforce the path limit on the driver side, so CO MUST gracefully handle that and surface a good error.
Rex Ray library enforces path limit -- but RexRay lib will be deprecated
Increase to 192 gives 60ish characters for prefix, will that be enough?
e.g. /var/lib/kubelet/pods/{uid}/volumes/{pluginName}...{volumeName}
If someone makes every field max length it will overflow.
Why enforce any limit at all?
OS have limits? Could put a floor instead of max
E.g. for file paths, SP MUST allow at least 128 bytes, and SHOULD allow max of what ever OS allows.
If no limit defined, SP can make a really long path, and it may not run on different OSes (e.g. Windows 260 char, Systemd 256 char, etc.).
Can put a limit on all the fields that go in in to the path.
What if SP only supports minimum? They are hurting themselves, by lowering compatibility.
What if plugin is Legally max on all fields (e.g. plugin name, etc.)
CSI limits plugin name to 63 characters or less
Decision
For file paths, change to a “min” required and OS limit as recommended.
Ensure on the k8s side it gracefully handles that and surface a good error

[saad-ali]

/lgtm
/approve

PTAL @gnufied @jdef @jsafrane @jieyu @julian-hj

[gnufied]

lgtm too!

[Jiawei0227]

@jdef Hey James, would you mind take another look at this?

[ddebroy]

lgtm

[saad-ali]

Merging

[jdef]

LGTM, thanks all!