Integrate with cloud-stats plugin

[francoisferrand]

Implement support for the cloud-stats plugin (https://github.com/jenkinsci/cloud-stats-plugin), which allows gaining some insight on the provisioning activities.

[pjdarton]

FYI this build failed because spotbugs found an issue (i.e. the redness isn't just because of the ongoing trouble with the DockerNodeStepTest). [ERROR] Possible null pointer dereference in io.jenkins.docker.DockerComputer.getId() due to return value of called method [io.jenkins.docker.DockerComputer, io.jenkins.docker.DockerComputer] Dereferenced at DockerComputer.java:[line 83]Known null at DockerComputer.java:[line 83] NP_NULL_ON_SOME_PATH_FROM_RETURN_VALUE https://ci.jenkins.io/blue/organizations/jenkins/Plugins%2Fdocker-plugin/detail/PR-797/2/pipeline/46#step-56-log-815

As of #794, Spotbugs is now (fully) enabled.

[francoisferrand]

@pjdarton : Hello! I fixed spotbug issue, can you please check this out?

[pjdarton]

I don't know how the cloud-stats code works; I've taken a look at the code and I have a question. In DockerCloud.provision, does the cloud-stats plugin find out that we've failed to provision if we fail before the call to slave.setProvisioningId(id); ? e.g. DockerCloud.this.getDockerApi(); or t.provisionNode(api, TaskListener.NULL); could throw an exception, causing execution to bypass slave.setProvisioningId(id); and go straight into exception logging - [how] can we be sure that the cloud-stats plugin will find out and log it as a failed provisioning attempt instead of just being left waiting?

[francoisferrand]

Thanks, that's a good point; I will try to check the code and test, to see how it behaves.

[pjdarton]

@Typz Has this been abandoned? ...and would you consider #800 to be a better option, given its claim to keeping cloud-stats an optional dependency?

[francoisferrand]

Hello, No, this has not been abandoned : I simply could not find the time to check this yet :-( #800 does indeed seem to do the same thing, and keeping the dependency on cloud-stats would indeed be better ; also, it is already merged with more recent updates to docker-plugin, which I would need to adapt to. I did not try it though, and having a very quick glance at it it does not relate to the issues I had, which leaves me kind of confused...

[pjdarton]

I've not had the time to check anything out either (in fact, I'd not even noticed the arrival of #800); no criticism was intended. It may well be that both PRs have their merits and what we really want is something that provides the benefits of both ... but I'm not a subject matter expert on the cloud-stats plugin so I've no idea (and haven't touched "real code" for months due to other work issues). I would welcome any information anyone can provide, so if you have any concerns about "the other PR" then please do let me know (either there or here).

[theshadow27]

Reiterating here so it doesn't get lost - I do not believe my PR #800 was working either, but I haven't touched it in a while and couldn't say for sure (I don't maintain our Jenkins deployments). If this one is working, and mine isn't, then it will take some effort to figure out why.

Disclaimer: I'm a Jenkins n00b and PR #800 was my first work on Jenkins plugins :D

[basil]

[how] can we be sure that the cloud-stats plugin will find out and log it as a failed provisioning attempt instead of just being left waiting?

By testing with

diff --git a/src/main/java/io/jenkins/docker/connector/DockerComputerSSHConnector.java b/src/main/java/io/jenkins/docker/connector/DockerComputerSSHConnector.java
index 1547362..382fd10 100644
--- a/src/main/java/io/jenkins/docker/connector/DockerComputerSSHConnector.java
+++ b/src/main/java/io/jenkins/docker/connector/DockerComputerSSHConnector.java
@@ -258,7 +258,7 @@ public class DockerComputerSSHConnector extends DockerComputerConnector {
                         "/usr/sbin/sshd",
                         "-D",
                         "-p",
-                        String.valueOf(port),
+                        String.valueOf(port + 1),
                         // override sshd_config to force retrieval of InstanceIdentity public for as authentication
                         "-o",
                         "AuthorizedKeysCommand=/root/authorized_key",

and observing

onFailure:504, CloudStatistics$ProvisioningListener (org.jenkinsci.plugins.cloudstats)
fireOnFailure:849, NodeProvisioner (hudson.slaves)
update:241, NodeProvisioner (hudson.slaves)
doRun:823, NodeProvisioner$NodeProvisionerInvoker (hudson.slaves)
run:92, SafeTimerTask (hudson.triggers)
run:67, ImpersonatingScheduledExecutorService$1 (jenkins.security)
call:515, Executors$RunnableAdapter (java.util.concurrent)
runAndReset:305, FutureTask (java.util.concurrent)
run:305, ScheduledThreadPoolExecutor$ScheduledFutureTask (java.util.concurrent)
runWorker:1128, ThreadPoolExecutor (java.util.concurrent)
run:628, ThreadPoolExecutor$Worker (java.util.concurrent)
run:829, Thread (java.lang)

and

onFailure:518, CloudStatistics$ProvisioningListener (org.jenkinsci.plugins.cloudstats)
lambda$onFailure$1:507, CloudStatistics$ProvisioningListener (org.jenkinsci.plugins.cloudstats)
run:-1, 119306147 (org.jenkinsci.plugins.cloudstats.CloudStatistics$ProvisioningListener$$Lambda$756)
run:67, ImpersonatingScheduledExecutorService$1 (jenkins.security)
call:515, Executors$RunnableAdapter (java.util.concurrent)
run:264, FutureTask (java.util.concurrent)
run:304, ScheduledThreadPoolExecutor$ScheduledFutureTask (java.util.concurrent)
runWorker:1128, ThreadPoolExecutor (java.util.concurrent)
run:628, ThreadPoolExecutor$Worker (java.util.concurrent)
run:829, Thread (java.lang)