Skip to content

Commit f064f5d

Browse files
mprokopchukPearl1594
authored andcommitted
Addressed code review comments
1 parent a4d05c2 commit f064f5d

3 files changed

Lines changed: 65 additions & 6 deletions

File tree

agent/src/main/java/com/cloud/agent/AgentShell.java

Lines changed: 12 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@
2222
import java.lang.reflect.Constructor;
2323
import java.lang.reflect.InvocationTargetException;
2424
import java.util.ArrayList;
25+
import java.util.Arrays;
2526
import java.util.Collections;
2627
import java.util.Enumeration;
2728
import java.util.HashMap;
@@ -159,10 +160,14 @@ public String[] getHosts() {
159160
// Add the last successful setup host as a fallback option at the end of the host list.
160161
// This host is tried only after all configured hosts have failed, providing a
161162
// last-resort connection option since this host previously completed setup successfully.
162-
if (StringUtils.isNotBlank(lastSetupCompletedHost)
163-
&& StringUtils.isNotBlank(_host)
164-
&& !_host.contains(lastSetupCompletedHost)) {
165-
host = _host + "," + lastSetupCompletedHost;
163+
if (StringUtils.isNotBlank(lastSetupCompletedHost) && StringUtils.isNotBlank(_host)) {
164+
final String candidate = lastSetupCompletedHost.trim();
165+
// Match against the exact comma-separated entries so a substring (e.g. 10.0.0.1 in
166+
// 10.0.0.10) does not wrongly suppress the fallback.
167+
final boolean alreadyPresent = Arrays.stream(_host.split(","))
168+
.map(String::trim)
169+
.anyMatch(candidate::equalsIgnoreCase);
170+
host = alreadyPresent ? _host : _host + "," + candidate;
166171
} else {
167172
host = _host;
168173
}
@@ -479,7 +484,9 @@ public Integer getSslHandshakeTimeout() {
479484

480485
@Override
481486
public void setLastSetupCompletedHost(String host) {
482-
setPersistentProperty(null, AgentProperties.LAST_SETUP_COMPLETED_HOST.getName(), host);
487+
if (StringUtils.isNotBlank(host)) {
488+
setPersistentProperty(null, AgentProperties.LAST_SETUP_COMPLETED_HOST.getName(), host);
489+
}
483490
}
484491

485492
/**

agent/src/main/java/com/cloud/agent/IAgentShell.java

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -76,7 +76,8 @@ public interface IAgentShell {
7676
/**
7777
* Sets the last host where the agent successfully completed its setup process
7878
* and received a Ready command. This value is persisted across agent restarts
79-
* and used to prioritize reconnection attempts to previously working hosts.
79+
* and used as a last-resort fallback during reconnection: it is appended after
80+
* the configured hosts and tried only once all of them have failed.
8081
*
8182
* @param host the hostname or IP address where the agent setup completed successfully
8283
*/

agent/src/test/java/com/cloud/agent/AgentShellTest.java

Lines changed: 51 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -369,4 +369,55 @@ public void testGetSslHandshakeTimeout() {
369369
agentPropertiesFileHandlerMocked.when(() -> AgentPropertiesFileHandler.getPropertyValue(Mockito.eq(AgentProperties.SSL_HANDSHAKE_TIMEOUT))).thenReturn(expected);
370370
Assert.assertEquals(expected, agentShellSpy.getSslHandshakeTimeout());
371371
}
372+
373+
private void mockLastSetupCompletedHost(String value) {
374+
PowerMockito.mockStatic(AgentPropertiesFileHandler.class);
375+
PowerMockito.when(AgentPropertiesFileHandler.getPropertyValue(Mockito.eq(AgentProperties.LAST_SETUP_COMPLETED_HOST))).thenReturn(value);
376+
}
377+
378+
@Test
379+
@PrepareForTest(AgentPropertiesFileHandler.class)
380+
public void getHostsTestAppendsLastSetupCompletedHostAsFallback() {
381+
mockLastSetupCompletedHost("30.3.3.3");
382+
agentShellSpy.setHosts("10.1.1.1,20.2.2.2");
383+
384+
Assert.assertArrayEquals(new String[] {"10.1.1.1", "20.2.2.2", "30.3.3.3"}, agentShellSpy.getHosts());
385+
}
386+
387+
@Test
388+
@PrepareForTest(AgentPropertiesFileHandler.class)
389+
public void getHostsTestSubstringHostDoesNotSuppressFallback() {
390+
// 10.0.0.1 is a substring of 10.0.0.10 but not the same host, so it must still be appended.
391+
mockLastSetupCompletedHost("10.0.0.1");
392+
agentShellSpy.setHosts("10.0.0.10");
393+
394+
Assert.assertArrayEquals(new String[] {"10.0.0.10", "10.0.0.1"}, agentShellSpy.getHosts());
395+
}
396+
397+
@Test
398+
@PrepareForTest(AgentPropertiesFileHandler.class)
399+
public void getHostsTestExactMatchIsNotDuplicated() {
400+
mockLastSetupCompletedHost("20.2.2.2");
401+
agentShellSpy.setHosts("10.1.1.1,20.2.2.2");
402+
403+
Assert.assertArrayEquals(new String[] {"10.1.1.1", "20.2.2.2"}, agentShellSpy.getHosts());
404+
}
405+
406+
@Test
407+
@PrepareForTest(AgentPropertiesFileHandler.class)
408+
public void getHostsTestMatchIsCaseInsensitiveAndTrimmed() {
409+
mockLastSetupCompletedHost(" HOSTA.EXAMPLE.COM ");
410+
agentShellSpy.setHosts("hosta.example.com,hostb.example.com");
411+
412+
Assert.assertArrayEquals(new String[] {"hosta.example.com", "hostb.example.com"}, agentShellSpy.getHosts());
413+
}
414+
415+
@Test
416+
@PrepareForTest(AgentPropertiesFileHandler.class)
417+
public void getHostsTestBlankLastSetupCompletedHostReturnsConfiguredHostsOnly() {
418+
mockLastSetupCompletedHost(" ");
419+
agentShellSpy.setHosts("10.1.1.1,20.2.2.2");
420+
421+
Assert.assertArrayEquals(new String[] {"10.1.1.1", "20.2.2.2"}, agentShellSpy.getHosts());
422+
}
372423
}

0 commit comments

Comments
 (0)