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

Adding '--cgroup-parent' option. #11428

Merged
merged 3 commits into from Mar 19, 2015

Conversation

Projects
None yet
10 participants
@vishh
Contributor

vishh commented Mar 16, 2015

#8551

The parent cgroup can be absolute or relative to the cgroup of the init process. Docker will create the parent cgroup if it does not exist.

Note: I am having trouble getting integration tests to work locally. Will update the PR once the tests have been validated.

cc @crosbymichael @LK4D4

@GordonTheTurtle

This comment has been minimized.

Show comment
Hide comment
@GordonTheTurtle

GordonTheTurtle Mar 16, 2015

Can you please sign your commits following these rules:

https://github.com/docker/docker/blob/master/CONTRIBUTING.md#sign-your-work

The easiest way to do this is to amend the last commit:

$ git clone -b "parent-cgroup" git@github.com:vishh/docker.git somewhere
$ cd somewhere
$ git rebase -i HEAD~3
editor opens
change each 'pick' to 'edit'
save the file and quit
$ git commit --amend -s --no-edit
$ git rebase --continue # and repeat the amend for each commit
$ git push -f

This will update the existing PR, so you do not need to open a new one.

GordonTheTurtle commented Mar 16, 2015

Can you please sign your commits following these rules:

https://github.com/docker/docker/blob/master/CONTRIBUTING.md#sign-your-work

The easiest way to do this is to amend the last commit:

$ git clone -b "parent-cgroup" git@github.com:vishh/docker.git somewhere
$ cd somewhere
$ git rebase -i HEAD~3
editor opens
change each 'pick' to 'edit'
save the file and quit
$ git commit --amend -s --no-edit
$ git rebase --continue # and repeat the amend for each commit
$ git push -f

This will update the existing PR, so you do not need to open a new one.

@jessfraz

This comment has been minimized.

Show comment
Hide comment
@jessfraz

jessfraz Mar 16, 2015

Contributor

hmmm you have sigs, ignore gordon

Contributor

jessfraz commented Mar 16, 2015

hmmm you have sigs, ignore gordon

@vishh

This comment has been minimized.

Show comment
Hide comment
@vishh

vishh Mar 17, 2015

Contributor

@jfrazelle: I get the following error whenever I run integration tests.

