Damans227 commented on code in PR #13656: URL: https://github.com/apache/cloudstack/pull/13656#discussion_r3624822274
########## plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java: ########## @@ -19,10 +19,20 @@ import static com.cloud.host.Host.HOST_CDROM_MAX_COUNT; import static com.cloud.host.Host.HOST_INSTANCE_CONVERSION; import static com.cloud.host.Host.HOST_OVFTOOL_VERSION; +import static com.cloud.host.Host.HOST_QEMU_RBD_SUPPORT; +import static com.cloud.host.Host.HOST_QEMU_IMG_VERSION; +import static com.cloud.host.Host.HOST_QEMU_IO_VERSION; +import static com.cloud.host.Host.HOST_QEMU_NBD_VERSION; import static com.cloud.host.Host.HOST_VDDK_LIB_DIR; +import static com.cloud.host.Host.HOST_VDDK_RBD_DIRECT_IMPORT_SUPPORT; import static com.cloud.host.Host.HOST_VDDK_SUPPORT; import static com.cloud.host.Host.HOST_VDDK_VERSION; +import static com.cloud.host.Host.HOST_VIRTV2V_INPLACE_SUPPORT; +import static com.cloud.host.Host.HOST_VIRTV2V_INPLACE_VERSION; import static com.cloud.host.Host.HOST_VIRTV2V_VERSION; +import static com.cloud.host.Host.HOST_VDDK_BLOCKCOPY_INPLACE_FINALIZATION_SUPPORT; Review Comment: Nit: these new static imports (`HOST_VDDK_BLOCKCOPY_*`) break the existing alphabetical import order (they land after `HOST_VDDK_VERSION` instead of before `HOST_VDDK_LIB_DIR`). ########## plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/VmwareCbtSyncPlan.java: ########## @@ -0,0 +1,273 @@ +// 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.hypervisor.kvm.resource.wrapper; + +import java.util.ArrayList; +import java.util.Collections; +import java.util.Comparator; +import java.util.LinkedHashMap; +import java.util.List; +import java.util.Map; + +import org.apache.commons.collections.CollectionUtils; Review Comment: Nit: this imports the legacy `org.apache.commons.collections.CollectionUtils` (also in `LibvirtVmwareCbtCleanupCommandWrapper.java`, `LibvirtVmwareCbtCutoverCommandWrapper.java`, `LibvirtVmwareCbtPrepareCommandWrapper.java`), while other new files in this PR use `commons.collections4`. Worth picking one. ########## ui/public/locales/en.json: ########## @@ -3529,8 +3557,8 @@ "message.desc.created.ssh.key.pair": "Created a SSH key pair.", "message.desc.host": "Each Cluster must contain at least one host (computer) for guest Instances to run on. We will add the first host now. For a host to function in CloudStack, you must install hypervisor software on the host, assign an IP address to the host, and ensure the host is connected to the CloudStack management server.<br/><br/>Give the host's DNS or IP address, the user name (usually root) and password, and any labels you use to categorize hosts.", "message.desc.import.ext.kvm.wizard": "Import libvirt domain from External KVM Host not managed by CloudStack", -"message.desc.import.local.kvm.wizard": "Import QCOW2 image from Local Storage of selected KVM Host", -"message.desc.import.shared.kvm.wizard": "Import QCOW2 image from selected Primary Storage Pool", +"message.desc.import.local.kvm.wizard": "Import an Instance using an existing ROOT disk from the Local Storage of the selected KVM host", Review Comment: Nit: this changes "selected KVM Host" to "selected KVM host" (lowercase) and the sibling string below drops "Storage Pool" to "Storage pool", a casing regression from the previous strings. ########## plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtVmwareCbtRbdProbeCommandWrapper.java: ########## @@ -0,0 +1,218 @@ +// 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.hypervisor.kvm.resource.wrapper; + +import java.util.regex.Pattern; + +import org.apache.commons.lang3.StringUtils; + +import com.cloud.agent.api.Answer; +import com.cloud.agent.api.VmwareCbtRbdProbeCommand; +import com.cloud.agent.api.to.VmwareCbtTargetStorageType; +import com.cloud.hypervisor.kvm.resource.LibvirtComputingResource; +import com.cloud.hypervisor.kvm.storage.KVMPhysicalDisk; +import com.cloud.hypervisor.kvm.storage.KVMStoragePool; +import com.cloud.hypervisor.kvm.storage.KVMStoragePoolManager; +import com.cloud.resource.CommandWrapper; +import com.cloud.resource.ResourceWrapper; +import com.cloud.storage.Storage; +import com.cloud.utils.script.Script; + +@ResourceWrapper(handles = VmwareCbtRbdProbeCommand.class) +public class LibvirtVmwareCbtRbdProbeCommandWrapper extends CommandWrapper<VmwareCbtRbdProbeCommand, Answer, LibvirtComputingResource> { + + static final String PROBE_IMAGE_PREFIX = "cloudstack-cbt-probe-"; + static final String BLOCK_DEVICE_PROBE_PREFIX = "cbt-probe-"; + private static final Pattern PROBE_IMAGE_NAME_PATTERN = Pattern.compile("^" + PROBE_IMAGE_PREFIX + + "[0-9a-fA-F]{8}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{12}$"); + private static final Pattern BLOCK_DEVICE_PROBE_NAME_PATTERN = Pattern.compile("^" + BLOCK_DEVICE_PROBE_PREFIX + + "[0-9a-fA-F]{8}$"); + private static final long PROBE_SIZE_BYTES = 4L * 1024L * 1024L; + + @Override + public Answer execute(VmwareCbtRbdProbeCommand cmd, LibvirtComputingResource serverResource) { + String probeImageName = StringUtils.trimToNull(cmd.getProbeImageName()); + if (cmd.getTargetStorageType() == VmwareCbtTargetStorageType.RAW_BLOCK_DEVICE) { + return executeBlockDeviceProbe(cmd, probeImageName, serverResource); + } + try { + validateCommand(cmd, probeImageName); + KVMStoragePool targetPool = getTargetStoragePool(cmd, serverResource.getStoragePoolMgr()); + String qemuRbdPath = KVMPhysicalDisk.RBDStringBuilder(targetPool, getRbdImagePath(targetPool, probeImageName)); + + Exception probeFailure = null; + try { + executeProbeCommand(buildCreateCommand(qemuRbdPath), getTimeout(cmd), "create", targetPool, probeImageName); + executeProbeCommand(buildWriteReadCommand(qemuRbdPath), getTimeout(cmd), "write/read", targetPool, probeImageName); + } catch (Exception e) { + probeFailure = e; + throw e; + } finally { + try { + cleanupProbeImage(targetPool, probeImageName); + } catch (RuntimeException e) { + if (probeFailure == null) { + throw e; + } + logger.warn("Unable to clean up RBD probe image {} after a failed probe: {}", probeImageName, + StringUtils.defaultIfBlank(e.getMessage(), e.getClass().getSimpleName())); + } + } + + String message = String.format("Selected KVM host can create, write, read and delete temporary RBD image %s in storage pool %s.", + probeImageName, cmd.getDestinationStoragePoolUuid()); + logger.info(message); + return new Answer(cmd, true, message); + } catch (Exception e) { + String message = String.format("Cannot verify VMware CBT access to selected RBD primary storage pool %s on host %s: %s", + cmd.getDestinationStoragePoolUuid(), serverResource.getPrivateIp(), + StringUtils.defaultIfBlank(e.getMessage(), e.getClass().getSimpleName())); + logger.warn(message, e); + return new Answer(cmd, false, message); + } + } + + private Answer executeBlockDeviceProbe(VmwareCbtRbdProbeCommand cmd, String probeVolumeName, + LibvirtComputingResource serverResource) { + try { + validateBlockDeviceCommand(cmd, probeVolumeName); + KVMStoragePool targetPool = getBlockDeviceTargetStoragePool(cmd, serverResource.getStoragePoolMgr()); + + Exception probeFailure = null; + try { + KVMPhysicalDisk probeDisk = targetPool.createPhysicalDisk(probeVolumeName, + org.apache.cloudstack.utils.qemu.QemuImg.PhysicalDiskFormat.RAW, + Storage.ProvisioningType.THIN, PROBE_SIZE_BYTES, null); + String devicePath = probeDisk == null ? null : probeDisk.getPath(); + if (StringUtils.isBlank(devicePath)) { + throw new IllegalStateException(String.format("could not resolve a local device path for probe volume %s", probeVolumeName)); + } + executeProbeCommand(buildWriteReadCommand(devicePath), getTimeout(cmd), "write/read", targetPool, probeVolumeName); + } catch (Exception e) { + probeFailure = e; + throw e; + } finally { + try { + cleanupProbeImage(targetPool, probeVolumeName); + } catch (RuntimeException e) { + if (probeFailure == null) { + throw e; + } + logger.warn("Unable to clean up block device probe volume {} after a failed probe: {}", probeVolumeName, + StringUtils.defaultIfBlank(e.getMessage(), e.getClass().getSimpleName())); + } + } + + String message = String.format("Selected KVM host can create, write, read and delete temporary block device volume %s in storage pool %s.", + probeVolumeName, cmd.getDestinationStoragePoolUuid()); + logger.info(message); + return new Answer(cmd, true, message); + } catch (Exception e) { + String message = String.format("Cannot verify VMware CBT access to selected block device primary storage pool %s on host %s: %s", + cmd.getDestinationStoragePoolUuid(), serverResource.getPrivateIp(), + StringUtils.defaultIfBlank(e.getMessage(), e.getClass().getSimpleName())); + logger.warn(message, e); + return new Answer(cmd, false, message); + } + } + + private void validateBlockDeviceCommand(VmwareCbtRbdProbeCommand cmd, String probeVolumeName) { + if (cmd.getDestinationStoragePoolType() != Storage.StoragePoolType.Linstor) { + throw new IllegalArgumentException("VMware CBT block device probe requires a Linstor destination storage pool"); + } + if (StringUtils.isBlank(cmd.getDestinationStoragePoolUuid())) { + throw new IllegalArgumentException("destination storage pool UUID is required"); Review Comment: Nit: exception message capitalization is inconsistent within this file, e.g. line 135 starts uppercase ("VMware CBT block device probe requires...") while this one starts lowercase. -- 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]
