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

config: Bump Hyper-V condition from root.path to root itself #838

Merged
merged 2 commits into from
Jun 29, 2017
Merged

config: Bump Hyper-V condition from root.path to root itself #838

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2022e3e
config: Bump Hyper-V condition from root.path to root itself
wking May 18, 2017
767eeaf
config: Use "POSIX" instead of "Linux and Solaris"
wking Jun 1, 2017
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
34 changes: 19 additions & 15 deletions config.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,22 +26,26 @@ For all platform-specific configuration values, the scope defined below in the [

## <a name="configRoot" />Root

**`root`** (object, REQUIRED) specifies the container's root filesystem.
**`root`** (object, OPTIONAL) specifies the container's root filesystem.
On Windows, for Windows Server Containers, this field is REQUIRED.
For [Hyper-V Containers](config-windows.md#hyperv), this field MUST NOT be set.

* **`path`** (string, OPTIONAL) Specifies the path to the root filesystem for the container.
The path is either an absolute path or a relative path to the bundle.
On all other platforms, this field is REQUIRED.

* On Windows, for Windows Server Containers, this field is REQUIRED and MUST be specified as a [volume GUID path][naming-a-volume].
For Hyper-V Containers, this field MUST be omitted.
* On all other platforms, this field is REQUIRED.
* **`path`** (string, REQUIRED) Specifies the path to the root filesystem for the container.

* On Windows, `path` MUST be a [volume GUID path][naming-a-volume].

* On POSIX platforms, `path` is either an absolute path or a relative path to the bundle.
Copy link

Choose a reason for hiding this comment

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

I think POSIX here is also not suitable as in #835

For example, with a bundle at `/to/bundle` and a root filesystem at `/to/bundle/rootfs`, the `path` value can be either `/to/bundle/rootfs` or `rootfs`.
The value SHOULD be the conventional `rootfs`.
* On Linux, for example, with a bundle at `/to/bundle` and a root filesystem at `/to/bundle/rootfs`, the `path` value can be either `/to/bundle/rootfs` or `rootfs`.

If defined, a directory MUST exist at the path declared by the field.
A directory MUST exist at the path declared by the field.

* **`readonly`** (bool, OPTIONAL) If true then the root filesystem MUST be read-only inside the container, defaults to false.
* On Windows, this field MUST be omitted or false.

### Example (POSIX)
### Example (POSIX platforms)

```json
"root": {
Expand Down Expand Up @@ -90,9 +94,9 @@ For all platform-specific configuration values, the scope defined below in the [
]
```

### <a name="configLinuxAndSolarisMounts" />Linux and Solaris Mounts
### <a name="configPOSIXMounts" />POSIX-platform Mounts

For Linux and Solaris based systems the mounts structure has the following fields:
For POSIX platforms 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").
Expand Down Expand Up @@ -191,9 +195,9 @@ For Linux-based systems the process structure supports the following process-spe

The user for the process is a platform-specific structure that allows specific control over which user the process runs as.

#### <a name="configLinuxAndSolarisUser" />Linux and Solaris User
#### <a name="configPOSIXUser" />POSIX-platform User

For Linux and Solaris based systems the user structure has the following fields:
For POSIX platforms the `user` structure has the following fields:

* **`uid`** (int, REQUIRED) specifies the user ID in the [container namespace](glossary.md#container-namespace).
* **`gid`** (int, REQUIRED) specifies the group ID in the [container namespace](glossary.md#container-namespace).
Expand Down Expand Up @@ -344,9 +348,9 @@ For Windows based systems the user structure has the following fields:
}
```

## <a name="configHooks" />Linux and Solaris Hooks
## <a name="configHooks" />POSIX-platform Hooks

For Linux- and Solaris-based systems, the configuration structure supports `hooks` for configuring custom actions related to the [lifecycle](runtime.md#lifecycle) of the container.
For POSIX platforms, the configuration structure supports `hooks` for configuring custom actions related to the [lifecycle](runtime.md#lifecycle) of the container.

* **`hooks`** (object, OPTIONAL) MAY contain any of the following properties:
* **`prestart`** (array of objects, OPTIONAL) is an array of [pre-start hooks](#prestart).
Expand Down
6 changes: 4 additions & 2 deletions schema/config-schema.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,9 @@
"description": "Configures the container's root filesystem.",
"id": "https://opencontainers.org/schema/bundle/root",
"type": "object",
"required": [
"path"
],
"properties": {
"path": {
"id": "https://opencontainers.org/schema/bundle/root/path",
Expand Down Expand Up @@ -214,7 +217,6 @@
}
},
"required": [
"ociVersion",
"root"
"ociVersion"
]
}
4 changes: 2 additions & 2 deletions specs-go/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ type Spec struct {
// Process configures the container process.
Process *Process `json:"process,omitempty"`
// Root configures the container's root filesystem.
Root Root `json:"root"`
Root *Root `json:"root,omitempty"`
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This may fall under the no-pointers-if-required-on-any-platform policy that @mrunalp was going to document. If so, I can drop the pointer (and omitempty?), but it would be nice to get that policy documented.

Copy link
Contributor

Choose a reason for hiding this comment

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

Does it make sense to have a pointer-required-if-field-must-not-be-set-on-a-platform policy instead?

In this case: keep *Root and omitempty because it MUST NOT be set for Hyper-V containers

Copy link
Contributor Author

@wking wking May 26, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Does it make sense to have a pointer-required-if-field-must-not-be-set-on-a-platform policy instead?

We already have some policy wording for pointers, but they focus on input requirements. I think we also want to consider output requirements with Go's default JSON serializer, and would recommend:

Use omitempty on properties which MAY be unset for a platform that uses the parent type. Of omitempty properties, use pointers if the Go zero value is legal (so we can serialize "uid": 0 and such) or if the property is a Go type (to work around golang/go#11939).

as a new hole we'd want to poke in the current no-pointer preference. That would mean that we'd want omitempty here (because root MUST NOT be set for Hyper-V containers, which passes “MAY be unset for a platform that uses the parent type”) and we'd want a pointer here (because property is a Go type). However, it would also mean we'd want omitempty for User.UID and User.GID (because they MAY be unset on Windows, and Windows uses User too for User.Username) and we'd want pointers for them (because 0 is a valid ID), and those changes were rejected in #759. So instead of proposing a pointer policy, I'm just trying to flag changes that may be impacted by the maintainers' apparent policy and waiting for them to us how they want that case handled.

// Hostname configures the container's hostname.
Hostname string `json:"hostname,omitempty"`
// Mounts configures additional mounts (on top of Root).
Expand Down Expand Up @@ -94,7 +94,7 @@ type User struct {
// Root contains information about the container's root filesystem on the host.
type Root struct {
// Path is the absolute path to the container's root filesystem.
Path string `json:"path,omitempty"`
Path string `json:"path"`
// Readonly makes the root filesystem for the container readonly before the process is executed.
Readonly bool `json:"readonly,omitempty"`
}
Expand Down