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.

Sign up for GitHub

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

Jump to bottom

[JENKINS-46719] Check if container exists in pod at all #222

Merged
merged 2 commits into from
Sep 14, 2017
Merged

[JENKINS-46719] Check if container exists in pod at all #222

merged 2 commits into from
Sep 14, 2017

Conversation

marvinthepa
Copy link
Contributor

@marvinthepa marvinthepa commented Sep 12, 2017 •
edited
Loading

@marvinthepa marvinthepa changed the title Check if container exists in pod at all [JENKINS-46719] Check if container exists in pod at all Sep 12, 2017
carlossg
carlossg reviewed Sep 12, 2017
View reviewed changes
Copy link
Contributor

@carlossg carlossg left a comment

Choose a reason for hiding this comment

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

I haven't checked but I guess waitUntilReady will return when one of the containers errors?

src/main/java/org/csanchez/jenkins/plugins/kubernetes/pipeline/ContainerExecDecorator.java Outdated
.waitUntilReady(CONTAINER_READY_TIMEOUT, TimeUnit.MINUTES);

for (ContainerStatus info : pod.getStatus().getContainerStatuses()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

there was a check for pod.getStatus() == null || pod.getStatus().getContainerStatuses() == null IIRC null was possible during startup

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought that waitUntilReady should take care of that, but I'll re-add it to make sure.

@marvinthepa
Copy link
Contributor Author

I haven't checked but I guess waitUntilReady will return when one of the containers errors?

Unfortunately, it doesn't, but in that situation we will rather get into the #196 situation in that the pipeline will block at node until we run into the timeout.

This pull request is mainly about the situation where a user accidentally tries to execute a command in a container that is not even specified in the current node - they shouldn't have to wait for the timeout.

@marvinthepa
Copy link
Contributor Author

N.B. maybe the switch on info.getReady() does not make sense (because waitUntilReady should actually only return ready containers), but I guess it does not hurt, either.

@carlossg carlossg merged commit c4a54cb into jenkinsci:master Sep 14, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@carlossg carlossg carlossg left review comments

@iocanel iocanel Awaiting requested review from iocanel

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@marvinthepa @carlossg