Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Mount correction #854

Merged
merged 1 commit into from
Jun 1, 2017
Merged

Mount correction #854

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 20 additions & 17 deletions config.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,22 +65,37 @@ For all platform-specific configuration values, the scope defined below in the [
For Linux, the parameters are as documented in [mount(2)][mount.2] system call man page.
For Solaris, the mount entry corresponds to the 'fs' resource in the [zonecfg(1M)][zonecfg.1m] man page.


* **`destination`** (string, REQUIRED) Destination of mount point: path inside container.
This value MUST be an absolute path.
* Windows: one mount destination MUST NOT be nested within another mount (e.g., c:\\foo and c:\\foo\\bar).
* Solaris: corresponds to "dir" of the fs resource in [zonecfg(1M)][zonecfg.1m].
* **`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").
* Windows: this field MUST NOT be supplied.
* Solaris: corresponds to "type" of the fs resource in [zonecfg(1M)][zonecfg.1m].
* **`source`** (string, OPTIONAL) A device name, but can also be a directory name or a dummy.
* 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.
* Solaris: corresponds to "options" of the fs resource in [zonecfg(1M)][zonecfg.1m].

### Example (Windows)

```json
"mounts": [
{
"destination": "C:\\folder-inside-container",
"source": "C:\\folder-on-host",
"options": []
}
]
```

### <a name="configLinuxAndSolarisMounts" />Linux and Solaris Mounts

For Linux and Solaris based systems the mounts structure has the following fields:

* **`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].
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


### Example (Linux)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


```json
Expand All @@ -100,18 +115,6 @@ For all platform-specific configuration values, the scope defined below in the [
]
```

### Example (Windows)

```json
"mounts": [
{
"destination": "C:\\folder-inside-container",
"source": "C:\\folder-on-host",
"options": []
}
]
```

### Example (Solaris)

```json
Expand Down
7 changes: 3 additions & 4 deletions specs-go/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,12 +112,11 @@ type Platform struct {

// Mount specifies a mount for a container.
type Mount struct {
// Destination is the path where the mount will be placed relative to the container's root. The path and child directories MUST exist, a runtime MUST NOT create directories automatically to a mount point.
// Destination is the absolute path where the mount will be placed in the container.
Destination string `json:"destination"`
// Type specifies the mount kind.
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"`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

@wking wking May 24, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@lowenna lowenna May 24, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

// Source specifies the source path of the mount.
Source string `json:"source,omitempty"`
// Options are fstab style mount options.
Options []string `json:"options,omitempty"`
Expand Down