-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Remove containers grace period #686
Conversation
There's a test failure in ContainerExecDecoratorTest.testCommandExecution() Is that existing or was that test relying on the grace period to execute? |
It's also on master. I never hit it when running tests locally but it seems to happen systematically when running on kind 0.7.0 (#685), not sure why. |
Saves about 14 minutes (40 minutes total vs. 54 min) |
Test failure already seen in #553 |
Looks like buffer from other threads is printed instead.
See duplicated |
@jglick looks like some synchronization issue when running in cpu constrainted environment. |
@jglick Don't review this right now, cleaning up the patch first. |
…r pod termination.
4cf95f2
to
e5faa7f
Compare
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.
(trying to ignore changes from #690)
@@ -198,6 +198,7 @@ public Pod build() { | |||
builder.withNodeSelector(nodeSelector); | |||
} | |||
|
|||
builder.withTerminationGracePeriodSeconds(0L); |
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.
Is this really safe?
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.
I was wondering as well in case some process really need to do cleanup. Probably safer to set it for all tests.
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.
safer to set it for all tests
Do you mean set it only for tests?
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.
Yes this is what I meant
@@ -120,7 +120,7 @@ public void configureCloud() throws Exception { | |||
.withCommand("cat").withTty(true).withWorkingDir("/home/jenkins/agent1").build(); | |||
String podName = "test-command-execution-" + RandomStringUtils.random(5, "bcdfghjklmnpqrstvwxz0123456789"); | |||
pod = client.pods().create(new PodBuilder().withNewMetadata().withName(podName) | |||
.withLabels(getLabels(this, name)).endMetadata().withNewSpec().withContainers(c, d).withNodeSelector(Collections.singletonMap("kubernetes.io/os", "linux")).endSpec().build()); | |||
.withLabels(getLabels(this, name)).endMetadata().withNewSpec().withContainers(c, d).withNodeSelector(Collections.singletonMap("kubernetes.io/os", "linux")).withTerminationGracePeriodSeconds(0L).endSpec().build()); |
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.
Fine here in tests.
Nothing interesting should be running in agents containers when it is requested to be terminated, so don't wait these 30 seconds to kill remaining processes inside it.
Should speed up a lot tests hopefully.
Subsumes #689 and #690, please review this commit.