Skip to content

Commit 64df4e3

Browse files
sureshanapartisureshanaparti
committed
review changes
1 parent d9b60bc commit 64df4e3

14 files changed

Lines changed: 178 additions & 58 deletions

File tree

‎agent/src/main/java/com/cloud/agent/HostConnectProcess.java‎

Lines changed: 10 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -103,10 +103,11 @@ public void scheduleConnectProcess(Link link, boolean connectionTransfer) {
103103
logger.debug("Scheduling connect process for {}", link);
104104
initExecutors();
105105

106-
var task = new HostStatusTask(link, connectionTransfer, agent, hostStatusFutureRef);
106+
var task = new HostStatusTask(link, connectionTransfer, agent);
107107
var future = hostStatusExecutor.scheduleWithFixedDelay(ThreadContextUtil.wrapThreadContext(task),
108108
HOST_STATUS_CHECK_INITIAL_DELAY_SEC,
109109
hostStatusCheckDelaySec, TimeUnit.SECONDS);
110+
task.setFuture(future);
110111
hostStatusFutureRef.set(future);
111112
}
112113

@@ -135,20 +136,23 @@ public static class HostStatusTask implements Runnable, AsyncSend {
135136
private final Link link;
136137
private final boolean forceConnect;
137138
private final Agent agent;
138-
private final AtomicReference<? extends ScheduledFuture<?>> futureRef;
139+
// each task generation cancels only its own schedule, never a newer connect process
140+
private final AtomicReference<ScheduledFuture<?>> ownFutureRef = new AtomicReference<>();
139141

140-
public HostStatusTask(Link link, boolean forceConnect, Agent agent,
141-
AtomicReference<? extends ScheduledFuture<?>> futureRef) {
142+
public HostStatusTask(Link link, boolean forceConnect, Agent agent) {
142143
logger.debug("{} created", this.getClass().getSimpleName());
143144
this.link = link;
144145
this.forceConnect = forceConnect;
145146
this.agent = agent;
146-
this.futureRef = futureRef;
147+
}
148+
149+
public void setFuture(ScheduledFuture<?> future) {
150+
ownFutureRef.set(future);
147151
}
148152

149153
private void cancel() {
150154
logger.debug("Cancelling future");
151-
Optional.ofNullable(futureRef.get())
155+
Optional.ofNullable(ownFutureRef.get())
152156
.filter(Predicate.not(ScheduledFuture::isCancelled))
153157
.ifPresent(future -> future.cancel(true));
154158
logger.debug("Cancelled future");

‎api/src/main/java/com/cloud/agent/api/Command.java‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -117,6 +117,10 @@ public String getTraceContextParam(String name) {
117117
return traceContextMap.get(name);
118118
}
119119

120+
public void removeTraceContextParam(String name) {
121+
traceContextMap.remove(name);
122+
}
123+
120124
public boolean allowCaching() {
121125
return true;
122126
}

‎core/src/main/java/org/apache/cloudstack/threadcontext/ThreadContextCommandUtil.java‎

Lines changed: 11 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -59,16 +59,18 @@ private static void putOrRemove(String mdcKey, String value) {
5959
* @param cmd the command to set trace context parameters on
6060
*/
6161
public static void setContextInCommand(Command cmd) {
62-
if (cmd != null) {
63-
String logContextId = (String) ThreadContext.get(ThreadContextUtil.MDC_LOG_CONTEXT_ID_KEY);
64-
if (StringUtils.isNotEmpty(logContextId)) {
65-
cmd.setTraceContextParam(ThreadContextUtil.CONTEXT_LOG_ID_KEY, logContextId);
66-
}
62+
if (cmd == null) {
63+
return;
64+
}
65+
setOrRemove(cmd, ThreadContextUtil.CONTEXT_LOG_ID_KEY, (String) ThreadContext.get(ThreadContextUtil.MDC_LOG_CONTEXT_ID_KEY));
66+
setOrRemove(cmd, ThreadContextUtil.CONTEXT_UUID_KEY, (String) ThreadContext.get(ThreadContextUtil.MDC_UUID_KEY));
67+
}
6768

68-
String uuid = (String) ThreadContext.get(ThreadContextUtil.MDC_UUID_KEY);
69-
if (StringUtils.isNotEmpty(uuid)) {
70-
cmd.setTraceContextParam(ThreadContextUtil.CONTEXT_UUID_KEY, uuid);
71-
}
69+
private static void setOrRemove(Command cmd, String contextKey, String value) {
70+
if (StringUtils.isNotEmpty(value)) {
71+
cmd.setTraceContextParam(contextKey, value);
72+
} else {
73+
cmd.removeTraceContextParam(contextKey);
7274
}
7375
}
7476
}

‎engine/orchestration/src/main/java/com/cloud/agent/manager/ClusteredAgentManagerImpl.java‎

Lines changed: 9 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -81,6 +81,7 @@
8181
import com.cloud.cluster.ClusterManagerListener;
8282
import com.cloud.cluster.ClusterServicePdu;
8383
import com.cloud.cluster.ClusteredAgentRebalanceService;
84+
import com.cloud.cluster.ManagementServerAddressUtil;
8485
import com.cloud.cluster.ManagementServerHostVO;
8586
import com.cloud.cluster.agentlb.AgentLoadBalancerPlanner;
8687
import com.cloud.cluster.agentlb.HostTransferMapVO;
@@ -589,25 +590,20 @@ public SocketChannel connectToPeer(final String peerName, final SocketChannel pr
589590
}
590591

591592
/**
592-
* Gets the hostname or IP address to use for connecting to a management server.
593-
* Prefers hostname (for CNAME support) but falls back to IP if hostname is null.
593+
* Gets the hostname or IP address to use for connecting to a management server, in the same format as the
594+
* configured management server address list (hostname when that list holds hostnames, for CNAME support,
595+
* otherwise service IP), falling back to whichever of the two is set.
594596
*
595597
* @param host the management server host
596-
* @return hostname if available, otherwise service IP
598+
* @return the address to connect to
597599
* @throws CloudRuntimeException if both hostname and IP are null
598600
*/
599601
private String getHostIdentifier(final ManagementServerHost host) {
600-
final String hostName = host.getName();
601-
if (StringUtils.isNotBlank(hostName)) {
602-
return hostName;
602+
final String identifier = ManagementServerAddressUtil.getPreferredAddress(host.getName(), host.getServiceIP());
603+
if (StringUtils.isBlank(identifier)) {
604+
throw new CloudRuntimeException("Management server host has neither hostname nor IP address: " + host);
603605
}
604-
605-
final String serviceIP = host.getServiceIP();
606-
if (StringUtils.isNotBlank(serviceIP)) {
607-
return serviceIP;
608-
}
609-
610-
throw new CloudRuntimeException("Management server host has neither hostname nor IP address: " + host);
606+
return identifier;
611607
}
612608

613609
private InetAddress resolveManagementServerAddress(final ManagementServerHost host) {

‎framework/cluster/src/main/java/com/cloud/cluster/ClusterManagerImpl.java‎

Lines changed: 10 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1256,19 +1256,23 @@ private boolean pingManagementNode(final ManagementServerHostVO mshost) {
12561256
final String targetHostName = mshost.getName();
12571257
final String currentHostName = NetUtils.getCanonicalHostName();
12581258

1259-
// Prefer hostname (CNAME support) but fall back to the non-null service IP when name is unset.
1260-
final String connectTarget = StringUtils.isNotBlank(targetHostName) ? targetHostName : targetIp;
1261-
1262-
// Check for self-detection using hostname comparison (for CNAME support)
1263-
if (currentHostName != null && currentHostName.equals(targetHostName)) {
1264-
logger.info("ping management node cluster service can not be performed on self (hostname match: {})", currentHostName);
1259+
if (mshost.getMsid() == _msId) {
1260+
logger.info("ping management node cluster service can not be performed on self (msid: {})", _msId);
12651261
return false;
12661262
}
12671263
if (LOCALHOST_IP.equals(targetIp) || ANY_IP.equals(targetIp)) {
12681264
logger.info("ping management node cluster service can not be performed on self");
12691265
return false;
12701266
}
12711267

1268+
// a peer sharing this node's hostname can only be reached by its IP because the name resolves to this node
1269+
final boolean sameHostName = currentHostName != null && currentHostName.equalsIgnoreCase(targetHostName);
1270+
final String connectTarget = sameHostName ? targetIp : ManagementServerAddressUtil.getPreferredAddress(targetHostName, targetIp);
1271+
if (StringUtils.isBlank(connectTarget)) {
1272+
logger.warn("Unable to ping management node {} (msid: {}), it has neither hostname nor service IP", mshost.getId(), mshost.getMsid());
1273+
return false;
1274+
}
1275+
12721276
int retry = 10;
12731277
while (--retry > 0) {
12741278
SocketChannel sch = null;

‎framework/cluster/src/main/java/com/cloud/cluster/ClusterServiceServletAdapter.java‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@
2323
import javax.inject.Inject;
2424
import javax.naming.ConfigurationException;
2525

26+
import com.cloud.utils.StringUtils;
2627
import com.cloud.utils.exception.CloudRuntimeException;
2728
import org.apache.cloudstack.ca.CAManager;
2829
import org.apache.cloudstack.framework.config.ConfigDepot;
@@ -94,10 +95,9 @@ public int getServicePort() {
9495
}
9596

9697
private String composeEndpointName(ManagementServerHostVO msHost, int port) {
97-
final String hostName = msHost.getName();
98-
final String identifier = hostName != null ? hostName : msHost.getServiceIP();
98+
final String identifier = ManagementServerAddressUtil.getPreferredAddress(msHost.getName(), msHost.getServiceIP());
9999

100-
if (identifier == null) {
100+
if (StringUtils.isBlank(identifier)) {
101101
throw new CloudRuntimeException("Management server host has neither hostname nor IP address: " + msHost);
102102
}
103103

‎framework/cluster/src/main/java/com/cloud/cluster/ManagementServerAddressUtil.java‎

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -119,6 +119,26 @@ public static boolean isManagementServerAddressListUsingHostnames() {
119119
return hasHostname;
120120
}
121121

122+
/**
123+
* Picks the address other nodes should use to reach a management server, in the same format as the
124+
* 'host' configuration: the persisted hostname when that list holds hostnames, otherwise the service IP.
125+
* Falls back to whichever of the two is set, so an IP-only deployment never depends on DNS for the
126+
* canonical hostname that {@code ClusterManagerImpl} persists.
127+
*
128+
* @param hostName the hostname persisted for the management server, may be blank
129+
* @param serviceIP the service IP persisted for the management server, may be blank
130+
* @return the preferred address, or null when neither is set
131+
*/
132+
public static String getPreferredAddress(String hostName, String serviceIP) {
133+
final boolean preferHostName = isManagementServerAddressListUsingHostnames();
134+
final String primary = preferHostName ? hostName : serviceIP;
135+
final String fallback = preferHostName ? serviceIP : hostName;
136+
if (StringUtils.isNotBlank(primary)) {
137+
return primary;
138+
}
139+
return StringUtils.isNotBlank(fallback) ? fallback : null;
140+
}
141+
122142
/**
123143
* Returns the entries of the 'host' configuration (ApiServiceConfiguration.ManagementServerAddresses) as a list,
124144
* in the configured order.

‎framework/cluster/src/test/java/com/cloud/cluster/ManagementServerAddressUtilTest.java‎

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -190,4 +190,21 @@ public void testConfiguredAddressesSkipUnresolvableAliases() throws Exception {
190190
assertEquals(List.of("ms1.example.com"),
191191
ManagementServerAddressUtil.getConfiguredAddresses(List.of("ms1.example.com"), List.of("192.168.1.1")));
192192
}
193+
194+
@Test
195+
public void testPreferredAddressFollowsConfiguredFormat() throws Exception {
196+
setConfigValue("ms1.example.com,ms2.example.com");
197+
assertEquals("ms1.example.com", ManagementServerAddressUtil.getPreferredAddress("ms1.example.com", "192.168.1.1"));
198+
setConfigValue("192.168.1.1,192.168.1.2");
199+
assertEquals("192.168.1.1", ManagementServerAddressUtil.getPreferredAddress("ms1.example.com", "192.168.1.1"));
200+
}
201+
202+
@Test
203+
public void testPreferredAddressFallsBackToWhateverIsSet() throws Exception {
204+
setConfigValue("192.168.1.1,192.168.1.2");
205+
assertEquals("ms1.example.com", ManagementServerAddressUtil.getPreferredAddress("ms1.example.com", " "));
206+
setConfigValue("ms1.example.com,ms2.example.com");
207+
assertEquals("192.168.1.1", ManagementServerAddressUtil.getPreferredAddress(null, "192.168.1.1"));
208+
assertEquals(null, ManagementServerAddressUtil.getPreferredAddress(null, null));
209+
}
193210
}

‎server/src/main/java/org/apache/cloudstack/agent/lb/IndirectAgentLBServiceImpl.java‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -529,7 +529,7 @@ private void logFailedMigrations(String hostKind, Set<Long> failedHostIds, long
529529
if (failedHostIds.isEmpty()) {
530530
return;
531531
}
532-
logger.warn(String.format("Failed to migrate %d %s host agent(s) from management server node %d (id: %s), host IDs: %s",
532+
logger.warn(String.format("Migration of %d %s host agent(s) from management server node %d (id: %s) failed or was not confirmed, host IDs: %s",
533533
failedHostIds.size(), hostKind, fromMsId, fromMsUuid, failedHostIds));
534534
}
535535

@@ -631,6 +631,7 @@ protected void runInContext() {
631631
cmd.setWait(IndirectAgentLBMigrateCommandWait.value());
632632
final Answer answer = agentManager.easySend(hostId, cmd); //may not receive answer when the agent disconnects immediately and try reconnecting to other ms host
633633
if (answer == null) {
634+
failedHostIds.add(hostId);
634635
logger.warn(String.format("Got empty answer while initiating migration of agent connection for host agent ID: %d", hostId));
635636
} else if (!answer.getResult()) {
636637
failedHostIds.add(hostId);

‎utils/src/main/java/com/cloud/utils/backoff/impl/ExponentialWithJitterBackoff.java‎

Lines changed: 8 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -92,25 +92,23 @@ public void waitBeforeRetry() {
9292
/**
9393
* Calculates next attempt and direction.
9494
*/
95-
private void calculateNextAttempt() {
95+
private synchronized void calculateNextAttempt() {
9696
if (increasing) {
9797
int nextAttemptNumber = attemptNumber + 1;
98-
increasing = getNextDelay() <= maxDelayMs && nextAttemptNumber <= maxAttempts;
98+
increasing = nextAttemptNumber <= maxAttempts;
9999
if (increasing) {
100100
attemptNumber = nextAttemptNumber;
101101
}
102102
} else {
103-
int nextAttemptNumber = Math.max(attemptNumber - 1, 0);
104-
increasing = nextAttemptNumber == 0;
105-
if (!increasing) {
106-
attemptNumber = nextAttemptNumber;
107-
}
103+
attemptNumber = Math.max(attemptNumber - 1, 0);
104+
increasing = attemptNumber == 0;
108105
}
109106
}
110107

111108
@Override
112-
public void reset() {
109+
public synchronized void reset() {
113110
attemptNumber = 0;
111+
increasing = true;
114112
}
115113

116114
@Override
@@ -123,7 +121,7 @@ public Map<String, String> getConfiguration() {
123121
}
124122

125123
@Override
126-
public boolean configure(String name, Map<String, Object> params) {
124+
public synchronized boolean configure(String name, Map<String, Object> params) {
127125
minDelayMs = NumbersUtil.parseInt((String) params.get(MIN_DELAY_MS_CONFIG_KEY), MIN_DELAY_MS_DEFAULT);
128126
maxDelayMs = NumbersUtil.parseInt((String) params.get(MAX_DELAY_MS_CONFIG_KEY), MAX_DELAY_MS_DEFAULT);
129127
maxAttempts = (int) Math.round(Math.log((double) maxDelayMs / minDelayMs) / Math.log(2));
@@ -154,15 +152,10 @@ private long getNextDelay() {
154152
}
155153

156154
@Override
157-
public long getTimeToWait() {
155+
public synchronized long getTimeToWait() {
158156
long delay = Math.max(1L, getNextDelay());
159157
int jitterBound = (int) Math.max(1L, delay / 2);
160158
int jitter = random.nextInt(jitterBound);
161159
return delay + jitter;
162160
}
163-
164-
@Override
165-
public void setTimeToWait(long seconds) {
166-
// ignore
167-
}
168161
}

0 commit comments

Comments
 (0)