Async startup of the SSHD server #19

olivergondza · 2017-09-14T08:36:04Z

Fix race condition in SSHD.start().
Do not let Jenkins to wait until SSHD is fully up.
- I have seen launch stuck for minutes while running parallel jenkins-test-harness tests as it was waiting for system entropy to launch a service I do not care for on the first place.
Some javadoc fixes while on it.

SEVERE: Failed SSHD.init java.lang.Error: java.lang.reflect.InvocationTargetException at hudson.init.TaskMethodFinder.invoke(TaskMethodFinder.java:110) at hudson.init.TaskMethodFinder$TaskImpl.run(TaskMethodFinder.java:176) at org.jvnet.hudson.reactor.Reactor.runTask(Reactor.java:282) at jenkins.model.Jenkins$8.runTask(Jenkins.java:924) at org.jvnet.hudson.reactor.Reactor$2.run(Reactor.java:210) at org.jvnet.hudson.reactor.Reactor$Node.run(Reactor.java:117) at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1145) at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:615) at java.lang.Thread.run(Thread.java:745) Caused by: java.lang.reflect.InvocationTargetException at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method) at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:57) at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43) at java.lang.reflect.Method.invoke(Method.java:606) at hudson.init.TaskMethodFinder.invoke(TaskMethodFinder.java:106) ... 8 more Caused by: java.lang.IllegalArgumentException: Bad port number: -1 at org.apache.sshd.SshServer.checkConfig(SshServer.java:282) at org.apache.sshd.SshServer.start(SshServer.java:342) at org.jenkinsci.main.modules.sshd.SSHD.start(SSHD.java:109) at org.jenkinsci.main.modules.sshd.SSHD.init(SSHD.java:146) ... 13 more

oleg-nenashev

I suggest using Timer.get() instead of Remoting executor service: https://github.com/jenkinsci/jenkins/blob/7f6f495f866bc234fe5a54ce866e8b214ba693b2/core/src/main/java/jenkins/util/Timer.java . It seams to be designed specifically for such background tasks

oleg-nenashev · 2017-09-14T08:44:08Z

src/main/java/org/jenkinsci/main/modules/sshd/SSHD.java

