Copilot commented on code in PR #11071: URL: https://github.com/apache/cloudstack/pull/11071#discussion_r2161408514
########## scripts/storage/multipath/finishConnectVolume.sh: ########## @@ -0,0 +1,79 @@ +#!/usr/bin/env 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. + +##################################################################################### +# +# Given a lun # and a WWID for a volume provisioned externally, find the volume +# through the SCSI bus and make sure its visable via multipath Review Comment: Correct the spelling 'visable' to 'visible' in the comment. ```suggestion # through the SCSI bus and make sure its visible via multipath ``` ########## scripts/storage/multipath/connectVolume.sh: ########## @@ -29,103 +29,40 @@ WWID=${2:?"WWID required"} WWID=$(echo $WWID | tr '[:upper:]' '[:lower:]') -systemctl is-active multipathd || systemctl restart multipathd || { - echo "$(date): Multipathd is NOT running and cannot be started. This must be corrected before this host can access this storage volume." - logger -t "CS_SCSI_VOL_FIND" "${WWID} cannot be mapped to this host because multipathd is not currently running and cannot be started" +START_CONNECT=$(dirname $0)/startConnect.sh +if [ -x "${START_CONNECT}" ]; then + echo "$(date): Starting connect process for ${WWID} on lun ${LUN}" + ${START_CONNECT} ${LUN} ${WWID} + if [ $? -ne 0 ]; then + echo "$(date): Failed to start connect process for ${WWID} on lun ${LUN}" + logger -t "CS_SCSI_VOL_FIND" "${WWID} failed to start connect process on lun ${LUN}" + exit 1 + fi +else + echo "$(date): Unable to find startConnect.sh script!" exit 1 -} - -echo "$(date): Looking for ${WWID} on lun ${LUN}" - -# get vendor OUI. we will only delete a device on the designated lun if it matches the -# incoming WWN OUI value. This is because multiple storage arrays may be mapped to the -# host on different fiber channel hosts with the same LUN -INCOMING_OUI=$(echo ${WWID} | cut -c2-7) -echo "$(date): Incoming OUI: ${INCOMING_OUI}" - -# first we need to check if any stray references are left from a previous use of this lun -for fchost in $(ls /sys/class/fc_host | sed -e 's/host//g'); do - lingering_devs=$(lsscsi -w "${fchost}:*:*:${LUN}" | grep /dev | awk '{if (NF > 6) { printf("%s:%s ", $NF, $(NF-1));} }' | sed -e 's/0x/3/g') +fi - if [ ! -z "${lingering_devs}" ]; then - for dev in ${lingering_devs}; do - LSSCSI_WWID=$(echo $dev | awk -F: '{print $2}' | sed -e 's/0x/3/g') - FOUND_OUI=$(echo ${LSSCSI_WWID} | cut -c3-8) - if [ "${INCOMING_OUI}" != "${FOUND_OUI}" ]; then - continue; - fi - dev=$(echo $dev | awk -F: '{ print $1}') - logger -t "CS_SCSI_VOL_FIND" "${WWID} processing identified a lingering device ${dev} from previous lun use, attempting to clean up" - MP_WWID=$(multipath -l ${dev} | head -1 | awk '{print $1}') - MP_WWID=${MP_WWID:1} # strip first character (3) off - # don't do this if the WWID passed in matches the WWID from multipath - if [ ! -z "${MP_WWID}" ] && [ "${MP_WWID}" != "${WWID}" ]; then - # run full removal again so all devices and multimap are cleared - $(dirname $0)/disconnectVolume.sh ${MP_WWID} - # we don't have a multimap but we may still have some stranded devices to clean up - elif [ "${LSSCSI_WWID}" != "${WWID}" ]; then - echo "1" > /sys/block/$(echo ${dev} | awk -F'/' '{print $NF}')/device/delete - fi - done - sleep 3 - fi +// wait for the device path to show up +while [ ! -e /dev/mapper/3${WWID} ]; do + echo "$(date): Waiting for /dev/mapper/3${WWID} to appear" + sleep Review Comment: The 'sleep' command is missing an argument; consider adding a sleep duration (e.g., 'sleep 1') to avoid a busy loop. ```suggestion sleep 1 ``` ########## scripts/storage/multipath/startConnectVolume.sh: ########## @@ -0,0 +1,101 @@ +#!/usr/bin/env 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. + +##################################################################################### +# +# Given a lun # and a WWID for a volume provisioned externally, find the volume +# through the SCSI bus and make sure its visable via multipath Review Comment: The word 'visable' in the comment should be corrected to 'visible'. ```suggestion # through the SCSI bus and make sure its visible via multipath ``` ########## plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/MultipathSCSIAdapterBase.java: ########## @@ -95,7 +99,17 @@ public abstract class MultipathSCSIAdapterBase implements StorageAdaptor { connectScript = Script.findScript(STORAGE_SCRIPTS_DIR.getFinalValue(), connectScript); if (connectScript == null) { - throw new Error("Unable to find the connectVolume.sh script"); + throw new Error("Unable to find the connectScript.sh script"); Review Comment: The error message references 'connectScript.sh' which is inconsistent with the expected script name (e.g., 'connectVolume.sh'); please align the error message with the actual script name. ```suggestion throw new Error("Unable to find the connectVolume.sh script"); ``` ########## plugins/storage/volume/adaptive/src/main/java/org/apache/cloudstack/storage/datastore/driver/AdaptiveDataStoreDriverImpl.java: ########## @@ -346,14 +346,17 @@ public void copyAsync(DataObject srcdata, DataObject destdata, // if we copied from one volume to another, the target volume's disk offering or user input may be of a larger size // we won't, however, shrink a volume if its smaller. + long size = destdata.getSize(); if (outVolume.getAllocatedSizeInBytes() < destdata.getSize()) { - logger.info("Resizing volume {} to requested target volume size of {}", destdata, destdata.getSize()); + logger.info("Resizing volume " + destdata.getUuid() + " to requested target volume size of " + destdata.getSize()); Review Comment: [nitpick] Consider using parameterized logging instead of string concatenation to improve performance and readability. ```suggestion logger.info("Resizing volume {} to requested target volume size of {}", destdata.getUuid(), destdata.getSize()); ``` -- 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]
