Copilot commented on code in PR #13182:
URL: https://github.com/apache/cloudstack/pull/13182#discussion_r3261552836
##########
server/src/main/java/com/cloud/resource/ResourceManagerImpl.java:
##########
@@ -2585,6 +2613,27 @@ private Host createHostAndAgent(final ServerResource
resource, final Map<String,
return host;
}
+ void checkForDuplicateHost(final String url) {
+ String hostIpOrName = null;
+ String ipAddress = null;
+ try {
+ hostIpOrName = new URI(UriUtils.encodeURIComponent(url)).getHost();
+ InetAddress ip = InetAddress.getByName(hostIpOrName);
+ ipAddress = ip.getHostAddress();
+ } catch (final URISyntaxException | UnknownHostException ignore) {
+ // unparseable URL or unknown host - discoverer will reject it
shortly anyway
+ }
+ if (StringUtils.isBlank(hostIpOrName)) {
+ return;
+ }
+ final HostVO existingByIp = _hostDao.findByIp(ipAddress);
+ if (existingByIp != null) {
+ throw new InvalidParameterValueException(String.format(
+ "A host with IP address / hostname '%s' (%s) already
exists (id: %s). Remove it before adding again.",
+ hostIpOrName, ipAddress, existingByIp.getUuid()));
+ }
Review Comment:
The linked issue states: "validate if the host already exists in the
database with same hostname or IP address". This implementation only looks up
the host by resolved IP (`findByIp`). If a previously added host's stored IP
differs from what the new URL's hostname resolves to (e.g., DNS changes,
multi-NIC hosts, or hosts stored with a different management IP than what DNS
now returns for the same name), the duplicate-by-name case will not be caught.
Consider also checking by `name`/hostname in addition to IP to fully cover the
issue's requirement.
##########
server/src/main/java/com/cloud/resource/ResourceManagerImpl.java:
##########
@@ -2585,6 +2613,27 @@ private Host createHostAndAgent(final ServerResource
resource, final Map<String,
return host;
}
+ void checkForDuplicateHost(final String url) {
+ String hostIpOrName = null;
+ String ipAddress = null;
+ try {
+ hostIpOrName = new URI(UriUtils.encodeURIComponent(url)).getHost();
+ InetAddress ip = InetAddress.getByName(hostIpOrName);
+ ipAddress = ip.getHostAddress();
+ } catch (final URISyntaxException | UnknownHostException ignore) {
+ // unparseable URL or unknown host - discoverer will reject it
shortly anyway
+ }
+ if (StringUtils.isBlank(hostIpOrName)) {
+ return;
+ }
+ final HostVO existingByIp = _hostDao.findByIp(ipAddress);
Review Comment:
When `InetAddress.getByName(hostIpOrName)` throws `UnknownHostException`,
`hostIpOrName` has already been set (non-blank) but `ipAddress` remains `null`.
The early-return guard only checks `hostIpOrName` for blankness, so execution
falls through to `_hostDao.findByIp(null)` with a null argument. Consider
returning early on `UnknownHostException` (or also guarding on `ipAddress ==
null`) so an unresolvable hostname doesn't trigger an unnecessary DAO query
with a null IP.
##########
server/src/main/java/com/cloud/resource/ResourceManagerImpl.java:
##########
@@ -2281,6 +2286,27 @@ private HostVO getNewHost(StartupCommand[]
startupCommands) {
return null;
}
+ protected void validateExistingHostLocationImmutable(final HostVO host,
final boolean newHost,
+ final long dcId, final Long podId, final Long clusterId, final
StartupCommand startup) {
+ if (newHost || host == null || host.getType() != Host.Type.Routing) {
Review Comment:
`validateExistingHostLocationImmutable` is skipped for any host whose `Type
!= Routing` (e.g., SecondaryStorageVM, ConsoleProxy, storage hosts). The PR
title and linked issue both speak about disallowing zone/pod/cluster changes
for "an existing host" in general, not just routing/KVM-style hypervisor hosts.
If the intent is to protect all persistent hosts, restricting the check to
`Routing` will silently allow other host types to have their
`data_center_id`/`pod_id`/`cluster_id` mutated on reconnect. Please confirm
this restriction is intentional and document the rationale; otherwise, broaden
the check.
##########
server/src/test/java/com/cloud/resource/ResourceManagerImplTest.java:
##########
@@ -218,6 +219,19 @@ public void tearDown() throws Exception {
closeable.close();
}
+ @Test(expected = InvalidParameterValueException.class)
+ public void testCheckForDuplicateHostThrowsWhenIpAlreadyExists() {
+ when(hostDao.findByIp("10.0.0.10")).thenReturn(host);
+ resourceManager.checkForDuplicateHost("http://10.0.0.10";);
+ }
+
+ @Test
+ public void testCheckForDuplicateHostAllowsUniqueHost() {
+ when(hostDao.findByIp("10.0.0.30")).thenReturn(null);
+ resourceManager.checkForDuplicateHost("http://10.0.0.30";);
+ verify(hostDao, times(1)).findByIp("10.0.0.30");
+ }
+
@Test
Review Comment:
There is no unit test covering the `checkForDuplicateHost` path that is
invoked from `discoverHostsFull` (the integration of the duplicate check into
the add-host flow). The two new tests only invoke `checkForDuplicateHost`
directly. Consider adding a test that ensures
`discoverHostsFull`/`discoverHosts` propagates the
`InvalidParameterValueException` for a duplicate IP before any discoverer is
invoked, mirroring the convention of other end-to-end tests in this class.
##########
server/src/main/java/com/cloud/resource/ResourceManagerImpl.java:
##########
@@ -2281,6 +2286,27 @@ private HostVO getNewHost(StartupCommand[]
startupCommands) {
return null;
}
+ protected void validateExistingHostLocationImmutable(final HostVO host,
final boolean newHost,
+ final long dcId, final Long podId, final Long clusterId, final
StartupCommand startup) {
+ if (newHost || host == null || host.getType() != Host.Type.Routing) {
+ return;
+ }
+ final Long existingDcId = host.getDataCenterId();
+ final Long existingPodId = host.getPodId();
+ final Long existingClusterId = host.getClusterId();
+ if (existingDcId == null || existingPodId == null || existingClusterId
== null) {
+ return;
+ }
+ if (existingDcId == dcId && Objects.equals(existingPodId, podId) &&
Objects.equals(existingClusterId, clusterId)) {
+ return;
+ }
+ final String identity = host.getUuid() != null ? host.getUuid() :
host.getGuid();
+ final String ip = startup != null ? startup.getPrivateIpAddress() :
"unknown";
+ throw new InvalidParameterValueException(String.format(
+ "Host %s (ip: %s) is already registered in [zone: %d, pod: %d,
cluster: %d] and cannot be re-added or reconnected with [zone: %d, pod: %s,
cluster: %s]. Zone, pod and cluster of an existing host are immutable.",
Review Comment:
The error message embeds `existingPodId`/`existingClusterId` with `%d`
format specifiers but `podId`/`clusterId` with `%s`. Since both pairs are
`Long` and may be null in the input case (podId/clusterId can legitimately be
null), using `%d` on the existing values is technically safe due to the earlier
null guard, but mixing `%d` and `%s` for the same logical fields is
inconsistent and slightly confusing. Consider using `%s` consistently for all
six values so a null in any of them is rendered cleanly rather than risking a
`IllegalFormatConversionException` if future refactors loosen the null guard.
--
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]
