Skip to content
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

cleanup/fix integration-cli for overlay in overlay #12206

Merged
merged 1 commit into from
Apr 9, 2015

Conversation

jessfraz
Copy link
Contributor

@jessfraz jessfraz commented Apr 8, 2015

closes #11966

@jessfraz
Copy link
Contributor Author

jessfraz commented Apr 8, 2015

@jessfraz
Copy link
Contributor Author

jessfraz commented Apr 8, 2015

dammit, worked on my machine, there is a more annoying problem from these "Dead" containers

@jessfraz
Copy link
Contributor Author

jessfraz commented Apr 8, 2015

back to the drawing board

@jessfraz
Copy link
Contributor Author

jessfraz commented Apr 8, 2015

oooo i know

@jessfraz jessfraz force-pushed the 11966-fix-racy-tests-on-overlay branch from 52453c4 to cb9b8db Compare April 8, 2015 23:55
if err != nil || exitCode != 0 {
t.Fatal("failed to create a container", out, err)
}

cleanedContainerID := strings.TrimSpace(out)
defer deleteContainer(cleanedContainerID)
defer dockerCmd(t, "rm", "-f", cleanedContainerID)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is the test that was creating the "Dead" container, and this is the fix

Signed-off-by: Jessica Frazelle <jess@docker.com>
@@ -472,7 +472,7 @@ func TestCpVolumePath(t *testing.T) {
}

cleanedContainerID := strings.TrimSpace(out)
defer deleteContainer(cleanedContainerID)
defer dockerCmd(t, "rm", "-fv", cleanedContainerID)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is the test that was creating the "Dead" container, and this is the fix

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

its gotta be something with creating the volume via vfs in overlay in overlay that makes the container need a SIGTERM and SIGKILL

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but -f is always doing a sigkill, and isn't deleteContainer doing this exact thing?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it is the super intricate difference of doing a SIGTERM, then SIGKILL that is different and somehow saves the day

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh, I know why!
Dead containers happen when rm failed, but when you rm -f them again it forcibly removes them from docker regardless if there is an issue.

So first time with rm -f it had an issue, is marked as dead. If you just rm (no -f) it won't remove if the issue isn't resolved. But when you rm -f again, it will remove from docker regardless of issues.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

-f calls container.Stop(3) which does the TERM & KILL after 3 seconds, deleteContainer does just a SIGKILL

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but what it is causing the 2nd one to work isn't the TERM vs KILL, it's that -f has tells Docker to actually remove a container that is in a dead state no matter what.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh damn you were better than me, and i went into the depths of the sigs :(

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but i am only calling rm -f once not twice

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

deleteContainers does just a kill and rm

@jessfraz
Copy link
Contributor Author

jessfraz commented Apr 9, 2015

@@ -3449,7 +3450,8 @@ func TestRunContainerWithRmFlagExitCodeNotEqualToZero(t *testing.T) {
func TestRunContainerWithRmFlagCannotStartContainer(t *testing.T) {
defer deleteAllContainers()

runCmd := exec.Command(dockerBinary, "run", "--rm", "busybox", "commandNotFound")
name := "sparkles"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sunshine! Sparkle!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😎 ya kinda left over from debuigging but i like

@LK4D4
Copy link
Contributor

LK4D4 commented Apr 9, 2015

LGTM

1 similar comment
@cpuguy83
Copy link
Member

cpuguy83 commented Apr 9, 2015

LGTM

cpuguy83 added a commit that referenced this pull request Apr 9, 2015
cleanup/fix integration-cli for overlay in overlay
@cpuguy83 cpuguy83 merged commit 91abff4 into moby:master Apr 9, 2015
@jessfraz jessfraz deleted the 11966-fix-racy-tests-on-overlay branch April 9, 2015 00:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

racy tests TestRunContainerWithRmFlagExitCodeNotEqualToZero TestRunContainerWithRmFlagCannotStartContainer
4 participants