Add memory.kernelTCP support for linux by yongtang · Pull Request #37043 · moby/moby

yongtang · 2018-05-11T19:50:15Z

- What I did
This fix tries to address the issue raised in #37038 where
there were no memory.kernelTCP support for linux.

- How I did it

This fix add MemoryKernelTCP to HostConfig, and pass
the config to runtime-spec.

- How to verify it

Additional test case has been added.

- Description for the changelog

This fix fixes #37038.

Signed-off-by: Yong Tang yong.tang.github@outlook.com

AkihiroSuda · 2018-05-12T04:28:35Z

This requires bumping up API version?

AkihiroSuda · 2018-05-12T04:29:16Z

api/types/container/host_config.go

KernelMemoryTCP

AkihiroSuda · 2018-05-12T04:31:52Z

integration/container/run_linux_test.go

DaemonInfo.KernelMemoryTCPLimit?

yongtang · 2018-05-12T23:39:18Z

@AkihiroSuda Updated. Please take a look.

AkihiroSuda

Please also update https://github.com/moby/moby/blob/master/docs/api/version-history.md

codecov · 2018-05-13T14:34:11Z

Codecov Report

❗ No coverage uploaded for pull request base (master@ea3ac62). Click here to learn what that means.
The diff coverage is 28.57%.

@@            Coverage Diff            @@
##             master   #37043   +/-   ##
=========================================
  Coverage          ?   36.13%           
=========================================
  Files             ?      610           
  Lines             ?    45219           
  Branches          ?        0           
=========================================
  Hits              ?    16340           
  Misses            ?    26642           
  Partials          ?     2237

yongtang · 2018-05-14T00:01:46Z

@AkihiroSuda The PR has been updated. Please take a look.

AkihiroSuda · 2018-05-14T01:08:08Z

daemon/daemon_unix.go

!= 0 as in KernelMemory?

@AkihiroSuda The PR has been updated. Please take a look.

vdemeester

Design and Code LGTM 👼

thaJeztah

left some nits; generally this looks good to me.

I'm curious (n00b warning ahead):

how does this relate to general "memory" limits for the container? I take it this is outside of that limit? (i.e., a container with a memory limit can still exhaust the host by consuming memory for TCP)?
Would there be some "formula" we can apply so that there's an easy way to set all memory constraints in one go (in addition to per type) - similar to how we added the --cpus option for services?
I assume we want to add this option for services as well?

thaJeztah · 2018-05-14T08:43:17Z

api/types/container/host_config.go

" (in bytes)" ?

ping @yongtang

thaJeztah · 2018-05-14T08:44:05Z

api/swagger.yaml

" in bytes" ?

thaJeztah · 2018-05-14T21:28:45Z

daemon/daemon_unix.go

If we anticipate adding this to docker container update as well, we need to think how to reset this

kolyshkin · 2018-05-17T18:45:31Z

~~I guess we should add a sanity check that memory.kernelTCP < memory.kernel in case user specifies both. I have yet to check but assume that memory.kernel also limits the tcp memory.~~

Update: I was wrong, it's a separate limit, not accounted into kmem

thaJeztah · 2018-05-17T18:45:39Z

Discussing in the maintainers meeting; how this relates to sysctl memory limits for TCP; @kolyshkin mentions those are per network namespace and the configuration in this PR is per cgroup (we should look into that; also what happens if two containers join the same networking namespace and have different settings).

@kolyshkin will also look into the questions I put above (unless @yongtang has the answers 😄)

AkihiroSuda · 2018-11-24T08:58:04Z

could you rebase?

This fix tries to address the issue raised in 37038 where there were no memory.kernelTCP support for linux. This fix add MemoryKernelTCP to HostConfig, and pass the config to runtime-spec. Additional test case has been added. This fix fixes 37038. Signed-off-by: Yong Tang <yong.tang.github@outlook.com>

Signed-off-by: Yong Tang <yong.tang.github@outlook.com>

yongtang · 2018-11-26T21:06:52Z

@thaJeztah @AkihiroSuda The PR has been rebased. Please take a look and see if it is ready?

