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

CHE-249: Setting workspace Pod terminationGracePeriodSeconds to zero #5007

Merged
merged 1 commit into from
May 5, 2017

Conversation

ibuziuk
Copy link
Member

@ibuziuk ibuziuk commented May 4, 2017

What does this PR do?

Setting workspace Pod terminationGracePeriodSeconds to zero

What issues does this PR fix or reference?

https://issues.jboss.org/browse/CHE-249

@ibuziuk ibuziuk added the target/branch Indicates that a PR will be merged into a branch other than master. label May 4, 2017
@ibuziuk ibuziuk requested review from amisevsk and l0rd May 4, 2017 19:55
@codenvy-ci
Copy link

Can one of the admins verify this patch?

Copy link
Contributor

@amisevsk amisevsk left a comment

Choose a reason for hiding this comment

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

Thanks Ilya! Looks mostly good -- just a minor issue with how the grace period is being set.

I've tested shutting down a workspace with the 0 second grace period, and it brings workspace shutdown down to ~5 seconds when running on minishift.

@@ -1530,6 +1532,7 @@ private String waitAndRetrieveContainerID(String deploymentName) throws IOExcept
if (OPENSHIFT_POD_STATUS_RUNNING.equals(status)) {
String containerID = pod.getStatus().getContainerStatuses().get(0).getContainerID();
String normalizedID = KubernetesStringUtils.normalizeContainerID(containerID);
pod.getSpec().setTerminationGracePeriodSeconds(OPENSHIFT_POD_TERMINATION_GRACE_PERIOD);
Copy link
Contributor

Choose a reason for hiding this comment

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

Doing it this way will not update the actual Pod spec. I think we need to add this in createOpenShiftDeployment()

        PodSpec podSpec = new PodSpecBuilder()
                                 .withContainers(container)
                                 .withVolumes(getVolumesFrom(volumes, workspaceID))
                                 .withTerminationGracePeriodSeconds(OPENSHIFT_POD_TERMINATION_GRACE_PERIOD)
                                 .build();

@ibuziuk
Copy link
Member Author

ibuziuk commented May 4, 2017

@amisevsk oh, thank you for review - I had no chance to test this PR due to weird issue with building che (I even posted a question on stack overflow - http://stackoverflow.com/questions/43792427/maven-hangs-forever-during-the-project-build-used-to-work-fine) lemme update the PR

Signed-off-by: Ilya Buziuk <ibuziuk@redhat.com>
@ibuziuk
Copy link
Member Author

ibuziuk commented May 4, 2017

PR has been updated

Copy link
Contributor

@amisevsk amisevsk left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks.

Copy link
Contributor

@l0rd l0rd left a comment

Choose a reason for hiding this comment

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

LGTM great job @ibuziuk and @amisevsk

@ibuziuk
Copy link
Member Author

ibuziuk commented May 4, 2017

@l0rd @amisevsk shall we apply ? (code freeze is over I guess)

@l0rd
Copy link
Contributor

l0rd commented May 4, 2017 via email

@ibuziuk ibuziuk merged commit adac6f7 into eclipse-che:openshift-connector May 5, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
target/branch Indicates that a PR will be merged into a branch other than master.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants