From 1836e0951adda1c169e813703ca8aeaf8ff2886b Mon Sep 17 00:00:00 2001 From: Mashrfee Aryan Date: Mon, 19 Oct 2026 10:26:14 -0500 Subject: [PATCH 1/6] Keep management server IDs stable across restarts Save the initial ID and reuse it when network interfaces change. Add JVM overrides and regression tests for issue #14253. --- .../utils/identity/ManagementServerId.java | 84 ++++++++ .../utils/identity/ManagementServerNode.java | 3 +- .../identity/ManagementServerIdTest.java | 179 ++++++++++++++++++ 3 files changed, 264 insertions(+), 2 deletions(-) create mode 100644 utils/src/main/java/org/apache/cloudstack/utils/identity/ManagementServerId.java create mode 100644 utils/src/test/java/org/apache/cloudstack/utils/identity/ManagementServerIdTest.java diff --git a/utils/src/main/java/org/apache/cloudstack/utils/identity/ManagementServerId.java b/utils/src/main/java/org/apache/cloudstack/utils/identity/ManagementServerId.java new file mode 100644 index 000000000000..dcebb5d9ad13 --- /dev/null +++ b/utils/src/main/java/org/apache/cloudstack/utils/identity/ManagementServerId.java @@ -0,0 +1,84 @@ +// 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 org.apache.cloudstack.utils.identity; + +import java.io.IOException; +import java.nio.ByteBuffer; +import java.nio.channels.FileChannel; +import java.nio.channels.FileLock; +import java.nio.charset.StandardCharsets; +import java.nio.file.Files; +import java.nio.file.Path; +import java.nio.file.Paths; +import java.nio.file.StandardOpenOption; +import java.util.function.LongSupplier; + +import com.cloud.utils.exception.CloudRuntimeException; +import com.cloud.utils.net.MacAddress; + +final class ManagementServerId { + + private ManagementServerId() { + } + + static long get() { + Path path = Paths.get(System.getProperty("management.server.id.file", + Paths.get(System.getProperty("user.home"), ".cloudstack", "management-server.id").toString())); + try { + return load(path, System.getProperty("management.server.id"), () -> MacAddress.getMacAddress().toLong()); + } catch (IOException | IllegalArgumentException e) { + throw new CloudRuntimeException("Unable to load the management server ID from " + path, e); + } + } + + static long load(Path path, String configuredId, LongSupplier initialId) throws IOException { + if (configuredId != null) { + return parse(configuredId); + } + + Files.createDirectories(path.toAbsolutePath().getParent()); + try (FileChannel lockChannel = FileChannel.open(path.resolveSibling(path.getFileName() + ".lock"), + StandardOpenOption.CREATE, StandardOpenOption.WRITE); + FileLock lock = lockChannel.lock()) { + if (Files.exists(path)) { + if (Files.size(path) > 64) { + throw new IllegalArgumentException("Invalid management server ID file"); + } + return parse(new String(Files.readAllBytes(path), StandardCharsets.UTF_8)); + } + + long id = parse(Long.toString(initialId.getAsLong())); + try (FileChannel channel = FileChannel.open(path, StandardOpenOption.CREATE_NEW, StandardOpenOption.WRITE)) { + ByteBuffer buffer = StandardCharsets.UTF_8.encode(id + "\n"); + while (buffer.hasRemaining()) { + channel.write(buffer); + } + channel.force(true); + } + return id; + } + } + + private static long parse(String value) { + long id = Long.parseLong(value.trim()); + if (id <= 0) { + throw new IllegalArgumentException("Management server ID must be a positive integer"); + } + return id; + } +} diff --git a/utils/src/main/java/org/apache/cloudstack/utils/identity/ManagementServerNode.java b/utils/src/main/java/org/apache/cloudstack/utils/identity/ManagementServerNode.java index 32989c3497c5..cc85ca18c02b 100644 --- a/utils/src/main/java/org/apache/cloudstack/utils/identity/ManagementServerNode.java +++ b/utils/src/main/java/org/apache/cloudstack/utils/identity/ManagementServerNode.java @@ -25,11 +25,10 @@ import com.cloud.utils.component.ComponentLifecycle; import com.cloud.utils.component.SystemIntegrityChecker; import com.cloud.utils.exception.CloudRuntimeException; -import com.cloud.utils.net.MacAddress; public class ManagementServerNode extends AdapterBase implements SystemIntegrityChecker { - private static final long s_nodeId = MacAddress.getMacAddress().toLong(); + private static final long s_nodeId = ManagementServerId.get(); public ManagementServerNode() { setRunLevel(ComponentLifecycle.RUN_LEVEL_FRAMEWORK_BOOTSTRAP); diff --git a/utils/src/test/java/org/apache/cloudstack/utils/identity/ManagementServerIdTest.java b/utils/src/test/java/org/apache/cloudstack/utils/identity/ManagementServerIdTest.java new file mode 100644 index 000000000000..030161895206 --- /dev/null +++ b/utils/src/test/java/org/apache/cloudstack/utils/identity/ManagementServerIdTest.java @@ -0,0 +1,179 @@ +// 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 org.apache.cloudstack.utils.identity; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertThrows; +import static org.junit.Assert.assertTrue; + +import java.io.IOException; +import java.nio.charset.StandardCharsets; +import java.nio.file.Files; +import java.nio.file.Path; +import java.nio.file.Paths; +import java.util.ArrayList; +import java.util.List; +import java.util.concurrent.TimeUnit; +import java.util.function.LongSupplier; + +import org.junit.Rule; +import org.junit.Test; +import org.junit.rules.TemporaryFolder; + +import com.cloud.utils.exception.CloudRuntimeException; + +public class ManagementServerIdTest { + + @Rule + public TemporaryFolder folder = new TemporaryFolder(); + + private Path idFile() { + return folder.getRoot().toPath().resolve("identity/management-server.id"); + } + + private LongSupplier unexpectedLookup() { + return () -> { + throw new AssertionError("Network interfaces must not be consulted"); + }; + } + + @Test + public void preservesInitialIdWhenNetworkInterfacesChange() throws IOException { + Path path = idFile(); + assertEquals(130014950130305L, ManagementServerId.load(path, null, () -> 130014950130305L)); + assertEquals(130014950130305L, ManagementServerId.load(path, null, () -> 42L)); + assertEquals(130014950130305L, ManagementServerId.load(path, null, unexpectedLookup())); + } + + @Test + public void configuredIdTakesPrecedenceOverSavedId() throws IOException { + Path path = idFile(); + ManagementServerId.load(path, null, () -> 42L); + assertEquals(123L, ManagementServerId.load(path, "123", unexpectedLookup())); + assertEquals(42L, ManagementServerId.load(path, null, unexpectedLookup())); + } + + @Test + public void configuredIdDoesNotRequireWritableStorage() throws IOException { + Path path = folder.newFile("not-a-directory").toPath().resolve("id"); + assertEquals(123L, ManagementServerId.load(path, "123", unexpectedLookup())); + } + + @Test + public void rejectsInvalidConfiguredIds() { + for (String value : new String[] {"", "0", "-1", "invalid", "9223372036854775808"}) { + assertThrows(IllegalArgumentException.class, + () -> ManagementServerId.load(idFile(), value, unexpectedLookup())); + } + assertFalse(Files.exists(idFile())); + } + + @Test + public void rejectsInvalidSavedIdsWithoutReplacingThem() throws IOException { + Path path = folder.newFile("id").toPath(); + for (String value : new String[] {"", "0", "-1", "invalid", "9223372036854775808", "1".repeat(65)}) { + Files.write(path, value.getBytes(StandardCharsets.UTF_8)); + assertThrows(IllegalArgumentException.class, + () -> ManagementServerId.load(path, null, unexpectedLookup())); + assertEquals(value, Files.readString(path)); + } + } + + @Test + public void rejectsInvalidInitialIdsWithoutSavingThem() { + for (long value : new long[] {0L, -1L}) { + assertThrows(IllegalArgumentException.class, + () -> ManagementServerId.load(idFile(), null, () -> value)); + } + assertFalse(Files.exists(idFile())); + } + + @Test + public void failsWhenIdentityCannotBeSaved() throws IOException { + Path path = folder.newFile("not-a-directory").toPath().resolve("id"); + assertThrows(IOException.class, () -> ManagementServerId.load(path, null, unexpectedLookup())); + } + + @Test + public void readsWhitespaceAndFullLongIds() throws IOException { + Path path = folder.newFile("id").toPath(); + Files.writeString(path, " " + Long.MAX_VALUE + "\n", StandardCharsets.UTF_8); + assertEquals(Long.MAX_VALUE, ManagementServerId.load(path, null, unexpectedLookup())); + } + + @Test + public void usesJvmSettingsAndReportsInvalidStorage() throws IOException { + String previousFile = System.getProperty("management.server.id.file"); + String previousId = System.getProperty("management.server.id"); + Path path = folder.newFile("id").toPath(); + try { + System.setProperty("management.server.id.file", path.toString()); + System.clearProperty("management.server.id"); + Files.writeString(path, "42\n", StandardCharsets.UTF_8); + assertEquals(42L, ManagementServerId.get()); + + System.setProperty("management.server.id", "123"); + assertEquals(123L, ManagementServerId.get()); + + System.clearProperty("management.server.id"); + Files.writeString(path, "invalid", StandardCharsets.UTF_8); + assertThrows(CloudRuntimeException.class, ManagementServerId::get); + } finally { + restoreProperty("management.server.id.file", previousFile); + restoreProperty("management.server.id", previousId); + } + } + + @Test + public void concurrentProcessesUseTheSameId() throws Exception { + Path path = idFile(); + List processes = new ArrayList<>(); + try { + for (int i = 1; i <= 3; i++) { + processes.add(new ProcessBuilder(Paths.get(System.getProperty("java.home"), "bin", "java").toString(), + "-cp", System.getProperty("java.class.path"), IdWriter.class.getName(), path.toString(), Integer.toString(i)) + .redirectErrorStream(true).start()); + } + for (Process process : processes) { + assertTrue("ID writer timed out", process.waitFor(30, TimeUnit.SECONDS)); + String output = new String(process.getInputStream().readAllBytes(), StandardCharsets.UTF_8).trim(); + assertEquals(output, 0, process.exitValue()); + assertEquals(Files.readString(path).trim(), output); + } + } finally { + for (Process process : processes) { + process.destroyForcibly(); + } + } + } + + public static class IdWriter { + public static void main(String[] args) throws IOException { + System.out.println(ManagementServerId.load(Paths.get(args[0]), null, () -> Long.parseLong(args[1]))); + } + } + + private void restoreProperty(String name, String value) { + if (value == null) { + System.clearProperty(name); + } else { + System.setProperty(name, value); + } + } +} From 4a7fa729ef7fbf5abddcc4703fb9aa53ffe15e9d Mon Sep 17 00:00:00 2001 From: Mashrfee Aryan Date: Mon, 19 Oct 2026 10:26:39 -0500 Subject: [PATCH 2/6] Document management server ID storage and recovery Explain the ID file, JVM settings, and upgrade steps for issue #14253. --- INSTALL.md | 27 +++++++++++++++++++++++++++ PendingReleaseNotes | 6 ++++++ 2 files changed, 33 insertions(+) diff --git a/INSTALL.md b/INSTALL.md index 0619b97f21c1..49a1aaf158dd 100644 --- a/INSTALL.md +++ b/INSTALL.md @@ -188,6 +188,33 @@ Then: To create packages for noredist add the `-p noredist` option to the package script. All the rpm packages will be created in `dist/rpmbuild/RPMS/x86_64` directory. +## Management server identity + +The management server saves its numeric ID in +`${user.home}/.cloudstack/management-server.id`. Packaged installations run as +`cloud`, whose home is normally `/var/lib/cloudstack/management`. On first use, +the server saves the ID derived from its MAC address. Later starts reuse the +saved ID, even if network interface order changes. + +Keep this file when upgrading or restoring the same management server. Do not +copy it to a different management server. The service user needs read and write +access to the directory, including the adjacent `.lock` file. If the ID cannot +be read, validated, or saved, startup fails. + +These JVM options can be added to `JAVA_OPTS` in +`/etc/default/cloudstack-management`: + +* `-Dmanagement.server.id.file=/path/to/management-server.id` changes the file + location. Use a persistent local directory writable by the service user. +* `-Dmanagement.server.id=123456789` overrides the saved ID with a positive + decimal integer. Each management server must have its own ID. This option does + not change the saved file; removing it restores the saved ID. + +For an existing installation, if interface order has already changed before the +first start with this fix, set `management.server.id` to that server's original +`msid` from `cloud.mshost`. Keep the override on subsequent starts. Existing stale +rows in `cloud.mshost` are not removed by this change. + ## Notes If you will be using Xen as your hypervisor, please download [vhd-util](https://download.cloudstack.org/tools/vhd-util) diff --git a/PendingReleaseNotes b/PendingReleaseNotes index 02d63811e36e..ad914a97124b 100644 --- a/PendingReleaseNotes +++ b/PendingReleaseNotes @@ -40,6 +40,12 @@ example.ver.1 > example.ver.2: Storage to grow to enormous sizes as Linux Distributions keep growing in size while a stripped down Linux should fit on a 2.88MB floppy. +4.22.1.0 > 4.22.2.0: + * Management servers now save their ID locally and reuse it after restarts, + preventing network interface reordering from changing the ID. Keep the ID + file when upgrading or restoring a server. See INSTALL.md for the file + location and JVM options to override the ID or its storage location. + 4.22.0.0 > 4.22.0.1: * Disk-only instance snapshots for KVM UEFI VMs now include a sidecar copy of the active NVRAM state so revert operations restore both disk and firmware From b92ff81ce65129b50a00307d2b6275afbc9b9026 Mon Sep 17 00:00:00 2001 From: Mashrfee Aryan Date: Sat, 31 Oct 2026 22:27:48 -0500 Subject: [PATCH 3/6] Fix interrupted management server ID initialization Flush the initial identity to a temporary sibling file and atomically publish it while holding the existing process lock. Clean up failed writes and verify that initialization can be retried without an invalid identity file. Related to #14253. --- .../utils/identity/ManagementServerId.java | 17 ++++++--- .../identity/ManagementServerIdTest.java | 36 +++++++++++++++++++ 2 files changed, 48 insertions(+), 5 deletions(-) diff --git a/utils/src/main/java/org/apache/cloudstack/utils/identity/ManagementServerId.java b/utils/src/main/java/org/apache/cloudstack/utils/identity/ManagementServerId.java index dcebb5d9ad13..fb4f40d252e0 100644 --- a/utils/src/main/java/org/apache/cloudstack/utils/identity/ManagementServerId.java +++ b/utils/src/main/java/org/apache/cloudstack/utils/identity/ManagementServerId.java @@ -25,6 +25,7 @@ import java.nio.file.Files; import java.nio.file.Path; import java.nio.file.Paths; +import java.nio.file.StandardCopyOption; import java.nio.file.StandardOpenOption; import java.util.function.LongSupplier; @@ -63,12 +64,18 @@ static long load(Path path, String configuredId, LongSupplier initialId) throws } long id = parse(Long.toString(initialId.getAsLong())); - try (FileChannel channel = FileChannel.open(path, StandardOpenOption.CREATE_NEW, StandardOpenOption.WRITE)) { - ByteBuffer buffer = StandardCharsets.UTF_8.encode(id + "\n"); - while (buffer.hasRemaining()) { - channel.write(buffer); + Path temporary = Files.createTempFile(path.toAbsolutePath().getParent(), "management-server-id-", ".tmp"); + try { + try (FileChannel channel = FileChannel.open(temporary, StandardOpenOption.WRITE)) { + ByteBuffer buffer = StandardCharsets.UTF_8.encode(id + "\n"); + while (buffer.hasRemaining()) { + channel.write(buffer); + } + channel.force(true); } - channel.force(true); + Files.move(temporary, path, StandardCopyOption.ATOMIC_MOVE); + } finally { + Files.deleteIfExists(temporary); } return id; } diff --git a/utils/src/test/java/org/apache/cloudstack/utils/identity/ManagementServerIdTest.java b/utils/src/test/java/org/apache/cloudstack/utils/identity/ManagementServerIdTest.java index 030161895206..a385c085cb2a 100644 --- a/utils/src/test/java/org/apache/cloudstack/utils/identity/ManagementServerIdTest.java +++ b/utils/src/test/java/org/apache/cloudstack/utils/identity/ManagementServerIdTest.java @@ -23,6 +23,8 @@ import static org.junit.Assert.assertTrue; import java.io.IOException; +import java.nio.ByteBuffer; +import java.nio.channels.FileChannel; import java.nio.charset.StandardCharsets; import java.nio.file.Files; import java.nio.file.Path; @@ -30,11 +32,15 @@ import java.util.ArrayList; import java.util.List; import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicReference; import java.util.function.LongSupplier; +import java.util.stream.Stream; import org.junit.Rule; import org.junit.Test; import org.junit.rules.TemporaryFolder; +import org.mockito.MockedStatic; +import org.mockito.Mockito; import com.cloud.utils.exception.CloudRuntimeException; @@ -110,6 +116,36 @@ public void failsWhenIdentityCannotBeSaved() throws IOException { assertThrows(IOException.class, () -> ManagementServerId.load(path, null, unexpectedLookup())); } + @Test + public void failedInitialWriteDoesNotPublishAnInvalidIdentity() throws IOException { + Path path = idFile(); + AtomicReference underlyingWriter = new AtomicReference<>(); + FileChannel failedWriter = Mockito.mock(FileChannel.class); + Mockito.when(failedWriter.write(Mockito.any(ByteBuffer.class))).thenThrow(new IOException("Interrupted write")); + Mockito.doAnswer(invocation -> { + underlyingWriter.get().close(); + return null; + }).when(failedWriter).close(); + + try (MockedStatic channels = Mockito.mockStatic(FileChannel.class, invocation -> { + FileChannel channel = (FileChannel) invocation.callRealMethod(); + Path openedPath = invocation.getArgument(0); + if (openedPath.getFileName().toString().endsWith(".lock")) { + return channel; + } + underlyingWriter.compareAndSet(null, channel); + return failedWriter; + })) { + assertThrows(IOException.class, () -> ManagementServerId.load(path, null, () -> 42L)); + } + + assertFalse(Files.exists(path)); + try (Stream files = Files.list(path.getParent())) { + assertEquals(1L, files.count()); // Only the interprocess lock remains. + } + assertEquals(43L, ManagementServerId.load(path, null, () -> 43L)); + } + @Test public void readsWhitespaceAndFullLongIds() throws IOException { Path path = folder.newFile("id").toPath(); From 81987abfa78263f6e5b759250fbea822ce699102 Mon Sep 17 00:00:00 2001 From: Mashrfee Aryan Date: Sat, 31 Oct 2026 22:27:49 -0500 Subject: [PATCH 4/6] Use the persistent management server ID consistently Route the remaining management identity consumers through ManagementServerNode instead of reading the current MAC address. Cover the management service ID and disk statistics ownership when the saved identity differs from the MAC. Related to #14253. --- .../CloudZonesStartupProcessor.java | 7 +- .../network/NetworkUsageManagerImpl.java | 5 +- .../VirtualNetworkApplianceManagerImpl.java | 3 +- .../cloud/server/ManagementServerImpl.java | 3 +- .../java/com/cloud/server/StatsCollector.java | 3 +- .../server/ManagementServerIdentityTest.java | 83 +++++++++++++++++++ 6 files changed, 92 insertions(+), 12 deletions(-) create mode 100644 server/src/test/java/com/cloud/server/ManagementServerIdentityTest.java diff --git a/server/src/main/java/com/cloud/hypervisor/CloudZonesStartupProcessor.java b/server/src/main/java/com/cloud/hypervisor/CloudZonesStartupProcessor.java index f2704f35fcdd..83000f1967d2 100644 --- a/server/src/main/java/com/cloud/hypervisor/CloudZonesStartupProcessor.java +++ b/server/src/main/java/com/cloud/hypervisor/CloudZonesStartupProcessor.java @@ -22,6 +22,8 @@ import javax.inject.Inject; import javax.naming.ConfigurationException; +import org.apache.cloudstack.utils.identity.ManagementServerNode; + import org.springframework.stereotype.Component; import com.cloud.agent.AgentManager; @@ -38,7 +40,6 @@ import com.cloud.host.HostVO; import com.cloud.hypervisor.Hypervisor.HypervisorType; import com.cloud.utils.component.AdapterBase; -import com.cloud.utils.net.MacAddress; import com.cloud.utils.net.NetUtils; /** @@ -60,9 +61,7 @@ public class CloudZonesStartupProcessor extends AdapterBase implements StartupCo public boolean configure(String name, Map params) throws ConfigurationException { _agentManager.registerForInitialConnects(this, false); if (_nodeId == -1) { - // FIXME: We really should not do this like this. It should be done - // at config time and is stored as a config variable. - _nodeId = MacAddress.getMacAddress().toLong(); + _nodeId = ManagementServerNode.getManagementServerId(); } return true; } diff --git a/server/src/main/java/com/cloud/network/NetworkUsageManagerImpl.java b/server/src/main/java/com/cloud/network/NetworkUsageManagerImpl.java index 0aee5f234969..37bc183058cd 100644 --- a/server/src/main/java/com/cloud/network/NetworkUsageManagerImpl.java +++ b/server/src/main/java/com/cloud/network/NetworkUsageManagerImpl.java @@ -28,6 +28,8 @@ import javax.inject.Inject; import javax.naming.ConfigurationException; +import org.apache.cloudstack.utils.identity.ManagementServerNode; + import org.springframework.stereotype.Component; import org.apache.cloudstack.api.command.admin.usage.AddTrafficMonitorCmd; @@ -88,7 +90,6 @@ import com.cloud.utils.db.TransactionCallbackNoReturn; import com.cloud.utils.db.TransactionStatus; import com.cloud.utils.exception.CloudRuntimeException; -import com.cloud.utils.net.MacAddress; @Component public class NetworkUsageManagerImpl extends ManagerBase implements NetworkUsageService, NetworkUsageManager, ResourceStateAdapter { @@ -251,7 +252,7 @@ protected class DirectNetworkStatsListener implements Listener { private int _interval; - private final long mgmtSrvrId = MacAddress.getMacAddress().toLong(); + private final long mgmtSrvrId = ManagementServerNode.getManagementServerId(); protected DirectNetworkStatsListener(int interval) { _interval = interval; diff --git a/server/src/main/java/com/cloud/network/router/VirtualNetworkApplianceManagerImpl.java b/server/src/main/java/com/cloud/network/router/VirtualNetworkApplianceManagerImpl.java index e2415e200768..3d3249f212f3 100644 --- a/server/src/main/java/com/cloud/network/router/VirtualNetworkApplianceManagerImpl.java +++ b/server/src/main/java/com/cloud/network/router/VirtualNetworkApplianceManagerImpl.java @@ -250,7 +250,6 @@ import com.cloud.utils.fsm.StateListener; import com.cloud.utils.fsm.StateMachine2; import com.cloud.utils.net.Ip; -import com.cloud.utils.net.MacAddress; import com.cloud.utils.net.NetUtils; import com.cloud.vm.DomainRouterVO; import com.cloud.vm.Nic; @@ -365,7 +364,7 @@ public class VirtualNetworkApplianceManagerImpl extends ManagerBase implements V private int _routerExtraPublicNics = 2; private int _usageAggregationRange = 1440; private String _usageTimeZone = "GMT"; - private final long mgmtSrvrId = MacAddress.getMacAddress().toLong(); + private final long mgmtSrvrId = ManagementServerNode.getManagementServerId(); private static final int ACQUIRE_GLOBAL_LOCK_TIMEOUT_FOR_COOPERATION = 5; // 5 seconds private boolean _dailyOrHourly = false; diff --git a/server/src/main/java/com/cloud/server/ManagementServerImpl.java b/server/src/main/java/com/cloud/server/ManagementServerImpl.java index 95e688b5d8cb..7d16e8fbf71b 100644 --- a/server/src/main/java/com/cloud/server/ManagementServerImpl.java +++ b/server/src/main/java/com/cloud/server/ManagementServerImpl.java @@ -847,7 +847,6 @@ import com.cloud.utils.db.TransactionStatus; import com.cloud.utils.exception.CloudRuntimeException; import com.cloud.utils.fsm.StateMachine2; -import com.cloud.utils.net.MacAddress; import com.cloud.utils.net.NetUtils; import com.cloud.utils.security.CertificateHelper; import com.cloud.utils.ssh.SSHKeysHelper; @@ -1200,7 +1199,7 @@ public DetailVO findDetail(final long hostId, final String name) { @Override public long getId() { - return MacAddress.getMacAddress().toLong(); + return ManagementServerNode.getManagementServerId(); } protected void checkPortParameters(final String publicPort, final String privatePort, final String privateIp, final String proto) { diff --git a/server/src/main/java/com/cloud/server/StatsCollector.java b/server/src/main/java/com/cloud/server/StatsCollector.java index 3eec27d4f54d..b224a7f1a2fb 100644 --- a/server/src/main/java/com/cloud/server/StatsCollector.java +++ b/server/src/main/java/com/cloud/server/StatsCollector.java @@ -150,7 +150,6 @@ import com.cloud.utils.db.TransactionCallbackNoReturn; import com.cloud.utils.db.TransactionStatus; import com.cloud.utils.exception.CloudRuntimeException; -import com.cloud.utils.net.MacAddress; import com.cloud.utils.script.Script; import com.cloud.vm.NicVO; import com.cloud.vm.UserVmManager; @@ -391,7 +390,7 @@ public String toString() { private ScheduledExecutorService _diskStatsUpdateExecutor; private int _usageAggregationRange = 1440; private String _usageTimeZone = "GMT"; - private final long mgmtSrvrId = MacAddress.getMacAddress().toLong(); + private final long mgmtSrvrId = ManagementServerNode.getManagementServerId(); private static final int ACQUIRE_GLOBAL_LOCK_TIMEOUT_FOR_COOPERATION = 5; // 5 seconds private boolean _dailyOrHourly = false; protected long managementServerNodeId = ManagementServerNode.getManagementServerId(); diff --git a/server/src/test/java/com/cloud/server/ManagementServerIdentityTest.java b/server/src/test/java/com/cloud/server/ManagementServerIdentityTest.java new file mode 100644 index 000000000000..7b62ed97cbb4 --- /dev/null +++ b/server/src/test/java/com/cloud/server/ManagementServerIdentityTest.java @@ -0,0 +1,83 @@ +// 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.server; + +import static org.junit.Assert.assertEquals; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.mockStatic; +import static org.mockito.Mockito.when; + +import org.apache.cloudstack.utils.identity.ManagementServerNode; +import org.junit.Test; +import org.mockito.MockedStatic; +import org.mockito.Mockito; +import org.springframework.test.util.ReflectionTestUtils; + +import com.cloud.cluster.ManagementServerHostVO; +import com.cloud.cluster.dao.ManagementServerHostDao; +import com.cloud.utils.db.Filter; +import com.cloud.utils.db.GlobalLock; +import com.cloud.utils.db.Transaction; +import com.cloud.utils.db.TransactionCallbackNoReturn; +import com.cloud.utils.net.MacAddress; + +public class ManagementServerIdentityTest { + private static final long SAVED_ID = 123L; + private static final long CHANGED_MAC_ID = 456L; + + @Test + public void managementServiceReportsSavedIdAfterMacChanges() { + try (MockedStatic node = mockStatic(ManagementServerNode.class); + MockedStatic mac = mockStatic(MacAddress.class)) { + node.when(ManagementServerNode::getManagementServerId).thenReturn(SAVED_ID); + mac.when(MacAddress::getMacAddress).thenReturn(new MacAddress(CHANGED_MAC_ID)); + + ManagementServerImpl server = mock(ManagementServerImpl.class, Mockito.CALLS_REAL_METHODS); + assertEquals(SAVED_ID, server.getId()); + } + } + + @Test + public void owningServerStillUpdatesDiskStatsAfterMacChanges() { + StatsCollector previous = StatsCollector.getInstance(); + try (MockedStatic node = mockStatic(ManagementServerNode.class); + MockedStatic mac = mockStatic(MacAddress.class); + MockedStatic locks = mockStatic(GlobalLock.class); + MockedStatic transactions = mockStatic(Transaction.class)) { + node.when(ManagementServerNode::getManagementServerId).thenReturn(SAVED_ID); + mac.when(MacAddress::getMacAddress).thenReturn(new MacAddress(CHANGED_MAC_ID)); + GlobalLock lock = mock(GlobalLock.class); + locks.when(() -> GlobalLock.getInternLock("vm.disk.stats")).thenReturn(lock); + when(lock.lock(5)).thenReturn(true); + + ManagementServerHostVO host = new ManagementServerHostVO(); + host.setMsid(SAVED_ID); + ManagementServerHostDao dao = mock(ManagementServerHostDao.class); + when(dao.findOneInUpState(any(Filter.class))).thenReturn(host); + StatsCollector collector = new StatsCollector(); + ReflectionTestUtils.setField(collector, "managementServerHostDao", dao); + + collector.new VmDiskStatsUpdaterTask().runInContext(); + + transactions.verify(() -> Transaction.execute(any(TransactionCallbackNoReturn.class))); + } finally { + ReflectionTestUtils.setField(StatsCollector.class, "s_instance", previous); + } + } +} From 79800388b3344dfdb993843a5d5343fb8e05bf2d Mon Sep 17 00:00:00 2001 From: Mashrfee Aryan Date: Sat, 31 Oct 2026 22:27:50 -0500 Subject: [PATCH 5/6] Keep active management IPs out of agent avoid lists Build the avoid list from one snapshot of nonremoved management server records. An Up record takes precedence over stale non-Up identities sharing its IP; unavailable addresses remain avoided without duplicates. Retain database records and document the recovery behavior. Related to #14253. --- INSTALL.md | 3 +- .../dao/ManagementServerHostDaoImpl.java | 26 ++++--- .../dao/ManagementServerHostDaoImplTest.java | 71 +++++++++++++++++++ 3 files changed, 89 insertions(+), 11 deletions(-) create mode 100644 framework/cluster/src/test/java/com/cloud/cluster/dao/ManagementServerHostDaoImplTest.java diff --git a/INSTALL.md b/INSTALL.md index 49a1aaf158dd..da9d5de52357 100644 --- a/INSTALL.md +++ b/INSTALL.md @@ -213,7 +213,8 @@ These JVM options can be added to `JAVA_OPTS` in For an existing installation, if interface order has already changed before the first start with this fix, set `management.server.id` to that server's original `msid` from `cloud.mshost`. Keep the override on subsequent starts. Existing stale -rows in `cloud.mshost` are not removed by this change. +rows in `cloud.mshost` are retained, but an old non-Up row no longer causes agents +to avoid an IP address that also has an Up management server. ## Notes 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 ec943a9c26be..5bfc45efabfb 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 @@ -21,7 +21,10 @@ import java.sql.SQLException; import java.util.ArrayList; import java.util.Date; +import java.util.HashSet; +import java.util.LinkedHashSet; import java.util.List; +import java.util.Set; import java.util.TimeZone; @@ -35,7 +38,6 @@ import com.cloud.utils.db.DB; import com.cloud.utils.db.Filter; import com.cloud.utils.db.GenericDaoBase; -import com.cloud.utils.db.GenericSearchBuilder; import com.cloud.utils.db.SearchBuilder; import com.cloud.utils.db.SearchCriteria; import com.cloud.utils.db.TransactionLegacy; @@ -47,7 +49,6 @@ public class ManagementServerHostDaoImpl extends GenericDaoBase ActiveSearch; private final SearchBuilder InactiveSearch; private final SearchBuilder StateSearch; - protected GenericSearchBuilder NonUpStateMsSearch; @Override public void invalidateRunSession(long id, long runid) { @@ -229,11 +230,6 @@ protected ManagementServerHostDaoImpl() { StateSearch.and("state", StateSearch.entity().getState(), SearchCriteria.Op.IN); StateSearch.and("runid", StateSearch.entity().getRunid(), SearchCriteria.Op.GT); StateSearch.done(); - - NonUpStateMsSearch = createSearchBuilder(String.class); - NonUpStateMsSearch.selectFields(NonUpStateMsSearch.entity().getServiceIP()); - NonUpStateMsSearch.and("state", NonUpStateMsSearch.entity().getState(), SearchCriteria.Op.NLIKE); - NonUpStateMsSearch.done(); } @Override @@ -269,9 +265,19 @@ public List listBy(ManagementServerHost.State... states) @Override public List listNonUpStateMsIPs() { - SearchCriteria sc = NonUpStateMsSearch.create(); - sc.addAnd("state", SearchCriteria.Op.NLIKE, State.Up); - return customSearch(sc, null); + Set upIPs = new HashSet<>(); + Set nonUpIPs = new LinkedHashSet<>(); + // Read one snapshot, excluding removed records. An old identity must not + // make agents avoid an address that still has an Up management server. + for (ManagementServerHostVO host : listAll()) { + if (State.Up.equals(host.getState())) { + upIPs.add(host.getServiceIP()); + } else { + nonUpIPs.add(host.getServiceIP()); + } + } + nonUpIPs.removeAll(upIPs); + return new ArrayList<>(nonUpIPs); } @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 new file mode 100644 index 000000000000..43a17b634495 --- /dev/null +++ b/framework/cluster/src/test/java/com/cloud/cluster/dao/ManagementServerHostDaoImplTest.java @@ -0,0 +1,71 @@ +// 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 static org.junit.Assert.assertEquals; +import static org.mockito.Mockito.doReturn; +import static org.mockito.Mockito.spy; + +import java.util.Arrays; +import java.util.Collections; +import java.util.List; + +import org.apache.cloudstack.management.ManagementServerHost.State; +import org.junit.Test; + +import com.cloud.cluster.ManagementServerHostVO; + +public class ManagementServerHostDaoImplTest { + private final ManagementServerHostDaoImpl dao = spy(new ManagementServerHostDaoImpl()); + + @Test + public void staleDownIdentityDoesNotBlockAnUpServerAtTheSameIp() { + ManagementServerHostVO down = host("10.0.0.1", State.Down); + ManagementServerHostVO up = host("10.0.0.1", State.Up); + assertAvoided(Arrays.asList(down, up)); + assertAvoided(Arrays.asList(up, down)); + } + + @Test + public void unavailableAddressesRemainAvoidedWithoutDuplicates() { + assertAvoided(Arrays.asList(host("10.0.0.1", State.Down), host("10.0.0.1", State.Down), + host("10.0.0.2", State.Maintenance), host("10.0.0.3", State.Up)), "10.0.0.1", "10.0.0.2"); + } + + @Test + public void upServerTakesPrecedenceOverAnOldMaintenanceRecord() { + assertAvoided(Arrays.asList(host("10.0.0.1", State.Maintenance), host("10.0.0.1", State.Up))); + } + + @Test + public void emptyServerListHasNoAvoidedAddresses() { + assertAvoided(Collections.emptyList()); + } + + private void assertAvoided(List hosts, String... expected) { + doReturn(hosts).when(dao).listAll(); + assertEquals(Arrays.asList(expected), dao.listNonUpStateMsIPs()); + } + + private ManagementServerHostVO host(String ip, State state) { + ManagementServerHostVO host = new ManagementServerHostVO(); + host.setServiceIP(ip); + host.setState(state); + return host; + } +} From c22a289db2dfe9847d286515696bdf085a7a2f6c Mon Sep 17 00:00:00 2001 From: Mashrfee Aryan Date: Sun, 1 Nov 2026 12:52:39 -0600 Subject: [PATCH 6/6] Clarify management server identity comments and documentation Explain the persistent ID flow, identity consumers, and regression tests with short comments without ending periods. Keep installation instructions concise and document preserving the ID file, configuration overrides, and recovery for existing installations. Update release notes for stable IDs and active IP handling. Restore CloudZonesStartupProcessor to its pre-fix version because its local node ID is unused. Keep its existing configure() behavior and exclude the file from the overall PR diff. --- INSTALL.md | 42 ++++++++----------- PendingReleaseNotes | 9 ++-- .../dao/ManagementServerHostDaoImpl.java | 10 ++++- .../dao/ManagementServerHostDaoImplTest.java | 7 ++++ .../CloudZonesStartupProcessor.java | 7 ++-- .../network/NetworkUsageManagerImpl.java | 1 + .../VirtualNetworkApplianceManagerImpl.java | 1 + .../cloud/server/ManagementServerImpl.java | 1 + .../java/com/cloud/server/StatsCollector.java | 1 + .../server/ManagementServerIdentityTest.java | 6 +++ .../utils/identity/ManagementServerId.java | 17 ++++++++ .../utils/identity/ManagementServerNode.java | 6 +++ .../identity/ManagementServerIdTest.java | 19 ++++++++- 13 files changed, 92 insertions(+), 35 deletions(-) diff --git a/INSTALL.md b/INSTALL.md index da9d5de52357..76e2a389eee8 100644 --- a/INSTALL.md +++ b/INSTALL.md @@ -190,31 +190,23 @@ All the rpm packages will be created in `dist/rpmbuild/RPMS/x86_64` directory. ## Management server identity -The management server saves its numeric ID in -`${user.home}/.cloudstack/management-server.id`. Packaged installations run as -`cloud`, whose home is normally `/var/lib/cloudstack/management`. On first use, -the server saves the ID derived from its MAC address. Later starts reuse the -saved ID, even if network interface order changes. - -Keep this file when upgrading or restoring the same management server. Do not -copy it to a different management server. The service user needs read and write -access to the directory, including the adjacent `.lock` file. If the ID cannot -be read, validated, or saved, startup fails. - -These JVM options can be added to `JAVA_OPTS` in -`/etc/default/cloudstack-management`: - -* `-Dmanagement.server.id.file=/path/to/management-server.id` changes the file - location. Use a persistent local directory writable by the service user. -* `-Dmanagement.server.id=123456789` overrides the saved ID with a positive - decimal integer. Each management server must have its own ID. This option does - not change the saved file; removing it restores the saved ID. - -For an existing installation, if interface order has already changed before the -first start with this fix, set `management.server.id` to that server's original -`msid` from `cloud.mshost`. Keep the override on subsequent starts. Existing stale -rows in `cloud.mshost` are retained, but an old non-Up row no longer causes agents -to avoid an IP address that also has an Up management server. +The server saves its ID in `${user.home}/.cloudstack/management-server.id` +and reuses it after restarts. Keep this file when upgrading or restoring the +same server. Do not copy it to a different management server. + +The service account needs write access to the directory and its `.lock` file. +Startup fails if the ID cannot be read, validated, or saved. + +These options can be added to `JAVA_OPTS` in `/etc/default/cloudstack-management`: + +* `-Dmanagement.server.id.file=/path/to/management-server.id` chooses a different + persistent local file location. +* `-Dmanagement.server.id=123456789` overrides the ID with a positive number + unique to this server. It does not change the saved file. + +If interface order already changed before upgrading, set `management.server.id` +to this server's original `msid` from `cloud.mshost` before the first start. +Keep that override configured on later starts. ## Notes diff --git a/PendingReleaseNotes b/PendingReleaseNotes index ad914a97124b..3a0ee3a836c4 100644 --- a/PendingReleaseNotes +++ b/PendingReleaseNotes @@ -41,10 +41,11 @@ example.ver.1 > example.ver.2: size while a stripped down Linux should fit on a 2.88MB floppy. 4.22.1.0 > 4.22.2.0: - * Management servers now save their ID locally and reuse it after restarts, - preventing network interface reordering from changing the ID. Keep the ID - file when upgrading or restoring a server. See INSTALL.md for the file - location and JVM options to override the ID or its storage location. + * Management servers now reuse a saved ID after restarts, so network interface + reordering does not change their identity. Keep the ID file during upgrades + and restores. See INSTALL.md for the file location and ID settings. + * Old non-Up management server records no longer cause agents to avoid an IP + that also has an Up record. 4.22.0.0 > 4.22.0.1: * Disk-only instance snapshots for KVM UEFI VMs now include a sidecar copy of 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 5bfc45efabfb..99610747ab97 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 @@ -263,20 +263,26 @@ public List listBy(ManagementServerHost.State... states) return listBy(sc); } + // The saved ID stops future ID changes but leaves old records behind + // This method handles duplicate records that already exist + // Old Down records must not make agents skip an address that also has an Up record @Override public List listNonUpStateMsIPs() { Set upIPs = new HashSet<>(); Set nonUpIPs = new LinkedHashSet<>(); - // Read one snapshot, excluding removed records. An old identity must not - // make agents avoid an address that still has an Up management server. + // Read server records that have not been removed for (ManagementServerHostVO host : listAll()) { if (State.Up.equals(host.getState())) { + // Collect addresses marked Up upIPs.add(host.getServiceIP()); } else { + // Collect addresses in other states nonUpIPs.add(host.getServiceIP()); } } + // Keep every Up address out of the avoid list nonUpIPs.removeAll(upIPs); + // Give agents the remaining addresses to skip return new ArrayList<>(nonUpIPs); } 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 43a17b634495..7a2ea02877e1 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 @@ -30,9 +30,11 @@ import com.cloud.cluster.ManagementServerHostVO; +// Checks that old server records do not make agents skip an available address public class ManagementServerHostDaoImplTest { private final ManagementServerHostDaoImpl dao = spy(new ManagementServerHostDaoImpl()); + // Do not avoid an address if one record says Up and an old record says Down @Test public void staleDownIdentityDoesNotBlockAnUpServerAtTheSameIp() { ManagementServerHostVO down = host("10.0.0.1", State.Down); @@ -41,27 +43,32 @@ public void staleDownIdentityDoesNotBlockAnUpServerAtTheSameIp() { assertAvoided(Arrays.asList(up, down)); } + // Keep unavailable addresses in the avoid list and include each address only once @Test public void unavailableAddressesRemainAvoidedWithoutDuplicates() { assertAvoided(Arrays.asList(host("10.0.0.1", State.Down), host("10.0.0.1", State.Down), host("10.0.0.2", State.Maintenance), host("10.0.0.3", State.Up)), "10.0.0.1", "10.0.0.2"); } + // Allow an address marked Up even if an old record says it is in maintenance @Test public void upServerTakesPrecedenceOverAnOldMaintenanceRecord() { assertAvoided(Arrays.asList(host("10.0.0.1", State.Maintenance), host("10.0.0.1", State.Up))); } + // No server records means there are no addresses to avoid @Test public void emptyServerListHasNoAvoidedAddresses() { assertAvoided(Collections.emptyList()); } + // Run the avoid-list code with pretend records and check the returned addresses private void assertAvoided(List hosts, String... expected) { doReturn(hosts).when(dao).listAll(); assertEquals(Arrays.asList(expected), dao.listNonUpStateMsIPs()); } + // Make a pretend server record with an address and a state private ManagementServerHostVO host(String ip, State state) { ManagementServerHostVO host = new ManagementServerHostVO(); host.setServiceIP(ip); diff --git a/server/src/main/java/com/cloud/hypervisor/CloudZonesStartupProcessor.java b/server/src/main/java/com/cloud/hypervisor/CloudZonesStartupProcessor.java index 83000f1967d2..f2704f35fcdd 100644 --- a/server/src/main/java/com/cloud/hypervisor/CloudZonesStartupProcessor.java +++ b/server/src/main/java/com/cloud/hypervisor/CloudZonesStartupProcessor.java @@ -22,8 +22,6 @@ import javax.inject.Inject; import javax.naming.ConfigurationException; -import org.apache.cloudstack.utils.identity.ManagementServerNode; - import org.springframework.stereotype.Component; import com.cloud.agent.AgentManager; @@ -40,6 +38,7 @@ import com.cloud.host.HostVO; import com.cloud.hypervisor.Hypervisor.HypervisorType; import com.cloud.utils.component.AdapterBase; +import com.cloud.utils.net.MacAddress; import com.cloud.utils.net.NetUtils; /** @@ -61,7 +60,9 @@ public class CloudZonesStartupProcessor extends AdapterBase implements StartupCo public boolean configure(String name, Map params) throws ConfigurationException { _agentManager.registerForInitialConnects(this, false); if (_nodeId == -1) { - _nodeId = ManagementServerNode.getManagementServerId(); + // FIXME: We really should not do this like this. It should be done + // at config time and is stored as a config variable. + _nodeId = MacAddress.getMacAddress().toLong(); } return true; } diff --git a/server/src/main/java/com/cloud/network/NetworkUsageManagerImpl.java b/server/src/main/java/com/cloud/network/NetworkUsageManagerImpl.java index 37bc183058cd..fa669acac024 100644 --- a/server/src/main/java/com/cloud/network/NetworkUsageManagerImpl.java +++ b/server/src/main/java/com/cloud/network/NetworkUsageManagerImpl.java @@ -252,6 +252,7 @@ protected class DirectNetworkStatsListener implements Listener { private int _interval; + // Use the saved ID to check which traffic monitors this server is in charge of private final long mgmtSrvrId = ManagementServerNode.getManagementServerId(); protected DirectNetworkStatsListener(int interval) { diff --git a/server/src/main/java/com/cloud/network/router/VirtualNetworkApplianceManagerImpl.java b/server/src/main/java/com/cloud/network/router/VirtualNetworkApplianceManagerImpl.java index 3d3249f212f3..b057d53f01b0 100644 --- a/server/src/main/java/com/cloud/network/router/VirtualNetworkApplianceManagerImpl.java +++ b/server/src/main/java/com/cloud/network/router/VirtualNetworkApplianceManagerImpl.java @@ -364,6 +364,7 @@ public class VirtualNetworkApplianceManagerImpl extends ManagerBase implements V private int _routerExtraPublicNics = 2; private int _usageAggregationRange = 1440; private String _usageTimeZone = "GMT"; + // Use the saved ID to find the routers this server is in charge of private final long mgmtSrvrId = ManagementServerNode.getManagementServerId(); private static final int ACQUIRE_GLOBAL_LOCK_TIMEOUT_FOR_COOPERATION = 5; // 5 seconds private boolean _dailyOrHourly = false; diff --git a/server/src/main/java/com/cloud/server/ManagementServerImpl.java b/server/src/main/java/com/cloud/server/ManagementServerImpl.java index 7d16e8fbf71b..a417ff84a3a3 100644 --- a/server/src/main/java/com/cloud/server/ManagementServerImpl.java +++ b/server/src/main/java/com/cloud/server/ManagementServerImpl.java @@ -1197,6 +1197,7 @@ public DetailVO findDetail(final long hostId, final String name) { return _detailsDao.findDetail(hostId, name); } + // Return the persistent server ID shared by the other CloudStack components @Override public long getId() { return ManagementServerNode.getManagementServerId(); diff --git a/server/src/main/java/com/cloud/server/StatsCollector.java b/server/src/main/java/com/cloud/server/StatsCollector.java index b224a7f1a2fb..ee606fb838f8 100644 --- a/server/src/main/java/com/cloud/server/StatsCollector.java +++ b/server/src/main/java/com/cloud/server/StatsCollector.java @@ -390,6 +390,7 @@ public String toString() { private ScheduledExecutorService _diskStatsUpdateExecutor; private int _usageAggregationRange = 1440; private String _usageTimeZone = "GMT"; + // Use the saved ID to check which stats jobs this server should do private final long mgmtSrvrId = ManagementServerNode.getManagementServerId(); private static final int ACQUIRE_GLOBAL_LOCK_TIMEOUT_FOR_COOPERATION = 5; // 5 seconds private boolean _dailyOrHourly = false; diff --git a/server/src/test/java/com/cloud/server/ManagementServerIdentityTest.java b/server/src/test/java/com/cloud/server/ManagementServerIdentityTest.java index 7b62ed97cbb4..00c3d9373ebb 100644 --- a/server/src/test/java/com/cloud/server/ManagementServerIdentityTest.java +++ b/server/src/test/java/com/cloud/server/ManagementServerIdentityTest.java @@ -37,10 +37,13 @@ import com.cloud.utils.db.TransactionCallbackNoReturn; import com.cloud.utils.net.MacAddress; +// Check that the server and its stats jobs use the saved ID public class ManagementServerIdentityTest { + // Use different values for the saved ID and the ID from the network card private static final long SAVED_ID = 123L; private static final long CHANGED_MAC_ID = 456L; + // Return the saved ID even when the network card gives a different ID @Test public void managementServiceReportsSavedIdAfterMacChanges() { try (MockedStatic node = mockStatic(ManagementServerNode.class); @@ -53,6 +56,7 @@ public void managementServiceReportsSavedIdAfterMacChanges() { } } + // Still start the disk stats job when the saved ID says this server is in charge @Test public void owningServerStillUpdatesDiskStatsAfterMacChanges() { StatsCollector previous = StatsCollector.getInstance(); @@ -66,6 +70,7 @@ public void owningServerStillUpdatesDiskStatsAfterMacChanges() { locks.when(() -> GlobalLock.getInternLock("vm.disk.stats")).thenReturn(lock); when(lock.lock(5)).thenReturn(true); + // Pretend the database says this server should do the job ManagementServerHostVO host = new ManagementServerHostVO(); host.setMsid(SAVED_ID); ManagementServerHostDao dao = mock(ManagementServerHostDao.class); @@ -77,6 +82,7 @@ public void owningServerStillUpdatesDiskStatsAfterMacChanges() { transactions.verify(() -> Transaction.execute(any(TransactionCallbackNoReturn.class))); } finally { + // Put the old stats collector back so other tests are not affected ReflectionTestUtils.setField(StatsCollector.class, "s_instance", previous); } } diff --git a/utils/src/main/java/org/apache/cloudstack/utils/identity/ManagementServerId.java b/utils/src/main/java/org/apache/cloudstack/utils/identity/ManagementServerId.java index fb4f40d252e0..edeb78e2f431 100644 --- a/utils/src/main/java/org/apache/cloudstack/utils/identity/ManagementServerId.java +++ b/utils/src/main/java/org/apache/cloudstack/utils/identity/ManagementServerId.java @@ -32,38 +32,51 @@ import com.cloud.utils.exception.CloudRuntimeException; import com.cloud.utils.net.MacAddress; +// Keeps the management server ID the same across restarts final class ManagementServerId { private ManagementServerId() { } + // Find the ID file and load the ID static long get() { Path path = Paths.get(System.getProperty("management.server.id.file", Paths.get(System.getProperty("user.home"), ".cloudstack", "management-server.id").toString())); try { + // Use the ID from management.server.id if it is set + // Otherwise, read the saved ID from the file at path + // If the file does not exist, create an ID from the MAC address and save it return load(path, System.getProperty("management.server.id"), () -> MacAddress.getMacAddress().toLong()); } catch (IOException | IllegalArgumentException e) { + // Stop with a clear error instead of silently choosing a different identity throw new CloudRuntimeException("Unable to load the management server ID from " + path, e); } } static long load(Path path, String configuredId, LongSupplier initialId) throws IOException { + // An operator-provided ID takes priority without overwriting the saved file if (configuredId != null) { return parse(configuredId); } + // Ensure the directory exists, then lock the neighboring .lock file so + // two processes cannot initialize the same identity file at the same time Files.createDirectories(path.toAbsolutePath().getParent()); try (FileChannel lockChannel = FileChannel.open(path.resolveSibling(path.getFileName() + ".lock"), StandardOpenOption.CREATE, StandardOpenOption.WRITE); FileLock lock = lockChannel.lock()) { + // Reuse the saved ID without checking network interfaces again if (Files.exists(path)) { + // An ID is a short number; reject an unexpectedly large file if (Files.size(path) > 64) { throw new IllegalArgumentException("Invalid management server ID file"); } return parse(new String(Files.readAllBytes(path), StandardCharsets.UTF_8)); } + // First startup: obtain the initial MAC-derived ID and validate it long id = parse(Long.toString(initialId.getAsLong())); + // Write in the same directory so the completed file can be renamed atomically Path temporary = Files.createTempFile(path.toAbsolutePath().getParent(), "management-server-id-", ".tmp"); try { try (FileChannel channel = FileChannel.open(temporary, StandardOpenOption.WRITE)) { @@ -71,16 +84,20 @@ static long load(Path path, String configuredId, LongSupplier initialId) throws while (buffer.hasRemaining()) { channel.write(buffer); } + // Flush the complete ID to storage before publishing it channel.force(true); } + // Make the completed file appear under its final name in one operation Files.move(temporary, path, StandardCopyOption.ATOMIC_MOVE); } finally { + // Clean up after a failed save; a successful move already removed this name Files.deleteIfExists(temporary); } return id; } } + // Ignore surrounding whitespace and require a positive whole number that fits in a Java long private static long parse(String value) { long id = Long.parseLong(value.trim()); if (id <= 0) { diff --git a/utils/src/main/java/org/apache/cloudstack/utils/identity/ManagementServerNode.java b/utils/src/main/java/org/apache/cloudstack/utils/identity/ManagementServerNode.java index cc85ca18c02b..e9df0f991573 100644 --- a/utils/src/main/java/org/apache/cloudstack/utils/identity/ManagementServerNode.java +++ b/utils/src/main/java/org/apache/cloudstack/utils/identity/ManagementServerNode.java @@ -26,14 +26,18 @@ import com.cloud.utils.component.SystemIntegrityChecker; import com.cloud.utils.exception.CloudRuntimeException; +// Holds the management server ID and shares it with other CloudStack components public class ManagementServerNode extends AdapterBase implements SystemIntegrityChecker { + // Load the ID once and keep the same value for the life of this process private static final long s_nodeId = ManagementServerId.get(); + // Register this component to run early during CloudStack startup public ManagementServerNode() { setRunLevel(ComponentLifecycle.RUN_LEVEL_FRAMEWORK_BOOTSTRAP); } + // A valid server ID must be greater than zero @Override public void check() { if (s_nodeId <= 0) { @@ -41,10 +45,12 @@ public void check() { } } + // Other components call this to get the same server ID public static long getManagementServerId() { return s_nodeId; } + // Check the ID during startup. Log an error and stop if the check fails @Override public boolean start() { try { diff --git a/utils/src/test/java/org/apache/cloudstack/utils/identity/ManagementServerIdTest.java b/utils/src/test/java/org/apache/cloudstack/utils/identity/ManagementServerIdTest.java index a385c085cb2a..8962150ec4ff 100644 --- a/utils/src/test/java/org/apache/cloudstack/utils/identity/ManagementServerIdTest.java +++ b/utils/src/test/java/org/apache/cloudstack/utils/identity/ManagementServerIdTest.java @@ -46,19 +46,23 @@ public class ManagementServerIdTest { + // Give each test its own temporary folder @Rule public TemporaryFolder folder = new TemporaryFolder(); + // Choose where this test will save its ID private Path idFile() { return folder.getRoot().toPath().resolve("identity/management-server.id"); } + // Fail the test if the code asks for a new ID when it should not private LongSupplier unexpectedLookup() { return () -> { throw new AssertionError("Network interfaces must not be consulted"); }; } + // Keep the saved ID even when the network card would give a different ID @Test public void preservesInitialIdWhenNetworkInterfacesChange() throws IOException { Path path = idFile(); @@ -67,6 +71,7 @@ public void preservesInitialIdWhenNetworkInterfacesChange() throws IOException { assertEquals(130014950130305L, ManagementServerId.load(path, null, unexpectedLookup())); } + // Use the chosen ID first and keep the saved ID for later @Test public void configuredIdTakesPrecedenceOverSavedId() throws IOException { Path path = idFile(); @@ -75,12 +80,14 @@ public void configuredIdTakesPrecedenceOverSavedId() throws IOException { assertEquals(42L, ManagementServerId.load(path, null, unexpectedLookup())); } + // A chosen ID still works when we cannot save a file @Test public void configuredIdDoesNotRequireWritableStorage() throws IOException { Path path = folder.newFile("not-a-directory").toPath().resolve("id"); assertEquals(123L, ManagementServerId.load(path, "123", unexpectedLookup())); } + // Reject bad IDs from the settings and do not save them @Test public void rejectsInvalidConfiguredIds() { for (String value : new String[] {"", "0", "-1", "invalid", "9223372036854775808"}) { @@ -90,6 +97,7 @@ public void rejectsInvalidConfiguredIds() { assertFalse(Files.exists(idFile())); } + // Reject a bad saved ID without changing its file @Test public void rejectsInvalidSavedIdsWithoutReplacingThem() throws IOException { Path path = folder.newFile("id").toPath(); @@ -101,6 +109,7 @@ public void rejectsInvalidSavedIdsWithoutReplacingThem() throws IOException { } } + // Do not save a new ID if it is zero or negative @Test public void rejectsInvalidInitialIdsWithoutSavingThem() { for (long value : new long[] {0L, -1L}) { @@ -110,16 +119,19 @@ public void rejectsInvalidInitialIdsWithoutSavingThem() { assertFalse(Files.exists(idFile())); } + // Report an error when the ID file cannot be saved @Test public void failsWhenIdentityCannotBeSaved() throws IOException { Path path = folder.newFile("not-a-directory").toPath().resolve("id"); assertThrows(IOException.class, () -> ManagementServerId.load(path, null, unexpectedLookup())); } + // A failed write leaves no broken ID file and a later try can succeed @Test public void failedInitialWriteDoesNotPublishAnInvalidIdentity() throws IOException { Path path = idFile(); AtomicReference underlyingWriter = new AtomicReference<>(); + // Make the file writer fail on purpose FileChannel failedWriter = Mockito.mock(FileChannel.class); Mockito.when(failedWriter.write(Mockito.any(ByteBuffer.class))).thenThrow(new IOException("Interrupted write")); Mockito.doAnswer(invocation -> { @@ -141,11 +153,12 @@ public void failedInitialWriteDoesNotPublishAnInvalidIdentity() throws IOExcepti assertFalse(Files.exists(path)); try (Stream files = Files.list(path.getParent())) { - assertEquals(1L, files.count()); // Only the interprocess lock remains. + assertEquals(1L, files.count()); // Only the interprocess lock remains } assertEquals(43L, ManagementServerId.load(path, null, () -> 43L)); } + // Allow spaces around a valid ID and accept the biggest allowed number @Test public void readsWhitespaceAndFullLongIds() throws IOException { Path path = folder.newFile("id").toPath(); @@ -153,6 +166,7 @@ public void readsWhitespaceAndFullLongIds() throws IOException { assertEquals(Long.MAX_VALUE, ManagementServerId.load(path, null, unexpectedLookup())); } + // Use the chosen file and ID settings and report a bad saved file @Test public void usesJvmSettingsAndReportsInvalidStorage() throws IOException { String previousFile = System.getProperty("management.server.id.file"); @@ -176,6 +190,7 @@ public void usesJvmSettingsAndReportsInvalidStorage() throws IOException { } } + // Three programs using the same file must all get the same ID @Test public void concurrentProcessesUseTheSameId() throws Exception { Path path = idFile(); @@ -199,12 +214,14 @@ public void concurrentProcessesUseTheSameId() throws Exception { } } + // A small helper program that loads an ID and prints it public static class IdWriter { public static void main(String[] args) throws IOException { System.out.println(ManagementServerId.load(Paths.get(args[0]), null, () -> Long.parseLong(args[1]))); } } + // Put the settings back so one test does not affect another private void restoreProperty(String name, String value) { if (value == null) { System.clearProperty(name);