You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@cloudstack.apache.org by sw...@apache.org on 2016/04/21 23:00:18 UTC
[1/2] git commit: updated refs/heads/master to d518b61
Repository: cloudstack
Updated Branches:
refs/heads/master 549817046 -> d518b619d
Handle SSH if server "forget" to send exit status
Continued the work started by https://github.com/likitha
commit (b9181c6) from PR #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 #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 #561, I did not
cherry-picked to keep the master file, otherwise it would look that I
had changed all the file.
by me.
Project: http://git-wip-us.apache.org/repos/asf/cloudstack/repo
Commit: http://git-wip-us.apache.org/repos/asf/cloudstack/commit/abae9086
Tree: http://git-wip-us.apache.org/repos/asf/cloudstack/tree/abae9086
Diff: http://git-wip-us.apache.org/repos/asf/cloudstack/diff/abae9086
Branch: refs/heads/master
Commit: abae9086742b4b83c623a0d0ece0a410a4f1bc48
Parents: 5251eed
Author: gabrascher <ga...@hotmail.com>
Authored: Tue Apr 5 14:05:22 2016 -0300
Committer: gabrascher <ga...@hotmail.com>
Committed: Wed Apr 20 13:51:17 2016 -0300
----------------------------------------------------------------------
.../java/com/cloud/utils/ssh/SshHelper.java | 108 ++++++++++---
.../java/com/cloud/utils/ssh/SshHelperTest.java | 151 +++++++++++++++++++
2 files changed, 241 insertions(+), 18 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/cloudstack/blob/abae9086/utils/src/main/java/com/cloud/utils/ssh/SshHelper.java
----------------------------------------------------------------------
diff --git a/utils/src/main/java/com/cloud/utils/ssh/SshHelper.java b/utils/src/main/java/com/cloud/utils/ssh/SshHelper.java
index d3c88c8..4d7a852 100644
--- a/utils/src/main/java/com/cloud/utils/ssh/SshHelper.java
+++ b/utils/src/main/java/com/cloud/utils/ssh/SshHelper.java
@@ -20,18 +20,26 @@
package com.cloud.utils.ssh;
import java.io.File;
+import java.io.IOException;
import java.io.InputStream;
import org.apache.log4j.Logger;
import com.trilead.ssh2.ChannelCondition;
-
+import com.trilead.ssh2.Connection;
+import com.trilead.ssh2.Session;
import com.cloud.utils.Pair;
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 long 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 +48,19 @@ public class SshHelper {
}
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 +96,7 @@ public class SshHelper {
}
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 +131,8 @@ public class SshHelper {
}
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;
@@ -144,7 +153,7 @@ public class SshHelper {
throw new Exception(msg);
}
}
- sess = conn.openSession();
+ sess = openConnectionSession(conn);
sess.execCommand(command);
@@ -156,22 +165,22 @@ public class SshHelper {
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 class SshHelper {
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,66 @@ public class SshHelper {
conn.close();
}
}
+
+ /**
+ * It gets a {@link Session} from the given {@link Connection}; then, it waits
+ * {@value #WAITING_OPEN_SSH_SESSION} milliseconds before returning the session, given a time to
+ * ensure that the connection is open before proceeding the execution.
+ */
+ protected static Session openConnectionSession(Connection conn) throws IOException, InterruptedException {
+ Session sess = conn.openSession();
+ Thread.sleep(WAITING_OPEN_SSH_SESSION);
+ return sess;
+ }
+
+ /**
+ * 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.
+ */
+ 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}
+ */
+ 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}.
+ */
+ 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.
+ */
+ 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);
+ }
+ }
}
http://git-wip-us.apache.org/repos/asf/cloudstack/blob/abae9086/utils/src/test/java/com/cloud/utils/ssh/SshHelperTest.java
----------------------------------------------------------------------
diff --git a/utils/src/test/java/com/cloud/utils/ssh/SshHelperTest.java b/utils/src/test/java/com/cloud/utils/ssh/SshHelperTest.java
new file mode 100644
index 0000000..355a514
--- /dev/null
+++ b/utils/src/test/java/com/cloud/utils/ssh/SshHelperTest.java
@@ -0,0 +1,151 @@
+//
+// 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.IOException;
+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.Connection;
+import com.trilead.ssh2.Session;
+
+@PrepareForTest({ Thread.class, SshHelper.class })
+@RunWith(PowerMockRunner.class)
+public class SshHelperTest {
+
+ @Test
+ public void canEndTheSshConnectionTest() throws Exception {
+ PowerMockito.spy(SshHelper.class);
+ Session mockedSession = Mockito.mock(Session.class);
+
+ PowerMockito.doReturn(true).when(SshHelper.class, "isChannelConditionEof", Mockito.anyInt());
+ Mockito.when(mockedSession.waitForCondition(ChannelCondition.EXIT_STATUS, 1l)).thenReturn(0);
+ PowerMockito.doNothing().when(SshHelper.class, "throwSshExceptionIfConditionsTimeout", Mockito.anyInt());
+
+ SshHelper.canEndTheSshConnection(1, mockedSession, 0);
+
+ PowerMockito.verifyStatic();
+ SshHelper.isChannelConditionEof(Mockito.anyInt());
+ SshHelper.throwSshExceptionIfConditionsTimeout(Mockito.anyInt());
+
+ Mockito.verify(mockedSession).waitForCondition(ChannelCondition.EXIT_STATUS, 1l);
+ }
+
+ @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 openConnectionSessionTest() throws IOException, InterruptedException {
+ Connection conn = Mockito.mock(Connection.class);
+ PowerMockito.mockStatic(Thread.class);
+ SshHelper.openConnectionSession(conn);
+
+ Mockito.verify(conn).openSession();
+
+ PowerMockito.verifyStatic();
+ Thread.sleep(Mockito.anyLong());
+ }
+}
[2/2] git commit: updated refs/heads/master to d518b61
Posted by sw...@apache.org.
Merge pull request #1459 from GabrielBrascher/CLOUDSTACK-8611
This closes #561
CLOUDSTACK-8611:Handle SSH if server "forget" to send exit statusContinuing the work started by @likitha, I did not cherry-picked the
commit (b9181c689e0e7b5f1e28c81d73710196dfabd0ba) from PR <https://github.com/apache/cloudstack/pull/561> due to the fact that the path of that SshHelper class was different of the current SshHelper; that is because the fact that by cherry-picking it would seem that I had changed all the class as the code is from another file.
I made some changes from the cherry-picked commit adding @wilderrodrigues suggestions (create simple methods to have reusable code, make unit tests and create the `WAITING_OPEN_SSH_SESSION` variable to manipulate with the delay of 1000 milliseconds).
Also, I tried to simplify the logic by assuming that ....
if ((conditions & ChannelCondition.EXIT_STATUS) != 0) {
if ((conditions & (ChannelCondition.STDOUT_DATA | ChannelCondition.STDERR_DATA)) == 0) {
break;
}
}
... is the same as `((conditions & ChannelCondition.EXIT_STATUS) != 0) && ((conditions & (ChannelCondition.STDOUT_DATA | ChannelCondition.STDERR_DATA)) == 0)`. This expression has the following results according to each possible condition.
|Condition|Value|result
|-----------------|-------|------|
TIMEOUT | 0000001|false
CLOSED | 0000010 |false
STDERR_DATA | 0000100 | false
STDERR_DATA | 0001000 | false
EOF | 0010000 | false
EXIT_STATUS | 0100000 | **true**
EXIT_SIGNAL | 1000000 | false
After testing all the possibilities we can note that the condition of `(conditions & ChannelCondition.EXIT_STATUS) != 0` is sufficient; thus, the simplified "if" conditional can be:
`if ((conditions & ChannelCondition.EXIT_STATUS) != 0) {
break;
}`
This proposed work can be explained by quoting @likitha:
>CheckS2SVpnConnectionsCommand execution involves executing a script (checkbatchs2svpn.sh) in the virtual router. Once CS has opened a session to a virtual router and executed a script in the router, it waits indefinitely till the session either times out or the exit status of the remote process is available. But it is possible that an EOF is reached by the process in the router and the router never set the exit status.
>References -
>1. Some servers never send the exit status, or occasionally "forget" to do so (http://grepcode.com/file/repo1.maven.org/maven2/org.jvnet.hudson/trilead-ssh2/build212-hudson-1/com/trilead/ssh2/ChannelCondition.java).
>2. Get the exit code/status from the remote command - if available. Be careful - not all server implementations return this value - (http://grepcode.com/file/repo1.maven.org/maven2/org.jvnet.hudson/trilead-ssh2/build212-hudson-1/com/trilead/ssh2/Session.java#Session.waitForCondition%28int%2Clong%29).
* pr/1459:
Handle SSH if server "forget" to send exit status
Signed-off-by: Will Stevens <wi...@gmail.com>
Project: http://git-wip-us.apache.org/repos/asf/cloudstack/repo
Commit: http://git-wip-us.apache.org/repos/asf/cloudstack/commit/d518b619
Tree: http://git-wip-us.apache.org/repos/asf/cloudstack/tree/d518b619
Diff: http://git-wip-us.apache.org/repos/asf/cloudstack/diff/d518b619
Branch: refs/heads/master
Commit: d518b619dda69dde4ecc1640e6c007182c9a9b75
Parents: 5498170 abae908
Author: Will Stevens <wi...@gmail.com>
Authored: Thu Apr 21 16:58:23 2016 -0400
Committer: Will Stevens <wi...@gmail.com>
Committed: Thu Apr 21 16:59:16 2016 -0400
----------------------------------------------------------------------
.../java/com/cloud/utils/ssh/SshHelper.java | 108 ++++++++++---
.../java/com/cloud/utils/ssh/SshHelperTest.java | 151 +++++++++++++++++++
2 files changed, 241 insertions(+), 18 deletions(-)
----------------------------------------------------------------------