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

Adding cgroups path to the Spec. #137

Merged
merged 1 commit into from
Sep 10, 2015
Merged

Adding cgroups path to the Spec. #137

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
54 changes: 54 additions & 0 deletions runtime-config-linux.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,6 +131,60 @@ Also known as cgroups, they are used to restrict resource usage for a container
cgroups provide controls to restrict cpu, memory, IO, pids and network for the container.
For more information, see the [kernel cgroups documentation](https://www.kernel.org/doc/Documentation/cgroups/cgroups.txt).

The path to the cgroups can to be specified in the Spec via `cgroupsPath`.
`cgroupsPath` is expected to be relative to the cgroups mount point.
If not specified, cgroups will be created under '/'.
Implementations of the Spec can choose to name cgroups in any manner.
Copy link
Contributor

Choose a reason for hiding this comment

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

This is only true if cgroupsPath is unset. So maybe rephrase as “In that case, implementations can choose to name cgroups in any manner.” to tie it into the previous ”If not specified…” sentence.

Copy link
Member

Choose a reason for hiding this comment

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

I feel it's clear enough for now.

On Thu, Sep 10, 2015 at 2:55 PM, W. Trevor King notifications@github.com
wrote:

In runtime-config-linux.md
#137 (comment):

@@ -131,6 +131,60 @@ Also known as cgroups, they are used to restrict resource usage for a container
cgroups provide controls to restrict cpu, memory, IO, pids and network for the container.
For more information, see the kernel cgroups documentation.

+The path to the cgroups can to be specified in the Spec via cgroupsPath.
+cgroupsPath is expected to be relative to the cgroups mount point.
+If not specified, cgroups will be created under '/'.
+Implementations of the Spec can choose to name cgroups in any manner.

This is only true if cgroupsPath is unset. So maybe rephrase as “In that
case, implementations can choose to name cgroups in any manner.” to tie it
into the previous ”If not specified…” sentence.

—
Reply to this email directly or view it on GitHub
https://github.com/opencontainers/specs/pull/137/files#r39198980.

The Spec does not include naming schema for cgroups.
The Spec does not support [split hierarchy](https://www.kernel.org/doc/Documentation/cgroups/unified-hierarchy.txt).
The cgroups will be created if they don't exist.

```json
"cgroupsPath": "/myRuntime/myContainer"
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd mirror cgexec(1) with something like:

"cgroups": {
  "join": [
    {
      "path": "/myRuntime/test1",
      "controllers": ["cpu", "memory"]
    },
    {
      "path": "/myRuntime/test2",
      "controllers": ["swap"]
    }
  ]
}

And it would be nice to move the current "resources" into:

"cgroups": {
  "create": …,
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Won't this break with unified hierarchy?

Copy link
Contributor

Choose a reason for hiding this comment

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

On Wed, Sep 02, 2015 at 05:09:05PM -0700, Vish Kannan wrote:

Won't this break with unified
hierarchy?

Ah, that's interesting reading. In that case, a single path is fine.

Can we link to those Kernel docs from our “The Spec does not support
split hierarchy.”?

```

`cgroupsPath` can be used to either control the cgroups hierarchy for containers or to run a new process in an existing container.

Optionally, cgroups limits can be specified via `resources`.

```json
"resources": {
"disableOOMKiller": false,
"memory": {
"limit": 0,
"reservation": 0,
"swap": 0,
"kernel": 0,
"swappiness": -1
},
"cpu": {
"shares": 0,
"quota": 0,
"period": 0,
"realtimeRuntime": 0,
"realtimePeriod": 0,
"cpus": "",
"mems": ""
},
"blockIO": {
"blkioWeight": 0,
"blkioWeightDevice": "",
"blkioThrottleReadBpsDevice": "",
"blkioThrottleWriteBpsDevice": "",
"blkioThrottleReadIopsDevice": "",
"blkioThrottleWriteIopsDevice": ""
},
"hugepageLimits": null,
"network": {
"classId": "",
"priorities": null
}
}
```

Do not specify `resources` unless limits have to be updated.
For example, to run a new process in an existing container without updating limits, `resources` need not be specified.

## Sysctl

sysctl allows kernel parameters to be modified at runtime for the container.
Expand Down
6 changes: 5 additions & 1 deletion runtime_config_linux.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,11 @@ type LinuxRuntime struct {
Sysctl map[string]string `json:"sysctl"`
// Resources contain cgroup information for handling resource constraints
// for the container
Resources Resources `json:"resources"`
Resources *Resources `json:"resources"`
// CgroupsPath specifies the path to cgroups that are created and/or joined by the container.
// The path is expected to be relative to the cgroups mountpoint.
// If resources are specified, the cgroups at CgroupsPath will be updated based on resources.
CgroupsPath string `json:"cgroupsPath"`
Copy link
Contributor

Choose a reason for hiding this comment

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

@vishh It will be either CgroupsPath or Resources, right? We should either clarify that in the spec or have a layer of indirection.

Copy link
Contributor

Choose a reason for hiding this comment

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

i agree that it should either be path or resources as it doesnt really make sense ( or even wrong ) to join with existing paths and then modify those resources.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mrunalp: How about creating the cgroups for the user and applying the limits expressed via Resources?
@dqminh: Why would it be wrong? AFAIK the Spec doesn't try to limit how the existing Linux functionalities can be consumed right?

Copy link
Contributor

Choose a reason for hiding this comment

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

@vishh There could be two interpretations. One is simply joining the cgroups (that were configured by some other tool or by another container) and other is using that path and then going in and modifying the values under that path. The first is necessary for us to support exec like functionality. Does that work for your use cases?

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm actually i withdraw my comment about enforcing the limitation of cgroupPaths vs resources. I think there can be usecases where having that capabilities is helpful ( like launching an agent container that modifies the cgroup resources at boot time and/or during the agent runtime maybe ? ).

I think that clarifying the order of resources modification is good enough i.e, if cgroups path is specified then we join the paths, otherwise we create new paths, and then apply the resources limit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mrunalp: Your reasoning is sound! I see use cases for both exec and resource updates. Would making Resources optional resolve the issue?

Copy link
Contributor

Choose a reason for hiding this comment

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

On Tue, Sep 08, 2015 at 03:23:20PM -0700, Daniel, Dao Quang Minh wrote:

I think that clarifying the order of resources modification is good
enough i.e, if cgroups path is specified then we join the paths,
otherwise we create new paths, and then apply the resources limit.

See also comments in #155 and #158, where we're having the same
discussion about namespaces.

Copy link
Contributor

Choose a reason for hiding this comment

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

@mrunalp: Your reasoning is sound! I see use cases for both exec and resource updates. Would making Resources optional resolve the issue?

@vishh Sounds good to me!

// Namespaces contains the namespaces that are created and/or joined by the container
Namespaces []Namespace `json:"namespaces"`
// Devices are a list of device nodes that are created and enabled for the container
Expand Down