Some refactoring on PidsLimit by thaJeztah · Pull Request #38793 · moby/moby

thaJeztah · 2019-02-24T15:36:53Z

Some follow-ups to #38791 ~~(first commit is #38791)~~

Normalize values for pids-limit

Don't set PidsLimit when creating a container and no limit was set (or the limit was set to "unlimited")
Don't set PidsLimit if the host does not have pids-limit support (previously "unlimited" was set).
Do not generate a warning if the host does not have pids-limit support, but pids-limit was set to unlimited (having no limit set, or the limit set to "unlimited" is equivalent, so no warning is nescessary in that case).
When updating a container, convert 0, and -1 to "unlimited" (0).

Update TestUpdatePidsLimit to be more atomic

Create a new container for each subtest, so that individual subtests are self-contained, and there's no need to execute them in the exact order, or resetting the container in between.

This makes the test slower (6.54s vs 3.43s), but reduced the difference by using network=host, which made a substantial difference (without network=host, the test took more than twice as long: 13.96s).

codecov · 2019-02-24T17:52:48Z

Codecov Report

Merging #38793 into master will decrease coverage by <.01%.
The diff coverage is 22.22%.

@@            Coverage Diff             @@
##           master   #38793      +/-   ##
==========================================
- Coverage   36.47%   36.46%   -0.01%     
==========================================
  Files         613      613              
  Lines       45814    45845      +31     
==========================================
+ Hits        16709    16716       +7     
- Misses      26823    26848      +25     
+ Partials     2282     2281       -1

thaJeztah · 2019-02-25T12:15:08Z

integration/internal/container/ops.go

