This repository has been archived by the owner on Aug 14, 2020. It is now read-only.
types/volume: add recursive flag #630
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
3 tasks
| @@ -172,6 +173,7 @@ JSON Schema for the Pod Manifest, conforming to [RFC4627](https://tools.ietf.org | |||
| * **volumes** (list of objects, optional) list of volumes which will be mounted into each application's filesystem | |||
| * **name** (string, required) descriptive label for the volume (restricted to the [AC Name](types.md#ac-name-type) formatting), used as an index by the `mounts` objects (above). | |||
| * **readOnly** (boolean, optional, defaults to "false" if unsupplied) whether or not the volume will be mounted read only. | |||
| * **recursive** (boolean, optional) whether or not the volume will be mounted recursively. When **recursive** is not specified, the executor SHOULD default to recursive. However, executors in specific run environment might prefer to default to non-recursive. | |||
There was a problem hiding this comment.
This only applies to kind = host right?
There was a problem hiding this comment.
I guess so... I don't see what differences the recursive bool would make on an empty volume.
I will update the text to make it clearer.
alban
force-pushed
the
alban/volume-recursive
branch
from
45d2f89 to
d45f0c0
Compare
2 tasks
| @@ -172,6 +173,7 @@ JSON Schema for the Pod Manifest, conforming to [RFC4627](https://tools.ietf.org | |||
| * **volumes** (list of objects, optional) list of volumes which will be mounted into each application's filesystem | |||
| * **name** (string, required) descriptive label for the volume (restricted to the [AC Name](types.md#ac-name-type) formatting), used as an index by the `mounts` objects (above). | |||
| * **readOnly** (boolean, optional, defaults to "false" if unsupplied) whether or not the volume will be mounted read only. | |||
| * **recursive** (boolean, optional) whether or not the volume will be mounted recursively. This only applies to volumes of type **host**. Executors MUST ignore **recursive** on **empty** volumes. When **recursive** is not specified, the executor SHOULD default to recursive. However, executors in specific run environment might prefer to default to non-recursive. | |||
There was a problem hiding this comment.
sorry, couple of things I should have mentioned on first pass:
- s/type/kind/
- can you move this after the kind entry? (i.e. before/after "source") - you can adopt similar language to mode/uid - "boolean, optional, only interpreted if kind is "host"" (then you can remove the ignore part you have now)
- can you link to a reference to what recursive means?
There was a problem hiding this comment.
Updated.
I linked to an LWN article because I found that the mount manpage didn't really explain recursive bind mount. And the shared subtree document does not mention it either.
|
ping? |
alban
force-pushed
the
alban/volume-recursive
branch
4 times, most recently
from
f3622a3 to
5957f48
Compare
alban
changed the title
|
Updated. I also added tests in |
| @@ -175,6 +176,7 @@ JSON Schema for the Pod Manifest, conforming to [RFC4627](https://tools.ietf.org | |||
| * **kind** (string, required) either: | |||
| * **empty** - creates an empty directory on the host and bind mounts it into the container. All containers in the pod share the mount, and the lifetime of the volume is equal to the lifetime of the pod (i.e. the directory on the host machine is removed when the pod's filesystem is garbage collected) | |||
| * **host** - fulfills a mount point with a bind mount from a **source** directory on the host. | |||
| * **recursive** (boolean, optional, only interpreted if **kind** is "host") whether or not the volume will be mounted [recursively](http://lwn.net/Articles/690679/). When **recursive** is not specified, the executor SHOULD default to recursive. However, executors in specific run environment might prefer to default to non-recursive. | |||
There was a problem hiding this comment.
can you move this after "source"?
|
One nit, lgtm other than that |
alban
force-pushed
the
alban/volume-recursive
branch
from
5957f48 to
ac3e4e5
Compare
|
Updated. |
| @@ -176,6 +177,7 @@ JSON Schema for the Pod Manifest, conforming to [RFC4627](https://tools.ietf.org | |||
| * **empty** - creates an empty directory on the host and bind mounts it into the container. All containers in the pod share the mount, and the lifetime of the volume is equal to the lifetime of the pod (i.e. the directory on the host machine is removed when the pod's filesystem is garbage collected) | |||
| * **host** - fulfills a mount point with a bind mount from a **source** directory on the host. | |||
| * **source** (string, required if **kind** is "host") absolute path on host to be bind mounted under a mount point in each app's chroot. | |||
| * **recursive** (boolean, optional, only interpreted if **kind** is "host") whether or not the volume will be mounted [recursively](http://lwn.net/Articles/690679/). When **recursive** is not specified, the executor SHOULD default to recursive. However, executors in specific run environment might prefer to default to non-recursive. | |||
There was a problem hiding this comment.
Can you just remove the last sentence? the grammar is slightly off but it's really superfluous since it's a SHOULD anyway.
|
sorry, I didn't read proper good yesterday. One more nit.. |
alban
force-pushed
the
alban/volume-recursive
branch
from
ac3e4e5 to
1ef8507
Compare
|
Updated :) |
This includes a unit test.
The recursive field is a boolean. I set the default to true, but only with a "SHOULD" (rfc2119): > 3. SHOULD This word, or the adjective "RECOMMENDED", mean that there > may exist valid reasons in particular circumstances to ignore a > particular item, but the full implications must be understood and > carefully weighed before choosing a different course. See discussion on: appc#622 (comment)
|
thanks for your patience |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes #622.
It is still "WIP" for now because I would like to test it first.
/cc @steveej @jonboulle