spec: Clarify target_path and staging_target_path #312
Conversation
csi.proto
Outdated
| // staging_target_path per volume. | ||
| // `staging_target_path` per volume. The SP MUST NOT use symbolic link | ||
| // to staged the volume. The CO SHALL ensure that the path exists, and | ||
| // that the process serving the request has `read` and `write` |
There was a problem hiding this comment.
typo in to staged the volume ?
spec.md
Outdated
| @@ -1848,8 +1854,12 @@ message NodePublishVolumeRequest { | |||
| // The path to which the volume will be published. It MUST be an | |||
| // absolute path in the root filesystem of the process serving this | |||
| // request. The CO SHALL ensure uniqueness of target_path per volume. | |||
| // The CO SHALL ensure that the path exists, and that the process | |||
| // serving the request has `read` and `write` permissions to the path. | |||
| // The SP MUST NOT use symbolic link to publish the volume. The CO | |||
There was a problem hiding this comment.
I can imagine a local ephemeral volume that only does Mount Volume not Block Volume, it might make sense to use symlink for such volumes. And with the added requirement below that the CO will create an empty file for block and an empty dir for mount, I'm not sure explicitly excluding symlink is necessary. I'd prefer to remove this line unless we have a strong reason for it.
There was a problem hiding this comment.
The use of symlink is naturally incompatible with the need to create empty file/directory (i.e., mount points) for volumes because SP then needs to delete the mount points created by the CO, and replace it with a symlink. The same argument applies to the use of mknod for block volumes. Symlink is almost certainly a bad idea because it's relative to the filesystem root of the Plugin process, which might be different that that of the CO's.
I am actually leaning toward just dictating that it is has to be mount based. I think this align with our goal for SP to write one plugin that works for all COs.
What do you guys think? cc @saad-ali @julian-hj ?
There was a problem hiding this comment.
Do we also stop people from having part of PATH being symbolic link? I remember - some users define /var/lib/kubelet to be a symbolic link for example and in that case the path will be /var/lib/kubelet/plugins/volumes/csi/xxx.
There was a problem hiding this comment.
We switched to not letting CO to create the mount point. SP can do that. CO needs to make sure SP has the permission to do that.
spec.md
Outdated
| @@ -1730,10 +1730,16 @@ message NodeStageVolumeRequest { | |||
| // this capability. This is an OPTIONAL field. | |||
| map<string, string> publish_info = 2; | |||
|
|
|||
| // The path to which the volume will be published. It MUST be an | |||
| // The path to which the volume will be staged. It MUST be an | |||
There was a problem hiding this comment.
Can you address Issue #295 (comment) while you are touching this code?
jieyu
force-pushed
the
target_path
branch
3 times, most recently
from
c61e1e1 to
be7fa84
Compare
|
Mostly LGTM. Is it worth adding: |
This patch clarifies the `target_path` and `staging_target_path` in the spec. It is CO's responsibility to make sure that SP has the permission to read or write `target_path` and `staging_target_path`, and is able to create files/directories if the path does not exist. This patch also changes the wording in `NodeStageVolume` call to enable those plugins that want to use the `NodeStageVolume` call as a lifecycle hook, but does not want to mount to the `staging_target_path`.
|
@saad-ali i don't think that's necessary. The comment already says that |
|
SGTM |
This patch clarifies the
target_pathandstaging_target_pathin thespec. It is CO's responsibility to make sure that SP has the permission
to read or write
target_pathandstaging_target_path, and is able tocreate files/directories if the path does not exist.
This patch also changes the wording in
NodeStageVolumecall to enablethose plugins that want to use the
NodeStageVolumecall as a lifecyclehook, but does not want to mount to the
staging_target_path.Closes #285
Closes #80
Closes #294
Closes #295