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

container: update real-time resources #33731

Merged
merged 1 commit into from
Nov 24, 2017
Merged

Conversation

runcom
Copy link
Member

@runcom runcom commented Jun 19, 2017

CPU real-time period and runtime are actually sent as part of an update
command via the cli:

https://github.com/docker/cli/blob/master/cli/command/container/update.go#L20-L21

This patch makes update effective because, until now, those settings
weren't actually updated in a container.

Tested in on a real-time kernel, this fixes the issue. Can't add a test
though as I don't think CI has rt kernels enabled.

Reproducer on a rt kernel:

root@xxx ~]# docker run -p 9090:9090 -itd --cpu-rt-runtime=790000
--ulimit rtprio=99 --cap-add=sys_nice mytest
[root@xxx ~]# docker exec -it awesome_edison /bin/bash
[root@451a1aa4fd46 /]# cat /sys/fs/cgroup/cpu/cpu.rt_runtime_us
790000

[root@xxx ~]# docker update --cpu-rt-runtime=810000 awesome_edison
awesome_edison
[root@xxx ~]# docker exec -it awesome_edison /bin/bash
[root@451a1aa4fd46 /]# cat /sys/fs/cgroup/cpu/cpu.rt_runtime_us
790000

[root@xxx ~]# echo 811000 >
/sys/fs/cgroup/cpu/system.slice/docker-451a1aa4fd468f3b67783c23d00e70558183456c775119484319c3f17d49e409.scope/cpu.rt_runtime_us
[root@xxx ~]# docker exec -it awesome_edison /bin/bash
[root@451a1aa4fd46 /]# cat /sys/fs/cgroup/cpu/cpu.rt_runtime_us
811000

Signed-off-by: Antonio Murdaca runcom@redhat.com

- What I did

- How I did it

- How to verify it

- Description for the changelog

- A picture of a cute animal (not mandatory but encouraged)

@runcom runcom added kind/bug Bugs are bugs. The cause may or may not be known at triage time so debugging may be needed. status/2-code-review and removed status/0-triage labels Jun 19, 2017
Copy link
Member

@cpuguy83 cpuguy83 left a comment

Choose a reason for hiding this comment

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

LGTM

@runcom
Copy link
Member Author

runcom commented Jun 19, 2017

wait, this is still missing containerd changes...

@runcom runcom changed the title container: update real-time resources [WIP] container: update real-time resources Jun 19, 2017
@runcom
Copy link
Member Author

runcom commented Jun 19, 2017

Sorry for the confusion, I had time now to create containerd/containerd#1031 - after that's merged I'll re-vendor containerd here for this PR

@runcom runcom changed the title [WIP] container: update real-time resources container: update real-time resources Jun 21, 2017
@runcom
Copy link
Member Author

runcom commented Jun 21, 2017

@mlaventure @cpuguy83 good to go now

@mlaventure
Copy link
Contributor

@runcom looks like the default spec generation needs to be updated. Process is now a pointer.

@runcom
Copy link
Member Author

runcom commented Jun 21, 2017

Hopefully fixed

@mlaventure
Copy link
Contributor

Looks like oom_score_adj is broken now:

17:33:07 ----------------------------------------------------------------------
17:33:07 FAIL: docker_cli_run_test.go:3830: DockerSuite.TestRunWithOomScoreAdj
17:33:07 
17:33:07 docker_cli_run_test.go:3837:
17:33:07     c.Fatalf("Expected oom_score_adj set to %q, got %q instead", expected, oomScoreAdj)
17:33:07 ... Error: Expected oom_score_adj set to "642", got "0" instead
17:33:07 
17:33:07 
17:33:07 ----------------------------------------------------------------------

@runcom runcom force-pushed the rt-fix-update branch 2 times, most recently from 05183be to 4b294ca Compare June 22, 2017 09:33
@runcom
Copy link
Member Author

runcom commented Jun 22, 2017

Looks like oom_score_adj is broken now:

bumped runc to take care of it I guess

@runcom
Copy link
Member Author

runcom commented Jun 22, 2017

alright, runc is still way to old wrt runtime-spec, I'm bumping it again...

@runcom
Copy link
Member Author

runcom commented Jun 22, 2017

looks like spec must be updated in containerd v0.2.x to use new runc....

@thaJeztah thaJeztah added the status/failing-ci Indicates that the PR in its current state fails the test suite label Jun 24, 2017
@mlaventure
Copy link
Contributor

ping @runcom

12:36:24 ----------------------------------------------------------------------
12:36:24 FAIL: docker_cli_run_unix_test.go:564: DockerSuite.TestRunWithBlkioWeight
12:36:24 
12:36:24 docker_cli_run_unix_test.go:569:
12:36:24     c.Assert(strings.TrimSpace(out), checker.Equals, "300")
12:36:24 ... obtained string = "500"
12:36:24 ... expected string = "300"
12:36:24 
12:36:25 
12:36:25 ----------------------------------------------------------------------

@thaJeztah
Copy link
Member

thaJeztah commented Sep 21, 2017

ping @runcom what's the status on this one? Looks like it needs a rebase at least 😅

@coolljt0725
Copy link
Contributor

ping @runcom what's the status on this pr? needs a rebase

@runcom
Copy link
Member Author

runcom commented Nov 22, 2017

looks like spec must be updated in containerd v0.2.x to use new runc....

need to take care of that but it wasn't trivial as many stuff have alerady been updated there...I'll re-work on this asap

@runcom runcom removed the status/failing-ci Indicates that the PR in its current state fails the test suite label Nov 22, 2017
Signed-off-by: Antonio Murdaca <runcom@redhat.com>
@runcom
Copy link
Member Author

runcom commented Nov 22, 2017

@coolljt0725 @thaJeztah @mlaventure updated

@coolljt0725
Copy link
Contributor

LGTM

Copy link
Member

@yongtang yongtang left a comment

Choose a reason for hiding this comment

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

LGTM

@yongtang yongtang merged commit dfe2c02 into moby:master Nov 24, 2017
@runcom runcom deleted the rt-fix-update branch November 24, 2017 07:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
impact/changelog kind/bug Bugs are bugs. The cause may or may not be known at triage time so debugging may be needed. status/2-code-review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants