Copilot commented on code in PR #14320: URL: https://github.com/apache/cloudstack/pull/14320#discussion_r4562144290
########## scripts/vm/hypervisor/kvm/kvmheartbeat_rbd.py: ########## @@ -0,0 +1,165 @@ +#!/usr/bin/env python3 +# 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. + +# Ceph RBD flavor of kvmheartbeat.sh/kvmsmpheartbeat.sh. +# +# There is no shared POSIX mount point to write a heartbeat file to when the +# primary storage pool is Ceph RBD, so the heartbeat timestamp is instead +# stored as a small RADOS object (one object per host) in the same RBD pool. +# Any host with a working path to the Ceph cluster can write/read this object, +# which gives the same semantics as the NFS/SharedMountPoint heartbeat file. +# +# The object is written and read with the Python bindings of librados +# (python3-rados), see kvmha_rbd.py. The cephx key is not an argument, as it would be visible in the process list: it is +# read from the environment variable CEPH_KEY. +# +# A host is only reported as DEAD when its heartbeat is there and stale. If there is none (the agent of the host is not +# upgraded yet, or has no bindings, or the first heartbeat is not written yet), or if it can not be read, the state of the +# host is not known and the exit status is 2, as for a Ceph cluster which can not be reached. + +import argparse +import os +import re +import subprocess +import sys +import time + +from kvmha_rbd import BindingsError, Ceph, CephError, DEFAULT_WRITE_TIMEOUT, KEY_ENV, ObjectNotFoundError, check_bindings, heartbeat_max_age, silence_standard_error + + +class Parser(argparse.ArgumentParser): + def error(self, message): + self.print_usage(sys.stderr) + sys.exit(1) + + +def main(): + parser = Parser(description="Writes or checks the heartbeat of a KVM host on Ceph RBD storage", add_help=False) + parser.add_argument("-s", dest="mon_hosts", default="", help="ceph monitor host(s), comma separated") + parser.add_argument("-o", dest="pool", default="", help="ceph/rbd pool name") + parser.add_argument("-n", dest="user", default="", help="cephx auth user (optional, the key is then in " + KEY_ENV + ")") + parser.add_argument("-h", dest="host", default="", help="host") + parser.add_argument("-r", dest="check", action="store_true", help="read the heartbeat, instead of writing it") + parser.add_argument("-c", dest="cleanup", action="store_true", help="cleanup: reboot the host") + parser.add_argument("-t", dest="interval", default="", help="interval in seconds at which the heartbeat is written, when it is read") + parser.add_argument("-w", dest="write_timeout", default="", + help="seconds after which the agent stops a write of the heartbeat, when it is read, default %d" % DEFAULT_WRITE_TIMEOUT) + parser.add_argument("--check-bindings", dest="check_bindings", action="store_true", + help="only check that the Python bindings of librados and librbd are installed") + parser.add_argument("--help", action="help") + args = parser.parse_args() + + if args.check_bindings: + # asked by the agent when it registers a storage pool for HA, the RBD scripts need both bindings + try: + check_bindings(watchers=True) + except BindingsError as error: + print("=====> The Python bindings of Ceph are missing, %s <======" % error) + sys.exit(1) + return + + # first of all: the Python bindings are only suggested by the agent packages. The self-fencing (-c) does not + # need them, as it does not use Ceph + if not args.cleanup: + try: + check_bindings() + except BindingsError as error: + if args.check: + # not a proof that the host is dead, so DEAD is not reported + print("=====> Unable to check the heartbeat, %s <======" % error) + sys.exit(2) + print("=====> Unable to write the heartbeat, %s <======" % error) + sys.exit(1) + + key = os.environ.pop(KEY_ENV, "") + if not args.mon_hosts or not args.pool: + sys.exit(1) + # the host (its GUID) names the heartbeat object, so it is required except for the self-fencing (-c) + if not args.cleanup and not args.host: + sys.exit(1) + if args.user and not key: + sys.exit(1) + if args.check and not re.fullmatch(r"[0-9]+", args.interval): + sys.exit(1) + if args.write_timeout and not re.fullmatch(r"[0-9]+", args.write_timeout): + sys.exit(1) + write_timeout = int(args.write_timeout) if args.write_timeout else DEFAULT_WRITE_TIMEOUT + + if args.cleanup: + reboot() + return + + heartbeat_object = "KVMHA-hb-" + args.host + silence_standard_error() + ceph = Ceph(args.mon_hosts, args.pool, args.user, key) + try: + if args.check: + check(ceph, heartbeat_object, args.pool, int(args.interval), write_timeout) + return + try: + ceph.write(heartbeat_object, "%d\n" % time.time()) Review Comment: `time.time()` returns a float, but `%d` formatting requires an integer in Python 3, which will raise a `TypeError` and crash the heartbeat writer. Convert the timestamp to an `int` (or use integer formatting without `%d`) so the heartbeat is written reliably and can be parsed by the reader regex. ########## plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/KVMStoragePoolManager.java: ########## @@ -445,7 +452,7 @@ public boolean disconnectPhysicalDisk(StoragePoolType type, String poolUuid, Str public synchronized boolean deleteStoragePool(StoragePoolType type, String uuid) { StorageAdaptor adaptor = getStorageAdaptor(type); - if (type == StoragePoolType.NetworkFilesystem) { + if (HighAvailabilityManager.LIBVIRT_STORAGE_POOL_TYPES_WITH_HA_SUPPORT.contains(type)) { _haMonitor.unregisterStoragePool(uuid); } boolean deleteStatus = adaptor.deleteStoragePool(uuid); Review Comment: HA monitoring is unregistered before attempting the pool deletion. If the delete operation fails (returns false or throws), the pool may remain present/usable but no longer monitored for HA, which can cause incorrect HA behavior on that host. Consider unregistering only after a successful delete, or re-registering on failure (and/or handling exceptions) to keep HA state consistent with actual pool lifecycle. ########## scripts/vm/hypervisor/kvm/kvmha_rbd.py: ########## @@ -0,0 +1,181 @@ +# 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. + +# Access to Ceph for kvmheartbeat_rbd.py and kvmvmactivity_rbd.py. +# +# The objects of the heartbeat and of the activity state, and the watchers of the +# volumes, are read and written over one connection to the Ceph cluster with the +# Python bindings of librados and librbd (the packages python3-rados and python3-rbd). +# An operation which does not finish in TIMEOUT seconds fails, so that it does not +# block forever if the cluster can not be reached. +# +# The cephx key is not an argument of the scripts, as it would be visible in the +# process list: the scripts read it from the environment variable CEPH_KEY, and give +# it to the bindings in their configuration. +# +# Nothing is written to the standard error output: the agent merges it with the +# standard output and reads only the first line of the result, so a message of +# librados would hide the result of the script. + +import os + +try: + import rados +except ImportError: + rados = None + +try: + import rbd +except ImportError: + rbd = None + +KEY_ENV = "CEPH_KEY" +TIMEOUT = 30 +MAX_OBJECT_SIZE = 1024 * 1024 +# A heartbeat is written every <interval> seconds, but a write which starts on time can take long when Ceph is slow +# (connecting and writing take up to TIMEOUT seconds each), so the heartbeat of a host which is alive can be older than +# the interval. The agent stops a write after heartbeat.update.timeout (60 seconds by default), which is the longest a +# write can take, and the agent gives it to the scripts with -w. +DEFAULT_WRITE_TIMEOUT = 2 * TIMEOUT + + +def heartbeat_max_age(interval, write_timeout=DEFAULT_WRITE_TIMEOUT): + """The age in seconds up to which a heartbeat, which is written every interval seconds, is not stale.""" + return interval + write_timeout + + +def silence_standard_error(): + """Sends the standard error output, which librados may write to, to /dev/null.""" + try: + devnull = os.open(os.devnull, os.O_WRONLY) + os.dup2(devnull, 2) + os.close(devnull) + except OSError: + pass + + +class BindingsError(Exception): + """The Python bindings of librados or librbd are not installed or can not be used.""" + + +class CephError(Exception): + """The cluster could not be connected, or an object could not be read, written or found out if it exists.""" + + +class ObjectNotFoundError(CephError): + """The object does not exist, which is different from a cluster which can not be reached.""" + + +class WatchersError(Exception): + """The watchers of a volume could not be listed: it is not known whether the host is alive.""" + + +def check_bindings(watchers=False): + """ + Raises BindingsError if the Python bindings are not installed. The scripts call it first, as the packages + python3-rados and python3-rbd are only suggested by the agent packages. + + :param watchers: whether the watchers of volumes are listed, which needs librbd + """ + if rados is None: + raise BindingsError("the package python3-rados is not installed") + if watchers and (rbd is None or not hasattr(rbd.Image, "watchers_list")): + raise BindingsError("the package python3-rbd is not installed, or is too old to list the watchers of a volume") + + +class Ceph: + def __init__(self, mon_hosts, pool, user, key, watchers=False): + """ + Connects to the cluster. Raises BindingsError if the bindings can not be used (see check_bindings). If the cluster can not be connected, the operations raise CephError. + + :param watchers: whether the watchers of volumes are listed, which needs librbd + """ + check_bindings(watchers) + self.pool = pool + self.cluster = None + self.ioctx = None + conf = {"mon_host": mon_hosts, "rados_osd_op_timeout": str(TIMEOUT), "rados_mon_op_timeout": str(TIMEOUT), + "client_mount_timeout": str(TIMEOUT), "log_to_stderr": "false", "err_to_stderr": "false"} + if user: + conf["key"] = key + cluster = rados.Rados(rados_id=user if user else None, conf=conf) + try: + cluster.connect(timeout=TIMEOUT) + self.ioctx = cluster.open_ioctx(pool) + self.cluster = cluster + except rados.Error: + cluster.shutdown() Review Comment: On connection/open failure, the exception is swallowed and the instance is left in a disconnected state, causing later operations to raise `CephError(name)` where `name` is typically an object name (which can obscure that the root cause was connection failure). Consider raising a `CephError` (or re-raising with context) here so failures are reported closer to the source with clearer diagnostics. ########## plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/KVMStoragePoolManager.java: ########## @@ -458,10 +465,14 @@ public synchronized boolean deleteStoragePool(StoragePoolType type, String uuid) public boolean deleteStoragePool(StoragePoolType type, String uuid, Map<String, String> details) { StorageAdaptor adaptor = getStorageAdaptor(type); boolean deleteStatus; - if (type == StoragePoolType.NetworkFilesystem) { + if (HighAvailabilityManager.LIBVIRT_STORAGE_POOL_TYPES_WITH_HA_SUPPORT.contains(type)) { _haMonitor.unregisterStoragePool(uuid); + } + if (type == StoragePoolType.NetworkFilesystem) { deleteStatus = adaptor.deleteStoragePool(uuid); } else { + // 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 deleteStatus = adaptor.deleteStoragePool(uuid, details); } Review Comment: HA monitoring is unregistered before attempting the pool deletion. If the delete operation fails (returns false or throws), the pool may remain present/usable but no longer monitored for HA, which can cause incorrect HA behavior on that host. Consider unregistering only after a successful delete, or re-registering on failure (and/or handling exceptions) to keep HA state consistent with actual pool lifecycle. ########## scripts/vm/hypervisor/kvm/kvmvmactivity_rbd.py: ########## @@ -0,0 +1,219 @@ +#!/usr/bin/env python3 +# 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. + +# Ceph RBD flavor of kvmvmactivity.sh. +# +# On NFS/SharedMountPoint storage, VM disk activity is detected via the mtime +# of the volume files on the shared mount point. RBD volumes aren't files on +# a mount point, so instead activity is detected via RBD watchers: as long as +# qemu has an RBD image open (i.e. a VM using that volume is running +# somewhere), the image will have a live watcher. The most recent +# suspect-time/watcher-state is persisted as a RADOS object (per host) +# in place of the "ac-<host>" file used by the NFS/SMP scripts. +# +# The volumes are given in -u, or in a file with -U, which has no limit on the number +# of volumes: one argument of a command is limited to 128 KiB, which is about 3500 +# volumes. The file has the uuids separated by commas or by lines. +# +# The objects and the watchers are read and written over one connection to the Ceph +# cluster with the Python bindings of librados and librbd (python3-rados and +# python3-rbd), which is much faster than "rbd status" for each volume when a host has +# many volumes, see kvmha_rbd.py. The cephx key is not an argument, as it would be +# visible in the process list: it is read from the environment variable CEPH_KEY. +# +# Exit status: 0 with ALIVE or DEAD in the output, 2 if the arguments are not +# valid or if the state of the host could not be found out, which is not a DEAD host. + +import argparse +import os +import re +import sys +import time + +from kvmha_rbd import BindingsError, Ceph, check_bindings, CephError, WatchersError, DEFAULT_WRITE_TIMEOUT, KEY_ENV, heartbeat_max_age, silence_standard_error + +# the interval at which the agents write their heartbeat, if it is not given with -i +DEFAULT_HEARTBEAT_INTERVAL = 60 + + +class Parser(argparse.ArgumentParser): + def error(self, message): + self.print_usage(sys.stderr) + sys.exit(1) Review Comment: This script’s header comment states it exits with status 2 when arguments are not valid, but `ArgumentParser.error()` currently exits with 1. Aligning the exit code with the documented contract (and with the other argument-validation exits in this script) will make behavior more predictable for callers and future maintenance. ########## plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/LibvirtStoragePool.java: ########## @@ -357,37 +366,109 @@ 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() { + if (StoragePoolType.RBD.equals(type) && StringUtils.isNotBlank(authSecret)) { + return Map.of(RBD_KEY_ENVIRONMENT_VARIABLE, authSecret); } return null; } Review Comment: This exports `CEPH_KEY` to the script environment whenever `authSecret` is set, even if `authUsername` is blank. Since `addRbdConnectionArgs` enforces user+key must be both set or both unset, consider applying the same condition here (require both username and secret) to keep behavior consistent and avoid passing a secret unnecessarily. -- 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]