Was in doubt if I should change this to a non-pointer to make it easier to use (and if no limit is needed, don't pass WithPidsLimit())

I can see both ways here. In the end I think making a pointer is simpler than dealing with filtering options... e.g.

opts := []ConfigOpts{WithFoo,WithBar} if hasLimit { opts = append(opts, WithPidsLimit(limit)) } container.Run(..., opts...)

MHBauer

needs rebase on #38791 I think?

MHBauer · 2019-03-12T19:09:53Z

integration/container/update_linux_test.go

seems like a valid choice. we're not doing anything with the network here.

MHBauer · 2019-03-12T19:11:06Z

api/server/router/container/container_routes.go

good comment

MHBauer · 2019-03-12T19:11:44Z

daemon/daemon_unix.go

nice cleanup of the comment.

- Don't set `PidsLimit` when creating a container and no limit was set (or the limit was set to "unlimited") - Don't set `PidsLimit` if the host does not have pids-limit support (previously "unlimited" was set). - Do not generate a warning if the host does not have pids-limit support, but pids-limit was set to unlimited (having no limit set, or the limit set to "unlimited" is equivalent, so no warning is nescessary in that case). - When updating a container, convert `0`, and `-1` to "unlimited" (`0`). Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

Create a new container for each subtest, so that individual subtests are self-contained, and there's no need to execute them in the exact order, or resetting the container in between. This makes the test slower (6.54s vs 3.43s), but reduced the difference by using `network=host`, which made a substantial difference (without `network=host`, the test took more than twice as long: 13.96s). Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

thaJeztah · 2019-03-12T23:28:57Z

rebased; PTAL

cpuguy83

Oy, the more I look at this the more I'm worried about the change we made to support updates for pids limit.

This change LGTM, though.

cpuguy83 · 2019-03-13T22:34:02Z

integration/internal/container/ops.go

+// WithPidsLimit sets the container's "pids-limit
+func WithPidsLimit(limit *int64) func(*TestContainerConfig) {
+	return func(c *TestContainerConfig) {
+		if c.HostConfig == nil {


nit: I don't think this is needed at this point.

hm.. possibly; I might have run into a situation where it was nil, but perhaps that was when I was working on the test. Don't think it hurts to have it 😅

thaJeztah · 2019-03-13T22:59:33Z

Oy, the more I look at this the more I'm worried about the change we made to support updates for pids limit.

I was trying to make the flow more consistent (some attempts in #38676). I think one of the problems we have currently is that in many cases, the (Host-)Config we store on disk is reflecting the JSON we sent over the API. Perhaps this was the design, but it means that API conventions trickle down all the way into the daemon, so when converting that config to an OCI spec, we have to keep the API conventions ("over the wire format") into account.

I've been following some of the code-paths, and validation of values is interweaved throughout the codebase; here's some notes I was taking;

-> postContainersCreate
  -> containerRouter.decoder.DecodeConfig
    -> runconfig.decodeContainerConfig
      -> ContainerConfigWrapper.getHostConfig (modifies config for backward-compatibility)
      -> validateNetMode
        -> validateNetContainerMode
      -> validateIsolation
      -> validateQoS
      -> validateResources (checks if kernel supports requested options)
      -> validatePrivileged (platform-specific validation; error on Windows for privileged)
      -> validateReadonlyRootfs (platform-specific validation; error on Windows as it's not supported)
  -> do some manipulation of values related to API versions
  -> backend.ContainerCreate
    -> daemon.containerCreate
      -> daemon.adaptContainerSettings (updates passed configuration, also does some validation)
      -> daemon.validateContainerConfig
        -> translateWorkingDir (modifies working-dir based on platform)
      -> daemon.validateContainerHostConfig
        -> validateHostConfig
        -> verifyPlatformContainerSettings
      -> daemon.verifyNetworkingConfig

I think what would be nice if we would handle the API "conversion" in the API code (where possible), so that the config as stored on disk could more easily be converted to an OCI-spec without having to handle those conversions (again), and an "update" would be more of a "reconciliation". Validation might become easier then as well.

Probably that would be problematic for upgrade scenarios (configs created by older versions of the daemon), and there's likely many things I didn't consider.

Also; I keep looking at our tests, and thinking that the client we use in our tests has a much nicer interface than the API client (with the functional arguments) (disclaimer: I've become a bit of a fan of those 😅)

MHBauer

I thought I had decoupled the API, but it looks like it's back to sharing structs.

LGTM

GordonTheTurtle added the status/0-triage label Feb 24, 2019

thaJeztah requested a review from cpuguy83 February 24, 2019 15:36

thaJeztah added status/2-code-review and removed status/0-triage labels Feb 24, 2019

thaJeztah commented Feb 25, 2019

View reviewed changes

thaJeztah mentioned this pull request Feb 27, 2019

Adding support for memory swap settings for services #37872

Closed

GordonTheTurtle assigned MHBauer Mar 11, 2019

MHBauer reviewed Mar 12, 2019

View reviewed changes

MHBauer self-requested a review March 12, 2019 19:13

thaJeztah added 2 commits March 13, 2019 00:27

thaJeztah force-pushed the pids_limit_improvements branch from 379f150 to 1101568 Compare March 12, 2019 23:27

thaJeztah added the rebuild/janky label Mar 13, 2019

GordonTheTurtle removed the rebuild/janky label Mar 13, 2019

cpuguy83 approved these changes Mar 13, 2019

View reviewed changes

MHBauer approved these changes Mar 14, 2019

View reviewed changes

tonistiigi merged commit 25661a3 into moby:master Mar 21, 2019

thaJeztah deleted the pids_limit_improvements branch March 21, 2019 23:47

olljanat mentioned this pull request May 7, 2019

Add capabilities list to container specification moby/swarmkit#2795

Merged

Conversation

thaJeztah commented Feb 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Normalize values for pids-limit

Update TestUpdatePidsLimit to be more atomic

Uh oh!

codecov bot commented Feb 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

thaJeztah Feb 25, 2019

Choose a reason for hiding this comment

Uh oh!

cpuguy83 Feb 25, 2019

Choose a reason for hiding this comment

Uh oh!

MHBauer left a comment

Choose a reason for hiding this comment

Uh oh!

MHBauer Mar 12, 2019

Choose a reason for hiding this comment

Uh oh!

MHBauer Mar 12, 2019

Choose a reason for hiding this comment

Uh oh!

MHBauer Mar 12, 2019

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Mar 12, 2019

Uh oh!

cpuguy83 left a comment

Choose a reason for hiding this comment

Uh oh!

cpuguy83 Mar 13, 2019

Choose a reason for hiding this comment

Uh oh!

thaJeztah Mar 13, 2019

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Mar 13, 2019

Uh oh!

MHBauer left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

thaJeztah commented Feb 24, 2019 •

edited

Loading

codecov bot commented Feb 24, 2019 •

edited

Loading