Copilot commented on code in PR #14320: URL: https://github.com/apache/cloudstack/pull/14320#discussion_r4564007148
########## utils/src/test/java/com/cloud/utils/RbdUtilsTest.java: ########## @@ -0,0 +1,144 @@ +// 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 static org.junit.Assert.assertEquals; + +import org.junit.Test; + +public class RbdUtilsTest { + + @Test + public void testRbdMonitorsWithoutPort() { + assertEquals("10.0.0.1", RbdUtils.getRbdMonitors("10.0.0.1", 0)); + assertEquals("10.0.0.1,10.0.0.2,10.0.0.3", RbdUtils.getRbdMonitors("10.0.0.1,10.0.0.2,10.0.0.3", 0)); + assertEquals("fd00::1,fd00::2", RbdUtils.getRbdMonitors("fd00::1,fd00::2", 0)); + } Review Comment: Given `RbdUtils.getRbdMonitors` is a new public helper, consider adding test coverage for `null` and fully-blank inputs (e.g. `null`, `\"\"`, `\" , \"`) to lock in the intended behavior and prevent regressions (especially if callers outside `LibvirtStoragePool` reuse it). ########## 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); + } Review Comment: `getRbdMonitors` will throw a `NullPointerException` when `sourceHost` is `null` due to `sourceHost.split(\",\")`. Since this is a public utility method, it should defensively handle `null` (and ideally blank input) by returning an empty string (or another clearly documented behavior), so callers don’t need to guard it everywhere. ########## 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); + } + } Review Comment: `addRbdConnectionArgs` validates monitors but does not validate `sourceDir` (the RBD pool name). If `sourceDir` is blank/null, the scripts will exit with an error and HA monitoring will silently degrade into “cannot tell” behavior. Consider adding an explicit `StringUtils.isNotBlank(sourceDir)` check and throwing a `CloudRuntimeException` with a specific message (similar to the monitors check) when it’s missing. -- 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]
