Copilot commented on code in PR #14320:
URL: https://github.com/apache/cloudstack/pull/14320#discussion_r4563508913
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/LibvirtStoragePool.java:
##########
@@ -357,37 +366,110 @@ public boolean isPoolSupportHA() {
public String getHearthBeatPath() {
if (StoragePoolType.NetworkFilesystem.equals(type)) {
- String kvmScriptsDir =
AgentPropertiesFileHandler.getPropertyValue(AgentProperties.KVM_SCRIPTS_DIR);
- String scriptPath = Script.findScript(kvmScriptsDir,
"kvmheartbeat.sh");
- if (scriptPath == null) {
- throw new CloudRuntimeException("Unable to find heartbeat
script 'kvmheartbeat.sh' in directory: " + kvmScriptsDir);
- }
- return scriptPath;
+ return findKvmHaScript("kvmheartbeat.sh");
} else if (StoragePoolType.SharedMountPoint.equals(type)) {
- String kvmScriptsDir =
AgentPropertiesFileHandler.getPropertyValue(AgentProperties.KVM_SCRIPTS_DIR);
- String scriptPath = Script.findScript(kvmScriptsDir,
"kvmsmpheartbeat.sh");
- if (scriptPath == null) {
- throw new CloudRuntimeException("Unable to find heartbeat
script 'kvmsmpheartbeat.sh' in directory: " + kvmScriptsDir);
- }
- return scriptPath;
+ return findKvmHaScript("kvmsmpheartbeat.sh");
+ } else if (StoragePoolType.RBD.equals(type)) {
+ return findKvmHaScript("kvmheartbeat_rbd.py");
+ }
+ return null;
+ }
+
+ private String findKvmHaScript(String scriptName) {
+ String kvmScriptsDir =
AgentPropertiesFileHandler.getPropertyValue(AgentProperties.KVM_SCRIPTS_DIR);
+ String scriptPath = Script.findScript(kvmScriptsDir, scriptName);
+ if (scriptPath == null) {
+ throw new CloudRuntimeException(String.format("Unable to find
script '%s' in directory: %s", scriptName, kvmScriptsDir));
+ }
+ return scriptPath;
+ }
+
+ /**
+ * Returns the environment of a heartbeat/VM-activity check script: for a
RBD storage pool with cephx, the
+ * Ceph key, which is not an argument of the script as it would be visible
in the process list.
+ */
+ protected Map<String, String> getHeartBeatScriptEnvironment() {
+ // the key is only given with its user, as addRbdConnectionArgs
requires
+ if (StoragePoolType.RBD.equals(type) &&
StringUtils.isNotBlank(authUsername) && StringUtils.isNotBlank(authSecret)) {
+ return Map.of(RBD_KEY_ENVIRONMENT_VARIABLE, authSecret);
}
return null;
}
+ /**
+ * Adds the Ceph cluster connection details (monitors, pool and, if cephx
is enabled, the user; the key is in
+ * {@link #getHeartBeatScriptEnvironment()}) to a heartbeat/VM-activity
check {@link Script} for a RBD storage pool. Mirrors the "mon_host"/"id"/"key"
+ * options that qemu itself uses to talk to RBD (see {@link
KVMPhysicalDisk#RBDStringBuilder}).
+ */
+ protected void addRbdConnectionArgs(Script cmd) {
+ boolean hasUser = StringUtils.isNotBlank(authUsername);
+ boolean hasSecret = StringUtils.isNotBlank(authSecret);
+ if (hasUser != hasSecret) {
+ throw new CloudRuntimeException(String.format("The Ceph user and
key must be either both set or both unset for the storage pool %s", uuid));
+ }
+ String monitors = StringUtils.isBlank(sourceHost) ? "" :
RbdUtils.getRbdMonitors(sourceHost, sourcePort);
+ if (monitors.isEmpty()) {
+ throw new CloudRuntimeException(String.format("The storage pool %s
has no Ceph monitors", uuid));
+ }
+ cmd.add("-s", monitors);
+ cmd.add("-o", sourceDir);
+ if (hasUser) {
+ cmd.add("-n", authUsername);
+ }
+ }
+
+ /**
+ * Adds the arguments identifying the storage to a heartbeat/VM-activity
check {@link Script}:
+ * the Ceph connection details for a RBD pool, or the NFS server, path and
mount point otherwise.
+ */
+ private void addPoolConnectionArgs(Script cmd, HAStoragePool pool) {
+ if (StoragePoolType.RBD.equals(type)) {
+ addRbdConnectionArgs(cmd);
+ } else {
+ cmd.add("-i", pool.getPoolIp());
+ cmd.add("-p", pool.getPoolMountSourcePath());
+ cmd.add("-m", pool.getMountDestPath());
+ }
+ }
+
+ /**
+ * Adds the volumes of the host to a VM-activity check {@link Script}. For
a RBD storage pool they are given in
+ * a file, as they can be too many for an argument of a command (the limit
is 128 KiB, about 3500 volumes),
+ * which would also be written to the log, with all the volumes, at each
check. Otherwise, or if the file can not
+ * be written, they are an argument.
+ *
+ * @return the file, which has to be deleted after the script ran, or null
if there is none
+ */
+ protected File addVolumeListArgs(Script cmd, String volumes) {
+ if (StoragePoolType.RBD.equals(type)) {
+ File volumeListFile = null;
+ try {
+ volumeListFile = File.createTempFile("kvmha-volumes-", ".txt");
+ Files.write(volumeListFile.toPath(),
volumes.getBytes(StandardCharsets.UTF_8));
+ cmd.add("-U", volumeListFile.getAbsolutePath());
+ return volumeListFile;
+ } catch (IOException e) {
+ logger.warn("Unable to write the list of volumes to a file,
giving it to the script as an argument: {}", e.getMessage());
+ if (volumeListFile != null && !volumeListFile.delete()) {
+ logger.debug("Unable to delete the file {}",
volumeListFile);
+ }
Review Comment:
The temp file is created with default permissions (in the default temp
directory) and then populated with data passed to an external script. To
minimize risk of unintended exposure on misconfigured systems, consider
explicitly restricting permissions (e.g., owner-read/write only) and/or
creating the file using `java.nio.file.Files.createTempFile` with secure
permissions where supported.
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/KVMStoragePoolManager.java:
##########
@@ -445,24 +453,41 @@ public boolean disconnectPhysicalDisk(StoragePoolType
type, String poolUuid, Str
public synchronized boolean deleteStoragePool(StoragePoolType type, String
uuid) {
StorageAdaptor adaptor = getStorageAdaptor(type);
- if (type == StoragePoolType.NetworkFilesystem) {
- _haMonitor.unregisterStoragePool(uuid);
- }
- boolean deleteStatus = adaptor.deleteStoragePool(uuid);
- synchronized (_storagePools) {
- _storagePools.remove(uuid);
- }
- return deleteStatus;
+ return deleteStoragePoolWithoutHaMonitoring(type, uuid, () ->
adaptor.deleteStoragePool(uuid));
}
public boolean deleteStoragePool(StoragePoolType type, String uuid,
Map<String, String> details) {
StorageAdaptor adaptor = getStorageAdaptor(type);
- boolean deleteStatus;
- if (type == StoragePoolType.NetworkFilesystem) {
+ return deleteStoragePoolWithoutHaMonitoring(type, uuid, () -> {
+ if (type == StoragePoolType.NetworkFilesystem) {
+ return adaptor.deleteStoragePool(uuid);
+ }
+ // the other pool types, also RBD and SharedMountPoint, keep the
delete which takes the details: it does not
+ // destroy the libvirt pool of a pool which is still used by the
VMs, e.g. in maintenance of the pool
+ return adaptor.deleteStoragePool(uuid, details);
+ });
+ }
+
+ /**
+ * Deletes a storage pool after it is no longer monitored for HA. The
monitor writes the heartbeats while it holds its lock,
+ * so none is on going when the pool is unregistered, and none is written
while the pool is removed, which could fail
+ * and fence the host. If the pool is not deleted (the delete does not
succeed or throws) it is still there, so it is
+ * monitored for HA again: the heartbeat of the host on it must go on.
+ */
+ private boolean deleteStoragePoolWithoutHaMonitoring(StoragePoolType type,
String uuid, BooleanSupplier delete) {
+ KVMHABase.HAStoragePool haPool = null;
+ if
(HighAvailabilityManager.LIBVIRT_STORAGE_POOL_TYPES_WITH_HA_SUPPORT.contains(type))
{
+ haPool = _haMonitor.getStoragePool(uuid);
_haMonitor.unregisterStoragePool(uuid);
- deleteStatus = adaptor.deleteStoragePool(uuid);
- } else {
- deleteStatus = adaptor.deleteStoragePool(uuid, details);
+ }
+ boolean deleteStatus = false;
+ try {
+ deleteStatus = delete.getAsBoolean();
+ } finally {
+ if (!deleteStatus && haPool != null) {
+ logger.warn("The storage pool {} was not deleted, so it is
monitored for HA again.", uuid);
+ _haMonitor.addStoragePool(haPool);
+ }
}
synchronized (_storagePools) {
_storagePools.remove(uuid);
Review Comment:
When `deleteStatus` is `false`, the pool is re-added to HA monitoring but is
still unconditionally removed from `_storagePools`. This leaves the manager’s
internal state inconsistent (pool not deleted but no longer tracked). Only
remove from `_storagePools` when the delete actually succeeds; if the delete
fails (returns false), keep the entry so subsequent operations can still find
the pool.
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/KVMHAMonitor.java:
##########
@@ -60,7 +67,11 @@ public void removeStoragePool(String uuid) {
synchronized (haStoragePools) {
HAStoragePool pool = haStoragePools.get(uuid);
if (pool != null) {
- Script.runSimpleBashScript("umount " +
pool.getMountDestPath());
+ if (StringUtils.isNotEmpty(pool.getMountDestPath())) {
+ Script umount = new Script("umount", logger);
+ umount.add(pool.getMountDestPath());
+ umount.execute();
Review Comment:
Unmount operations can block/hang (e.g., stalled I/O or kernel wait), and
this call provides no explicit timeout or failure handling. Consider using a
`Script` constructor that sets a bounded timeout and logging/handling of
non-zero exit status, so pool removal cannot indefinitely block
agent/HA-related flows.
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/KVMStoragePool.java:
##########
@@ -155,6 +155,32 @@ default String getLocalPathFor(String relativePath) {
String createHeartBeatCommand(HAStoragePool primaryStoragePool, String
hostPrivateIp, boolean hostValidation);
+ /**
+ * The id which names the heartbeat of a host on this storage pool: by
default the private IP address of the host.
+ *
+ * @param hostPrivateIp the private IP address of the host
+ * @param hostGuid the GUID of the host as the management server
knows it
+ */
+ default String getHeartBeatHostId(String hostPrivateIp, String hostGuid) {
+ return hostPrivateIp;
Review Comment:
With the introduction of `getHeartBeatHostId(...)`,
`createHeartBeatCommand(..., String hostPrivateIp, ...)` is no longer
necessarily passed a private IP (it may be a GUID for RBD). To avoid confusion
for implementers, consider renaming the parameter in the interface (parameter
name only) and/or updating the Javadoc to describe it as a generic 'heartbeat
host identifier' rather than strictly an IP.
##########
utils/src/main/java/com/cloud/utils/RbdUtils.java:
##########
@@ -0,0 +1,78 @@
+// 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.utils;
+
+import java.util.ArrayList;
+import java.util.List;
+
+import org.apache.commons.lang3.StringUtils;
+
+public class RbdUtils {
+
+ private RbdUtils() {
+ }
+
+ /**
+ * Returns the Ceph monitors as expected by "--mon-host": the
comma-separated monitors of the pool, trimmed
+ * and without empty entries. If the pool has a monitor port, it is added
to each monitor which has none yet.
+ *
+ * @param sourceHost the comma-separated monitors, IPv4 or IPv6 addresses
or host names
+ * @param sourcePort the port of the monitors, or 0 (or less) if the pool
has none
+ */
+ public static String getRbdMonitors(String sourceHost, int sourcePort) {
+ List<String> monitors = new ArrayList<>();
+ for (String monitor : sourceHost.split(",")) {
+ monitor = monitor.trim();
+ if (monitor.isEmpty()) {
+ continue;
+ }
+ monitors.add(sourcePort > 0 ? addPortToRbdMonitor(monitor,
sourcePort) : monitor);
+ }
+ return String.join(",", monitors);
+ }
+
+ protected static String addPortToRbdMonitor(String monitor, int port) {
+ if (monitor.startsWith("[")) {
+ // IPv6 address in square brackets, which has a port if followed
by ":<port>"
+ return monitor.contains("]:") ? monitor : monitor + ":" + port;
+ }
+ int colons = StringUtils.countMatches(monitor, ":");
+ if (colons == 0) {
+ return monitor + ":" + port;
+ }
+ if (colons == 1) {
+ // IPv4 address or host name, with a port
+ return monitor;
+ }
Review Comment:
Inputs like an unbracketed IPv6 monitor that already includes a port (e.g.
`fd00::1:3300`) will be treated as 'IPv6 without port' and will incorrectly get
wrapped and have an additional `:<port>` appended. Consider either (a)
explicitly rejecting this ambiguous form with a clear exception, or (b)
detecting an IPv6+port pattern (e.g., trailing `:<digits>`) and leaving it
unchanged. Adding a dedicated unit test for this case would prevent regressions.
--
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]
