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.

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

config-linux: make interface name clear #713

Merged

Conversation

Mashimiao
Copy link

Signed-off-by: Ma Shimiao [email protected]

@crosbymichael
Copy link
Member

Are you sure this is correct? There are an awful lot of eth0 when you run containers.

@Mashimiao
Copy link
Author

cgroups works on host, you can't run a container with limiting an interface which not exists on host., cgroups will not accept that. For example by runc:
$ cat ~/testdir/config.json |grep -A 5 -B 5 eth0
},
"network": {
"classID": 1048577,
"priorities": [
{
"name": "eth0",
"priority": 500
}
]
}
},

$ sudo runc run --bundle ~/testdir test
container_linux.go:247: starting container process caused "process_linux.go:386: container init caused "process_linux.go:327: setting cgroup config for ready process caused \"failed to write eth0 500 to net_prio.ifpriomap: write /sys/fs/cgroup/net_cls,net_prio/test/net_prio.ifpriomap: no such device\"""

@cyphar
Copy link
Member

cyphar commented Mar 9, 2017

While I'm sure this is a correct statement, this is IMO another instance where we shouldn't be adding wording around Linux kernel implementation details. If someone wants to put a random value in a field they'd better understand what they're actually doing (in terms of the kernel features they're using) otherwise things would break no matter how much guidance we give.

From my PoV I see no reason why this restriction will always be true in the kernel -- how is the case handled where you have a file descriptor set up in a different network namespace (for instance)? if the kernel changes some behaviour (or they extend it) then we'd have to update the spec.

@Mashimiao
Copy link
Author

I think if we don't make it clear, people may be confused interface name should be which in container network namespace or in runtime network namespace.
At least, it seems @crosbymichael is thinking about interface name is which in container network namespace.

@crosbymichael
Copy link
Member

I think it mis-read it the first time sorry. It does seem correct. I'm indifferent about adding this line, I think its fine leaving it out like @cyphar said but its also fine adding it as the additional is pretty straight forward.

@Mashimiao
Copy link
Author

ping @opencontainers/runtime-spec-maintainers

@hqhq
Copy link
Contributor

hqhq commented May 10, 2017

LGTM

Approved with PullApprove

@@ -438,7 +438,7 @@ The following parameters can be specified to setup the controller:

* **`priorities`** *(array, OPTIONAL)* - specifies a list of objects of the priorities assigned to traffic originating from processes in the group and egressing the system on various interfaces.
The following parameters can be specified per-priority:
* **`name`** *(string, REQUIRED)* - interface name
* **`name`** *(string, REQUIRED)* - interface name in [runtime network namespace](glossary.md#runtime-namespace)
Copy link
Member

Choose a reason for hiding this comment

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

It appears the rest of this file is using footer links ([link text][link ID] with then a [link ID]: URL at the bottom of the file such as [container-namespace2]: glossary.md#container_namespace); should we stay consistent on that?

Copy link
Contributor

@wking wking May 10, 2017

Choose a reason for hiding this comment

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

It appears the rest of this file is using footer links…

I think those just slipped through #687 and have filed #799 to catch them up. @Mashimiao's current inline links match the current style for internal references.

@tianon tianon merged commit cd92a0e into opencontainers:master May 10, 2017
@vbatts vbatts mentioned this pull request Jul 5, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants