Conversation
ctelfer
left a comment
ctelfer
left a comment
There was a problem hiding this comment.
LGTM FWIW
Obviously swarm doesn't care much about this ... just needs to carry the information.
api/specs.proto
Outdated
| // Sysctls sets namespaced kernel parameters (sysctls) in the container. This | ||
| // option is equivalent to passing --sysctl to docker run. | ||
| // | ||
| // Note that while options are are subject to the same restrictions as |
thaJeztah
left a comment
thaJeztah
left a comment
There was a problem hiding this comment.
LGTM (perhaps after @anshulpundir's nit was addressed, but not a blocker for me)
| i++ | ||
| i = encodeVarintSpecs(dAtA, i, uint64(m.PidsLimit)) | ||
| } | ||
| if len(m.Sysctls) > 0 { |
There was a problem hiding this comment.
this check looks redundant; the code inside the for loop won't be executed if it's empty; https://play.golang.org/p/SkHL_G_iJtv
edit: nevermind. Didn't notice I was looking at generated code ("Code generated by protoc-gen-gogo. DO NOT EDIT.") 😊
| if m.PidsLimit != 0 { | ||
| n += 2 + sovSpecs(uint64(m.PidsLimit)) | ||
| } | ||
| if len(m.Sysctls) > 0 { |
There was a problem hiding this comment.
Same here; this check looks redundant
edit: nevermind. Didn't notice I was looking at generated code ("Code generated by protoc-gen-gogo. DO NOT EDIT.") 😊
Adds support for sysctl options to the container spec. This is equivalent to the --sysctl flag on `docker run`. Only API changes are required for swarmkit. All of the other changes involving plumbing through these options happens downstream in the engine. Signed-off-by: Drew Erny <[email protected]>
dperny
force-pushed
the
add-sysctls-support
branch
from
0e685ac to
ae22e33
Compare
|
Removed one of two consecutive "are"s. |
Codecov Report
@@ Coverage Diff @@
## master #2729 +/- ##
==========================================
+ Coverage 61.71% 61.72% +<.01%
==========================================
Files 134 134
Lines 21888 21888
==========================================
+ Hits 13508 13510 +2
+ Misses 6916 6912 -4
- Partials 1464 1466 +2 |
| Isolation isolation = 24; | ||
|
|
||
| // PidsLimit prevents from OS resource damage by applications inside the container | ||
| // PidsLimit prevents from OS resource damage by applications inside the container |
There was a problem hiding this comment.
supernit: remove 'from'
|
I see this was merged into master. What version of I'm currently running docker version |
|
@thaJeztah The #2729 was pushed to SwarmKit on Aug 25. SwarmKit hasn't been updated in docker-ce since Aug 4th. You did the bump. This would be an important missing feature to add to docker-ce 18.9.0 while it's still in beta. Is this a possibility? |
This also brings in these PRs from swarmkit: - moby/swarmkit#2691 - moby/swarmkit#2744 - moby/swarmkit#2732 - moby/swarmkit#2729 - moby/swarmkit#2748 Signed-off-by: Tibor Vass <[email protected]>
This also brings in these PRs from swarmkit: - moby/swarmkit#2691 - moby/swarmkit#2744 - moby/swarmkit#2732 - moby/swarmkit#2729 - moby/swarmkit#2748 Signed-off-by: Tibor Vass <[email protected]> Upstream-commit: cce1763d57b5c8fc446b0863517bb5313e7e53be Component: engine
|
Support for swarm and sysctls is available in 19.03 RC2 |
7 participants
- What I did
Adds support for sysctl options to the container spec. This is equivalent to the --sysctl flag on
docker run.- How I did it
Added a field to the protocol buffer for sysctl options.
Only API changes are required for swarmkit. All of the other changes involving plumbing through these options happens downstream in the engine.
- How to test it
N/A, we don't even use this field directly in swarmkit.
- Description for the changelog
Added support for sysctls in services. This is equivalent to the
--sysctlflag ondocker run.