Adding support for swap settings by wk8 · Pull Request #2747 · moby/swarmkit

wk8 · 2018-09-12T23:43:18Z

- What I did

API change to support memory-swap and memory-swappiness settings on services

- How I did it

Added the two new fields to the protobuf definition

- How to test it

N/A, this is not used in swarkit. Follow-up PR to come on moby/moby

- Description for the changelog

Added support for tuning swap in services. This is equivalent to docker run's --memory-swap and --memory-swappiness flags.

GordonTheTurtle · 2018-09-12T23:43:19Z

Please sign your commits following these rules:
https://github.com/moby/moby/blob/master/CONTRIBUTING.md#sign-your-work
The easiest way to do this is to amend the last commit:

$ git clone -b "wk8/memory_flags" git@github.com:wk8/swarmkit.git somewhere
$ cd somewhere
$ git rebase -i HEAD~842354427968
editor opens
change each 'pick' to 'edit'
save the file and quit
$ git commit --amend -s --no-edit
$ git rebase --continue # and repeat the amend for each commit
$ git push -f

Amending updates the existing PR. You DO NOT need to open a new one.

codecov · 2018-09-12T23:54:36Z

Codecov Report

Merging #2747 into master will increase coverage by 0.14%.
The diff coverage is n/a.

@@            Coverage Diff            @@
##           master   #2747      +/-   ##
=========================================
+ Coverage   61.75%   61.9%   +0.14%     
=========================================
  Files         134     134              
  Lines       21890   21890              
=========================================
+ Hits        13518   13550      +32     
+ Misses       6905    6884      -21     
+ Partials     1467    1456      -11

anshulpundir · 2018-09-13T00:05:15Z

+	int64 memory_swap = 27;
+
+	// Tune container memory swappiness (0 to 100) (default -1)
+	google.protobuf.Int64Value memory_swappiness = 28;


why 0 to 100? nit: maybe add a comment?

also, curious why Int64Value instead of int64?

Lazily copied from docker run's help message: --memory-swappiness int Tune container memory swappiness (0 to 100) (default -1) :)

As to Int64Value, stayed consistent with https://github.com/moby/moby/blob/03e089e169a2889f096aceac6ad4a4e2ed6e186b/api/types/container/host_config.go#L333-L334 .

what's the difference between google.protobuf.Int64Value and the int64 above it? Does it make this compile to *int64 in practice?

at first, i was 🤔 🤔 🤔 about google.protobuf.Int64Value, but now I see that there is a pretty substantial semantic difference between set and not set for this value, and Int64Value is probably cleaner than a pointer, even if we can hack a pointer into the compiled proto (which we can, I think).

LGTM.

And even if Int64Value was a pain in the butt, it's not like we do anything with it other than pass it down into the docker engine.

@anshulpundir "what does (default -1) mean? Where is the default set?" => I guess somewhere deeper in moby?

Does it make sense to specify that default value here then? What does the default value mean mean to the API consumer?

anshulpundir · 2018-09-13T23:28:47Z

 	map<string, string> sysctls = 26;
+
+	// Swap limit equal to memory plus swap: '-1' to enable unlimited swap
+	int64 memory_swap = 27;


why do we not need google.protobuf.Int64Value for this field?

Because that value only needs to be an int64, not a *int64, https://github.com/moby/moby/blob/03e089e169a2889f096aceac6ad4a4e2ed6e186b/api/types/container/host_config.go#L333-L334

right. if I don't want to change previously set memory swap value, how do I specify that?
Basically, why does memory_swappiness need to be nullable but memory_swap does not?

"if I don't want to change previously set memory swap value, how do I specify that?" => I think that's the same for all the params here:

moby's API does expect a full service spec for both creates and updates, you can't just PATCH an existing service

if your services are created/updated using docker-compose.yml file, then you have all the params in there, and they will all be around when updating

if you use the CLI, then it's responsible for fetching the current service spec and merging with the new params (https://github.com/docker/cli/blob/master/cli/command/service/update.go#L239-L482); arguably it would be nice to have the API handle that eventually?

So in particular, memory_swappiness doesn't need to be nullable... It just seems good to be consistent with moby's container spec struct?

ok cool lets move forward as is for consistency.

Signed-off-by: Jean Rouge <jer329@cornell.edu>

anshulpundir · 2018-09-21T21:11:45Z

LGMT!

With integration tests Relevant Swarmkit PR: moby/swarmkit#2747 (updated the vendored version of Swarkit to that) Signed-off-by: Jean Rouge <jer329@cornell.edu>

GordonTheTurtle added the dco/no label Sep 12, 2018

wk8 force-pushed the wk8/memory_flags branch from 98af3e7 to 5bd4de6 Compare September 12, 2018 23:44

GordonTheTurtle added the dco/no label Sep 12, 2018

wk8 force-pushed the wk8/memory_flags branch from 5bd4de6 to dac1258 Compare September 12, 2018 23:44

GordonTheTurtle removed the dco/no label Sep 12, 2018

wk8 changed the title ~~Wk8/memory flags~~ Adding support for swap settings Sep 12, 2018

anshulpundir approved these changes Sep 13, 2018

View reviewed changes

anshulpundir reviewed Sep 13, 2018

View reviewed changes

wk8 force-pushed the wk8/memory_flags branch from dac1258 to 53a974e Compare September 17, 2018 20:24

GordonTheTurtle added the dco/no label Sep 17, 2018

wk8 force-pushed the wk8/memory_flags branch from 6f816f3 to 9c1eb4e Compare September 17, 2018 20:26

GordonTheTurtle removed the dco/no label Sep 17, 2018

wk8 force-pushed the wk8/memory_flags branch 3 times, most recently from 1fb3a3a to bc0eadf Compare September 17, 2018 22:53

GordonTheTurtle added the dco/no label Sep 20, 2018

Adding two new settings to the services' spec API

78d9068

Signed-off-by: Jean Rouge <jer329@cornell.edu>

wk8 force-pushed the wk8/memory_flags branch from 2ebd05c to 78d9068 Compare September 21, 2018 20:52

GordonTheTurtle removed the dco/no label Sep 21, 2018

dperny merged commit 7da74a7 into moby:master Sep 22, 2018

thaJeztah mentioned this pull request Oct 18, 2018

WIP [18.09 backport] Bump swarmkit to a84c01f docker-archive/engine#89

Closed

Conversation

wk8 commented Sep 12, 2018

Uh oh!

GordonTheTurtle commented Sep 12, 2018

Uh oh!

codecov Bot commented Sep 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

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!

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!

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!

Choose a reason for hiding this comment

Uh oh!

anshulpundir commented Sep 21, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov Bot commented Sep 12, 2018 •

edited

Loading