From 0f3f2a09370a18301db28ec3d28efe746b6437c9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gabriel=20Beims=20Br=C3=A4scher?= Date: Wed, 30 Sep 2020 08:05:17 -0300 Subject: [PATCH] oobm: Retry redfish requests (#4352) It is not common, but HTTP requests can fail due to connection issues. In order to mitigate such situations and also improve logging, this PR enhances the Redfish request handling by adding an execution flow for re-trying HTTP requests; the retry happens only if the global settings redfish.retries is set to 1 or more retries; default is of 2 (two). One can disable the retries by setting redfish.retries to 0 (zero). --- .../RedfishOutOfBandManagementDriver.java | 8 ++- .../utils/redfish/RedfishClient.java | 39 +++++++++++- .../utils/redfish/RedfishClientTest.java | 63 ++++++++++++++++--- 3 files changed, 98 insertions(+), 12 deletions(-) diff --git a/plugins/outofbandmanagement-drivers/redfish/src/main/java/org/apache/cloudstack/outofbandmanagement/driver/redfish/RedfishOutOfBandManagementDriver.java b/plugins/outofbandmanagement-drivers/redfish/src/main/java/org/apache/cloudstack/outofbandmanagement/driver/redfish/RedfishOutOfBandManagementDriver.java index 3e863342bad..14a41e131a0 100644 --- a/plugins/outofbandmanagement-drivers/redfish/src/main/java/org/apache/cloudstack/outofbandmanagement/driver/redfish/RedfishOutOfBandManagementDriver.java +++ b/plugins/outofbandmanagement-drivers/redfish/src/main/java/org/apache/cloudstack/outofbandmanagement/driver/redfish/RedfishOutOfBandManagementDriver.java @@ -51,6 +51,10 @@ public class RedfishOutOfBandManagementDriver extends AdapterBase implements Out public static final ConfigKey USE_HTTPS = new ConfigKey("Advanced", Boolean.class, "redfish.use.https", "true", "Use HTTPS/SSL for all connections.", true, ConfigKey.Scope.Global); + public static final ConfigKey REDFISHT_REQUEST_MAX_RETRIES = new ConfigKey("Advanced", Integer.class, "redfish.retries", "2", + "Number of retries allowed if a Redfish REST request experiment connection issues. If set to 0 (zero) there will be no retries.", true, ConfigKey.Scope.Global); + + private static final String HTTP_STATUS_OK = String.valueOf(HttpStatus.SC_OK); @Override @@ -74,7 +78,7 @@ public class RedfishOutOfBandManagementDriver extends AdapterBase implements Out String username = outOfBandOptions.get(OutOfBandManagement.Option.USERNAME); String password = outOfBandOptions.get(OutOfBandManagement.Option.PASSWORD); String hostAddress = outOfBandOptions.get(OutOfBandManagement.Option.ADDRESS); - RedfishClient redfishClient = new RedfishClient(username, password, USE_HTTPS.value(), IGNORE_SSL_CERTIFICATE.value()); + RedfishClient redfishClient = new RedfishClient(username, password, USE_HTTPS.value(), IGNORE_SSL_CERTIFICATE.value(), REDFISHT_REQUEST_MAX_RETRIES.value()); RedfishClient.RedfishPowerState powerState = null; if (cmd.getPowerOperation() == OutOfBandManagement.PowerOperation.STATUS) { @@ -114,7 +118,7 @@ public class RedfishOutOfBandManagementDriver extends AdapterBase implements Out @Override public ConfigKey[] getConfigKeys() { - return new ConfigKey[] {IGNORE_SSL_CERTIFICATE, USE_HTTPS}; + return new ConfigKey[] {IGNORE_SSL_CERTIFICATE, USE_HTTPS, REDFISHT_REQUEST_MAX_RETRIES}; } } diff --git a/utils/src/main/java/org/apache/cloudstack/utils/redfish/RedfishClient.java b/utils/src/main/java/org/apache/cloudstack/utils/redfish/RedfishClient.java index 8166b703a02..8b211c02606 100644 --- a/utils/src/main/java/org/apache/cloudstack/utils/redfish/RedfishClient.java +++ b/utils/src/main/java/org/apache/cloudstack/utils/redfish/RedfishClient.java @@ -28,6 +28,7 @@ import java.nio.charset.StandardCharsets; import java.security.KeyManagementException; import java.security.NoSuchAlgorithmException; import java.util.Base64; +import java.util.concurrent.TimeUnit; import javax.net.ssl.HostnameVerifier; import javax.net.ssl.HttpsURLConnection; @@ -71,6 +72,7 @@ public class RedfishClient { private String password; private boolean useHttps; private boolean ignoreSsl; + private int redfishRequestMaxRetries; private final static String SYSTEMS_URL_PATH = "redfish/v1/Systems/"; private final static String COMPUTER_SYSTEM_RESET_URL_PATH = "/Actions/ComputerSystem.Reset"; @@ -81,6 +83,8 @@ public class RedfishClient { private final static String ODATA_ID = "@odata.id"; private final static String MEMBERS = "Members"; private final static String EXPECTED_HTTP_STATUS = "2XX"; + private final static int WAIT_FOR_REQUEST_RETRY = 2; + /** * Redfish Command type:
@@ -126,11 +130,12 @@ public class RedfishClient { ForceOff, ForceOn, ForceRestart, GracefulRestart, GracefulShutdown, Nmi, On, PowerCycle, PushPowerButton } - public RedfishClient(String username, String password, boolean useHttps, boolean ignoreSsl) { + public RedfishClient(String username, String password, boolean useHttps, boolean ignoreSsl, int redfishRequestRetries) { this.username = username; this.password = password; this.useHttps = useHttps; this.ignoreSsl = ignoreSsl; + this.redfishRequestMaxRetries = redfishRequestRetries; } protected String buildRequestUrl(String hostAddress, RedfishCmdType cmd, String resourceId) { @@ -213,10 +218,40 @@ public class RedfishClient { try { return client.execute(httpReq); } catch (IOException e) { - throw new RedfishException(String.format("Failed to execute POST request [URL: %s] due to exception.", url, e)); + if (redfishRequestMaxRetries == 0) { + throw new RedfishException(String.format("Failed to execute HTTP %s request [URL: %s] due to exception %s.", httpReq.getMethod(), url, e), e); + } + return retryHttpRequest(url, httpReq, client); } } + protected HttpResponse retryHttpRequest(String url, HttpRequestBase httpReq, HttpClient client) { + LOGGER.warn(String.format("Failed to execute HTTP %s request [URL: %s]. Executing the request again.", httpReq.getMethod(), url)); + HttpResponse response = null; + for (int attempt = 1; attempt < redfishRequestMaxRetries + 1; attempt++) { + try { + TimeUnit.SECONDS.sleep(WAIT_FOR_REQUEST_RETRY); + LOGGER.debug(String.format("Retry HTTP %s request [URL: %s], attempt %d/%d.", httpReq.getMethod(), url, attempt, redfishRequestMaxRetries)); + response = client.execute(httpReq); + } catch (IOException | InterruptedException e) { + if (attempt == redfishRequestMaxRetries) { + throw new RedfishException(String.format("Failed to execute HTTP %s request retry attempt %d/%d [URL: %s] due to exception %s", httpReq.getMethod(), attempt, redfishRequestMaxRetries,url, e)); + } else { + LOGGER.warn( + String.format("Failed to execute HTTP %s request retry attempt %d/%d [URL: %s] due to exception %s", httpReq.getMethod(), attempt, redfishRequestMaxRetries, + url, e)); + } + } + } + + if (response == null) { + throw new RedfishException(String.format("Failed to execute HTTP %s request [URL: %s].", httpReq.getMethod(), url)); + } + + LOGGER.debug(String.format("Successfully executed HTTP %s request [URL: %s].", httpReq.getMethod(), url)); + return response; + } + /** * Returns the proper URL path for the given Redfish command ({@link RedfishCmdType}). */ diff --git a/utils/src/test/java/org/apache/cloudstack/utils/redfish/RedfishClientTest.java b/utils/src/test/java/org/apache/cloudstack/utils/redfish/RedfishClientTest.java index 552f1874a4f..15a75bab212 100644 --- a/utils/src/test/java/org/apache/cloudstack/utils/redfish/RedfishClientTest.java +++ b/utils/src/test/java/org/apache/cloudstack/utils/redfish/RedfishClientTest.java @@ -19,23 +19,40 @@ package org.apache.cloudstack.utils.redfish; import org.apache.commons.httpclient.HttpStatus; +import org.apache.http.HttpResponse; import org.apache.http.StatusLine; +import org.apache.http.client.HttpClient; import org.apache.http.client.methods.CloseableHttpResponse; +import org.apache.http.client.methods.HttpGet; +import org.apache.http.client.methods.HttpRequestBase; import org.junit.Assert; import org.junit.Test; import org.junit.runner.RunWith; +import org.mockito.Mock; import org.mockito.Mockito; import org.mockito.junit.MockitoJUnitRunner; -@RunWith(MockitoJUnitRunner.class) public class RedfishClientTest { +import java.io.IOException; + +@RunWith(MockitoJUnitRunner.class) +public class RedfishClientTest { private static final String USERNAME = "user"; private static final String PASSWORD = "password"; private static final String oobAddress = "oob.host.address"; private static final String systemId = "SystemID.1"; private final static String COMPUTER_SYSTEM_RESET_URL_PATH = "/Actions/ComputerSystem.Reset"; + private final static Integer REDFISHT_REQUEST_RETRIES = Integer.valueOf(2); + private static final String url = "https://address.system.net/redfish/v1/Systems/"; + private static final HttpRequestBase httpReq = new HttpGet(url); - RedfishClient redfishClientspy = Mockito.spy(new RedfishClient(USERNAME, PASSWORD, true, true)); + @Mock + HttpClient client; + + @Mock + HttpResponse httpResponse; + + RedfishClient redfishClientspy = Mockito.spy(new RedfishClient(USERNAME, PASSWORD, true, true, REDFISHT_REQUEST_RETRIES)); @Test(expected = RedfishException.class) public void validateAddressAndPrepareForUrlTestExpect() { @@ -68,7 +85,7 @@ import org.mockito.junit.MockitoJUnitRunner; @Test public void buildRequestUrlTestHttpsGetSystemId() { - RedfishClient redfishclient = new RedfishClient(USERNAME, PASSWORD, true, false); + RedfishClient redfishclient = new RedfishClient(USERNAME, PASSWORD, true, false, REDFISHT_REQUEST_RETRIES); String result = redfishclient.buildRequestUrl(oobAddress, RedfishClient.RedfishCmdType.GetSystemId, systemId); String expected = String.format("https://%s/redfish/v1/Systems/", oobAddress, systemId); Assert.assertEquals(expected, result); @@ -76,7 +93,7 @@ import org.mockito.junit.MockitoJUnitRunner; @Test public void buildRequestUrlTestGetSystemId() { - RedfishClient redfishclient = new RedfishClient(USERNAME, PASSWORD, false, false); + RedfishClient redfishclient = new RedfishClient(USERNAME, PASSWORD, false, false, REDFISHT_REQUEST_RETRIES); String result = redfishclient.buildRequestUrl(oobAddress, RedfishClient.RedfishCmdType.GetSystemId, systemId); String expected = String.format("http://%s/redfish/v1/Systems/", oobAddress, systemId); Assert.assertEquals(expected, result); @@ -84,7 +101,7 @@ import org.mockito.junit.MockitoJUnitRunner; @Test public void buildRequestUrlTestHttpsComputerSystemReset() { - RedfishClient redfishclient = new RedfishClient(USERNAME, PASSWORD, true, false); + RedfishClient redfishclient = new RedfishClient(USERNAME, PASSWORD, true, false, REDFISHT_REQUEST_RETRIES); String result = redfishclient.buildRequestUrl(oobAddress, RedfishClient.RedfishCmdType.ComputerSystemReset, systemId); String expected = String.format("https://%s/redfish/v1/Systems/%s%s", oobAddress, systemId, COMPUTER_SYSTEM_RESET_URL_PATH); Assert.assertEquals(expected, result); @@ -92,7 +109,7 @@ import org.mockito.junit.MockitoJUnitRunner; @Test public void buildRequestUrlTestComputerSystemReset() { - RedfishClient redfishclient = new RedfishClient(USERNAME, PASSWORD, false, false); + RedfishClient redfishclient = new RedfishClient(USERNAME, PASSWORD, false, false, REDFISHT_REQUEST_RETRIES); String result = redfishclient.buildRequestUrl(oobAddress, RedfishClient.RedfishCmdType.ComputerSystemReset, systemId); String expected = String.format("http://%s/redfish/v1/Systems/%s%s", oobAddress, systemId, COMPUTER_SYSTEM_RESET_URL_PATH); Assert.assertEquals(expected, result); @@ -100,7 +117,7 @@ import org.mockito.junit.MockitoJUnitRunner; @Test public void buildRequestUrlTestHttpsGetPowerState() { - RedfishClient redfishclient = new RedfishClient(USERNAME, PASSWORD, true, false); + RedfishClient redfishclient = new RedfishClient(USERNAME, PASSWORD, true, false, REDFISHT_REQUEST_RETRIES); String result = redfishclient.buildRequestUrl(oobAddress, RedfishClient.RedfishCmdType.GetPowerState, systemId); String expected = String.format("https://%s/redfish/v1/Systems/%s", oobAddress, systemId); Assert.assertEquals(expected, result); @@ -108,7 +125,7 @@ import org.mockito.junit.MockitoJUnitRunner; @Test public void buildRequestUrlTestGetPowerState() { - RedfishClient redfishclient = new RedfishClient(USERNAME, PASSWORD, false, false); + RedfishClient redfishclient = new RedfishClient(USERNAME, PASSWORD, false, false, REDFISHT_REQUEST_RETRIES); String result = redfishclient.buildRequestUrl(oobAddress, RedfishClient.RedfishCmdType.GetPowerState, systemId); String expected = String.format("http://%s/redfish/v1/Systems/%s", oobAddress, systemId); Assert.assertEquals(expected, result); @@ -160,4 +177,34 @@ import org.mockito.junit.MockitoJUnitRunner; redfishClientspy.getSystemId(oobAddress); } + @Test(expected = RedfishException.class) + public void retryHttpRequestNoRetries() throws IOException { + RedfishClient newRedfishClientspy = Mockito.spy(new RedfishClient(USERNAME, PASSWORD, true, true, Integer.valueOf(0))); + newRedfishClientspy.retryHttpRequest(url, httpReq, client); + + Mockito.verify(newRedfishClientspy, Mockito.never()).retryHttpRequest(Mockito.anyString(), Mockito.any(), Mockito.any()); + Mockito.verify(client, Mockito.never()).execute(Mockito.any()); + } + + @Test(expected = RedfishException.class) + public void retryHttpRequestExceptionAfterOneRetry() throws IOException { + Mockito.when(client.execute(httpReq)).thenThrow(IOException.class).thenReturn(httpResponse); + + RedfishClient newRedfishClientspy = Mockito.spy(new RedfishClient(USERNAME, PASSWORD, true, true, Integer.valueOf(1))); + newRedfishClientspy.retryHttpRequest(url, httpReq, client); + + Mockito.verify(newRedfishClientspy, Mockito.never()).retryHttpRequest(Mockito.anyString(), Mockito.any(), Mockito.any()); + Mockito.verify(client, Mockito.never()).execute(Mockito.any()); + } + + @Test + public void retryHttpRequestNoException() throws IOException { + Mockito.when(client.execute(httpReq)).thenThrow(IOException.class).thenThrow(IOException.class).thenReturn(httpResponse); + + RedfishClient newRedfishClientspy = Mockito.spy(new RedfishClient(USERNAME, PASSWORD, true, true, Integer.valueOf(3))); + newRedfishClientspy.retryHttpRequest(url, httpReq, client); + + Mockito.verify(newRedfishClientspy, Mockito.times(1)).retryHttpRequest(Mockito.anyString(), Mockito.any(), Mockito.any()); + Mockito.verify(client, Mockito.times(3)).execute(Mockito.any()); + } }