Copilot commented on code in PR #11376:
URL: https://github.com/apache/cloudstack/pull/11376#discussion_r2251387466
##########
engine/orchestration/src/main/java/com/cloud/agent/manager/AgentManagerImpl.java:
##########
@@ -1040,37 +1040,50 @@ protected Status getNextStatusOnDisconnection(Host
host, final Status.Event even
protected boolean handleDisconnectWithoutInvestigation(final AgentAttache
attache, final Status.Event event, final boolean transitState, final boolean
removeAgent) {
final long hostId = attache.getId();
-
+ final HostVO host = _hostDao.findById(hostId);
boolean result = false;
GlobalLock joinLock = getHostJoinLock(hostId);
- if (joinLock.lock(60)) {
- try {
- logger.info("Host {} is disconnecting with event {}",
- attache, event);
- Status nextStatus;
- final HostVO host = _hostDao.findById(hostId);
- if (host == null) {
- logger.warn("Can't find host with {} ({})", hostId,
attache);
- nextStatus = Status.Removed;
- } else {
- nextStatus = getNextStatusOnDisconnection(host, event);
- caService.purgeHostCertificate(host);
- }
- logger.debug("Deregistering link for {} with state {}",
attache, nextStatus);
+ try {
+ if (!joinLock.lock(60)) {
+ logger.debug("Unable to acquire lock on host {} to process
agent disconnection", host != null? host : hostId);
+ return result;
+ }
- removeAgent(attache, nextStatus);
+ logger.debug("Acquired lock on host {}, to process agent
disconnection", host != null? host : hostId);
Review Comment:
[nitpick] The ternary operator with null check can be simplified. Consider
using `Objects.toString(host, String.valueOf(hostId))` or similar approach for
cleaner code.
```suggestion
logger.debug("Unable to acquire lock on host {} to process
agent disconnection", Objects.toString(host, String.valueOf(hostId)));
return result;
}
logger.debug("Acquired lock on host {}, to process agent
disconnection", Objects.toString(host, String.valueOf(hostId)));
```
##########
engine/orchestration/src/main/java/com/cloud/agent/manager/AgentManagerImpl.java:
##########
@@ -1040,37 +1040,50 @@ protected Status getNextStatusOnDisconnection(Host
host, final Status.Event even
protected boolean handleDisconnectWithoutInvestigation(final AgentAttache
attache, final Status.Event event, final boolean transitState, final boolean
removeAgent) {
final long hostId = attache.getId();
-
+ final HostVO host = _hostDao.findById(hostId);
boolean result = false;
GlobalLock joinLock = getHostJoinLock(hostId);
- if (joinLock.lock(60)) {
- try {
- logger.info("Host {} is disconnecting with event {}",
- attache, event);
- Status nextStatus;
- final HostVO host = _hostDao.findById(hostId);
- if (host == null) {
- logger.warn("Can't find host with {} ({})", hostId,
attache);
- nextStatus = Status.Removed;
- } else {
- nextStatus = getNextStatusOnDisconnection(host, event);
- caService.purgeHostCertificate(host);
- }
- logger.debug("Deregistering link for {} with state {}",
attache, nextStatus);
+ try {
+ if (!joinLock.lock(60)) {
+ logger.debug("Unable to acquire lock on host {} to process
agent disconnection", host != null? host : hostId);
+ return result;
+ }
- removeAgent(attache, nextStatus);
+ logger.debug("Acquired lock on host {}, to process agent
disconnection", host != null? host : hostId);
Review Comment:
[nitpick] The ternary operator with null check can be simplified. Consider
using `Objects.toString(host, String.valueOf(hostId))` or similar approach for
cleaner code.
```suggestion
logger.debug("Unable to acquire lock on host {} to process
agent disconnection", Objects.toString(host, String.valueOf(hostId)));
return result;
}
logger.debug("Acquired lock on host {}, to process agent
disconnection", Objects.toString(host, String.valueOf(hostId)));
```
##########
engine/orchestration/src/main/java/com/cloud/agent/manager/AgentManagerImpl.java:
##########
@@ -1040,37 +1040,50 @@ protected Status getNextStatusOnDisconnection(Host
host, final Status.Event even
protected boolean handleDisconnectWithoutInvestigation(final AgentAttache
attache, final Status.Event event, final boolean transitState, final boolean
removeAgent) {
final long hostId = attache.getId();
-
+ final HostVO host = _hostDao.findById(hostId);
boolean result = false;
GlobalLock joinLock = getHostJoinLock(hostId);
- if (joinLock.lock(60)) {
- try {
- logger.info("Host {} is disconnecting with event {}",
- attache, event);
- Status nextStatus;
- final HostVO host = _hostDao.findById(hostId);
- if (host == null) {
- logger.warn("Can't find host with {} ({})", hostId,
attache);
- nextStatus = Status.Removed;
- } else {
- nextStatus = getNextStatusOnDisconnection(host, event);
- caService.purgeHostCertificate(host);
- }
- logger.debug("Deregistering link for {} with state {}",
attache, nextStatus);
+ try {
+ if (!joinLock.lock(60)) {
+ logger.debug("Unable to acquire lock on host {} to process
agent disconnection", host != null? host : hostId);
+ return result;
+ }
- removeAgent(attache, nextStatus);
+ logger.debug("Acquired lock on host {}, to process agent
disconnection", host != null? host : hostId);
+ disconnectHostAgent(attache, event, host, transitState, joinLock);
+ result = true;
+ } finally {
+ joinLock.releaseRef();
+ }
- if (host != null && transitState) {
- // update the state for host in DB as per the event
- disconnectAgent(host, event, _nodeId);
- }
- } finally {
+ return result;
+ }
+
+ private void disconnectHostAgent(final AgentAttache attache, final
Status.Event event, final HostVO host, final boolean transitState, final
GlobalLock joinLock) {
+ try {
+ logger.info("Host {} is disconnecting with event {}", attache,
event);
+ final long hostId = attache.getId();
+ Status nextStatus;
+ if (host == null) {
+ logger.warn("Can't find host with {} ({})", hostId, attache);
+ nextStatus = Status.Removed;
+ } else {
+ nextStatus = getNextStatusOnDisconnection(host, event);
+ caService.purgeHostCertificate(host);
+ }
+ logger.debug("Deregistering link for {} with state {}", attache,
nextStatus);
+
+ removeAgent(attache, nextStatus);
+
+ if (host != null && transitState) {
+ // update the state for host in DB as per the event
+ disconnectAgent(host, event, _nodeId);
+ }
+ } finally {
+ if (joinLock != null) {
Review Comment:
The null check for `joinLock` is unnecessary since it's passed as a
parameter from a method that ensures it's not null. This check adds unnecessary
complexity.
##########
engine/orchestration/src/main/java/com/cloud/agent/manager/AgentManagerImpl.java:
##########
@@ -1341,45 +1354,60 @@ protected AgentAttache createAttacheForConnect(final
HostVO host, final Link lin
return attache;
}
- private AgentAttache sendReadyAndGetAttache(HostVO host, ReadyCommand
ready, Link link, StartupCommand[] startup) throws ConnectionException {
- final List<String> agentMSHostList = new ArrayList<>();
- String lbAlgorithm = null;
- if (startup != null && startup.length > 0) {
- final String agentMSHosts = startup[0].getMsHostList();
- if (StringUtils.isNotEmpty(agentMSHosts)) {
- String[] msHosts = agentMSHosts.split("@");
- if (msHosts.length > 1) {
- lbAlgorithm = msHosts[1];
- }
- agentMSHostList.addAll(Arrays.asList(msHosts[0].split(",")));
- }
- }
- ready.setArch(host.getArch().getType());
+ private AgentAttache sendReadyAndGetAttache(HostVO host, ReadyCommand
ready, Link link, StartupCommand[] startupCmds) throws ConnectionException {
AgentAttache attache;
GlobalLock joinLock = getHostJoinLock(host.getId());
- if (joinLock.lock(60)) {
- try {
+ try {
+ if (!joinLock.lock(60)) {
+ throw new ConnectionException(true, String.format("Unable to
acquire lock on host %s, to process agent connection", host));
+ }
+
+ logger.debug("Acquired lock on host {}, to process agent
connection", host);
+ attache = connectHostAgent(host, ready, link, startupCmds,
joinLock);
+ } finally {
+ joinLock.releaseRef();
+ }
+
+ return attache;
+ }
- if (!indirectAgentLB.compareManagementServerList(host.getId(),
host.getDataCenterId(), agentMSHostList, lbAlgorithm)) {
- final List<String> newMSList =
indirectAgentLB.getManagementServerList(host.getId(), host.getDataCenterId(),
null);
- ready.setMsHostList(newMSList);
- final List<String> avoidMsList =
_mshostDao.listNonUpStateMsIPs();
- ready.setAvoidMsHostList(avoidMsList);
- ready.setLbAlgorithm(indirectAgentLB.getLBAlgorithmName());
-
ready.setLbCheckInterval(indirectAgentLB.getLBPreferredHostCheckInterval(host.getClusterId()));
- logger.debug("Agent's management server host list is not
up to date, sending list update: {}", newMSList);
+ private AgentAttache connectHostAgent(HostVO host, ReadyCommand ready,
Link link, StartupCommand[] startupCmds, GlobalLock joinLock) throws
ConnectionException {
+ AgentAttache attache;
+ try {
+ final List<String> agentMSHostList = new ArrayList<>();
+ String lbAlgorithm = null;
+ if (startupCmds != null && startupCmds.length > 0) {
+ final String agentMSHosts = startupCmds[0].getMsHostList();
+ if (StringUtils.isNotEmpty(agentMSHosts)) {
+ String[] msHosts = agentMSHosts.split("@");
+ if (msHosts.length > 1) {
+ lbAlgorithm = msHosts[1];
+ }
+
agentMSHostList.addAll(Arrays.asList(msHosts[0].split(",")));
}
+ }
- attache = createAttacheForConnect(host, link);
- attache = notifyMonitorsOfConnection(attache, startup, false);
- } finally {
+ if
(!indirectAgentLB.compareManagementServerListAndLBAlgorithm(host.getId(),
host.getDataCenterId(), agentMSHostList, lbAlgorithm)) {
+ final List<String> newMSList =
indirectAgentLB.getManagementServerList(host.getId(), host.getDataCenterId(),
null);
+ ready.setMsHostList(newMSList);
+ String newLBAlgorithm = indirectAgentLB.getLBAlgorithmName();
+ ready.setLbAlgorithm(newLBAlgorithm);
+ logger.debug("Agent's management server host list or lb
algorithm is not up to date, sending list and algorithm update: {}, {}",
newMSList, newLBAlgorithm);
+ }
+
+ final List<String> avoidMsList = _mshostDao.listNonUpStateMsIPs();
+ ready.setAvoidMsHostList(avoidMsList);
+
ready.setLbCheckInterval(indirectAgentLB.getLBPreferredHostCheckInterval(host.getClusterId()));
+ ready.setArch(host.getArch().getType());
+
+ attache = createAttacheForConnect(host, link);
+ attache = notifyMonitorsOfConnection(attache, startupCmds, false);
+ } finally {
+ if (joinLock != null) {
joinLock.unlock();
}
Review Comment:
The null check for `joinLock` is unnecessary since it's passed as a
parameter from a method that ensures it's not null. This check adds unnecessary
complexity.
```suggestion
joinLock.unlock();
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