thaJeztah

LGTM

vdemeester · 2018-11-27T07:54:39Z

@yongtang some CI issues

05:57:22 The swagger spec at "api/swagger.yaml" is valid against swagger specification 2.0
05:57:24 The result of hack/generate-swagger-api.sh differs
05:57:24 
05:57:24 diff --git a/api/types/container/container_wait.go b/api/types/container/container_wait.go
05:57:24 index 9e3910a6b4..06b0f02077 100644
05:57:24 --- a/api/types/container/container_wait.go
05:57:24 +++ b/api/types/container/container_wait.go
05:57:24 @@ -7,14 +7,6 @@ package container
05:57:24  // See hack/generate-swagger-api.sh
05:57:24  // ----------------------------------------------------------------------------
05:57:24  
05:57:24 -// ContainerWaitOKBodyError container waiting error, if any
05:57:24 -// swagger:model ContainerWaitOKBodyError
05:57:24 -type ContainerWaitOKBodyError struct {
05:57:24 -
05:57:24 -	// Details of an error
05:57:24 -	Message string `json:"Message,omitempty"`
05:57:24 -}
05:57:24 -
05:57:24  // ContainerWaitOKBody OK response to ContainerWait operation
05:57:24  // swagger:model ContainerWaitOKBody
05:57:24  type ContainerWaitOKBody struct {
05:57:24 @@ -27,3 +19,11 @@ type ContainerWaitOKBody struct {
05:57:24  	// Required: true
05:57:24  	StatusCode int64 `json:"StatusCode"`
05:57:24  }
05:57:24 +
05:57:24 +// ContainerWaitOKBodyError container waiting error, if any
05:57:24 +// swagger:model ContainerWaitOKBodyError
05:57:24 +type ContainerWaitOKBodyError struct {
05:57:24 +
05:57:24 +	// Details of an error
05:57:24 +	Message string `json:"Message,omitempty"`
05:57:24 +}
05:57:24 
05:57:24 Please update api/swagger.yaml with any api changes, then 
05:57:24 run `hack/generate-swagger-api.sh`.

thaJeztah · 2018-11-27T16:04:29Z

The swagger failure is due to swagger-gen using a map, therefore randomizing the order 😞 (that's being addressed by #36714, but it stalled, so will have to be carried)

vdemeester · 2018-11-27T16:06:47Z

@thaJeztah oohhh 😱 🙀 😅

thaJeztah · 2018-11-27T21:36:02Z

Failure is now due to a known flaky test #32673

17:28:43 FAIL: docker_api_swarm_test.go:296: DockerSwarmSuite.TestAPISwarmLeaderElection
17:28:43 
17:28:43 [d4d1c0cfecaee] waiting for daemon to start
17:28:43 [d4d1c0cfecaee] daemon started
17:28:43 
17:28:43 [d17b61e9c4c74] waiting for daemon to start
17:28:43 [d17b61e9c4c74] daemon started
17:28:43 
17:28:43 [db8e51e0badfc] waiting for daemon to start
17:28:43 [db8e51e0badfc] daemon started
17:28:43 
17:28:43 [d4d1c0cfecaee] exiting daemon
17:28:43 Waiting for election to occur...
17:28:43 assertion failed: error is not nil: Error response from daemon: rpc error: code = DeadlineExceeded desc = context deadline exceeded: [d17b61e9c4c74] (*Daemon).GetNode: NodeInspectWithRaw("1dq8drt7zguegiv3g88597n9y") failed
17:28:43 waited for 20.001376311s (out of 30s)
17:28:43 [d17b61e9c4c74] exiting daemon
17:28:43 [db8e51e0badfc] exiting daemon

everything else passed, so I'll merge

yongtang requested a review from vdemeester as a code owner May 11, 2018 19:50

GordonTheTurtle added the status/0-triage label May 11, 2018

yongtang mentioned this pull request May 11, 2018

Expose API for setting runtimeSpec.linux.resources.memory.kernelTCP #37038

Closed

AkihiroSuda reviewed May 12, 2018

View reviewed changes

api/types/container/host_config.go Outdated

Copy link

Member

AkihiroSuda May 12, 2018

Choose a reason for hiding this comment

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

KernelMemoryTCP

AkihiroSuda reviewed May 12, 2018

View reviewed changes

integration/container/run_linux_test.go Outdated

Copy link

Member

AkihiroSuda May 12, 2018

Choose a reason for hiding this comment

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

DaemonInfo.KernelMemoryTCPLimit?

yongtang force-pushed the 37038-kernelTCP branch from 95513ae to 0a37945 Compare May 12, 2018 23:37

AkihiroSuda reviewed May 13, 2018

View reviewed changes

yongtang force-pushed the 37038-kernelTCP branch 3 times, most recently from 15d1a14 to 66b32fe Compare May 13, 2018 14:34

yongtang added the rebuild/windowsRS1 label May 13, 2018

GordonTheTurtle removed the rebuild/windowsRS1 label May 13, 2018

AkihiroSuda reviewed May 14, 2018

View reviewed changes

AkihiroSuda added impact/api impact/changelog labels May 14, 2018

vdemeester added status/1-design-review and removed status/0-triage labels May 14, 2018

vdemeester approved these changes May 14, 2018

View reviewed changes

yongtang force-pushed the 37038-kernelTCP branch from 66b32fe to 563d7c8 Compare May 14, 2018 16:51

yongtang added rebuild/janky labels May 14, 2018

GordonTheTurtle removed rebuild/janky labels May 14, 2018

thaJeztah requested changes May 14, 2018

View reviewed changes

yongtang added 2 commits November 26, 2018 21:03

Skip KernelMemoryTCP if version is less than 1.40

ee74cd7

Signed-off-by: Yong Tang <yong.tang.github@outlook.com>

yongtang force-pushed the 37038-kernelTCP branch from 8c674ba to ee74cd7 Compare November 26, 2018 21:06

thaJeztah approved these changes Nov 26, 2018

View reviewed changes

thaJeztah added status/4-merge and removed status/2-code-review labels Nov 26, 2018

yongtang added rebuild/janky labels Nov 27, 2018

GordonTheTurtle removed rebuild/janky labels Nov 27, 2018

yongtang added the rebuild/janky label Nov 27, 2018

GordonTheTurtle removed the rebuild/janky label Nov 27, 2018

vdemeester added status/2-code-review and removed status/4-merge labels Nov 27, 2018

thaJeztah added the rebuild/janky label Nov 27, 2018

GordonTheTurtle removed the rebuild/janky label Nov 27, 2018

thaJeztah added status/4-merge rebuild/janky and removed status/2-code-review labels Nov 27, 2018

GordonTheTurtle removed the rebuild/janky label Nov 27, 2018

thaJeztah merged commit d3e75e4 into moby:master Nov 27, 2018

yongtang deleted the 37038-kernelTCP branch November 27, 2018 21:40

This was referenced Nov 27, 2018

Minor docs touch-ups for KernelMemoryTCP support #38281

Merged

Add warning to /info if KernelMemoryTCP is not supported #38282

Merged

Conversation

yongtang commented May 11, 2018

Uh oh!

AkihiroSuda commented May 12, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yongtang commented May 12, 2018

Uh oh!

AkihiroSuda left a comment

Choose a reason for hiding this comment

Uh oh!

codecov bot commented May 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

yongtang commented May 14, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vdemeester left a comment

Choose a reason for hiding this comment

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kolyshkin commented May 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thaJeztah commented May 17, 2018

Uh oh!

AkihiroSuda commented Nov 24, 2018

Uh oh!

yongtang commented Nov 26, 2018

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

vdemeester commented Nov 27, 2018

Uh oh!

thaJeztah commented Nov 27, 2018

Uh oh!

vdemeester commented Nov 27, 2018

Uh oh!

thaJeztah commented Nov 27, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

codecov bot commented May 13, 2018 •

edited

Loading

kolyshkin commented May 17, 2018 •

edited

Loading