From 855f611bfad35b05c6e7bd553edf78b2d411a132 Mon Sep 17 00:00:00 2001 From: Sachin R <32716246+sachindoddaguni@users.noreply.github.com> Date: Thu, 4 Dec 2025 14:42:35 -0800 Subject: [PATCH 1/3] * Use hostname during MS registration to avoid duplicate registrations --- .../com/cloud/cluster/ClusterManagerImpl.java | 31 +++++++- .../cluster/dao/ManagementServerHostDao.java | 2 + .../dao/ManagementServerHostDaoImpl.java | 31 ++++++++ .../dao/ManagementServerHostDaoImplTest.java | 72 +++++++++++++++++++ 4 files changed, 134 insertions(+), 2 deletions(-) create mode 100644 framework/cluster/src/test/java/com/cloud/cluster/dao/ManagementServerHostDaoImplTest.java diff --git a/framework/cluster/src/main/java/com/cloud/cluster/ClusterManagerImpl.java b/framework/cluster/src/main/java/com/cloud/cluster/ClusterManagerImpl.java index 2a282cbb1ef1..997fe6de48a9 100644 --- a/framework/cluster/src/main/java/com/cloud/cluster/ClusterManagerImpl.java +++ b/framework/cluster/src/main/java/com/cloud/cluster/ClusterManagerImpl.java @@ -40,6 +40,7 @@ import javax.inject.Inject; import javax.naming.ConfigurationException; +import org.apache.commons.lang3.StringUtils; import org.apache.cloudstack.framework.config.ConfigDepot; import org.apache.cloudstack.framework.config.ConfigKey; import org.apache.cloudstack.framework.config.Configurable; @@ -1043,13 +1044,39 @@ public ManagementServerHostVO doInTransaction(final TransactionStatus status) { final Class c = this.getClass(); final String version = c.getPackage().getImplementationVersion(); + final String currentHostname = NetUtils.getCanonicalHostName(); ManagementServerHostVO mshost = _mshostDao.findByMsid(_msId); + + // Look for duplicate hostname in the case of kubernetes setup where ip/mac changes but hostname is constant. + if (mshost == null && StringUtils.isNotBlank(currentHostname)) { + ManagementServerHostVO activeEntry = _mshostDao.findByName(currentHostname); + if (activeEntry != null) { + // Found an active entry with this hostname but different MSID + // This happens when a pod restarts with a new MAC address (new MSID) + if (activeEntry.getMsid() != _msId) { + logger.info(String.format( + "Found active entry for hostname '%s' with old MSID %d. " + + "Marking it as removed and creating new entry with MSID %d.", + currentHostname, activeEntry.getMsid(), _msId)); + // Mark the old entry as removed + activeEntry.setRemoved(DateUtil.currentGMTTime()); + activeEntry.setState(ManagementServerHost.State.Down); + _mshostDao.update(activeEntry.getId(), activeEntry); + // Set mshost to null so a new entry will be created below + mshost = null; + } else { + // Same MSID - this is our existing entry, use the update path + mshost = activeEntry; + } + } + } + if (mshost == null) { mshost = new ManagementServerHostVO(); mshost.setMsid(_msId); mshost.setRunid(_runId); - mshost.setName(NetUtils.getCanonicalHostName()); + mshost.setName(currentHostname); mshost.setVersion(version); mshost.setServiceIP(_clusterNodeIP); mshost.setServicePort(_currentServiceAdapter.getServicePort()); @@ -1063,7 +1090,7 @@ public ManagementServerHostVO doInTransaction(final TransactionStatus status) { logger.info("New instance of management server {}, runId {} is being started", mshost, _runId); } } else { - _mshostDao.update(mshost.getId(), _runId, NetUtils.getCanonicalHostName(), version, _clusterNodeIP, _currentServiceAdapter.getServicePort(), + _mshostDao.update(mshost.getId(), _runId, currentHostname, version, _clusterNodeIP, _currentServiceAdapter.getServicePort(), DateUtil.currentGMTTime()); if (logger.isInfoEnabled()) { logger.info("Management server {}, runId {} is being started", mshost, _runId); diff --git a/framework/cluster/src/main/java/com/cloud/cluster/dao/ManagementServerHostDao.java b/framework/cluster/src/main/java/com/cloud/cluster/dao/ManagementServerHostDao.java index 96d57ee04258..adcabc930282 100644 --- a/framework/cluster/src/main/java/com/cloud/cluster/dao/ManagementServerHostDao.java +++ b/framework/cluster/src/main/java/com/cloud/cluster/dao/ManagementServerHostDao.java @@ -31,6 +31,8 @@ public interface ManagementServerHostDao extends GenericDao implements ManagementServerHostDao { private final SearchBuilder MsIdSearch; + private final SearchBuilder NameSearch; private final SearchBuilder ActiveSearch; private final SearchBuilder InactiveSearch; private final SearchBuilder StateSearch; @@ -75,6 +76,32 @@ public ManagementServerHostVO findByMsid(long msid) { return null; } + @Override + public ManagementServerHostVO findByName(String name) { + if (name == null || name.trim().isEmpty()) { + return null; + } + + SearchCriteria sc = NameSearch.create(); + sc.setParameters("name", name); + // Only search for active (non-removed) entries to avoid non-deterministic results + // when multiple removed entries exist for the same hostname + sc.addAnd("removed", SearchCriteria.Op.NULL); + + List l = listBy(sc); + if (l != null && l.size() > 0) { + if (l.size() > 1) { + s_logger.error(String.format( + "Data corruption detected: Found %d active entries for hostname '%s'. " + + "This should never happen and indicates duplicate mshost records.", + l.size(), name)); + } + return l.get(0); + } + + return null; + } + @Override @DB public void update(long id, long runid, String name, String version, String serviceIP, int servicePort, Date lastUpdate) { @@ -191,6 +218,10 @@ protected ManagementServerHostDaoImpl() { MsIdSearch.and("msid", MsIdSearch.entity().getMsid(), SearchCriteria.Op.EQ); MsIdSearch.done(); + NameSearch = createSearchBuilder(); + NameSearch.and("name", NameSearch.entity().getName(), SearchCriteria.Op.EQ); + NameSearch.done(); + ActiveSearch = createSearchBuilder(); ActiveSearch.and("lastUpdateTime", ActiveSearch.entity().getLastUpdateTime(), SearchCriteria.Op.GT); ActiveSearch.and("removed", ActiveSearch.entity().getRemoved(), SearchCriteria.Op.NULL); diff --git a/framework/cluster/src/test/java/com/cloud/cluster/dao/ManagementServerHostDaoImplTest.java b/framework/cluster/src/test/java/com/cloud/cluster/dao/ManagementServerHostDaoImplTest.java new file mode 100644 index 000000000000..1f284cd55a83 --- /dev/null +++ b/framework/cluster/src/test/java/com/cloud/cluster/dao/ManagementServerHostDaoImplTest.java @@ -0,0 +1,72 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +package com.cloud.cluster.dao; + +import com.cloud.cluster.ManagementServerHostVO; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.mockito.junit.MockitoJUnitRunner; + +import static org.junit.Assert.assertNull; + +/** + * Unit tests for ManagementServerHostDaoImpl focusing on the new findByName method added in PR #641. + * + * Note: Full integration tests for findByName would require database setup. These unit tests + * verify the basic contract of the method (null handling, empty string handling). + */ +@RunWith(MockitoJUnitRunner.class) +public class ManagementServerHostDaoImplTest { + + private final ManagementServerHostDaoImpl dao = new ManagementServerHostDaoImpl(); + + // ========== TESTS FOR findByName METHOD (PR #641) ========== + + @Test + public void testFindByName_ReturnsNullForNullHostname() { + ManagementServerHostVO result = dao.findByName(null); + assertNull("Result should be null for null hostname", result); + } + + @Test + public void testFindByName_ReturnsNullForEmptyHostname() { + ManagementServerHostVO result = dao.findByName(""); + assertNull("Result should be null for empty hostname", result); + } + + @Test + public void testFindByName_ReturnsNullForWhitespaceHostname() { + ManagementServerHostVO result = dao.findByName(" "); + assertNull("Result should be null for whitespace-only hostname", result); + } + + @Test + public void testFindByName_ReturnsNullForTabAndSpaceHostname() { + ManagementServerHostVO result = dao.findByName(" \t "); + assertNull("Result should be null for tab/space-only hostname", result); + } + + /** + * Note: Tests for actual database lookups would require integration test setup. + * The key functionality - looking up by hostname to handle Kubernetes pod restarts + * with changing IPs - is tested through the behavior in ClusterManagerImpl which + * calls this method when a hostname is found but MSID differs. + * + * See ClusterManagerImpl.java lines 1064-1079 for usage. + */ +} From c64d1feb79302a857e013bc2c4f3a2f8fbfd098f Mon Sep 17 00:00:00 2001 From: mprokopchuk Date: Mon, 16 Feb 2026 22:24:30 +0100 Subject: [PATCH 2/3] Handle multiple active entries for same hostname in k8s setup --- .../com/cloud/cluster/ClusterManagerImpl.java | 4 +-- .../cluster/dao/ManagementServerHostDao.java | 2 +- .../dao/ManagementServerHostDaoImpl.java | 25 ++++--------------- .../dao/ManagementServerHostDaoImplTest.java | 19 +++++++------- 4 files changed, 18 insertions(+), 32 deletions(-) diff --git a/framework/cluster/src/main/java/com/cloud/cluster/ClusterManagerImpl.java b/framework/cluster/src/main/java/com/cloud/cluster/ClusterManagerImpl.java index 997fe6de48a9..2856b0a23f20 100644 --- a/framework/cluster/src/main/java/com/cloud/cluster/ClusterManagerImpl.java +++ b/framework/cluster/src/main/java/com/cloud/cluster/ClusterManagerImpl.java @@ -1050,8 +1050,8 @@ public ManagementServerHostVO doInTransaction(final TransactionStatus status) { // Look for duplicate hostname in the case of kubernetes setup where ip/mac changes but hostname is constant. if (mshost == null && StringUtils.isNotBlank(currentHostname)) { - ManagementServerHostVO activeEntry = _mshostDao.findByName(currentHostname); - if (activeEntry != null) { + List activeEntries = _mshostDao.findAllByName(currentHostname); + for (ManagementServerHostVO activeEntry : activeEntries) { // Found an active entry with this hostname but different MSID // This happens when a pod restarts with a new MAC address (new MSID) if (activeEntry.getMsid() != _msId) { diff --git a/framework/cluster/src/main/java/com/cloud/cluster/dao/ManagementServerHostDao.java b/framework/cluster/src/main/java/com/cloud/cluster/dao/ManagementServerHostDao.java index adcabc930282..439e8d21ef51 100644 --- a/framework/cluster/src/main/java/com/cloud/cluster/dao/ManagementServerHostDao.java +++ b/framework/cluster/src/main/java/com/cloud/cluster/dao/ManagementServerHostDao.java @@ -31,7 +31,7 @@ public interface ManagementServerHostDao extends GenericDao findAllByName(String name); int increaseAlertCount(long id); diff --git a/framework/cluster/src/main/java/com/cloud/cluster/dao/ManagementServerHostDaoImpl.java b/framework/cluster/src/main/java/com/cloud/cluster/dao/ManagementServerHostDaoImpl.java index 0eeced4044b8..ca3a6155107a 100644 --- a/framework/cluster/src/main/java/com/cloud/cluster/dao/ManagementServerHostDaoImpl.java +++ b/framework/cluster/src/main/java/com/cloud/cluster/dao/ManagementServerHostDaoImpl.java @@ -26,6 +26,7 @@ import org.apache.commons.collections.CollectionUtils; +import org.apache.commons.lang3.StringUtils; import com.cloud.cluster.ClusterInvalidSessionException; import org.apache.cloudstack.management.ManagementServerHost; @@ -77,29 +78,13 @@ public ManagementServerHostVO findByMsid(long msid) { } @Override - public ManagementServerHostVO findByName(String name) { - if (name == null || name.trim().isEmpty()) { - return null; + public List findAllByName(String name) { + if (StringUtils.isBlank(name)) { + return List.of(); } - SearchCriteria sc = NameSearch.create(); sc.setParameters("name", name); - // Only search for active (non-removed) entries to avoid non-deterministic results - // when multiple removed entries exist for the same hostname - sc.addAnd("removed", SearchCriteria.Op.NULL); - - List l = listBy(sc); - if (l != null && l.size() > 0) { - if (l.size() > 1) { - s_logger.error(String.format( - "Data corruption detected: Found %d active entries for hostname '%s'. " + - "This should never happen and indicates duplicate mshost records.", - l.size(), name)); - } - return l.get(0); - } - - return null; + return listBy(sc); } @Override diff --git a/framework/cluster/src/test/java/com/cloud/cluster/dao/ManagementServerHostDaoImplTest.java b/framework/cluster/src/test/java/com/cloud/cluster/dao/ManagementServerHostDaoImplTest.java index 1f284cd55a83..9fa0660c1cbf 100644 --- a/framework/cluster/src/test/java/com/cloud/cluster/dao/ManagementServerHostDaoImplTest.java +++ b/framework/cluster/src/test/java/com/cloud/cluster/dao/ManagementServerHostDaoImplTest.java @@ -22,7 +22,8 @@ import org.junit.runner.RunWith; import org.mockito.junit.MockitoJUnitRunner; -import static org.junit.Assert.assertNull; +import java.util.List; +import static org.junit.Assert.assertEquals; /** * Unit tests for ManagementServerHostDaoImpl focusing on the new findByName method added in PR #641. @@ -39,26 +40,26 @@ public class ManagementServerHostDaoImplTest { @Test public void testFindByName_ReturnsNullForNullHostname() { - ManagementServerHostVO result = dao.findByName(null); - assertNull("Result should be null for null hostname", result); + List result = dao.findAllByName(null); + assertEquals(0, result.size()); } @Test public void testFindByName_ReturnsNullForEmptyHostname() { - ManagementServerHostVO result = dao.findByName(""); - assertNull("Result should be null for empty hostname", result); + List result = dao.findAllByName(""); + assertEquals(0, result.size()); } @Test public void testFindByName_ReturnsNullForWhitespaceHostname() { - ManagementServerHostVO result = dao.findByName(" "); - assertNull("Result should be null for whitespace-only hostname", result); + List result = dao.findAllByName(" "); + assertEquals(0, result.size()); } @Test public void testFindByName_ReturnsNullForTabAndSpaceHostname() { - ManagementServerHostVO result = dao.findByName(" \t "); - assertNull("Result should be null for tab/space-only hostname", result); + List result = dao.findAllByName(" \t "); + assertEquals(0, result.size()); } /** From 78516eae780a4e6ec39ea9e198e48afffd8ef4ba Mon Sep 17 00:00:00 2001 From: mprokopchuk Date: Mon, 6 Jul 2026 14:15:18 -0700 Subject: [PATCH 3/3] Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- .../src/main/java/com/cloud/cluster/ClusterManagerImpl.java | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/framework/cluster/src/main/java/com/cloud/cluster/ClusterManagerImpl.java b/framework/cluster/src/main/java/com/cloud/cluster/ClusterManagerImpl.java index 2856b0a23f20..bb11258f9070 100644 --- a/framework/cluster/src/main/java/com/cloud/cluster/ClusterManagerImpl.java +++ b/framework/cluster/src/main/java/com/cloud/cluster/ClusterManagerImpl.java @@ -1048,8 +1048,9 @@ public ManagementServerHostVO doInTransaction(final TransactionStatus status) { ManagementServerHostVO mshost = _mshostDao.findByMsid(_msId); - // Look for duplicate hostname in the case of kubernetes setup where ip/mac changes but hostname is constant. - if (mshost == null && StringUtils.isNotBlank(currentHostname)) { + // Look for duplicate hostname in Kubernetes setups where IP/MAC changes but hostname is constant. + // Skip the default "localhost" hostname fallback to avoid removing other active nodes when hostname resolution fails. + if (mshost == null && StringUtils.isNotBlank(currentHostname) && !StringUtils.equalsIgnoreCase(currentHostname, "localhost")) { List activeEntries = _mshostDao.findAllByName(currentHostname); for (ManagementServerHostVO activeEntry : activeEntries) { // Found an active entry with this hostname but different MSID