-
Notifications
You must be signed in to change notification settings - Fork 620
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add sysctls support #2729
Add sysctls support #2729
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove one 'are'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM (perhaps after @anshulpundir's nit was addressed, but not a blocker for me)
@@ -2064,6 +2085,25 @@ func (m *ContainerSpec) MarshalTo(dAtA []byte) (int, error) { | |||
i++ | |||
i = encodeVarintSpecs(dAtA, i, uint64(m.PidsLimit)) | |||
} | |||
if len(m.Sysctls) > 0 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.") 😊
@@ -2781,6 +2821,14 @@ func (m *ContainerSpec) Size() (n int) { | |||
if m.PidsLimit != 0 { | |||
n += 2 + sovSpecs(uint64(m.PidsLimit)) | |||
} | |||
if len(m.Sysctls) > 0 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 <drew.erny@docker.com>
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 |
@@ -315,9 +315,24 @@ message ContainerSpec { | |||
// Runtimes that don't support it ignore that field | |||
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 <tibor@docker.com>
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 <tibor@docker.com> Upstream-commit: cce1763d57b5c8fc446b0863517bb5313e7e53be Component: engine
Support for swarm and sysctls is available in 19.03 RC2 |
- 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
--sysctl
flag ondocker run
.