Rebase and merge of docker sysctl support #3568
Conversation
|
Thanks a lot @preetapan . I really need this one! |
| @@ -75,6 +75,7 @@ The `docker` driver supports the following configuration in the job spec. Only | |||
| } | |||
| ``` | |||
|
|
|||
| <<<<<<< 657619c0ec51068e48a34817ddc4df267b6cce6f | |||
There was a problem hiding this comment.
left-over merge conflict marker
client/driver/docker.go
Outdated
| 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.
maybe include that it's the soft limit that is invalid? - same below for hard limit
There was a problem hiding this comment.
agreed with the limit specification for logging.
client/driver/docker.go
Outdated
| } | ||
|
|
||
| splitted := strings.SplitN(ulimitRaw, ":", 2) | ||
| soft, err := strconv.Atoi(splitted[0]) |
There was a problem hiding this comment.
Check length before indexing.
client/driver/docker.go
Outdated
| 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.
agreed with the limit specification for logging.
client/driver/docker.go
Outdated
| if err != nil { | ||
| return []docker.ULimit{}, fmt.Errorf("Malformed ulimit %v: %v", name, ulimitRaw) | ||
| } | ||
| hard, err := strconv.Atoi(splitted[1]) |
There was a problem hiding this comment.
Check limit before indexing.
client/driver/docker_test.go
Outdated
|
|
||
| container, err := client.InspectContainer(handle.ContainerID()) | ||
| if err != nil { | ||
| t.Fatalf("err: %v", err) |
There was a problem hiding this comment.
Use assert library for test assertions.
| @@ -97,6 +97,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.
This sentence is incomplete.
| @@ -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.
Should we test the failure conditions as well?
client/driver/docker_test.go
Outdated
| defer ctx.AllocDir.Destroy() | ||
|
|
||
| if _, err := driver.Prestart(ctx.ExecCtx, task); err == nil || err.Error() != tc.err.Error() { | ||
| t.Fatalf("error expected in prestart, got %v, expected %v", err, tc.err) |
|
Is there any plans for supporting this for |
|
@ashald Looks like there isn't support yet: rkt/rkt#2694 |
|
Sorry, missed that! |
|
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. |
No description provided.