Damans227 commented on code in PR #14551: URL: https://github.com/apache/cloudstack/pull/14551#discussion_r4468497132
########## utils/src/main/java/org/apache/cloudstack/utils/identity/ManagementServerId.java: ########## @@ -0,0 +1,108 @@ +// 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.StandardCopyOption; +import java.nio.file.StandardOpenOption; +import java.util.function.LongSupplier; + +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())); Review Comment: @MashrafeeAryan on upgrade, first start has no id file so it still takes id from mac. if the upgrade changes java and nic order flips, wont we save the wrong id forever? maybe reuse the old id from mshost table if one exists for this host? ########## utils/src/main/java/org/apache/cloudstack/utils/identity/ManagementServerNode.java: ########## @@ -25,27 +25,32 @@ import com.cloud.utils.component.ComponentLifecycle; import com.cloud.utils.component.SystemIntegrityChecker; import com.cloud.utils.exception.CloudRuntimeException; -import com.cloud.utils.net.MacAddress; +// Holds the management server ID and shares it with other CloudStack components public class ManagementServerNode extends AdapterBase implements SystemIntegrityChecker { - private static final long s_nodeId = MacAddress.getMacAddress().toLong(); + // Load the ID once and keep the same value for the life of this process + private static final long s_nodeId = ManagementServerId.get(); Review Comment: @MashrafeeAryan id is now loaded when the class loads. if reading the file fails, wont it break every class using ManagementServerNode instead of giving a clean error? can we load it in start() instead? ########## utils/src/main/java/org/apache/cloudstack/utils/identity/ManagementServerId.java: ########## @@ -0,0 +1,108 @@ +// 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.StandardCopyOption; +import java.nio.file.StandardOpenOption; +import java.util.function.LongSupplier; + +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)) { Review Comment: @MashrafeeAryan if someone clones the ms vm, the clone gets the same id file and same id as the original. today the new mac would give it a new one. should we catch this somehow? ########## framework/cluster/src/main/java/com/cloud/cluster/dao/ManagementServerHostDaoImpl.java: ########## @@ -267,11 +263,27 @@ public List<ManagementServerHostVO> 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<String> listNonUpStateMsIPs() { Review Comment: @MashrafeeAryan instead of skipping old down rows in the avoid list, can we just mark old rows with the same ip as removed when the ms registers? fixes the leftover rows too, like in #14448 -- 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]
