Add/use container.Exec() to integration - #36326
Merged
Merged
Conversation
dnephin
reviewed
Feb 15, 2018
Member
There was a problem hiding this comment.
There are some lint errors because of naming.
I think we can put this into integration/internal/container so that calling it Exec is fine.
Member
There was a problem hiding this comment.
This doesn't check if the container is running, just that it exists. Is this necessary or will execCreate handle this check for us?
Contributor
Author
There was a problem hiding this comment.
I wrote it in November so frankly I don't remember :) but it looks like this is indeed a check that container exists, and I think it can be dropped.
kolyshkin
force-pushed
the
integration-exec
branch
from
February 15, 2018 22:57
bd4fce1 to
da18010
Compare
Contributor
Author
|
@dnephin please see updated commits |
kolyshkin
force-pushed
the
integration-exec
branch
from
February 15, 2018 23:49
da18010 to
04d8502
Compare
Member
|
Linting error; |
thaJeztah
reviewed
Feb 16, 2018
Member
There was a problem hiding this comment.
Actually; perhaps this type doesn’t have to be exported
Contributor
Author
There was a problem hiding this comment.
I tried it and it the (way too strict IMO) linter gives:
15:55:50 integration/internal/container/exec.go:37:80:warning: exported func Exec returns unexported type container.execResult, which can be annoying to use (golint)
so I had to revert it back to they way it was
kolyshkin
force-pushed
the
integration-exec
branch
from
February 16, 2018 02:02
04d8502 to
af188a8
Compare
Contributor
Author
|
Scratch that; it's easier to do the check than to think/explain whether it should be done or not. |
kolyshkin
force-pushed
the
integration-exec
branch
from
February 16, 2018 02:08
af188a8 to
7dbb41f
Compare
Member
I think it does need to be exported |
cpuguy83
reviewed
Feb 16, 2018
Member
There was a problem hiding this comment.
I'm worried that this is really high level and will end up with other similar implementations to get at the lower level details.
I'm wondering if this can take an ExecConfig and return a hijack response (or some suitable wrapper)?
I think the main issue is that it wasn't clear that ExecAttach and ExecStart are really just calling the same endpoint and this is very different from the container API.
Contributor
Author
There was a problem hiding this comment.
The whole point was make it easy to use for the common case. A lower-level functions can definitely be added, too, in case there will be a need for those (so far I haven't seen it, but it might change).
I think the main issue is that it wasn't clear that ExecAttach and ExecStart are really just calling the same endpoint
There were other issues, too, the one I remember is not closing something that should be closing.
When a test case is being developed, one should concentrate on the test case itself, not the intricacies of how to correctly execute a simple command inside a container and get the result.
kolyshkin
force-pushed
the
integration-exec
branch
from
February 17, 2018 23:43
7dbb41f to
dfcb215
Compare
Contributor
Author
Apparently. Patch set updated. Weird, the semantics I used is a classical example of how to implement read-only access to a class member in C++. |
Member
Hm, yes, I recall I ran into the same issue at some point. Perhaps the linter is too strict there, idk.
Did you forgot to push? Looks like Janky is still failing on that issue |
Some test cases might need an ability to execute a command inside a container (in order to analyse its output and/or exit code). It is a bit complicated operation to do so using engine API. The function provided aims to hide this complexity, making exec almost as simple as 'docker exec'. NOTE that the exec is synchronous, and command's stdin is closed. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
An implementation of exec in TestUpdateCPUQUota had a few issues, including resource leaking and calling both ContainerExecAttach and ContainerExecRun. The last one makes the test flaky: update_linux_test.go:136: expected cgroup value 20000, got: Error: Exec command f923baf709525f6b38f6511126addc5d9bb88fb477eeca1c22440551090fa2bb is already running Fix by using the integration/internal/exec package. While at it, use require/assert to further improve code readability. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
1. Use integration/internal/exec, removing the getContainerSysFSValue(). 2. Avoid repeating magic numbers, use a variable for those. 3. Fix order of arguments to assert.Equal (first "expected", then "actual"). Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
kolyshkin
force-pushed
the
integration-exec
branch
from
February 19, 2018 19:25
dfcb215 to
0f9da07
Compare
Contributor
Author
|
powerpc CI stuck: I think I've seen something like this before; looks like the next test case to be executed is TestSwarmClusterRotateUnlockKey |
Contributor
Author
|
I could use some help with the above mentioned test case. There are three daemons
The last query received by daemon (d2 in this case) was this: This query sent here (judging by the logs, i == moby/integration-cli/docker_cli_swarm_test.go Line 1447 in 733ed2d and then the whole rig got stuck, with lots of messages in d1 and d2 logs. Here's the relevant part of d2 log (starting from the aforementioned line): Here's th A3E2 e relevant part of d1 log (starting from the same time as d2's): Perhaps someone can shed some light on this? I'm as stuck as these two daemons. |
Contributor
Author
|
@cyli can you PTAL (not at the PR itself, but at the powerpc failure that I wrote about in a couple of previous comments) |
Contributor
Author
|
Removed [WIP], I think it's ready for the prime time. The powerpc CI failure is defininitely unrelated. |
boaz0
reviewed
Feb 20, 2018
|
|
||
| body, err := getContainerSysFSValue(ctx, client, cID, "/sys/fs/cgroup/memory/memory.limit_in_bytes") | ||
| res, err := container.Exec(ctx, client, cID, | ||
| []string{"cat", "/sys/fs/cgroup/memory/memory.limit_in_bytes"}) |
Contributor
There was a problem hiding this comment.
Is there a reason why the binary here is cat while in TestUpdateCPUQuota it is explicitly given as /bin/cat?
Contributor
Author
There was a problem hiding this comment.
No particular reason; we expect that /bin entry is present in $PATH, so no need to use an absolute path. I general, I'd prefer relying on $PATH and not giving absolute paths -- it's a bit more portable that way.
boaz0
reviewed
Feb 20, 2018
| assert.Equal(t, strconv.FormatInt(setMemorySwap, 10), strings.TrimSpace(res.Stdout())) | ||
| } | ||
|
|
||
| func TestUpdateCPUQUota(t *testing.T) { |
Contributor
There was a problem hiding this comment.
I think the function name should be TestUpdateCPUQuota and not TestUpdateCPUQUota
Member
There was a problem hiding this comment.
Yes, that looks like a typo, but that code wasn't touched in this PR; perhaps you can open a separate PR for that?
Contributor
Author
There was a problem hiding this comment.
Good catch; I'll fix.
Codecov Report
@@ Coverage Diff @@
## master #36326 +/- ##
=========================================
Coverage ? 34.31%
=========================================
Files ? 611
Lines ? 45491
Branches ? 0
=========================================
Hits ? 15611
Misses ? 27867
Partials ? 2013Continue to review full report at Codecov.
|
Contributor
|
@kolyshkin Thanks for the logs - will have a look |
Contributor
Author
|
@cyli thanks for taking a look! You can get the full logs from https://jenkins.dockerproject.org/job/Docker-PRs-powerpc/8563/, let me know if you need anything. |
Contributor
|
Still not sure what's wrong yet, but notes so far: I think
Not sure why the dispatcher doesn't come back up, but would it be possible that it's related to moby/swarmkit#2495 @anshulpundir? |
Contributor
Author
|
@cyli yes you're right about |
Contributor
|
moby/swarmkit#2495 has been reverted and another fix put in (but not brought into moby yet). We could possibly expect the loss of leadership or node shutdown to hang because of that change, so I suppose Manager.handleLeadershipEvents() could be hung. Can you look at the backtrace to see where the different stacks are ? @cyli |
Contributor
|
@anshulpundir Unfortunately this was in CI and the daemons I think are no longer running. I have not been able to replicate this manually (although I think I've seen it in some logs for other things a couple times), but will try. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
9 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Some test cases might need an ability to execute a command inside a container (in order to analyse its output and/or exit code). It is a bit complicated operation to do so using engine API. The function provided aims to hide this complexity, making exec almost as simple as 'docker exec'.
Currently, there are two integration test cases that re-implement this functionality (with some mistakes and leaks along the way) -- let's convert those to use the newly added
container.Exec().