This is an automated email from the ASF dual-hosted git repository. Pearl1594 pushed a commit to branch store-lastsuccessful-MS in repository https://gitbox.apache.org/repos/asf/cloudstack.git
commit f064f5d2da59da97c8ac24cc07001727fd902ef9 Author: mprokopchuk <[email protected]> AuthorDate: Thu Jul 30 20:24:41 2026 -0700 Addressed code review comments --- .../src/main/java/com/cloud/agent/AgentShell.java | 17 +++++--- .../src/main/java/com/cloud/agent/IAgentShell.java | 3 +- .../test/java/com/cloud/agent/AgentShellTest.java | 51 ++++++++++++++++++++++ 3 files changed, 65 insertions(+), 6 deletions(-) diff --git a/agent/src/main/java/com/cloud/agent/AgentShell.java b/agent/src/main/java/com/cloud/agent/AgentShell.java index 2898a16bd2a..83e70540441 100644 --- a/agent/src/main/java/com/cloud/agent/AgentShell.java +++ b/agent/src/main/java/com/cloud/agent/AgentShell.java @@ -22,6 +22,7 @@ import java.io.IOException; import java.lang.reflect.Constructor; import java.lang.reflect.InvocationTargetException; import java.util.ArrayList; +import java.util.Arrays; import java.util.Collections; import java.util.Enumeration; import java.util.HashMap; @@ -159,10 +160,14 @@ public class AgentShell implements IAgentShell, Daemon { // Add the last successful setup host as a fallback option at the end of the host list. // This host is tried only after all configured hosts have failed, providing a // last-resort connection option since this host previously completed setup successfully. - if (StringUtils.isNotBlank(lastSetupCompletedHost) - && StringUtils.isNotBlank(_host) - && !_host.contains(lastSetupCompletedHost)) { - host = _host + "," + lastSetupCompletedHost; + if (StringUtils.isNotBlank(lastSetupCompletedHost) && StringUtils.isNotBlank(_host)) { + final String candidate = lastSetupCompletedHost.trim(); + // Match against the exact comma-separated entries so a substring (e.g. 10.0.0.1 in + // 10.0.0.10) does not wrongly suppress the fallback. + final boolean alreadyPresent = Arrays.stream(_host.split(",")) + .map(String::trim) + .anyMatch(candidate::equalsIgnoreCase); + host = alreadyPresent ? _host : _host + "," + candidate; } else { host = _host; } @@ -479,7 +484,9 @@ public class AgentShell implements IAgentShell, Daemon { @Override public void setLastSetupCompletedHost(String host) { - setPersistentProperty(null, AgentProperties.LAST_SETUP_COMPLETED_HOST.getName(), host); + if (StringUtils.isNotBlank(host)) { + setPersistentProperty(null, AgentProperties.LAST_SETUP_COMPLETED_HOST.getName(), host); + } } /** diff --git a/agent/src/main/java/com/cloud/agent/IAgentShell.java b/agent/src/main/java/com/cloud/agent/IAgentShell.java index c5c94c7e51d..c3cf41155bf 100644 --- a/agent/src/main/java/com/cloud/agent/IAgentShell.java +++ b/agent/src/main/java/com/cloud/agent/IAgentShell.java @@ -76,7 +76,8 @@ public interface IAgentShell { /** * Sets the last host where the agent successfully completed its setup process * and received a Ready command. This value is persisted across agent restarts - * and used to prioritize reconnection attempts to previously working hosts. + * and used as a last-resort fallback during reconnection: it is appended after + * the configured hosts and tried only once all of them have failed. * * @param host the hostname or IP address where the agent setup completed successfully */ diff --git a/agent/src/test/java/com/cloud/agent/AgentShellTest.java b/agent/src/test/java/com/cloud/agent/AgentShellTest.java index 6d9758cc3dc..7aa4540c663 100644 --- a/agent/src/test/java/com/cloud/agent/AgentShellTest.java +++ b/agent/src/test/java/com/cloud/agent/AgentShellTest.java @@ -369,4 +369,55 @@ public class AgentShellTest { agentPropertiesFileHandlerMocked.when(() -> AgentPropertiesFileHandler.getPropertyValue(Mockito.eq(AgentProperties.SSL_HANDSHAKE_TIMEOUT))).thenReturn(expected); Assert.assertEquals(expected, agentShellSpy.getSslHandshakeTimeout()); } + + private void mockLastSetupCompletedHost(String value) { + PowerMockito.mockStatic(AgentPropertiesFileHandler.class); + PowerMockito.when(AgentPropertiesFileHandler.getPropertyValue(Mockito.eq(AgentProperties.LAST_SETUP_COMPLETED_HOST))).thenReturn(value); + } + + @Test + @PrepareForTest(AgentPropertiesFileHandler.class) + public void getHostsTestAppendsLastSetupCompletedHostAsFallback() { + mockLastSetupCompletedHost("30.3.3.3"); + agentShellSpy.setHosts("10.1.1.1,20.2.2.2"); + + Assert.assertArrayEquals(new String[] {"10.1.1.1", "20.2.2.2", "30.3.3.3"}, agentShellSpy.getHosts()); + } + + @Test + @PrepareForTest(AgentPropertiesFileHandler.class) + public void getHostsTestSubstringHostDoesNotSuppressFallback() { + // 10.0.0.1 is a substring of 10.0.0.10 but not the same host, so it must still be appended. + mockLastSetupCompletedHost("10.0.0.1"); + agentShellSpy.setHosts("10.0.0.10"); + + Assert.assertArrayEquals(new String[] {"10.0.0.10", "10.0.0.1"}, agentShellSpy.getHosts()); + } + + @Test + @PrepareForTest(AgentPropertiesFileHandler.class) + public void getHostsTestExactMatchIsNotDuplicated() { + mockLastSetupCompletedHost("20.2.2.2"); + agentShellSpy.setHosts("10.1.1.1,20.2.2.2"); + + Assert.assertArrayEquals(new String[] {"10.1.1.1", "20.2.2.2"}, agentShellSpy.getHosts()); + } + + @Test + @PrepareForTest(AgentPropertiesFileHandler.class) + public void getHostsTestMatchIsCaseInsensitiveAndTrimmed() { + mockLastSetupCompletedHost(" HOSTA.EXAMPLE.COM "); + agentShellSpy.setHosts("hosta.example.com,hostb.example.com"); + + Assert.assertArrayEquals(new String[] {"hosta.example.com", "hostb.example.com"}, agentShellSpy.getHosts()); + } + + @Test + @PrepareForTest(AgentPropertiesFileHandler.class) + public void getHostsTestBlankLastSetupCompletedHostReturnsConfiguredHostsOnly() { + mockLastSetupCompletedHost(" "); + agentShellSpy.setHosts("10.1.1.1,20.2.2.2"); + + Assert.assertArrayEquals(new String[] {"10.1.1.1", "20.2.2.2"}, agentShellSpy.getHosts()); + } }
