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
Missing call to slave.terminate() #256
Conversation
Apparently it is needed or jenkins will think failed nodes are still available in some cases
@reviewbybees |
@@ -222,15 +222,16 @@ public void launch(SlaveComputer computer, TaskListener listener) { | |||
if (containerStatuses != null) { | |||
logLastLines(containerStatuses, podId, namespace, slave, null, client); | |||
} | |||
slave.terminate(); |
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 necessary? it's already called in the catch block
Do we want to test this in an installed cluster before merging it? |
The tests are run in my GKE cluster https://jenkins.gke.csanchez.org/blue/organizations/jenkins/jenkinsci%2Fkubernetes-plugin/detail/node-removal/1/ I'll go through some more testing before merging |
Technically oc-cloud is the best implementation... and Carlos should be able to see that, but there are some specifics to CJOC that may mean it is not as applicable to this plugin |
This might fix #17, slaves not reconnecting and pods not spawning up. I suspect jenkinsci/kubernetes-plugin#256 is the root cause and the fix is part of this release.
Apparently it is needed or jenkins will think failed nodes are still available in some cases,
not calling the plugin again to provision nodes,
thus leaving jobs stuck waiting for executor that will never be created
Based on yet-another-docker-plugin which was mentioned by @stephenc as a good example of cloud api implementation https://github.com/KostyaSha/yet-another-docker-plugin/blob/db0026ea46b1ca24a03c4212d09acefff0b1f2e4/yet-another-docker-plugin/src/main/java/com/github/kostyasha/yad/launcher/DockerComputerJNLPLauncher.java#L281