Skip to content

container: update real-time resources#33731

Merged
yongtang merged 1 commit into
moby:masterfrom
runcom:rt-fix-update
Nov 24, 2017
Merged

container: update real-time resources#33731
yongtang merged 1 commit into
moby:masterfrom
runcom:rt-fix-update

Conversation

@runcom
Copy link
Copy Markdown
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 [email protected]

- 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
cpuguy83
cpuguy83 approved these changes Jun 19, 2017
View reviewed changes
Copy link
Copy Markdown
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
Copy Markdown
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 runcom mentioned this pull request Jun 19, 2017
Merged
@runcom
Copy link
Copy Markdown
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
Copy Markdown
Member Author

runcom commented Jun 21, 2017

@mlaventure @cpuguy83 good to go now

@mlaventure
Copy link
Copy Markdown
Contributor

mlaventure commented Jun 21, 2017

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

@runcom
Copy link
Copy Markdown
Member Author

runcom commented Jun 21, 2017

Hopefully fixed

@mlaventure
Copy link
Copy Markdown
Contributor

mlaventure commented Jun 21, 2017

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor

mlaventure commented Jul 4, 2017

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
Copy Markdown
Member

thaJeztah commented Sep 21, 2017 •
edited
Loading

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

@coolljt0725
Copy link
Copy Markdown
Contributor

coolljt0725 commented Nov 22, 2017

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

@runcom
Copy link
Copy Markdown
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
@runcom
Copy link
Copy Markdown
Member Author

runcom commented Nov 22, 2017

@coolljt0725 @thaJeztah @mlaventure updated

@coolljt0725
Copy link
Copy Markdown
Contributor

coolljt0725 commented Nov 23, 2017

LGTM

yongtang
yongtang approved these changes Nov 24, 2017
View reviewed changes
Copy link
Copy Markdown
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

Reviewers

@cpuguy83 cpuguy83 cpuguy83 approved these changes

+1 more reviewer

@yongtang yongtang yongtang approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@coolljt0725 coolljt0725

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

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@runcom @mlaventure @thaJeztah @coolljt0725 @cpuguy83 @yongtang @GordonTheTurtle