+ go test -test.timeout=30m github.com/docker/docker/integration
time="2015-03-16T23:50:14Z" level=error msg="Failed to read /proc/sys/net/ipv4/ip_local_port_range kernel parameter: open /proc/sys/net/ipv4/ip_local_port_range: no such file or directory"
Pulling repository docker-test-image^M
Pulling image (latest) from docker-test-image ^M
Pulling image (latest) from docker-test-image, endpoint: https://registry-1.docker.io/v1/ ^M
Pulling dependent layers ^M
Pulling metadata ^M
Pulling fs layer ^M
Downloading [>
...
Error downloading dependent layers ^M
Error pulling image (latest) from docker-test-image, endpoint: https://registry-1.docker.io/v1/, unable to decode ApplyLayer JSON response: invalid character 'i' in literal true (expecting 'r') ^M
Error pulling image (latest) from docker-test-image, unable to decode ApplyLayer JSON response: invalid character 'i' in literal true (expecting 'r') ^M
Download complete ^M
time="2015-03-16T23:50:18Z" level=fatal msg="Unable to pull the test image: could not find image: no such id: 83599e29c455eb719f77d799bc7c51521b9551972f5a850d7ad265bc1b5292f6"
exit status 1

Any idea why I get this error?

Contributor

vishh commented Mar 17, 2015

@jfrazelle: I get the following error whenever I run integration tests.

+ go test -test.timeout=30m github.com/docker/docker/integration
time="2015-03-16T23:50:14Z" level=error msg="Failed to read /proc/sys/net/ipv4/ip_local_port_range kernel parameter: open /proc/sys/net/ipv4/ip_local_port_range: no such file or directory"
Pulling repository docker-test-image^M
Pulling image (latest) from docker-test-image ^M
Pulling image (latest) from docker-test-image, endpoint: https://registry-1.docker.io/v1/ ^M
Pulling dependent layers ^M
Pulling metadata ^M
Pulling fs layer ^M
Downloading [>
...
Error downloading dependent layers ^M
Error pulling image (latest) from docker-test-image, endpoint: https://registry-1.docker.io/v1/, unable to decode ApplyLayer JSON response: invalid character 'i' in literal true (expecting 'r') ^M
Error pulling image (latest) from docker-test-image, unable to decode ApplyLayer JSON response: invalid character 'i' in literal true (expecting 'r') ^M
Download complete ^M
time="2015-03-16T23:50:18Z" level=fatal msg="Unable to pull the test image: could not find image: no such id: 83599e29c455eb719f77d799bc7c51521b9551972f5a850d7ad265bc1b5292f6"
exit status 1

Any idea why I get this error?

@jessfraz

This comment has been minimized.

Show comment
Hide comment
@jessfraz

jessfraz Mar 17, 2015

Contributor

hmmm this could be because registry is being weird ping @dmcgowan idk... we need to get rid of the integration tests altogether

Contributor

jessfraz commented Mar 17, 2015

hmmm this could be because registry is being weird ping @dmcgowan idk... we need to get rid of the integration tests altogether

@vishh

This comment has been minimized.

Show comment
Hide comment
@vishh

vishh Mar 17, 2015

Contributor

@jfrazelle: I see similar errors with the cli integration tests as well.

Contributor

vishh commented Mar 17, 2015

@jfrazelle: I see similar errors with the cli integration tests as well.

@vishh

This comment has been minimized.

Show comment
Hide comment
@vishh

vishh Mar 17, 2015

Contributor

@jfrazelle: When is the code freeze for 1.6.0? Any chance that this feature will make it into 1.6.0? This feature will help improve node reliability quite a bit in Kubernetes.

Contributor

vishh commented Mar 17, 2015

@jfrazelle: When is the code freeze for 1.6.0? Any chance that this feature will make it into 1.6.0? This feature will help improve node reliability quite a bit in Kubernetes.

@jessfraz

This comment has been minimized.

Show comment
Hide comment
@jessfraz

jessfraz Mar 17, 2015

Contributor

it would need to be in by sunday, we can try our best, it already had design review on the issue right?
ping @crosbymichael

Contributor

jessfraz commented Mar 17, 2015

it would need to be in by sunday, we can try our best, it already had design review on the issue right?
ping @crosbymichael

@vishh

This comment has been minimized.

Show comment
Hide comment
@vishh

vishh Mar 17, 2015

Contributor

Yup. AFAIU design review is done.

On Mon, Mar 16, 2015 at 9:16 PM, Jessie Frazelle notifications@github.com
wrote:

it would need to be in by sunday, we can try our best, it already had
design review on the issue right?
ping @crosbymichael https://github.com/crosbymichael


Reply to this email directly or view it on GitHub
#11428 (comment).

Contributor

vishh commented Mar 17, 2015

Yup. AFAIU design review is done.

On Mon, Mar 16, 2015 at 9:16 PM, Jessie Frazelle notifications@github.com
wrote:

it would need to be in by sunday, we can try our best, it already had
design review on the issue right?
ping @crosbymichael https://github.com/crosbymichael


Reply to this email directly or view it on GitHub
#11428 (comment).

@LK4D4

This comment has been minimized.

Show comment
Hide comment
@LK4D4

LK4D4 Mar 17, 2015

Contributor

@vishh I think it has pretty good chance to go in 1.6 if we'll fix tests :)

Contributor

LK4D4 commented Mar 17, 2015

@vishh I think it has pretty good chance to go in 1.6 if we'll fix tests :)

@jessfraz

This comment has been minimized.

Show comment
Hide comment
@jessfraz

jessfraz Mar 17, 2015

Contributor

Can we change the title to not WIP if it's for potential inclusion in 1.6 :)

Contributor

jessfraz commented Mar 17, 2015

Can we change the title to not WIP if it's for potential inclusion in 1.6 :)

@jessfraz jessfraz added this to the 1.6.0 milestone Mar 17, 2015

@vishh vishh changed the title from WIP: Adding '--cgroup-parent' option. to Adding '--cgroup-parent' option. Mar 17, 2015

@vishh

This comment has been minimized.

Show comment
Hide comment
@vishh

vishh Mar 17, 2015

Contributor

@LK4D4: The tests are now passing in linux. What is the recommended way to skip linux specific integration tests in windows?

Contributor

vishh commented Mar 17, 2015

@LK4D4: The tests are now passing in linux. What is the recommended way to skip linux specific integration tests in windows?

@vishh

This comment has been minimized.

Show comment
Hide comment
@vishh

vishh Mar 17, 2015

Contributor

@jfrazelle: Removed WIP from the title.

Contributor

vishh commented Mar 17, 2015

@jfrazelle: Removed WIP from the title.

@jessfraz

This comment has been minimized.

Show comment
Hide comment
@jessfraz

jessfraz Mar 17, 2015

Contributor

i think you can add to _unix.go file in tests or there are skip testrequires functions

Contributor

jessfraz commented Mar 17, 2015

i think you can add to _unix.go file in tests or there are skip testrequires functions

@jessfraz

This comment has been minimized.

Show comment
Hide comment
@jessfraz

jessfraz Mar 17, 2015

Contributor

but yayyyy! so close

Contributor

jessfraz commented Mar 17, 2015

but yayyyy! so close

@jessfraz

This comment has been minimized.

Show comment
Hide comment
@jessfraz

jessfraz Mar 17, 2015

Contributor

the implementation is so simple :) LGTM after windows is skipped for the tests

Contributor

jessfraz commented Mar 17, 2015

the implementation is so simple :) LGTM after windows is skipped for the tests

