Skip to content

Do not enable kmem on RHEL7 kernels#38128

Merged
thaJeztah merged 2 commits intomoby:masterfrom
kolyshkin:runc
Nov 12, 2018
Merged

Do not enable kmem on RHEL7 kernels#38128
thaJeztah merged 2 commits intomoby:masterfrom
kolyshkin:runc

Conversation

@kolyshkin
Copy link
Copy Markdown
Contributor

@kolyshkin kolyshkin commented Nov 1, 2018 •
edited
Loading

1. bump runc

Changes: opencontainers/runc@a00bf01...9f1e944

This is primarily to pull in opencontainers/runc#1921

2. runc.installer: add nokmem build tag for rhel7 kernel

In case we're running on RHEL7 kernel, which has non-working
and broken kernel memory controller, add 'nokmem' build tag
so that runc never enables kmem accounting.

For more info, see the following runc commit:
opencontainers/runc@6a2c1559684

This behavior can be overriden by having RUNC_NOKMEM environment
variable set (e.g. to empty value to disable setting nokmem).

@kolyshkin kolyshkin requested a review from tianon as a code owner November 1, 2018 23:10
@kolyshkin
Copy link
Copy Markdown
Contributor Author

kolyshkin commented Nov 2, 2018

CI failure in janky (probably unrelated but I haven't seen it before)

00:07:10.767 time="2018-11-01T23:17:43Z" level=info msg="Trying to get region from EC2 Metadata"
00:07:10.767 time="2018-11-01T23:17:43Z" level=info msg="Log stream already exists" errorCode=ResourceAlreadyExistsException logGroupName= logStreamName= message= origError="<nil>"
00:07:10.768 --- FAIL: TestLogBlocking (0.00s)
00:07:10.768     cloudwatchlogs_test.go:255: Expected to be able to read from stream.messages but was unable to
00:07:10.768 time="2018-11-01T23:17:43Z" level=error msg=Error
00:07:10.768 time="2018-11-01T23:17:43Z" level=error msg="Failed to put log events" errorCode=InvalidSequenceTokenException logGroupName=groupName logStreamName=streamName message="use token token" origError="<nil>"
00:07:10.768 time="2018-11-01T23:17:43Z" level=error msg="Failed to put log events" errorCode=DataAlreadyAcceptedException logGroupName=groupName logStreamName=streamName message="use token token" origError="<nil>"
00:07:10.769 time="2018-11-01T23:17:43Z" level=info msg="Data already accepted, ignoring error" errorCode=DataAlreadyAcceptedException logGroupName=groupName logStreamName=streamName message="use token token"
00:07:10.769 FAIL
00:07:10.769 coverage: 71.7% of statements
00:07:10.769 FAIL	github.com/docker/docker/daemon/logger/awslogs	0.113s
00:07:11.627 Build step 'Execute shell' marked build as failure

vdemeester
vdemeester approved these changes Nov 2, 2018
View reviewed changes
Copy link
Copy Markdown
Member

@vdemeester vdemeester left a comment

Choose a reason for hiding this comment

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

LGTM 🐯

@thaJeztah
Copy link
Copy Markdown
Member

thaJeztah commented Nov 2, 2018 •
edited
Loading

Should we update the Makefile and Dockerfile to set the correct build-flag for the 3.10 kernels?

@kolyshkin
Copy link
Copy Markdown
Contributor Author

kolyshkin commented Nov 6, 2018

Should we update the Makefile and Dockerfile to set the correct build-flag for the 3.10 kernels?

I think this belongs to packaging -- specifically, when we create packages for RHEL7. I was looking at it (in https://github.com/docker/docker-ce-packaging) but got lost :(

Sure we can do it here instead.

@thaJeztah
Copy link
Copy Markdown
Member

thaJeztah commented Nov 6, 2018 •
edited
Loading

I think this belongs to packaging -- specifically, when we create packages for RHEL7.

Yes, definitely also is needed in packaging. My train of thought here was that;

  • we will be testing (running CI) on RHEL/CentOS, so if we do, it should be compiled with the right options
  • people will be developing on RHEL/CentOS, so for those, the option should be set correctly as well
  • we can make it a variable that can be overridden / set on make

@kolyshkin
Bump runc
335736f 
Changes: opencontainers/runc@a00bf01...9f1e944

Signed-off-by: Kir Kolyshkin <[email protected]>
@kolyshkin
runc.installer: add nokmem build tag for rhel7 kernel
8972aa9 
In case we're running on RHEL7 kernel, which has non-working
and broken kernel memory controller, add 'nokmem' build tag
so that runc never enables kmem accounting.

For more info, see the following runc commit:
opencontainers/runc@6a2c1559684

This behavior can be overriden by having `RUNC_NOKMEM` environment
variable set (e.g. to empty value to disable setting nokmem).

Signed-off-by: Kir Kolyshkin <[email protected]>
@kolyshkin kolyshkin changed the title Bump runc Do not enable kmem on RHEL7 kernels Nov 6, 2018
@codecov
Copy link
Copy Markdown

codecov Bot commented Nov 6, 2018

Codecov Report

❗ No coverage uploaded for pull request base (master@d022271). Click here to learn what that means.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master   #38128   +/-   ##
=========================================
  Coverage          ?   36.11%           
=========================================
  Files             ?      610           
  Lines             ?    45216           
  Branches          ?        0           
=========================================
  Hits              ?    16331           
  Misses            ?    26646           
  Partials          ?     2239

@kolyshkin
Copy link
Copy Markdown
Contributor Author

kolyshkin commented Nov 6, 2018

@thaJeztah makes sense. Please see added commit.

thaJeztah
thaJeztah approved these changes Nov 6, 2018
View reviewed changes
Copy link
Copy Markdown
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

SGTM

ping @tianon PTAL ❤️

@thaJeztah
Copy link
Copy Markdown
Member

thaJeztah commented Nov 12, 2018

Let's merge this; looks good to me, but @tianon if there's anything to improve/address in the installer script, let me know 🤗

@thaJeztah thaJeztah merged commit a5e2dd2 into moby:master Nov 12, 2018
This was referenced Nov 12, 2018
Closed
Closed
Closed
Merged
@thaJeztah thaJeztah mentioned this pull request Nov 29, 2018
Merged
@thaJeztah thaJeztah mentioned this pull request Jan 4, 2019
Merged
richardwhiuk added a commit to Metaswitch/docker-ce-packaging that referenced this pull request Mar 5, 2019
@richardwhiuk
Disable Kernel Memory Accounting on CentOS 7
50d91db 
This applies the fix developed in moby/moby#38128 to CentOS 7 RPMs, which are currently built without the correct flag.

This avoids kernel memory being leaked as described in https://bugzilla.redhat.com/show_bug.cgi?id=1507149
@richardwhiuk richardwhiuk mentioned this pull request Mar 5, 2019
Closed
@kolyshkin kolyshkin mentioned this pull request Jun 7, 2019
Closed
@thaJeztah thaJeztah mentioned this pull request Dec 9, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@thaJeztah thaJeztah thaJeztah approved these changes

@AkihiroSuda AkihiroSuda AkihiroSuda approved these changes

@tianon tianon Awaiting requested review from tianon tianon is a code owner

+1 more reviewer

@vdemeester vdemeester vdemeester approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

area/runtime Runtime status/2-code-review

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@kolyshkin @thaJeztah @vdemeester @AkihiroSuda @GordonTheTurtle