@@ -200,7 +207,15 @@ public static SSHD get() {

    @Initializer(after= InitMilestone.JOB_LOADED,fatal=false)
    public static void init() throws IOException, InterruptedException {
-        get().start();
+        MasterComputer.threadPoolForRemoting.submit(new Runnable() {


Log something like "Scheduling SSHD startup" on the FINE level?

oleg-nenashev · 2017-09-14T08:44:34Z

src/main/java/org/jenkinsci/main/modules/sshd/SSHD.java

+        MasterComputer.threadPoolForRemoting.submit(new Runnable() {
+            @Override public void run() {
+                try {
+                    get().start();


Some logging before and after may be also useful for diagnostic purposes

oleg-nenashev · 2017-09-14T08:45:54Z

src/main/java/org/jenkinsci/main/modules/sshd/SSHD.java

@@ -73,7 +74,7 @@ public int getPort() {
    /**
     * Gets the current TCP/IP port that this daemon is running with.
     *
-     * @return -1 if disabled, but never null.
+     * @return Actual port number or -1 if disabled.
     */
    public synchronized int getActualPort() {


Nice 2 have: @CheckForSigned to enforce checks

oleg-nenashev · 2017-09-14T08:46:00Z

src/main/java/org/jenkinsci/main/modules/sshd/SSHD.java

@@ -64,7 +65,7 @@ public SSHD() {
     * Returns the configured port to run SSHD.
     *
     * @return
-     *      -1 to disable this, 0 to run with a random port, otherwise the port number.
+     *      -1 if disabled, 0 if random port is selected, otherwise the port number configured.
     */
    public int getPort() {


Nice 2 have: @CheckForSigned to enforce checks

oleg-nenashev

👍 Thanks @olivergondza

oleg-nenashev · 2017-09-14T09:23:56Z

@jenkinsci/code-reviewers some reviews of edge cases would be appreciated.

The only risk I see is that SSH-dependent JenkinsRule-based tests and Configuration Management logic (e.g. SSH CLI) will need to wait and check the SSH status before they will be able to really connect to the instance. Looks fine for SSH clients, which should poll the status anyway

olivergondza · 2017-09-14T10:02:23Z

Given my main motivation here are the tests and if this turns out to cause some problems for the tests, we may want to rethink the startup logic for jenkins-test-harness so it will not start the server at all unless someone call SSHD.get() as a signal that the test do care. I presume 99+% of the tests does not...

oleg-nenashev · 2017-09-14T10:15:31Z

Regarding JTH, I agree.
The most of the tests should not wait for SSHD startup, and actually SSHD should be disabled in them at all if not required. Maybe JTH could just disable SSHD when running Jenkins and then tests can enable it by setting the SSHD port when it's required

olivergondza · 2017-09-14T15:51:48Z

I guess we will have to try running the core tests against this to see if it would cause problems before merging this.

olivergondza · 2017-09-21T11:37:30Z

@oleg-nenashev, no further reviews, mind if I release this and start using it as a test dependency so we start receiving feedback?

oleg-nenashev · 2017-09-21T11:57:46Z

@olivergondza fine for me

jglick · 2017-10-05T18:07:01Z

Sorry, just saw this and it looks potentially wrong.

The port was already defaulted to -1 (as of #12 in sshd-1.11 ~ jenkinsci/jenkins#2795 in 2.46.2 / 2.54), so start() should have returned immediately. Only tests which actually need SSHD should be doing

SSHD.get().setPort(0);

So what tests were you seeing stuck @olivergondza? Perhaps you were using an older Jenkins baseline and simply needed to update?

jglick · 2017-10-05T18:09:22Z

IOW this change was not necessary for test performance; and may be introducing a race condition in production environments: you would expect that after Jenkins claims to be “up and running” that you could connect to a configured SSH service, for example via CLI, but now there is a window where you may not be able to.

olivergondza · 2017-10-06T07:44:43Z

So what tests were you seeing stuck @olivergondza? Perhaps you were using an older Jenkins baseline and simply needed to update?

You are right this was resolved by updating sshd to deploy your fix. I experienced all the tests to be stuck for tens of seconds waiting for Jenkins/SSHD to come up.

olivergondza added 3 commits September 14, 2017 10:20

Fix incorrect javadoc

6101e9b

Do not delay Jenkins startup because of SSHD.init

0592f74

oleg-nenashev requested changes Sep 14, 2017

View reviewed changes

Address review comemnts

228fd7f

oleg-nenashev approved these changes Sep 14, 2017

View reviewed changes

oleg-nenashev merged commit 32ea91a into jenkinsci:master Sep 21, 2017

oleg-nenashev mentioned this pull request Sep 21, 2017

Launch SSHD service asynchronously jenkinsci/jenkins#3031

Merged

4 tasks

jglick added a commit to jglick/sshd-module that referenced this pull request Oct 5, 2017

Fixing jenkinsci#19: SSHD startup should *not* be asynchronous.

12e35ca

jglick mentioned this pull request Oct 5, 2017

SSHD startup should *not* be asynchronous #20

Merged

olivergondza deleted the async-startup branch October 6, 2017 12:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Async startup of the SSHD server #19

Async startup of the SSHD server #19

olivergondza commented Sep 14, 2017

oleg-nenashev left a comment

oleg-nenashev Sep 14, 2017

oleg-nenashev Sep 14, 2017

oleg-nenashev Sep 14, 2017

oleg-nenashev Sep 14, 2017

oleg-nenashev left a comment

oleg-nenashev commented Sep 14, 2017 •

edited

Loading

olivergondza commented Sep 14, 2017

oleg-nenashev commented Sep 14, 2017

olivergondza commented Sep 14, 2017

olivergondza commented Sep 21, 2017

oleg-nenashev commented Sep 21, 2017

jglick commented Oct 5, 2017

jglick commented Oct 5, 2017

olivergondza commented Oct 6, 2017

Async startup of the SSHD server #19

Async startup of the SSHD server #19

Conversation

olivergondza commented Sep 14, 2017

oleg-nenashev left a comment

Choose a reason for hiding this comment

oleg-nenashev Sep 14, 2017

Choose a reason for hiding this comment

oleg-nenashev Sep 14, 2017

Choose a reason for hiding this comment

oleg-nenashev Sep 14, 2017

Choose a reason for hiding this comment

oleg-nenashev Sep 14, 2017

Choose a reason for hiding this comment

oleg-nenashev left a comment

Choose a reason for hiding this comment

oleg-nenashev commented Sep 14, 2017 • edited Loading

olivergondza commented Sep 14, 2017

oleg-nenashev commented Sep 14, 2017

olivergondza commented Sep 14, 2017

olivergondza commented Sep 21, 2017

oleg-nenashev commented Sep 21, 2017

jglick commented Oct 5, 2017

jglick commented Oct 5, 2017

olivergondza commented Oct 6, 2017

oleg-nenashev commented Sep 14, 2017 •

edited

Loading