[Docker driver] - Adding support for sysctl and ulimit configuration #2501

samber · 2017-03-29T16:55:59Z

task "db" {
    driver = "docker"
    config {
      image: "redis:3.2"
      sysctl {
        net.core.somaxconn = "16384"
      }
      ulimit {
        nproc = "4242"
        nofile = "2048:4096"
      }
    }
}

jippi · 2017-03-30T17:23:24Z

client/driver/docker.go

@@ -142,6 +142,10 @@ type DockerDriverConfig struct {
 	PortMapRaw       []map[string]int    `mapstructure:"port_map"`           //
 	PortMap          map[string]int      `mapstructure:"-"`                  // A map of host port labels and the ports exposed on the container
 	Privileged       bool                `mapstructure:"privileged"`         // Flag to run the container in privileged mode
+	SysctlsRaw       []map[string]string `mapstructure:"sysctls"`            //


think the mapstructure here should be sysctl (singular) like all the other keys

jippi · 2017-03-30T17:23:32Z

client/driver/docker.go

@@ -142,6 +142,10 @@ type DockerDriverConfig struct {
 	PortMapRaw       []map[string]int    `mapstructure:"port_map"`           //
 	PortMap          map[string]int      `mapstructure:"-"`                  // A map of host port labels and the ports exposed on the container
 	Privileged       bool                `mapstructure:"privileged"`         // Flag to run the container in privileged mode
+	SysctlsRaw       []map[string]string `mapstructure:"sysctls"`            //
+	Sysctls          map[string]string   `mapstructure:"-"`                  // The sysctl custom configurations


singular here too(?)

jippi · 2017-03-30T17:23:47Z

client/driver/docker.go

@@ -142,6 +142,10 @@ type DockerDriverConfig struct {
 	PortMapRaw       []map[string]int    `mapstructure:"port_map"`           //
 	PortMap          map[string]int      `mapstructure:"-"`                  // A map of host port labels and the ports exposed on the container
 	Privileged       bool                `mapstructure:"privileged"`         // Flag to run the container in privileged mode
+	SysctlsRaw       []map[string]string `mapstructure:"sysctls"`            //
+	Sysctls          map[string]string   `mapstructure:"-"`                  // The sysctl custom configurations
+	UlimitsRaw       []map[string]string `mapstructure:"ulimits"`            //


jippi · 2017-03-30T17:23:53Z

client/driver/docker.go

+	SysctlsRaw       []map[string]string `mapstructure:"sysctls"`            //
+	Sysctls          map[string]string   `mapstructure:"-"`                  // The sysctl custom configurations
+	UlimitsRaw       []map[string]string `mapstructure:"ulimits"`            //
+	Ulimits          []docker.ULimit     `mapstructure:"-"`                  // The ulimit custom configurations


ashald · 2017-03-30T21:42:47Z

Is there is reason this must be docker specific? It'd be nice if was supported for all drivers (or at least exec/other containers), xref: #2352

samber · 2017-03-31T12:12:32Z

@jippi I made some fixes

CI does not pass, I think it's due to master branch

samber · 2017-04-03T08:51:04Z

rebased, CI passing

lfarnell · 2017-04-03T15:48:21Z

I would agree with @ashald on this. If you're going to do it for container's at the least, you should do it for rkt as well. Not to mention other drivers would be benefit from this.

samber · 2017-04-03T15:57:49Z

@lfarnell @ashald ok, some PRs should follow this one very soon, for other drivers

dadgar · 2017-04-03T17:21:07Z

Hey all, we are looking into adding a new stanza so this can be abstracted to all relevant drivers in 0.6.0. There won't be much activity on this PR until we have that design finalized.

nanoz · 2017-05-04T20:17:57Z

Any news on this @dadgar regarding the design of the sysctl stanza ? Not being able to configure somaxconn params and the like is a show stopper for production use

samber · 2017-05-05T08:22:40Z

If I can help in any way, just let me know ;)

nanoz · 2017-06-14T12:24:54Z

Hello @dadgar, any update on this issue? Do you now know what this generic stanza would look like? We are ready to work on a PR as soon as we have this information.

We are really looking for this feature :)

commarla · 2017-09-19T19:19:53Z

Hey,
Do you have any news on this ? Thanks.

commarla · 2017-11-13T09:43:06Z

Hey @dadgar any news on this ? Thanks

jippi · 2017-11-13T09:46:53Z

will sysctl and ulimit complain if you are trying to change a property that isn't within a cgroup?

as an example redis expect vm.overcommit_memory=1, which isn't currently implemented as a cgroup property, and thus will affect the entire host system - i would not want those to be applied outside my containers scope, and thus those should fail hard rather than being silently ignored or even applied

commarla · 2017-11-13T09:58:04Z

Hi @jippi,

no it doesn't, if I start my container in privileged mode and run sysctl vm.overcommit_memory=1. It affects the whole system and not only the container.

jippi · 2017-11-13T10:03:18Z

@commarla thats fine and expected, but what will the behaviour be if you start the container in non-privileged mode? just silently ignored?

commarla · 2017-11-13T10:08:02Z

@jippi it fails with an error

root@439a24aaa498:/data# sysctl vm.overcommit_memory=1
sysctl: setting key "vm.overcommit_memory": Read-only file system
root@439a24aaa498:/data# echo $?
255

jippi · 2017-11-13T10:21:25Z

same behaviour if you do like below?

task "redis" {
    driver = "docker"
    config {
      image: "redis:3.2"
      sysctl {
        vm.overcommit_memory = "1"
      }
    }
}

commarla · 2017-11-13T10:28:35Z

I don't understand. sysctl is not a valid field. It is the purpose of this PR.

jippi · 2017-11-13T10:32:07Z

yes, my question is, what will the behaviour be once this PR is merged in, when providing sysctl{} that can't be applied within a cgroup, or that is launched without priviledged access

commarla · 2017-11-13T10:40:46Z

Ok the result should be the same than running this command

❯ docker run --rm --sysctl="vm.overcommit_memory=1" redis:3.2
invalid argument "vm.overcommit_memory=1" for --sysctl=vm.overcommit_memory=1: sysctl 'vm.overcommit_memory=1' is not whitelisted
See 'docker run --help'.

docker only allow whitelisted sysctls options, see at the end of this page https://docs.docker.com/engine/reference/commandline/run/#configure-namespaced-kernel-parameters-sysctls-at-runtime

With a whitelisted sysctl it works:

docker run --rm --sysctl="net.core.somaxconn=16384" redis:3.2
Unable to find image 'redis:3.2' locally
3.2: Pulling from library/redis
d13d02fa248d: Pull complete
039f8341839e: Pull complete

jippi · 2017-11-13T10:41:45Z

great - couldn't be happier with this PR then :)

nanoz · 2017-11-17T07:02:25Z

How about merging this as is and create a second PR with rkt support later?
Right now, I need to maintain a prestart script and run containers as privileged because of this missing feature and Im not confortable with the level of security it implies.

ashald · 2017-11-17T13:08:55Z

We're using rkt and this feature is important for us as well so I hope we won't postpone rkt too much. :)

