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

Feature/thread start container jnlp #770

Closed
wants to merge 6 commits into from
Closed

Feature/thread start container jnlp #770

wants to merge 6 commits into from

Conversation

mat1e
Copy link
Member

@mat1e mat1e commented Jan 20, 2020

See #757 and JENKINS-59790.

With JNLP connection method, adding a Jenkins node is asynchronous with when the container starts.

It is a problem when Jenkins is slow and the docker container start very quickly. The node is not yet created on Jenkins when the container ask to connect.

This solution delays the container start until the Jenkins node is created for JNLP.

@mat1e mat1e requested a review from pjdarton January 20, 2020 14:09
Copy link
Member

@pjdarton pjdarton left a comment

Choose a reason for hiding this comment

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

While I appreciate that there's a race condition that sometimes interferes with a smooth startup of JNLP nodes, these code changes appear to break SSH-nodes.
See the automated test results: https://ci.jenkins.io/blue/organizations/jenkins/Plugins%2Fdocker-plugin/detail/PR-770/1/tests

Any improvement to JNLP nodes must not come as a cost of breaking SSH nodes; these code changes cannot be accepted if they break SSH.

@mat1e
Copy link
Member Author

mat1e commented Jan 20, 2020

While I appreciate that there's a race condition that sometimes interferes with a smooth startup of JNLP nodes, these code changes appear to break SSH-nodes.
See the automated test results: https://ci.jenkins.io/blue/organizations/jenkins/Plugins%2Fdocker-plugin/detail/PR-770/1/tests

Any improvement to JNLP nodes must not come as a cost of breaking SSH nodes; these code changes cannot be accepted if they break SSH.

Thanks, I was looking for theses results.

@pjdarton
Copy link
Member

FYI you can find the automated build results from the github page.
There's a "show all checks" link which, when clicked, lists the kinds of tests done and, on the right hand side of each, a "Details" link that, for Jenkins code, will take you to the ci.jenkins.io (Jenkins) server that runs the builds.
In there, you'll find links to the build console output, the unit test results, plus any build artifacts that it built (useful if you want to share your PR's code with others on the internet!).

@mat1e mat1e requested a review from pjdarton January 20, 2020 16:47
@mat1e
Copy link
Member Author

mat1e commented Jan 21, 2020

While I appreciate that there's a race condition that sometimes interferes with a smooth startup of JNLP nodes, these code changes appear to break SSH-nodes.
See the automated test results: https://ci.jenkins.io/blue/organizations/jenkins/Plugins%2Fdocker-plugin/detail/PR-770/1/tests

Any improvement to JNLP nodes must not come as a cost of breaking SSH nodes; these code changes cannot be accepted if they break SSH.

It is fixed.

@mat1e mat1e requested a review from ndeloof February 12, 2020 17:37
@pjdarton pjdarton added the bug An issue reporting a bug or a PR fixing one. label Mar 11, 2020
@pjdarton
Copy link
Member

pjdarton commented Oct 9, 2020

Closing. #789 superceded this PR.

@pjdarton pjdarton closed this Oct 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug An issue reporting a bug or a PR fixing one.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants