Copilot commented on code in PR #14438:
URL: https://github.com/apache/cloudstack/pull/14438#discussion_r4314695803
##########
server/src/main/java/com/cloud/resource/ResourceManagerImpl.java:
##########
@@ -2281,10 +2281,32 @@ private HostVO getNewHost(StartupCommand[]
startupCommands) {
return host;
}
+ host = reviveRemovedHostByGuid(fullGuid);
+ if (host != null) {
+ return host;
+ }
+
logger.debug(String.format("Could not find Host by guid %s",
fullGuid));
return null;
}
+ /**
+ * A host can be marked removed (e.g. a system VM force-stopped and
destroyed from the UI) while the
+ * underlying resource is still alive and later reconnects with the same
guid. The `guid` column is
+ * uniquely constrained regardless of removal, so blindly inserting a new
row for it fails with a
+ * SQLIntegrityConstraintViolationException. Revive the removed row
instead of creating a duplicate.
+ */
+ private HostVO reviveRemovedHostByGuid(String guid) {
+ HostVO removedHost = _hostDao.findByGuidIncludingRemoved(guid);
+ if (removedHost == null || removedHost.getRemoved() == null) {
+ return null;
+ }
+ logger.warn(String.format("Host with guid %s reconnected but was
previously removed (id: %s); reviving the removed host record instead of
creating a duplicate",
+ guid, removedHost.getId()));
+ removedHost.setRemoved(null);
+ return removedHost;
Review Comment:
Revival is currently implemented as a read-then-write on an entity, which
can race if multiple management servers/processes handle reconnects
concurrently (both can observe `removed != null`, then both try to revive).
Consider making the revival operation atomic at the DAO level (single UPDATE
with a `WHERE removed IS NOT NULL` guard, returning affected row count), then
re-read the host; this avoids races and ensures a consistent persisted outcome.
##########
server/src/main/java/com/cloud/resource/ResourceManagerImpl.java:
##########
@@ -2281,10 +2281,32 @@ private HostVO getNewHost(StartupCommand[]
startupCommands) {
return host;
}
+ host = reviveRemovedHostByGuid(fullGuid);
+ if (host != null) {
+ return host;
+ }
+
logger.debug(String.format("Could not find Host by guid %s",
fullGuid));
return null;
}
+ /**
+ * A host can be marked removed (e.g. a system VM force-stopped and
destroyed from the UI) while the
+ * underlying resource is still alive and later reconnects with the same
guid. The `guid` column is
+ * uniquely constrained regardless of removal, so blindly inserting a new
row for it fails with a
+ * SQLIntegrityConstraintViolationException. Revive the removed row
instead of creating a duplicate.
+ */
Review Comment:
The PR description template appears unfilled (no change type selected, no
testing details). Since automation relies on these fields for
labels/documentation, please update the PR description to mark this as a bug
fix and document the test coverage/steps used for validation.
##########
server/src/main/java/com/cloud/resource/ResourceManagerImpl.java:
##########
@@ -2281,10 +2281,32 @@ private HostVO getNewHost(StartupCommand[]
startupCommands) {
return host;
}
+ host = reviveRemovedHostByGuid(fullGuid);
+ if (host != null) {
+ return host;
+ }
+
logger.debug(String.format("Could not find Host by guid %s",
fullGuid));
return null;
}
+ /**
+ * A host can be marked removed (e.g. a system VM force-stopped and
destroyed from the UI) while the
+ * underlying resource is still alive and later reconnects with the same
guid. The `guid` column is
+ * uniquely constrained regardless of removal, so blindly inserting a new
row for it fails with a
+ * SQLIntegrityConstraintViolationException. Revive the removed row
instead of creating a duplicate.
+ */
+ private HostVO reviveRemovedHostByGuid(String guid) {
+ HostVO removedHost = _hostDao.findByGuidIncludingRemoved(guid);
+ if (removedHost == null || removedHost.getRemoved() == null) {
+ return null;
+ }
+ logger.warn(String.format("Host with guid %s reconnected but was
previously removed (id: %s); reviving the removed host record instead of
creating a duplicate",
+ guid, removedHost.getId()));
+ removedHost.setRemoved(null);
Review Comment:
This method mutates `removedHost` in-memory but does not persist the change,
so the host may remain marked as removed in the DB (and be filtered out by
future queries), potentially causing repeated revival attempts and inconsistent
behavior. Persist the update inside this method (e.g., via a DAO update) or
provide a DAO method that clears `removed` and saves atomically before
returning the host.
##########
server/src/test/java/com/cloud/resource/ResourceManagerImplTest.java:
##########
@@ -683,4 +685,49 @@ public void testDestroyLocalStoragePoolVolumesNoDisks() {
resourceManager.destroyLocalStoragePoolVolumes(poolId);
verify(volumeDao, never()).updateAndRemoveVolume(any(VolumeVO.class));
}
+
+ private HostVO invokeGetNewHost(StartupCommand[] cmds) throws Exception {
+ Method method =
ResourceManagerImpl.class.getDeclaredMethod("getNewHost",
StartupCommand[].class);
+ method.setAccessible(true);
+ return (HostVO) method.invoke(resourceManager, (Object) cmds);
+ }
+
+ @Test
+ public void testGetNewHostRevivesRemovedHostOnGuidCollision() throws
Exception {
+ String guid = "s-14-VM-PremiumSecondaryStorageResource";
+ String guidPrefix = "s-14-VM";
+ StartupCommand cmd = Mockito.mock(StartupCommand.class);
+ when(cmd.getGuid()).thenReturn(guid);
+ when(cmd.getGuidWithoutResource()).thenReturn(guidPrefix);
+
+ Mockito.doReturn(null).when(resourceManager).findHostByGuid(guid);
+
Mockito.doReturn(null).when(resourceManager).findHostByGuidPrefix(guidPrefix);
+
+ HostVO removedHost = Mockito.mock(HostVO.class);
+ when(removedHost.getId()).thenReturn(99L);
+ when(removedHost.getRemoved()).thenReturn(new Date());
+ when(hostDao.findByGuidIncludingRemoved(guid)).thenReturn(removedHost);
+
+ HostVO result = invokeGetNewHost(new StartupCommand[]{cmd});
+
+ Assert.assertSame(removedHost, result);
+ Mockito.verify(removedHost).setRemoved(null);
+ }
+
+ @Test
+ public void testGetNewHostReturnsNullWhenNoHostAndNoRemovedMatch() throws
Exception {
+ String guid = "unknown-guid";
+ String guidPrefix = "unknown";
+ StartupCommand cmd = Mockito.mock(StartupCommand.class);
+ when(cmd.getGuid()).thenReturn(guid);
+ when(cmd.getGuidWithoutResource()).thenReturn(guidPrefix);
+
+ Mockito.doReturn(null).when(resourceManager).findHostByGuid(guid);
+
Mockito.doReturn(null).when(resourceManager).findHostByGuidPrefix(guidPrefix);
+ when(hostDao.findByGuidIncludingRemoved(guid)).thenReturn(null);
+
+ HostVO result = invokeGetNewHost(new StartupCommand[]{cmd});
+
+ Assert.assertNull(result);
+ }
Review Comment:
These tests rely on reflective access to a private method, which is brittle
and can break on refactors without behavior changes. Prefer testing via a
public/protected entry point that exercises `getNewHost`, or adjust visibility
(e.g., package-private/protected) / extract the revival logic into a
collaborator that can be unit-tested without reflection.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