@vishh

This comment has been minimized.

Show comment
Hide comment
@vishh

vishh Mar 17, 2015

Contributor

@jfrazelle: Thanks for the tip. I moved the tests to an equivalent "_unix" file.

Contributor

vishh commented Mar 17, 2015

@jfrazelle: Thanks for the tip. I moved the tests to an equivalent "_unix" file.

@LK4D4

This comment has been minimized.

Show comment
Hide comment
@LK4D4

LK4D4 Mar 17, 2015

Contributor

Actually _unix postfix has no special meaning. _linux has.
ping @ahmetalpbalkan

Contributor

LK4D4 commented Mar 17, 2015

Actually _unix postfix has no special meaning. _linux has.
ping @ahmetalpbalkan

@vishh

This comment has been minimized.

Show comment
Hide comment
@vishh

vishh Mar 17, 2015

Contributor

@LK4D4: Updated the remote api docs.
@jfrazelle: The tests have testRequires(nativeExecDriver) already.

Contributor

vishh commented Mar 17, 2015

@LK4D4: Updated the remote api docs.
@jfrazelle: The tests have testRequires(nativeExecDriver) already.

@vishh

This comment has been minimized.

Show comment
Hide comment
@vishh

vishh Mar 18, 2015

Contributor

@jfrazelle: The windows build failure is most likely a flake and not related to this PR. Can you restart the windows build?
Here is the failing test log:

--- FAIL: TestPullVerified (11.32s)
    docker_cli_pull_test.go:71: pulling a verified image failed. expected: The image you are pulling has been verified
        got: Pulling repository hello-world
        e45a5af57b00: Pulling image (latest) from hello-world
        e45a5af57b00: Pulling image (latest) from hello-world, endpoint: https://registry-1.docker.io/v1/
        e45a5af57b00: Pulling dependent layers
        511136ea3c5a: Download complete
        31cbccb51277: Download complete
        e45a5af57b00: Download complete
        e45a5af57b00: Download complete
        Status: Image is up to date for hello-world:latest
        , <nil>
Contributor

vishh commented Mar 18, 2015

@jfrazelle: The windows build failure is most likely a flake and not related to this PR. Can you restart the windows build?
Here is the failing test log:

--- FAIL: TestPullVerified (11.32s)
    docker_cli_pull_test.go:71: pulling a verified image failed. expected: The image you are pulling has been verified
        got: Pulling repository hello-world
        e45a5af57b00: Pulling image (latest) from hello-world
        e45a5af57b00: Pulling image (latest) from hello-world, endpoint: https://registry-1.docker.io/v1/
        e45a5af57b00: Pulling dependent layers
        511136ea3c5a: Download complete
        31cbccb51277: Download complete
        e45a5af57b00: Download complete
        e45a5af57b00: Download complete
        Status: Image is up to date for hello-world:latest
        , <nil>
@jessfraz

This comment has been minimized.

Show comment
Hide comment
@jessfraz

jessfraz Mar 18, 2015

Contributor

ah man ya that is having problems i think it should be fine

Contributor

jessfraz commented Mar 18, 2015

ah man ya that is having problems i think it should be fine

@jessfraz

This comment has been minimized.

Show comment
Hide comment
@jessfraz

jessfraz Mar 18, 2015

Contributor

ping @crosbymichael for review :D

Contributor

jessfraz commented Mar 18, 2015

