Copilot commented on code in PR #14320: URL: https://github.com/apache/cloudstack/pull/14320#discussion_r4231700911
########## scripts/vm/hypervisor/kvm/kvmheartbeat_rbd.sh: ########## @@ -0,0 +1,167 @@ +#!/bin/bash +# 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. + +help() { + printf "Usage: $0 + -s ceph monitor host(s), comma separated + -o ceph/rbd pool name + -n cephx auth user (optional) + -k cephx auth key (optional, required if -n is set) + -h host + -r write/read hb log + -c cleanup + -t interval between read hb log\n" + exit 1 +} +#set -x +MonHosts= +PoolName= +CephUser= +CephKey= +HostIP= +interval= +rflag=0 +cflag=0 + +while getopts 's:o:n:k:h:t:rc' OPTION +do + case $OPTION in + s) + MonHosts="$OPTARG" + ;; + o) + PoolName="$OPTARG" + ;; + n) + CephUser="$OPTARG" + ;; + k) + CephKey="$OPTARG" + ;; + h) + HostIP="$OPTARG" + ;; + r) + rflag=1 + ;; + t) + interval="$OPTARG" + ;; + c) + cflag=1 + ;; + *) + help + ;; + esac +done + +if [ -z "$MonHosts" ] || [ -z "$PoolName" ] +then + exit 1 +fi + +# the host IP names the heartbeat object, so it is required except for the self-fencing (-c) +if [ "$cflag" != "1" ] && [ -z "$HostIP" ] +then + exit 1 +fi + +if [ -n "$CephUser" ] && [ -z "$CephKey" ] +then + exit 1 +fi + +# -t is the maximum age of the heartbeat, which is compared in a check +if [ "$rflag" == "1" ] && ! [[ "$interval" =~ ^[0-9]+$ ]] +then + exit 1 +fi + +RadosOpts=(--mon-host "$MonHosts") +if [ -n "$CephUser" ] && [ "$cflag" != "1" ] +then + # the key is given to rados in a file, to keep it out of the process list + KeyFile=$(mktemp) + trap 'rm -f "$KeyFile"' EXIT + printf '%s' "$CephKey" > "$KeyFile" Review Comment: The comment claims the Ceph key is kept out of the process list, but the script still receives the key via `-k` as a command-line argument (visible in `ps` for the lifetime of this script). To actually keep secrets out of the process list, consider changing the interface so the key is passed via a file path (created by the caller) or via stdin/environment, and have the script only ever receive a file descriptor/path—not the raw key value. ########## scripts/vm/hypervisor/kvm/kvmvmactivity_rbd.sh: ########## @@ -0,0 +1,236 @@ +#!/bin/bash +# 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. + +help() { + printf "Usage: $0 + -s ceph monitor host(s), comma separated + -o ceph/rbd pool name + -n cephx auth user (optional) + -k cephx auth key (optional, required if -n is set) + -h host + -u volume (rbd image) uuid list + -t current time in seconds (accepted for compatibility with kvmvmactivity.sh, not used) + -d suspect time\n" + exit 1 +} + +#set -x + +MonHosts= +PoolName= +CephUser= +CephKey= +HostIP= +UUIDList= +SuspectTime= + +while getopts 's:o:n:k:h:u:t:d:' OPTION +do + case $OPTION in + s) + MonHosts="$OPTARG" + ;; + o) + PoolName="$OPTARG" + ;; + n) + CephUser="$OPTARG" + ;; + k) + CephKey="$OPTARG" + ;; + h) + HostIP="$OPTARG" + ;; + u) + UUIDList="$OPTARG" + ;; + t) + # not used, see help + ;; + d) + SuspectTime="$OPTARG" + ;; + *) + help + ;; + esac +done + +if [ -z "$MonHosts" ] || [ -z "$PoolName" ] +then + exit 2 +fi + +if [ -z "$SuspectTime" ] +then + exit 2 +fi + +# the host IP names the heartbeat and activity objects +if [ -z "$HostIP" ] +then + exit 2 +fi + +if [ -n "$CephUser" ] && [ -z "$CephKey" ] +then + exit 2 +fi + +RadosOpts=(--mon-host "$MonHosts") +RbdOpts=(--mon-host "$MonHosts") +if [ -n "$CephUser" ] +then + # the key is given to rados and rbd in a file, to keep it out of the process list + KeyFile=$(mktemp) + trap 'rm -f "$KeyFile"' EXIT + printf '%s' "$CephKey" > "$KeyFile" Review Comment: Same issue as `kvmheartbeat_rbd.sh`: even though rados/rbd are fed via a keyfile, the script itself still receives the raw key via `-k` on its command line (visible in the process list). To avoid key disclosure, change the calling contract so the script is not invoked with the raw key value (e.g., pass a keyfile path created by the caller, or read the key from stdin). ########## scripts/vm/hypervisor/kvm/kvmvmactivity_rbd.sh: ########## @@ -0,0 +1,236 @@ +#!/bin/bash +# 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. + +help() { + printf "Usage: $0 + -s ceph monitor host(s), comma separated + -o ceph/rbd pool name + -n cephx auth user (optional) + -k cephx auth key (optional, required if -n is set) + -h host + -u volume (rbd image) uuid list + -t current time in seconds (accepted for compatibility with kvmvmactivity.sh, not used) + -d suspect time\n" + exit 1 +} + +#set -x + +MonHosts= +PoolName= +CephUser= +CephKey= +HostIP= +UUIDList= +SuspectTime= + +while getopts 's:o:n:k:h:u:t:d:' OPTION +do + case $OPTION in + s) + MonHosts="$OPTARG" + ;; + o) + PoolName="$OPTARG" + ;; + n) + CephUser="$OPTARG" + ;; + k) + CephKey="$OPTARG" + ;; + h) + HostIP="$OPTARG" + ;; + u) + UUIDList="$OPTARG" + ;; + t) + # not used, see help + ;; + d) + SuspectTime="$OPTARG" + ;; + *) + help + ;; + esac +done + +if [ -z "$MonHosts" ] || [ -z "$PoolName" ] +then + exit 2 +fi + +if [ -z "$SuspectTime" ] +then + exit 2 +fi + +# the host IP names the heartbeat and activity objects +if [ -z "$HostIP" ] +then + exit 2 +fi + +if [ -n "$CephUser" ] && [ -z "$CephKey" ] +then + exit 2 +fi + +RadosOpts=(--mon-host "$MonHosts") +RbdOpts=(--mon-host "$MonHosts") +if [ -n "$CephUser" ] +then + # the key is given to rados and rbd in a file, to keep it out of the process list + KeyFile=$(mktemp) + trap 'rm -f "$KeyFile"' EXIT + printf '%s' "$CephKey" > "$KeyFile" + RadosOpts+=(--id "$CephUser" --keyfile "$KeyFile") + RbdOpts+=(--id "$CephUser" --keyfile "$KeyFile") +fi + +hbObject="KVMHA-hb-$HostIP" +acObject="KVMHA-ac-$HostIP" + +# the reason why the heartbeat did not show that the host is alive, added to the messages about a DEAD host +hbNote= + +dead() { + echo "=====> Considering host as DEAD due to $1.$hbNote <======" +} + +# First check: heartbeat object, same as kvmheartbeat_rbd.sh +now=$(date +%s) +if hb=$(rados -p "$PoolName" "${RadosOpts[@]}" get "$hbObject" - 2> /dev/null) +then + if [[ "$hb" =~ ^[0-9]+$ ]] + then + diff=$(expr $now - $hb) + if [ $diff -lt 61 ] + then + echo "=====> ALIVE <=====" + exit 0 + fi + hbNote=" The heartbeat in RADOS object [$hbObject] is [$diff] seconds old." + else + hbNote=" The RADOS object [$hbObject] does not hold a heartbeat." + fi +else + hbNote=" The RADOS object [$hbObject] could not be read, it does not exist or Ceph can not be reached." +fi + +if [ -z "$UUIDList" ] +then + dead "empty UUIDList" + exit 0 +fi + +# Second check: RBD watcher based disk activity check. +# If any of the host's volumes still has a live watcher, something (most +# likely qemu on the host being checked) is actively using it right now. +latestUpdateTime=0 +IFS=',' read -ra images <<< "$UUIDList" +for image in "${images[@]}" +do + image=${image//[[:space:]]/} + if [ -z "$image" ] + then + continue + fi + # when the watchers can not be listed, it is unknown whether the host is alive: the script fails, which + # is not a DEAD host + if ! watchers=$(rbd status "$PoolName/$image" "${RbdOpts[@]}" 2> /dev/null) + then + echo "=====> Unable to get the watchers of the image [$PoolName/$image] <======" + exit 2 + fi + # "Watchers: none", or a line "watcher=<address> client.<id> cookie=<cookie>" per watcher + if grep -q '^[[:space:]]*watcher=' <<< "$watchers" + then + latestUpdateTime=$now + break + fi +done + +acTime= +if rados -p "$PoolName" "${RadosOpts[@]}" stat "$acObject" &> /dev/null +then + if ! acTime=$(rados -p "$PoolName" "${RadosOpts[@]}" get "$acObject" - 2> /dev/null) + then + echo "=====> Unable to read the RADOS object [$acObject] <======" + exit 2 + fi +fi +# a state which is not "<suspect time>:<update time>" (with an optional third field of older versions) +# is ignored, as if there was none +if ! [[ "$acTime" =~ ^[0-9]+:[0-9]+(:[0-9]+)?$ ]] +then + acTime= +fi + +tmpFile=$(mktemp) +echo "$SuspectTime:$latestUpdateTime" > "$tmpFile" +rados -p "$PoolName" "${RadosOpts[@]}" put "$acObject" "$tmpFile" &> /dev/null +putResult=$? +rm -f "$tmpFile" +# without the new state the next check would use an old one, so it is not known whether the host is alive +if [ $putResult -ne 0 ] +then + echo "=====> Unable to write the RADOS object [$acObject] <======" + exit 2 +fi + +if [ -z "$acTime" ]; then + if [[ $latestUpdateTime -gt $SuspectTime ]]; then + echo "=====> ALIVE <=====" + else + dead "RADOS object [$acObject] did not exist or holds no state, and condition [latestUpdateTime -gt SuspectTime] has not been satisfied" + fi +else + arrTime=(${acTime//:/ }) + lastSuspectTime=${arrTime[0]} + lastUpdateTime=${arrTime[1]} + + suspectTimeDiff=$(expr $SuspectTime - $lastSuspectTime) + if [[ $suspectTimeDiff -lt 0 ]]; then + if [[ $latestUpdateTime -gt $SuspectTime ]]; then + echo "=====> ALIVE <=====" + else + dead "RADOS object [$acObject] exists, condition [suspectTimeDiff -lt 0] was satisfied and [latestUpdateTime -gt SuspectTime] has not been satisfied" + fi + else + if [[ $latestUpdateTime -gt $lastUpdateTime ]]; then + echo "=====> ALIVE <=====" + else + dead "RADOS object [$acObject] exists and conditions [suspectTimeDiff -lt 0] and [latestUpdateTime -gt SuspectTime] have not been satisfied" Review Comment: This DEAD reason is misleading in this branch: execution is inside the `else` of `if [[ $suspectTimeDiff -lt 0 ]]`, so `suspectTimeDiff -lt 0` is actually known to be false here. Consider updating the message to reflect the real condition being evaluated in this branch (e.g., that `latestUpdateTime` did not advance beyond `lastUpdateTime` when `suspectTimeDiff >= 0`). -- 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]
