-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Rebase and merge of docker sysctl support #3568
Changes from 8 commits
84a0894
cf58699
d15f01d
db2b09e
657619c
e6c0372
48829e8
9e07471
bc08d31
0d47977
ee4b4d8
d522149
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -176,6 +176,10 @@ type DockerDriverConfig struct { | |
PortMapRaw []map[string]string `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 | ||
SysctlRaw []map[string]string `mapstructure:"sysctl"` // | ||
Sysctl map[string]string `mapstructure:"-"` // The sysctl custom configurations | ||
UlimitRaw []map[string]string `mapstructure:"ulimit"` // | ||
Ulimit []docker.ULimit `mapstructure:"-"` // The ulimit custom configurations | ||
DNSServers []string `mapstructure:"dns_servers"` // DNS Server for containers | ||
DNSSearchDomains []string `mapstructure:"dns_search_domains"` // DNS Search domains for containers | ||
DNSOptions []string `mapstructure:"dns_options"` // DNS Options | ||
|
@@ -199,6 +203,35 @@ type DockerDriverConfig struct { | |
Devices []DockerDevice `mapstructure:"devices"` // To allow mounting USB or other serial control devices | ||
} | ||
|
||
func sliceMergeUlimit(ulimitsRaw map[string]string) ([]docker.ULimit, error) { | ||
var ulimits []docker.ULimit | ||
|
||
for name, ulimitRaw := range ulimitsRaw { | ||
// hard limit is optional | ||
if strings.Contains(ulimitRaw, ":") == false { | ||
ulimitRaw = ulimitRaw + ":" + ulimitRaw | ||
} | ||
|
||
splitted := strings.SplitN(ulimitRaw, ":", 2) | ||
soft, err := strconv.Atoi(splitted[0]) | ||
if err != nil { | ||
return []docker.ULimit{}, fmt.Errorf("Malformed ulimit %v: %v", name, ulimitRaw) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. maybe include that it's the soft limit that is invalid? - same below for hard limit There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. agreed with the limit specification for logging. |
||
} | ||
hard, err := strconv.Atoi(splitted[1]) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Check limit before indexing. |
||
if err != nil { | ||
return []docker.ULimit{}, fmt.Errorf("Malformed ulimit %v: %v", name, ulimitRaw) | ||
} | ||
|
||
ulimit := docker.ULimit{ | ||
Name: name, | ||
Soft: int64(soft), | ||
Hard: int64(hard), | ||
} | ||
ulimits = append(ulimits, ulimit) | ||
} | ||
return ulimits, nil | ||
} | ||
|
||
// Validate validates a docker driver config | ||
func (c *DockerDriverConfig) Validate() error { | ||
if c.ImageName == "" { | ||
|
@@ -209,7 +242,6 @@ func (c *DockerDriverConfig) Validate() error { | |
if dev.HostPath == "" { | ||
return fmt.Errorf("host path must be set in configuration for devices") | ||
} | ||
|
||
if dev.CgroupPermissions != "" { | ||
for _, c := range dev.CgroupPermissions { | ||
ch := string(c) | ||
|
@@ -220,6 +252,18 @@ func (c *DockerDriverConfig) Validate() error { | |
} | ||
} | ||
} | ||
c.Sysctl = mapMergeStrStr(c.SysctlRaw...) | ||
c.Labels = mapMergeStrStr(c.LabelsRaw...) | ||
if len(c.Logging) > 0 { | ||
c.Logging[0].Config = mapMergeStrStr(c.Logging[0].ConfigRaw...) | ||
} | ||
|
||
mergedUlimitsRaw := mapMergeStrStr(c.UlimitRaw...) | ||
ulimit, err := sliceMergeUlimit(mergedUlimitsRaw) | ||
if err != nil { | ||
return err | ||
} | ||
c.Ulimit = ulimit | ||
return nil | ||
} | ||
|
||
|
@@ -254,6 +298,20 @@ func NewDockerDriverConfig(task *structs.Task, env *env.TaskEnv) (*DockerDriverC | |
dconf.MacAddress = env.ReplaceEnv(dconf.MacAddress) | ||
dconf.SecurityOpt = env.ParseAndReplace(dconf.SecurityOpt) | ||
|
||
for _, m := range dconf.SysctlRaw { | ||
for k, v := range m { | ||
delete(m, k) | ||
m[env.ReplaceEnv(k)] = env.ReplaceEnv(v) | ||
} | ||
} | ||
|
||
for _, m := range dconf.UlimitRaw { | ||
for k, v := range m { | ||
delete(m, k) | ||
m[env.ReplaceEnv(k)] = env.ReplaceEnv(v) | ||
} | ||
} | ||
|
||
for _, m := range dconf.LabelsRaw { | ||
for k, v := range m { | ||
delete(m, k) | ||
|
@@ -506,6 +564,12 @@ func (d *DockerDriver) Validate(config map[string]interface{}) error { | |
"userns_mode": { | ||
Type: fields.TypeString, | ||
}, | ||
"sysctl": { | ||
Type: fields.TypeArray, | ||
}, | ||
"ulimit": { | ||
Type: fields.TypeArray, | ||
}, | ||
"port_map": { | ||
Type: fields.TypeArray, | ||
}, | ||
|
@@ -1108,6 +1172,8 @@ func (d *DockerDriver) createContainerConfig(ctx *ExecContext, task *structs.Tas | |
hostConfig.UTSMode = driverConfig.UTSMode | ||
hostConfig.UsernsMode = driverConfig.UsernsMode | ||
hostConfig.SecurityOpt = driverConfig.SecurityOpt | ||
hostConfig.Sysctls = driverConfig.Sysctl | ||
hostConfig.Ulimits = driverConfig.Ulimit | ||
|
||
hostConfig.NetworkMode = driverConfig.NetworkMode | ||
if hostConfig.NetworkMode == "" { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -858,6 +858,60 @@ func TestDockerDriver_NetworkAliases_Bridge(t *testing.T) { | |
} | ||
} | ||
|
||
func TestDockerDriver_Sysctl_Ulimit(t *testing.T) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we test the failure conditions as well? |
||
task, _, _ := dockerTask(t) | ||
expectedUlimits := map[string]string{ | ||
"nproc": "4242", | ||
"nofile": "2048:4096", | ||
} | ||
task.Config["sysctl"] = []map[string]string{ | ||
{ | ||
"net.core.somaxconn": "16384", | ||
}, | ||
} | ||
task.Config["ulimit"] = []map[string]string{ | ||
expectedUlimits, | ||
} | ||
|
||
client, handle, cleanup := dockerSetup(t, task) | ||
defer cleanup() | ||
|
||
waitForExist(t, client, handle) | ||
|
||
container, err := client.InspectContainer(handle.ContainerID()) | ||
if err != nil { | ||
t.Fatalf("err: %v", err) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Use assert library for test assertions. |
||
} | ||
|
||
if want, got := "16384", container.HostConfig.Sysctls["net.core.somaxconn"]; want != got { | ||
t.Errorf("Wrong net.core.somaxconn config for docker job. Expect: %s, got: %s", want, got) | ||
} | ||
|
||
if want, got := 2, len(container.HostConfig.Ulimits); want != got { | ||
t.Errorf("Wrong number of ulimit configs for docker job. Expect: %d, got: %d", want, got) | ||
} | ||
for _, got := range container.HostConfig.Ulimits { | ||
if expectedStr, ok := expectedUlimits[got.Name]; ok == false { | ||
t.Errorf("%s config unexpected for docker job.", got.Name) | ||
} else { | ||
if strings.Contains(expectedStr, ":") == false { | ||
expectedStr = expectedStr + ":" + expectedStr | ||
} | ||
|
||
splitted := strings.SplitN(expectedStr, ":", 2) | ||
soft, _ := strconv.Atoi(splitted[0]) | ||
hard, _ := strconv.Atoi(splitted[1]) | ||
|
||
if got.Soft != int64(soft) { | ||
t.Errorf("Wrong soft %s ulimit for docker job. Expect: %d, got: %d", got.Name, soft, got.Soft) | ||
} | ||
if got.Hard != int64(hard) { | ||
t.Errorf("Wrong hard %s ulimit for docker job. Expect: %d, got: %d", got.Name, hard, got.Hard) | ||
} | ||
} | ||
} | ||
} | ||
|
||
func TestDockerDriver_Labels(t *testing.T) { | ||
if !tu.IsTravis() { | ||
t.Parallel() | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -75,6 +75,7 @@ The `docker` driver supports the following configuration in the job spec. Only | |
} | ||
``` | ||
|
||
<<<<<<< 657619c0ec51068e48a34817ddc4df267b6cce6f | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. left-over merge conflict marker There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed now |
||
* `dns_search_domains` - (Optional) A list of DNS search domains for the container | ||
to use. | ||
|
||
|
@@ -97,6 +98,34 @@ The `docker` driver supports the following configuration in the job spec. Only | |
* `interactive` - (Optional) `true` or `false` (default). Keep STDIN open on | ||
the container. | ||
|
||
* `sysctl` - (Optional) A key-value map of sysctl configurations to set to the | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This sentence is incomplete. |
||
|
||
|
||
```hcl | ||
config { | ||
sysctl { | ||
net.core.somaxconn = "16384" | ||
} | ||
} | ||
``` | ||
|
||
* `ulimit` - (Optional) A key-value map of ulimit configurations to set to the | ||
containers on start. | ||
|
||
```hcl | ||
config { | ||
ulimit { | ||
nproc = "4242" | ||
nofile = "2048:4096" | ||
} | ||
} | ||
``` | ||
|
||
* `privileged` - (Optional) `true` or `false` (default). Privileged mode gives | ||
the container access to devices on the host. Note that this also requires the | ||
nomad agent and docker daemon to be configured to allow privileged | ||
containers. | ||
|
||
* `ipc_mode` - (Optional) The IPC mode to be used for the container. The default | ||
is `none` for a private IPC namespace. Other values are `host` for sharing | ||
the host IPC namespace or the name or id of an existing container. Note that | ||
|
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.
Check length before indexing.