Handle SSH if server "forget" to send exit status

gabrascher · gabrascher · commit 4f671287e46b · 2016-04-07T10:52:01.000-03:00
Continued the work started by https://github.com/likitha commit (b9181c6) from PR apache#561. CS waits indefinitely for CheckS2SVpnConnectionsComm and to return. While remote executing commands through ssh, handle channel condition of EOF because we wait for the the condition. The SshHelper of the PR apache#561 is of another path from the current master, its path was https://github.com/likitha/cloudstack/commits/CLOUDSTACK-8611/utils/src/com/cloud/utils/ssh/SshHelper.java; thus, although this commit brings changes from PR apache#561, I did not cherry-picked to keep the master file, otherwise it would look that I had changed all the file. by me.
diff --git a/utils/src/main/java/com/cloud/utils/ssh/SshHelper.java b/utils/src/main/java/com/cloud/utils/ssh/SshHelper.java
@@ -32,6 +32,12 @@ public class SshHelper {
     private static final int DEFAULT_CONNECT_TIMEOUT = 180000;
     private static final int DEFAULT_KEX_TIMEOUT = 60000;
 
+    /**
+     * Waiting time to check if the SSH session was successfully opened. This value (of 1000
+     * milliseconds) represents one (1) second.
+     */
+    private static final int WAITING_OPEN_SSH_SESSION = 1000;
+
     private static final Logger s_logger = Logger.getLogger(SshHelper.class);
 
     public static Pair<Boolean, String> sshExecute(String host, int port, String user, File pemKeyFile, String password, String command) throws Exception {
@@ -40,19 +46,19 @@ public static Pair<Boolean, String> sshExecute(String host, int port, String use
     }
 
     public static void scpTo(String host, int port, String user, File pemKeyFile, String password, String remoteTargetDirectory, String localFile, String fileMode)
-        throws Exception {
+            throws Exception {
 
         scpTo(host, port, user, pemKeyFile, password, remoteTargetDirectory, localFile, fileMode, DEFAULT_CONNECT_TIMEOUT, DEFAULT_KEX_TIMEOUT);
     }
 
     public static void scpTo(String host, int port, String user, File pemKeyFile, String password, String remoteTargetDirectory, byte[] data, String remoteFileName,
-        String fileMode) throws Exception {
+            String fileMode) throws Exception {
 
         scpTo(host, port, user, pemKeyFile, password, remoteTargetDirectory, data, remoteFileName, fileMode, DEFAULT_CONNECT_TIMEOUT, DEFAULT_KEX_TIMEOUT);
     }
 
     public static void scpTo(String host, int port, String user, File pemKeyFile, String password, String remoteTargetDirectory, String localFile, String fileMode,
-        int connectTimeoutInMs, int kexTimeoutInMs) throws Exception {
+            int connectTimeoutInMs, int kexTimeoutInMs) throws Exception {
 
         com.trilead.ssh2.Connection conn = null;
         com.trilead.ssh2.SCPClient scpClient = null;
@@ -88,7 +94,7 @@ public static void scpTo(String host, int port, String user, File pemKeyFile, St
     }
 
     public static void scpTo(String host, int port, String user, File pemKeyFile, String password, String remoteTargetDirectory, byte[] data, String remoteFileName,
-        String fileMode, int connectTimeoutInMs, int kexTimeoutInMs) throws Exception {
+            String fileMode, int connectTimeoutInMs, int kexTimeoutInMs) throws Exception {
 
         com.trilead.ssh2.Connection conn = null;
         com.trilead.ssh2.SCPClient scpClient = null;
@@ -123,7 +129,8 @@ public static void scpTo(String host, int port, String user, File pemKeyFile, St
     }
 
     public static Pair<Boolean, String> sshExecute(String host, int port, String user, File pemKeyFile, String password, String command, int connectTimeoutInMs,
-        int kexTimeoutInMs, int waitResultTimeoutInMs) throws Exception {
+            int kexTimeoutInMs,
+            int waitResultTimeoutInMs) throws Exception {
 
         com.trilead.ssh2.Connection conn = null;
         com.trilead.ssh2.Session sess = null;
@@ -146,6 +153,8 @@ public static Pair<Boolean, String> sshExecute(String host, int port, String use
             }
             sess = conn.openSession();
 
+            throwSshExceptionIfSshConnectionIsNull(sess);
+
             sess.execCommand(command);
 
             InputStream stdout = sess.getStdout();
@@ -156,22 +165,22 @@ public static Pair<Boolean, String> sshExecute(String host, int port, String use
 
             int currentReadBytes = 0;
             while (true) {
+                throwSshExceptionIfStdoutOrStdeerIsNull(stdout, stderr);
+
                 if ((stdout.available() == 0) && (stderr.available() == 0)) {
-                    int conditions =
-                        sess.waitForCondition(ChannelCondition.STDOUT_DATA | ChannelCondition.STDERR_DATA | ChannelCondition.EOF | ChannelCondition.EXIT_STATUS,
+                    int conditions = sess.waitForCondition(ChannelCondition.STDOUT_DATA | ChannelCondition.STDERR_DATA | ChannelCondition.EOF | ChannelCondition.EXIT_STATUS,
                             waitResultTimeoutInMs);
 
-                    if ((conditions & ChannelCondition.TIMEOUT) != 0) {
-                        String msg = "Timed out in waiting SSH execution result";
-                        s_logger.error(msg);
-                        throw new Exception(msg);
-                    }
+                    throwSshExceptionIfConditionsTimeout(conditions);
 
                     if ((conditions & ChannelCondition.EXIT_STATUS) != 0) {
-                        if ((conditions & (ChannelCondition.STDOUT_DATA | ChannelCondition.STDERR_DATA)) == 0) {
-                            break;
-                        }
+                        break;
+                    }
+
+                    if (canEndTheSshConnection(waitResultTimeoutInMs, sess, conditions)) {
+                        break;
                     }
+
                 }
 
                 while (stdout.available() > 0) {
@@ -189,11 +198,12 @@ public static Pair<Boolean, String> sshExecute(String host, int port, String use
 
             if (sess.getExitStatus() == null) {
                 //Exit status is NOT available. Returning failure result.
+                s_logger.error(String.format("SSH execution of command %s has no exit status set. Result output: %s", command, result));
                 return new Pair<Boolean, String>(false, result);
             }
 
             if (sess.getExitStatus() != null && sess.getExitStatus().intValue() != 0) {
-                s_logger.error("SSH execution of command " + command + " has an error status code in return. result output: " + result);
+                s_logger.error(String.format("SSH execution of command %s has an error status code in return. Result output: %s", command, result));
                 return new Pair<Boolean, String>(false, result);
             }
 
@@ -206,4 +216,91 @@ public static Pair<Boolean, String> sshExecute(String host, int port, String use
                 conn.close();
         }
     }
+
+    /**
+     * Handles the SSH connection in case of timeout or exit. If the session ends with a timeout
+     * condition, it throws an exception; if the channel reaches an end of file condition, but it
+     * does not have an exit status, it returns true to break the loop; otherwise, it returns
+     * false.
+     *
+     * @param waitResultTimeoutInMs
+     * @param sess
+     * @param conditions
+     * @return boolean
+     * @throws SshException
+     */
+    protected static boolean canEndTheSshConnection(int waitResultTimeoutInMs, com.trilead.ssh2.Session sess, int conditions) throws SshException {
+        if (isChannelConditionEof(conditions)) {
+            int newConditions = sess.waitForCondition(ChannelCondition.EXIT_STATUS, waitResultTimeoutInMs);
+            throwSshExceptionIfConditionsTimeout(newConditions);
+            if ((newConditions & ChannelCondition.EXIT_STATUS) != 0) {
+                return true;
+            }
+        }
+        return false;
+    }
+
+    /**
+     * It throws a {@link SshException} if the channel condition is {@link ChannelCondition#TIMEOUT}
+     *
+     * @param conditions
+     * @throws SshException
+     */
+    protected static void throwSshExceptionIfConditionsTimeout(int conditions) throws SshException {
+        if ((conditions & ChannelCondition.TIMEOUT) != 0) {
+            String msg = "Timed out in waiting for SSH execution exit status";
+            s_logger.error(msg);
+            throw new SshException(msg);
+        }
+    }
+
+    /**
+     * Checks if the channel condition mask is of {@link ChannelCondition#EOF} and not
+     * {@link ChannelCondition#STDERR_DATA} or {@link ChannelCondition#STDOUT_DATA}.
+     *
+     * @param conditions
+     * @return boolean
+     */
+    protected static boolean isChannelConditionEof(int conditions) {
+        if ((conditions & ChannelCondition.EOF) != 0) {
+            return true;
+        }
+        return false;
+    }
+
+    /**
+     * Checks if the SSH session {@link com.trilead.ssh2.Session#getStdout()} or
+     * {@link com.trilead.ssh2.Session#getStderr()} is null.
+     *
+     * @param stdout
+     * @param stderr
+     * @throws SshException
+     */
+    protected static void throwSshExceptionIfStdoutOrStdeerIsNull(InputStream stdout, InputStream stderr) throws SshException {
+        if (stdout == null || stderr == null) {
+            String msg = "Stdout or Stderr of SSH session is null";
+            s_logger.error(msg);
+            throw new SshException(msg);
+        }
+    }
+
+    /**
+     * Checks if the session is open. It waits {@link SshHelper#WAITING_OPEN_SSH_SESSION}
+     * milliseconds; if the session is not open after the waiting time, then it assumes that the
+     * session could not open a SSH connection.
+     *
+     * @param sess
+     * @throws InterruptedException
+     * @throws SshException
+     */
+    protected static void throwSshExceptionIfSshConnectionIsNull(com.trilead.ssh2.Session sess) throws InterruptedException, SshException {
+        Thread.sleep(WAITING_OPEN_SSH_SESSION);
+
+        if (sess == null) {
+            String msg = "Cannot open SSH session";
+            s_logger.error(msg);
+            throw new SshException(msg);
+        }
+    }
+
 }
diff --git a/utils/src/test/java/com/cloud/utils/ssh/SshHelperTest.java b/utils/src/test/java/com/cloud/utils/ssh/SshHelperTest.java
@@ -0,0 +1,136 @@
+//
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements.  See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership.  The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License.  You may obtain a copy of the License at
+//
+//   http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied.  See the License for the
+// specific language governing permissions and limitations
+// under the License.
+//
+
+package com.cloud.utils.ssh;
+
+import java.io.InputStream;
+import org.junit.Assert;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.mockito.Mockito;
+import org.powermock.api.mockito.PowerMockito;
+import org.powermock.core.classloader.annotations.PrepareForTest;
+import org.powermock.modules.junit4.PowerMockRunner;
+
+import com.trilead.ssh2.ChannelCondition;
+import com.trilead.ssh2.Session;
+
+@PrepareForTest({ Thread.class, SshHelper.class, Session.class })
+@RunWith(PowerMockRunner.class)
+public class SshHelperTest {
+
+    @Test(expected = SshException.class)
+    public void throwSshExceptionIfConditionsTimeout() throws SshException {
+        SshHelper.throwSshExceptionIfConditionsTimeout(ChannelCondition.TIMEOUT);
+    }
+
+    @Test
+    public void doNotThrowSshExceptionIfConditionsClosed() throws SshException {
+        SshHelper.throwSshExceptionIfConditionsTimeout(ChannelCondition.CLOSED);
+    }
+
+    @Test
+    public void doNotThrowSshExceptionIfConditionsStdout() throws SshException {
+        SshHelper.throwSshExceptionIfConditionsTimeout(ChannelCondition.STDOUT_DATA);
+    }
+
+    @Test
+    public void doNotThrowSshExceptionIfConditionsStderr() throws SshException {
+        SshHelper.throwSshExceptionIfConditionsTimeout(ChannelCondition.STDERR_DATA);
+    }
+
+    @Test
+    public void doNotThrowSshExceptionIfConditionsEof() throws SshException {
+        SshHelper.throwSshExceptionIfConditionsTimeout(ChannelCondition.EOF);
+    }
+
+    @Test
+    public void doNotThrowSshExceptionIfConditionsExitStatus() throws SshException {
+        SshHelper.throwSshExceptionIfConditionsTimeout(ChannelCondition.EXIT_STATUS);
+    }
+
+    @Test
+    public void doNotThrowSshExceptionIfConditionsExitSignal() throws SshException {
+        SshHelper.throwSshExceptionIfConditionsTimeout(ChannelCondition.EXIT_SIGNAL);
+    }
+
+    @Test
+    public void isChannelConditionEofTestTimeout() {
+        Assert.assertFalse(SshHelper.isChannelConditionEof(ChannelCondition.TIMEOUT));
+    }
+
+    @Test
+    public void isChannelConditionEofTestClosed() {
+        Assert.assertFalse(SshHelper.isChannelConditionEof(ChannelCondition.CLOSED));
+    }
+
+    @Test
+    public void isChannelConditionEofTestStdout() {
+        Assert.assertFalse(SshHelper.isChannelConditionEof(ChannelCondition.STDOUT_DATA));
+    }
+
+    @Test
+    public void isChannelConditionEofTestStderr() {
+        Assert.assertFalse(SshHelper.isChannelConditionEof(ChannelCondition.STDERR_DATA));
+    }
+
+    @Test
+    public void isChannelConditionEofTestEof() {
+        Assert.assertTrue(SshHelper.isChannelConditionEof(ChannelCondition.EOF));
+    }
+
+    @Test
+    public void isChannelConditionEofTestExitStatus() {
+        Assert.assertFalse(SshHelper.isChannelConditionEof(ChannelCondition.EXIT_STATUS));
+    }
+
+    @Test
+    public void isChannelConditionEofTestExitSignal() {
+        Assert.assertFalse(SshHelper.isChannelConditionEof(ChannelCondition.EXIT_SIGNAL));
+    }
+
+    @Test(expected = SshException.class)
+    public void throwSshExceptionIfStdoutOrStdeerIsNullTestNull() throws SshException {
+        SshHelper.throwSshExceptionIfStdoutOrStdeerIsNull(null, null);
+    }
+
+    @Test
+    public void throwSshExceptionIfStdoutOrStdeerIsNullTestNotNull() throws SshException {
+        InputStream inputStream = Mockito.mock(InputStream.class);
+        SshHelper.throwSshExceptionIfStdoutOrStdeerIsNull(inputStream, inputStream);
+    }
+
+    @Test
+    public void throwSshExceptionIfSshConnectionIsNullTestNotNull() throws Exception {
+        Session sess = Mockito.mock(Session.class);
+        PowerMockito.spy(Thread.class);
+        PowerMockito.doNothing().when(Thread.class, "sleep", Mockito.anyLong());
+
+        SshHelper.throwSshExceptionIfSshConnectionIsNull(sess);
+    }
+
+    @Test(expected = SshException.class)
+    public void throwSshExceptionIfSshConnectionIsNullTestNull() throws Exception {
+        PowerMockito.spy(Thread.class);
+        PowerMockito.doNothing().when(Thread.class, "sleep", Mockito.anyLong());
+
+        SshHelper.throwSshExceptionIfSshConnectionIsNull(null);
+    }
+
+}
