Copilot commented on code in PR #11071:
URL: https://github.com/apache/cloudstack/pull/11071#discussion_r2526782545
##########
server/src/main/java/com/cloud/network/as/AutoScaleManagerImpl.java:
##########
@@ -1952,6 +1953,19 @@ public void updateVmDetails(Map<String, String>
deployParams, Map<String, String
@Override
public String getNextVmHostName(AutoScaleVmGroupVO asGroup) {
+ if (UseAutoscaleVmHostnamePrefixEnabled.value() == true) {
Review Comment:
Redundant comparison with boolean literal. The expression
`UseAutoscaleVmHostnamePrefixEnabled.value() == true` can be simplified to
`UseAutoscaleVmHostnamePrefixEnabled.value()`.
```suggestion
if (UseAutoscaleVmHostnamePrefixEnabled.value()) {
```
##########
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
Review Comment:
Invalid comment syntax. The comment uses `//` (double slash) syntax on line
46 of a bash script, which should use `#` instead. This will cause a syntax
error.
```suggestion
# wait for the device path to show up
```
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/MultipathSCSIAdapterBase.java:
##########
@@ -164,9 +178,10 @@ private KVMPhysicalDisk getPhysicalDisk(AddressInfo
address, KVMStoragePool pool
// validate we have a connection, if not we need to connect first.
if (!isConnected(address.getPath())) {
- if (!connectPhysicalDisk(address, pool, null)) {
- throw new CloudRuntimeException("Unable to connect to volume "
+ address.getPath());
- }
+ LOGGER.debug("Physical disk " + address.getPath() + " is not
connected, a request to connectPhysicalDisk must be made before it can be
used.");
+ } else {
+ LOGGER.debug("Physical disk " + address.getPath() + " is
connected, proceeding to get its size.");
+
Review Comment:
Unreachable code after break statement. Line 184 has code after a closing
brace that would have been unreachable in the original structure. The logic
flow appears broken - the `else` block contains only a debug statement with no
actual connection logic.
```suggestion
LOGGER.debug("Physical disk " + address.getPath() + " is not
connected, attempting to connectPhysicalDisk.");
connectPhysicalDisk(address.getPath());
} else {
LOGGER.debug("Physical disk " + address.getPath() + " is
connected, proceeding to get its size.");
```
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/MultipathSCSIAdapterBase.java:
##########
@@ -98,6 +102,16 @@ public abstract class MultipathSCSIAdapterBase implements
StorageAdaptor {
throw new Error("Unable to find the connectVolume.sh script");
}
+ startConnectScript =
Script.findScript(STORAGE_SCRIPTS_DIR.getFinalValue(), startConnectScript);
+ if (startConnectScript == null) {
+ throw new Error("Unable to find the startConnectScript.sh script");
+ }
+
+ finishConnectScript =
Script.findScript(STORAGE_SCRIPTS_DIR.getFinalValue(), finishConnectScript);
+ if (finishConnectScript == null) {
+ throw new Error("Unable to find the finishConnectScript.sh
script");
Review Comment:
Error message references wrong script name. The error message on line 112
says "Unable to find the finishConnectScript.sh script" but should reference
"finishConnectVolume.sh" based on the script filename being checked.
```suggestion
throw new Error("Unable to find the finishConnectVolume.sh
script");
```
##########
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 visible via multipath
+#
+#####################################################################################
+
+
+LUN=${1:?"LUN required"}
+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_CONN_START" "${WWID} cannot be mapped to this host
because multipathd is not currently running and cannot be started"
+ 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')
+
+ 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_CONN_START" "${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
+done
+
+logger -t "CS_SCSI_VOL_CONN_START" "${WWID} awaiting disk path at
/dev/mapper/3${WWID}"
+
+# wait for multipath to map the new lun to the WWID
+echo "$(date): Triggering discovery for multipath WWID ${WWID} on LUN ${LUN}"
+ls /dev/mapper/3${WWID} >/dev/null 2>&1
+if [ $? == 0 ]; then
+ logger -t "CS_SCSI_VOL_CONN_START" "${WWID} already available at
/dev/mapper/3${WWID}, no need to trigger a scan"
+ break
+fi
+
+# instruct bus to scan for new lun
+for fchost in $(ls /sys/class/fc_host); do
+ echo " --> Scanning ${fchost}"
+ echo "- - ${LUN}" > /sys/class/scsi_host/${fchost}/scan
+done
+
+multipath -v2 2>/dev/null
+
+ls /dev/mapper/3${WWID} >/dev/null 2>&1
+if [ $? == 0 ]; then
+ logger -t "CS_SCSI_VOL_CONN_START" "${WWID} scan triggered and device
immediately became visable at /dev/mapper/3${WWID}"
Review Comment:
Typo in error message: "visable" should be spelled "visible".
```suggestion
logger -t "CS_SCSI_VOL_CONN_START" "${WWID} scan triggered and device
immediately became visible at /dev/mapper/3${WWID}"
```
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/MultipathSCSIAdapterBase.java:
##########
@@ -65,11 +65,13 @@ public abstract class MultipathSCSIAdapterBase implements
StorageAdaptor {
* Property keys and defaults
*/
static final Property<Integer> CLEANUP_FREQUENCY_SECS = new
Property<Integer>("multimap.cleanup.frequency.secs", 60);
- static final Property<Integer> CLEANUP_TIMEOUT_SECS = new
Property<Integer>("multimap.cleanup.timeout.secs", 4);
+ static final Property<Integer> CLEANUP_TIMEOUT_SECS = new
Property<Integer>("multimap.cleanup.timeout.secs", 600);
static final Property<Boolean> CLEANUP_ENABLED = new
Property<Boolean>("multimap.cleanup.enabled", true);
static final Property<String> CLEANUP_SCRIPT = new
Property<String>("multimap.cleanup.script", "cleanStaleMaps.sh");
static final Property<String> CONNECT_SCRIPT = new
Property<String>("multimap.connect.script", "connectVolume.sh");
- static final Property<String> COPY_SCRIPT = new
Property<String>("multimap.copy.script", "copyVolume.sh");
+ static final Property<String> START_CONNECT_SCRIPT = new
Property<String>("multimap.startconnect.script", "startConnectVolume.sh");
+ static final Property<String> FINISH_CONNECT_SCRIPT = new
Property<String>("multimap.finishconnect.script", "finishConnectVolume.sh");
+ static final Property<String> COPY_SCRIPT = new
Property<String>("multimap.copy.script", "copyVolume.sh");
Review Comment:
Inconsistent indentation. Line 74 has an extra leading space before `static
final Property<String>`, which is inconsistent with the surrounding code
formatting.
```suggestion
static final Property<String> COPY_SCRIPT = new
Property<String>("multimap.copy.script", "copyVolume.sh");
```
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/KVMStoragePoolManager.java:
##########
@@ -187,17 +207,79 @@ public boolean
connectPhysicalDisksViaVmSpec(VirtualMachineTO vmSpec, boolean is
KVMStoragePool pool = getStoragePool(store.getPoolType(),
store.getUuid());
StorageAdaptor adaptor = getStorageAdaptor(pool.getType());
- result = adaptor.connectPhysicalDisk(vol.getPath(), pool,
disk.getDetails(), isVMMigrate);
+ if (adaptor instanceof AsyncPhysicalDiskConnectorDecorator) {
+ // If the adaptor supports async disk connection, we can start
the connection
+ // and return immediately, allowing the connection to complete
in the background.
+ result = ((AsyncPhysicalDiskConnectorDecorator)
adaptor).startConnectPhysicalDisk(vol.getPath(), pool, disk.getDetails());
+ if (!result) {
+ logger.error("Failed to start connecting disks via vm spec
for vm: " + vmName + " volume:" + vol.toString());
+ return false;
+ }
- if (!result) {
- logger.error("Failed to connect disks via vm spec for vm: " +
vmName + " volume:" + vol.toString());
- return result;
+ // add disk to list of disks to check later
+ connectingDisks.add(new ConnectingDiskInfo(vol, adaptor, pool,
disk.getDetails()));
+ } else {
+ result = adaptor.connectPhysicalDisk(vol.getPath(), pool,
disk.getDetails(), isVMMigrate);
+
+ if (!result) {
+ logger.error("Failed to connect disks via vm spec for vm:
" + vmName + " volume:" + vol.toString());
+ return result;
+ }
+ }
+ }
+
+ // if we have any connecting disks to check, wait for them to connect
or timeout
+ if (!connectingDisks.isEmpty()) {
+ for (ConnectingDiskInfo connectingDisk : connectingDisks) {
+ StorageAdaptor adaptor = connectingDisk.adapter;
+ KVMStoragePool pool = connectingDisk.pool;
+ VolumeObjectTO volume = connectingDisk.volume;
+ Map<String, String> details = connectingDisk.details;
+ long diskWaitTimeMillis = getDiskWaitTimeMillis(details);
+
+ // wait for the disk to connect
+ long startTime = System.currentTimeMillis();
+ while (System.currentTimeMillis() - startTime <
diskWaitTimeMillis) {
+ if (((AsyncPhysicalDiskConnectorDecorator)
adaptor).isConnected(volume.getPath(), pool, details)) {
+ logger.debug(String.format("Disk %s connected
successfully for VM %s", volume.getPath(), vmName));
+ break;
+ }
+
+ sleep(1000); // wait for 1 second before checking again
+ }
}
}
return result;
}
+ private long getDiskWaitTimeMillis(Map<String,String> details) {
+ int waitTimeInSec = 60; // default wait time in seconds
+ if (details != null &&
details.containsKey(StorageManager.STORAGE_POOL_DISK_WAIT.toString())) {
+ String waitTime =
details.get(StorageManager.STORAGE_POOL_DISK_WAIT.toString());
+ if (StringUtils.isNotEmpty(waitTime)) {
+ waitTimeInSec = Integer.valueOf(waitTime).intValue();
+ logger.debug(String.format("%s set to %s", waitTimeInSec,
StorageManager.STORAGE_POOL_DISK_WAIT.toString()));
Review Comment:
Incorrect format string arguments. The log message format uses
`String.format` with placeholders `%s` twice, but passes three arguments:
`waitTimeInSec`, `waitTimeInSec`, and
`StorageManager.STORAGE_POOL_DISK_WAIT.toString()`. This will cause a
formatting error.
```suggestion
logger.debug(String.format("%s set to %s",
StorageManager.STORAGE_POOL_DISK_WAIT.toString(), waitTimeInSec));
```
##########
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 1
done
-logger -t "CS_SCSI_VOL_FIND" "${WWID} awaiting disk path at
/dev/mapper/3${WWID}"
-
-# wait for multipath to map the new lun to the WWID
-echo "$(date): Waiting for multipath entry to show up for the WWID"
-while true; do
- ls /dev/mapper/3${WWID} >/dev/null 2>&1
- if [ $? == 0 ]; then
- break
- fi
-
- logger -t "CS_SCSI_VOL_FIND" "${WWID} not available yet, triggering scan"
-
- # instruct bus to scan for new lun
- for fchost in $(ls /sys/class/fc_host); do
- echo " --> Scanning ${fchost}"
- echo "- - ${LUN}" > /sys/class/scsi_host/${fchost}/scan
- done
-
- multipath -v2 2>/dev/null
-
- ls /dev/mapper/3${WWID} >/dev/null 2>&1
- if [ $? == 0 ]; then
- break
+FINISH_CONNECT=$(dirname $0)/finishConnect.sh
Review Comment:
Incorrect script filename reference. The script references `startConnect.sh`
and `finishConnect.sh`, but the actual script names appear to be
`startConnectVolume.sh` and `finishConnectVolume.sh` based on the new files
added in this PR.
##########
plugins/user-authenticators/ldap/pom.xml:
##########
@@ -114,11 +115,10 @@
<version>${groovy.version}</version>
<scope>test</scope>
</dependency>
- <!-- Optional dependencies for using Spock -->
- <dependency> <!-- enables mocking of classes (in addition to
interfaces) -->
+ <dependency>
<groupId>cglib</groupId>
<artifactId>cglib-nodep</artifactId>
- <scope>test</scope>
+ <version>${cs.cglib.version}</version>
Review Comment:
Removed scope declaration without adding version. The `<scope>test</scope>`
was removed from the cglib-nodep dependency, but a version was added. However,
removing the scope means this dependency will now be included in the
compile/runtime scope instead of just test scope, which could increase the
artifact size unnecessarily if this dependency is only needed for tests.
```suggestion
<version>${cs.cglib.version}</version>
<scope>test</scope>
```
##########
plugins/storage/volume/adaptive/src/main/java/org/apache/cloudstack/storage/datastore/driver/AdaptiveDataStoreDriverImpl.java:
##########
@@ -840,55 +840,95 @@ public boolean canHostAccessStoragePool(Host host,
StoragePool pool) {
}
void persistVolumeOrTemplateData(StoragePoolVO storagePool, Map<String,
String> storagePoolDetails,
- DataObject dataObject, ProviderVolume volume, Map<String,String>
connIdMap) {
+ DataObject dataObject, ProviderVolume volume, Map<String,String>
connIdMap, Long size) {
if (dataObject.getType() == DataObjectType.VOLUME) {
- persistVolumeData(storagePool, storagePoolDetails, dataObject,
volume, connIdMap);
+ persistVolumeData(storagePool, storagePoolDetails, dataObject,
volume, connIdMap, size);
} else if (dataObject.getType() == DataObjectType.TEMPLATE) {
- persistTemplateData(storagePool, storagePoolDetails, dataObject,
volume, connIdMap);
+ persistTemplateData(storagePool, storagePoolDetails, dataObject,
volume, connIdMap, size);
}
}
void persistVolumeData(StoragePoolVO storagePool, Map<String, String>
details, DataObject dataObject,
- ProviderVolume managedVolume, Map<String,String> connIdMap) {
+ ProviderVolume managedVolume, Map<String,String> connIdMap, Long
size) {
+
+ // Get the volume by dataObject id
VolumeVO volumeVO = _volumeDao.findById(dataObject.getId());
+ long volumeId = volumeVO.getId();
+ // Generate path for volume and details
String finalPath = generatePathInfo(managedVolume, connIdMap);
- volumeVO.setPath(finalPath);
- volumeVO.setFormat(ImageFormat.RAW);
- volumeVO.setPoolId(storagePool.getId());
- volumeVO.setExternalUuid(managedVolume.getExternalUuid());
- volumeVO.setDisplay(true);
- volumeVO.setDisplayVolume(true);
- _volumeDao.update(volumeVO.getId(), volumeVO);
- volumeVO = _volumeDao.findById(volumeVO.getId());
+ try {
+ if (finalPath != null) {
+ volumeVO.setPath(finalPath);
+ }
+ volumeVO.setFormat(ImageFormat.RAW);
+ volumeVO.setPoolId(storagePool.getId());
+ volumeVO.setExternalUuid(managedVolume.getExternalUuid());
+ volumeVO.setDisplay(true);
+ volumeVO.setDisplayVolume(true);
+ // the size may have been adjusted by the storage provider
+ if (size != null) {
+ volumeVO.setSize(size);
+ }
+ _volumeDao.update(volumeVO.getId(), volumeVO);
+ } catch (Throwable e) {
+ logger.error("Failed to persist volume path", e);
+ throw e;
+ }
- VolumeDetailVO volumeDetailVO = new VolumeDetailVO(volumeVO.getId(),
- DiskTO.PATH, finalPath, true);
- _volumeDetailsDao.persist(volumeDetailVO);
+ // PATH
+ try {
+ // If volume_detail exist
+ _volumeDetailsDao.removeDetail(volumeId, DiskTO.PATH);
+ VolumeDetailVO volumeDetailVO = new VolumeDetailVO(volumeId,
DiskTO.PATH, finalPath, true);
+ _volumeDetailsDao.persist(volumeDetailVO);
+ } catch (Exception e) {
+ logger.error("Failed to persist volume path", e);
+ throw e;
+ }
- volumeDetailVO = new VolumeDetailVO(volumeVO.getId(),
- ProviderAdapterConstants.EXTERNAL_NAME,
managedVolume.getExternalName(), true);
- _volumeDetailsDao.persist(volumeDetailVO);
+ // EXTERNAL_NAME
+ try {
+ _volumeDetailsDao.removeDetail(volumeId,
ProviderAdapterConstants.EXTERNAL_NAME);
+ VolumeDetailVO volumeDetailVO = new VolumeDetailVO(volumeId,
ProviderAdapterConstants.EXTERNAL_NAME, managedVolume.getExternalName(), true);
+ _volumeDetailsDao.persist(volumeDetailVO);
+ } catch (Exception e) {
+ logger.error("Failed to persist volume external name", e);
+ throw e;
+ }
- volumeDetailVO = new VolumeDetailVO(volumeVO.getId(),
- ProviderAdapterConstants.EXTERNAL_UUID,
managedVolume.getExternalUuid(), true);
- _volumeDetailsDao.persist(volumeDetailVO);
+ // EXTERNAL_UUID
+ try {
+ _volumeDetailsDao.removeDetail(volumeId,
ProviderAdapterConstants.EXTERNAL_UUID);
+ VolumeDetailVO volumeDetailVO = new VolumeDetailVO(volumeId,
ProviderAdapterConstants.EXTERNAL_UUID, managedVolume.getExternalUuid(), true);
+ _volumeDetailsDao.persist(volumeDetailVO);
+ } catch (Exception e) {
+ logger.error("Failed to persist volume external uuid", e);
+ throw e;
+ }
}
void persistTemplateData(StoragePoolVO storagePool, Map<String, String>
details, DataObject dataObject,
- ProviderVolume volume, Map<String,String> connIdMap) {
+ ProviderVolume volume, Map<String,String> connIdMap, Long size) {
TemplateInfo templateInfo = (TemplateInfo) dataObject;
VMTemplateStoragePoolVO templatePoolRef =
_vmTemplatePoolDao.findByPoolTemplate(storagePool.getId(),
templateInfo.getId(), null);
templatePoolRef.setInstallPath(generatePathInfo(volume, connIdMap));
templatePoolRef.setLocalDownloadPath(volume.getExternalName());
- templatePoolRef.setTemplateSize(volume.getAllocatedSizeInBytes());
- _vmTemplatePoolDao.update(templatePoolRef.getId(), templatePoolRef);
+ if (size == null) {
+ templatePoolRef.setTemplateSize(volume.getAllocatedSizeInBytes());
+ } else {
+ templatePoolRef.setTemplateSize(size);
+ } _vmTemplatePoolDao.update(templatePoolRef.getId(),
templatePoolRef);
Review Comment:
Missing line break between statement and closing brace. Line 924 has both
the closing brace and the update statement on the same line, making the code
harder to read.
```suggestion
}
_vmTemplatePoolDao.update(templatePoolRef.getId(), templatePoolRef);
```
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/MultipathSCSIAdapterBase.java:
##########
@@ -98,6 +102,16 @@ public abstract class MultipathSCSIAdapterBase implements
StorageAdaptor {
throw new Error("Unable to find the connectVolume.sh script");
}
+ startConnectScript =
Script.findScript(STORAGE_SCRIPTS_DIR.getFinalValue(), startConnectScript);
+ if (startConnectScript == null) {
+ throw new Error("Unable to find the startConnectScript.sh script");
Review Comment:
Error message references wrong script name. The error message on line 107
says "Unable to find the startConnectScript.sh script" but should reference
"startConnectVolume.sh" based on the script filename being checked.
```suggestion
throw new Error("Unable to find the startConnectVolume.sh
script");
```
##########
engine/schema/pom.xml:
##########
@@ -128,6 +130,7 @@
<artifactId>download-maven-plugin</artifactId>
<version>1.6.3</version>
<executions>
+ <?m2e execute onConfiguration,onIncremental?>
<execution>
Review Comment:
Incomplete/incorrect XML comment syntax. Line 133 contains `<?m2e execute
onConfiguration,onIncremental?>` which appears to be a processing instruction,
but it's placed outside of an `<execution>` element, which will likely cause
XML parsing issues or be ignored. It should be inside the execution element
that follows on line 134.
```suggestion
<execution>
<?m2e execute onConfiguration,onIncremental?>
```
--
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]