preetapan · 2017-11-17T23:48:59Z

Closing this in favor of #3568 where I rebased and fixed merge conflicts. That PR preserves commit history

github-actions · 2023-03-17T02:14:04Z

I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

samber force-pushed the driver-docker-sysctl-ulimit branch 4 times, most recently from 5cebee5 to 54cc9e2 Compare March 30, 2017 15:55

jippi reviewed Mar 30, 2017

View reviewed changes

samber force-pushed the driver-docker-sysctl-ulimit branch 2 times, most recently from afb7ca9 to cf248e6 Compare March 31, 2017 08:33

samber added 5 commits April 3, 2017 10:13

feat(docker driver): adds sysctls and ulimits configs

f141a10

test(docker driver): testing sysctls and ulimits

f7f02cf

💄

7851560

doc(docker driver): Adds doc for ulimit and sysctl

cdf12f5

Oops

1e9dd27

samber force-pushed the driver-docker-sysctl-ulimit branch from cf248e6 to 175646e Compare April 3, 2017 08:13

review(docker driver): sysctls -> sysctl + ulimits -> ulimit

26ffff7

samber force-pushed the driver-docker-sysctl-ulimit branch from 175646e to 26ffff7 Compare April 3, 2017 08:34

preetapan closed this Nov 17, 2017