ping @crosbymichael for review :D

@LK4D4

This comment has been minimized.

Show comment
Hide comment
@LK4D4

LK4D4 Mar 18, 2015

Contributor

@vishh Hmmm, stats test failing. Also your test failing too. This is probably because of systemd.
We should remember that some problems with systemd here :)

Contributor

LK4D4 commented Mar 18, 2015

@vishh Hmmm, stats test failing. Also your test failing too. This is probably because of systemd.
We should remember that some problems with systemd here :)

vishh added some commits Mar 16, 2015

Adding '--cgroup-parent' flag to docker run. This feature helps users…
… implement more complex

resource isolation policies on top of what native docker provides.

Docker-DCO-1.1-Signed-off-by: Vishnu Kannan <vishnuk@google.com> (github: vishh)
Adding documentation for '--cgroup-parent' option.
Docker-DCO-1.1-Signed-off-by: Vishnu Kannan <vishnuk@google.com> (github: vishh)
Adding integration tests for --cgroup-parent feature.
Docker-DCO-1.1-Signed-off-by: Vishnu Kannan <vishnuk@google.com> (github: vishh)
@vishh

This comment has been minimized.

Show comment
Hide comment
@vishh

vishh Mar 19, 2015

Contributor

@LK4D4: The tests failed once on a systemd machine. Are the tests scheduled on arbitrary linux distros? I added some more debug logs to capture failures. Can you re-run the tests?

Contributor

vishh commented Mar 19, 2015

@LK4D4: The tests failed once on a systemd machine. Are the tests scheduled on arbitrary linux distros? I added some more debug logs to capture failures. Can you re-run the tests?

@vishh

This comment has been minimized.

Show comment
Hide comment
@vishh

vishh Mar 19, 2015

Contributor
Contributor

vishh commented Mar 19, 2015

@LK4D4

This comment has been minimized.

Show comment
Hide comment
@LK4D4

LK4D4 Mar 19, 2015

Contributor

LGTM

Contributor

LK4D4 commented Mar 19, 2015

LGTM

@crosbymichael

This comment has been minimized.

Show comment
Hide comment
@crosbymichael

crosbymichael Mar 19, 2015

Contributor

LGTM

Contributor

crosbymichael commented Mar 19, 2015

LGTM

crosbymichael added a commit that referenced this pull request Mar 19, 2015

Merge pull request #11428 from vishh/parent-cgroup
Adding '--cgroup-parent' option.

@crosbymichael crosbymichael merged commit 455a272 into moby:master Mar 19, 2015

2 checks passed

janky Jenkins build Docker-PRs 3664 has succeeded
Details
windows Jenkins build Windows-PRs 728 has succeeded
Details
@jessfraz

This comment has been minimized.

Show comment
Hide comment
@jessfraz

jessfraz Mar 19, 2015

Contributor

omg yayyy!

On Thu, Mar 19, 2015 at 2:42 PM, Michael Crosby notifications@github.com
wrote:

Merged #11428 #11428.


Reply to this email directly or view it on GitHub
#11428 (comment).

Contributor

jessfraz commented Mar 19, 2015

omg yayyy!

On Thu, Mar 19, 2015 at 2:42 PM, Michael Crosby notifications@github.com
wrote:

Merged #11428 #11428.


Reply to this email directly or view it on GitHub
#11428 (comment).

@vishh

This comment has been minimized.

Show comment
Hide comment
@vishh

vishh Mar 19, 2015

Contributor

Thanks for the quick review @LK4D4 and @crosbymichael. I hope the integration tests don't remain flaky. If they fail ping me and I will fix them.

Contributor

vishh commented Mar 19, 2015

Thanks for the quick review @LK4D4 and @crosbymichael. I hope the integration tests don't remain flaky. If they fail ping me and I will fix them.

@smarterclayton

This comment has been minimized.

Show comment
Hide comment
@smarterclayton

smarterclayton Mar 19, 2015

Contributor

@mrunalp can you look at adding this to builds?

Contributor

smarterclayton commented Mar 19, 2015

@mrunalp can you look at adding this to builds?

@mrunalp

This comment has been minimized.

Show comment
Hide comment
@mrunalp

mrunalp Mar 20, 2015

Contributor

@smarterclayton Yes, it will quite useful for builds as well.

Contributor

mrunalp commented Mar 20, 2015

@smarterclayton Yes, it will quite useful for builds as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment