Mount correction #854

lowenna · 2017-05-24T17:48:12Z

Signed-off-by: John Howard [email protected]

a) Tagged the type of mount with the correct platform tag (not applicable on Windows.

b) Generally, there are not platform specific comments inside the .go files - that why we have the .md files. Hence moved the Linux comments for mount from config.go to config.md.

c) The comment about the path and child directories MUST exist, a runtime MUST NOT create directories automatically to a mount point. is incorrect in the way mapped directories (which is what the runtime uses through HCS to provide "pseudo"-mounts on Windows) work. Hence made that a Posix comment.

lowenna · 2017-05-24T17:54:50Z

Fixed white-space errors.

wking · 2017-05-24T18:39:06Z

config.md

@@ -59,6 +59,7 @@ For Solaris, the mount entry corresponds to the 'fs' resource in the [zonecfg(1M

 * **`destination`** (string, REQUIRED) Destination of mount point: path inside container.
  This value MUST be an absolute path.
+  * Posix: the path and child directories MUST exist, a runtime MUST NOT create directories automatically to a mount point.


Something very like this change was rejected in #591.

Right, but currently it is incorrect, as it states this in config.go for Windows where that statement is incorrect.

Right, but currently it is incorrect, as it states this in config.go for Windows where that statement is incorrect.

The Markdown is normative, and the Go comments are not. I'm in favor of removing the incorrect Go comment (like you do in this PR), and in general making the Go comments as simple as possible to avoid this sort of problem. But I don't want to carry incorrect Go comments back into the normative Markdown (like you do in this hunk).

To be clear then, you recommend removing the addition both here, and keep the removal in config.go.

To be clear then, you recommend removing the addition both here, and keep the removal in config.go.

Yes.

Done and update pushed.

wking · 2017-05-24T18:40:55Z

config.md

@@ -69,7 +70,7 @@ For Solaris, the mount entry corresponds to the 'fs' resource in the [zonecfg(1M
  * Windows: a local directory on the filesystem of the container host. UNC paths and mapped drives are not supported.
  * Solaris: corresponds to "special" of the fs resource in [zonecfg(1M)][zonecfg.1m].
 * **`options`** (array of strings, OPTIONAL) Mount options of the filesystem to be used.
-  * Linux: supported options are listed in the [mount(8)][mount.8] man page. Note both [filesystem-independent][mount.8-filesystem-independent] and [filesystem-specific][mount.8-filesystem-specific] options are listed.
+  * Linux: supported options are listed in the [mount(8)][mount.8] man page. Note both [filesystem-independent][mount.8-filesystem-independent] and [filesystem-specific][mount.8-filesystem-specific] options are listed. In the case of bind mounts, this would be the file on the host.


options does not include a file from the host for bind mounts. More on bind mounts in #771, which I hope to revive after opencontainers/runc#1442 lands and I can PR runC support for (no)lazytime, etc.

The change here (moving from config.go to config.md is purely for consistency. I haven't changed anything semantically as the statement already exists.

I haven't changed anything semantically as the statement already exists.

Moving text from the non-normative Go comments to the normative Markdown spec is a semantic change. I'd rather this PR just adjusted the Go comments to not conflict with the Markdown spec while leaving the Markdown spec alone (unless you intend to make a semantic change, in which case you have to adjust the Markdown spec).

I'd rather this PR just adjusted the Go comments to not conflict with the Markdown spec

I'm still confused. What exactly do you believe is incorrect here. Are you saying the statement In the case of bind mounts, this would be the file on the host. is invalid? It's inconsistent to have it in the go code, so where should it go? Or should it be entirely removed?

Are you saying the statement In the case of bind mounts, this would be the file on the host. is invalid?

Yes.

It's inconsistent to have it in the go code, so where should it go?

I think the Go comments should be as short and sweet as possible to avoid these inconsistencies. The more of the spec that gets echoed in the Go comments, the less DRY we are and the more likely we have these inconsistencies.

Or should it be entirely removed?

Yes, I'd like this PR to remove it from the Go comment but not inject it into the Markdown.

Done. Update pushed.

Are you saying the statement In the case of bind mounts, this would be the file on the host. is invalid?

Yes.

Actually, the current Go comment belongs to source, not options (although you're adding it under options here). So the current Go comment is correct in master, and might conceivably be useful in Markdown (under source). Although with the current spec having nothing to say about bind mounts and with #771 rejected at the moment, I think copying this one line over to Markdown without filling in the rest of a bind-mount spec isn't particularly useful.

I'm still in favor of removing it from the Go comment to avoid having this line go stale or conflict as the Markdown evolves.

wking · 2017-05-24T18:46:40Z

specs-go/config.go

-	Type string `json:"type,omitempty"`
-	// Source specifies the source path of the mount.  In the case of bind mounts on
-	// Linux based systems this would be the file on the host.
+	Type string `json:"type,omitempty" platform:"linux,solaris"`


Currently type is still defined for Windows, we just require it to be unset. I'd rather move it to a POSIX-specific subsection (like we already do for UID/GID) so the field would be undefined in the Windows-track spec. More on this distinction here and in #680. But as long as the Markdown docs define the property for Windows (even if they forbid it), I don't think we want to add this platform tag here.

I'm not sure I understand this comment. Is this change not entirely consistent with the User struct, and the UID, GID, AdditionalGids and Username fields of it?

I'm not sure I understand this comment.

I'm saying I'd like something closer to:

## Mounts …current section contents without `type`… ### Linux and Solaris Mounts For Linux- and Solaris-based systems the mount structure has the following additional properties: * **`type`** (string, OPTIONAL) …

Then a Windows config setting mounts[].type would be in the same boat as a Windows config setting process.users.uid (although the discussion in #680 shows that what that boat means could still use clarification).

Ah, I see. It confused me because the comment was against config.go when really you're referring to config.md....

It confused me because the comment was against config.go when really you're referring to config.md....

I'm saying that your current platform:"linux,solaris" addition here is not as clearly backed by the Markdown spec as I'd like. I agree that it's where we want the Go type to end up, so I'd like to see your Go change, but I'd also like to see the Markdown adjusted to more clearly support the Go change. In the absence of the Markdown change, I'd rather wait on the Go change.

OK, that's done. But I've been consistent with the existing User markdown. Specifically, it does not have a training - after Linux and Solaris as per your example. No use of the word additional. And uses the term fields rather than properties.

Update pushed.

No use of the word additional.

In the process.user case, the platform-specific sections cover the only properties for that object. In this case, the Linux/Solaris section will be adding a new property to the existing platform-independent properties. The best precedent we have for that is probably here, which doesn't use “additional” either. I'd be happier with “additional” or some other language that makes it clear that both the platform-independent properties and the POSIX-specific type are valid entries, but I'm ok punting that clarification to follow-up work.

I'd rather have the other bits changed too (e.g. we do use the hypenated “Linux-based” here), but I'm fine punting those to follow-up work as well.

Agreed on a follow-up consistency PR covering the markdown generally.

wking · 2017-05-24T19:40:00Z

config.md

+* **`type`** (string, OPTIONAL) The type of the filesystem to be mounted.
+  * Linux: filesystem types supported by the kernel as listed in */proc/filesystems* (e.g., "minix", "ext2", "ext3", "jfs", "xfs", "reiserfs", "msdos", "proc", "nfs", "iso9660").
+  * Solaris: corresponds to "type" of the fs resource in [zonecfg(1M)][zonecfg.1m].
+
 ### Example (Linux)


This (and the Solaris example) may want another # to nest them under Linux and Solaris Mounts. But since the examples are not just about type, I'm also ok leaving them with ### headers.

Agreed. I thought about adding another #, but decided against it for that reason.

wking · 2017-05-24T19:49:34Z

config.md

+
+* **`type`** (string, OPTIONAL) The type of the filesystem to be mounted.
+  * Linux: filesystem types supported by the kernel as listed in */proc/filesystems* (e.g., "minix", "ext2", "ext3", "jfs", "xfs", "reiserfs", "msdos", "proc", "nfs", "iso9660").
+  * Solaris: corresponds to "type" of the fs resource in [zonecfg(1M)][zonecfg.1m].


We can reduce future conflicts with #846 by adjusting these to use four-space indents when we touch the lines. That doesn't have to happen in this PR, but I think it's useful for reducing the rebase load on @Mashimiao if this PR lands first.

lowenna · 2017-05-25T17:57:39Z

Should this be tagged for 1.0?

crosbymichael · 2017-06-01T17:52:21Z

@jhowardmsft can you rebase this?

Signed-off-by: John Howard <[email protected]>

lowenna · 2017-06-01T18:55:21Z

@crosbymichael done

crosbymichael · 2017-06-01T20:41:34Z

LGTM

tianon · 2017-06-01T21:00:23Z

LGTM

lowenna force-pushed the mount branch from aaa2ee5 to 0cf6599 Compare May 24, 2017 17:54

lowenna mentioned this pull request May 24, 2017

Hooks to linux,solaris and doc clarification #855

Merged

wking reviewed May 24, 2017

View reviewed changes

lowenna force-pushed the mount branch 3 times, most recently from c18de44 to 4b40119 Compare May 24, 2017 19:30

wking reviewed May 24, 2017

View reviewed changes

wking mentioned this pull request May 24, 2017

Expand platform to be more sophisticated #830

Closed

mrunalp added this to the 1.0.0 milestone Jun 1, 2017

Mount correction

9685415

Signed-off-by: John Howard <[email protected]>

lowenna force-pushed the mount branch from 4b40119 to 9685415 Compare June 1, 2017 18:55

tianon merged commit ccfcf0b into opencontainers:master Jun 1, 2017

lowenna deleted the mount branch June 1, 2017 21:07

zhouhao3 mentioned this pull request Jun 16, 2017

validate: modify the validation of mounts.type opencontainers/runtime-tools#397

Open

vbatts mentioned this pull request Jul 5, 2017

bump to v1.0.0-rc6 #882

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Mount correction #854

Mount correction #854

lowenna commented May 24, 2017

lowenna commented May 24, 2017

wking May 24, 2017

lowenna May 24, 2017

wking May 24, 2017

lowenna May 24, 2017

wking May 24, 2017

lowenna May 24, 2017

wking May 24, 2017

lowenna May 24, 2017

wking May 24, 2017

lowenna May 24, 2017

wking May 24, 2017

lowenna May 24, 2017

wking May 24, 2017 •

edited

Loading

wking May 24, 2017

lowenna May 24, 2017

wking May 24, 2017 •

edited

Loading

lowenna May 24, 2017

wking May 24, 2017

lowenna May 24, 2017

wking May 24, 2017

lowenna May 24, 2017 •

edited

Loading

wking May 24, 2017

lowenna May 24, 2017

wking May 24, 2017

lowenna commented May 25, 2017

crosbymichael commented Jun 1, 2017

lowenna commented Jun 1, 2017

crosbymichael commented Jun 1, 2017 •

edited by caniszczyk

Loading

tianon commented Jun 1, 2017 •

edited by caniszczyk

Loading

Mount correction #854

Mount correction #854

Conversation

lowenna commented May 24, 2017

lowenna commented May 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wking May 24, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wking May 24, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lowenna May 24, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lowenna commented May 25, 2017

crosbymichael commented Jun 1, 2017

lowenna commented Jun 1, 2017

crosbymichael commented Jun 1, 2017 • edited by caniszczyk Loading

tianon commented Jun 1, 2017 • edited by caniszczyk Loading

wking May 24, 2017 •

edited

Loading

wking May 24, 2017 •

edited

Loading

lowenna May 24, 2017 •

edited

Loading

crosbymichael commented Jun 1, 2017 •

edited by caniszczyk

Loading

tianon commented Jun 1, 2017 •

edited by caniszczyk

Loading