github-actions bot locked as resolved and limited conversation to collaborators Mar 17, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Docker driver] - Adding support for sysctl and ulimit configuration #2501

[Docker driver] - Adding support for sysctl and ulimit configuration #2501

samber commented Mar 29, 2017 •

edited

Loading

jippi Mar 30, 2017

jippi Mar 30, 2017

jippi Mar 30, 2017

jippi Mar 30, 2017

ashald commented Mar 30, 2017 •

edited

Loading

samber commented Mar 31, 2017 •

edited

Loading

samber commented Apr 3, 2017

lfarnell commented Apr 3, 2017

samber commented Apr 3, 2017

dadgar commented Apr 3, 2017

nanoz commented May 4, 2017

samber commented May 5, 2017

nanoz commented Jun 14, 2017 •

edited

Loading

commarla commented Sep 19, 2017

commarla commented Nov 13, 2017

jippi commented Nov 13, 2017

commarla commented Nov 13, 2017

jippi commented Nov 13, 2017

commarla commented Nov 13, 2017

jippi commented Nov 13, 2017

commarla commented Nov 13, 2017

jippi commented Nov 13, 2017

commarla commented Nov 13, 2017 •

edited

Loading

jippi commented Nov 13, 2017

nanoz commented Nov 17, 2017

ashald commented Nov 17, 2017

preetapan commented Nov 17, 2017

github-actions bot commented Mar 17, 2023

[Docker driver] - Adding support for sysctl and ulimit configuration #2501

[Docker driver] - Adding support for sysctl and ulimit configuration #2501

Conversation

samber commented Mar 29, 2017 • edited Loading

jippi Mar 30, 2017

Choose a reason for hiding this comment

jippi Mar 30, 2017

Choose a reason for hiding this comment

jippi Mar 30, 2017

Choose a reason for hiding this comment

jippi Mar 30, 2017

Choose a reason for hiding this comment

ashald commented Mar 30, 2017 • edited Loading

samber commented Mar 31, 2017 • edited Loading

samber commented Apr 3, 2017

lfarnell commented Apr 3, 2017

samber commented Apr 3, 2017

dadgar commented Apr 3, 2017

nanoz commented May 4, 2017

samber commented May 5, 2017

nanoz commented Jun 14, 2017 • edited Loading

commarla commented Sep 19, 2017

commarla commented Nov 13, 2017

jippi commented Nov 13, 2017

commarla commented Nov 13, 2017

jippi commented Nov 13, 2017

commarla commented Nov 13, 2017

jippi commented Nov 13, 2017

commarla commented Nov 13, 2017

jippi commented Nov 13, 2017

commarla commented Nov 13, 2017 • edited Loading

jippi commented Nov 13, 2017

nanoz commented Nov 17, 2017

ashald commented Nov 17, 2017

preetapan commented Nov 17, 2017

github-actions bot commented Mar 17, 2023

samber commented Mar 29, 2017 •

edited

Loading

ashald commented Mar 30, 2017 •

edited

Loading

samber commented Mar 31, 2017 •

edited

Loading

nanoz commented Jun 14, 2017 •

edited

Loading

commarla commented Nov 13, 2017 •

edited

Loading