Daniel Erez has uploaded a new change for review. Change subject: core: ensure unique unit for VirtIO_SCSI and SPAR_VSCSI disks ......................................................................
core: ensure unique unit for VirtIO_SCSI and SPAR_VSCSI disks Ensuring a unique address unit value for disks attached with VirtIO_SCSI or SPAR_VSCSI interfaces. The selected units may collide as the search process is depended on the VmDevice addresses which are updated asynchronously by the VmsMonitoring. E.g. when hot-plugging multiple disks using a script, identical unit values might be selected for different disks. Hence: * Moved address creation logic to BLL layer (AbstractDiskVmCommand). * Updating VmDevice address in DB right after selecting an available unit value. * Taking an EngineLock on 'getDiskAddressMap' method to avoid races. Change-Id: I5e621618e8891fca52b48bff437cc4c2a1f695db Bug-Url: https://bugzilla.redhat.com/1206374 Signed-off-by: Daniel Erez <[email protected]> --- M backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AbstractDiskVmCommand.java M backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/locks/LockingGroup.java M backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/vdscommands/HotPlugDiskVDSParameters.java M backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/vdsbroker/HotPlugDiskVDSCommand.java 4 files changed, 111 insertions(+), 63 deletions(-) git pull ssh://gerrit.ovirt.org:29418/ovirt-engine refs/changes/19/42019/1 diff --git a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AbstractDiskVmCommand.java b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AbstractDiskVmCommand.java index ce30b87..7fcb692 100644 --- a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AbstractDiskVmCommand.java +++ b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AbstractDiskVmCommand.java @@ -1,9 +1,11 @@ package org.ovirt.engine.core.bll; import java.util.ArrayList; +import java.util.Collections; import java.util.List; import java.util.Map; +import org.apache.commons.lang.StringUtils; import org.ovirt.engine.core.bll.context.CommandContext; import org.ovirt.engine.core.bll.snapshots.SnapshotsValidator; import org.ovirt.engine.core.bll.storage.IStorageHelper; @@ -17,6 +19,7 @@ import org.ovirt.engine.core.common.action.VmDiskOperationParameterBase; import org.ovirt.engine.core.common.businessentities.Disk; import org.ovirt.engine.core.common.businessentities.Disk.DiskStorageType; +import org.ovirt.engine.core.common.businessentities.DiskInterface; import org.ovirt.engine.core.common.businessentities.LUNs; import org.ovirt.engine.core.common.businessentities.LunDisk; import org.ovirt.engine.core.common.businessentities.StorageServerConnections; @@ -31,6 +34,7 @@ import org.ovirt.engine.core.common.errors.VdcBLLException; import org.ovirt.engine.core.common.errors.VdcBllErrors; import org.ovirt.engine.core.common.errors.VdcBllMessages; +import org.ovirt.engine.core.common.locks.LockingGroup; import org.ovirt.engine.core.common.vdscommands.HotPlugDiskVDSParameters; import org.ovirt.engine.core.common.vdscommands.VDSCommandType; import org.ovirt.engine.core.compat.Guid; @@ -39,6 +43,13 @@ import org.ovirt.engine.core.dao.ImageStorageDomainMapDao; import org.ovirt.engine.core.dao.SnapshotDao; import org.ovirt.engine.core.dao.StoragePoolIsoMapDAO; +import org.ovirt.engine.core.utils.archstrategy.ArchStrategyFactory; +import org.ovirt.engine.core.utils.lock.EngineLock; +import org.ovirt.engine.core.utils.lock.LockManagerFactory; +import org.ovirt.engine.core.vdsbroker.architecture.GetControllerIndices; +import org.ovirt.engine.core.vdsbroker.vdsbroker.VdsProperties; +import org.ovirt.engine.core.vdsbroker.vdsbroker.VmInfoBuilder; +import org.ovirt.engine.core.vdsbroker.xmlrpc.XmlRpcStringUtils; public abstract class AbstractDiskVmCommand<T extends VmDiskOperationParameterBase> extends VmCommand<T> { @@ -73,8 +84,9 @@ } } } + Map<String, String> diskAddressMap = getDiskAddressMap(vmDevice, disk.getDiskInterface()); runVdsCommand(commandType, new HotPlugDiskVDSParameters(getVm().getRunOnVds(), - getVm(), disk, vmDevice)); + getVm(), disk, vmDevice, diskAddressMap)); } private IStorageHelper getStorageHelper(StorageType storageType) { @@ -307,4 +319,85 @@ getVm() != null && validate(new VmValidator(getVm()).vmNotLocked()); } + + /** + * Returns disk's address map by specified VmDevice and DiskInterface + * (note: for VirtIO_SCSI/SPAPR_VSCSI interfaces, the method updates the VM device's address accordingly). + * @param vmDevice + * @param diskInterface + * @return disk's address map + */ + public Map<String, String> getDiskAddressMap(VmDevice vmDevice, DiskInterface diskInterface) { + String address = vmDevice.getAddress(); + if (diskInterface != DiskInterface.VirtIO_SCSI && diskInterface != DiskInterface.SPAPR_VSCSI) { + if (StringUtils.isNotBlank(address)) { + return XmlRpcStringUtils.string2Map(address); + } + } else { + EngineLock vmDiskHotPlugEngineLock = null; + try { + vmDiskHotPlugEngineLock = lockVmDiskHotPlugWithWait(); + VM vm = getVmDAO().get(getParameters().getVmId()); + Map<DiskInterface, Integer> controllerIndexMap = + ArchStrategyFactory.getStrategy(vm.getClusterArch()).run(new GetControllerIndices()).returnValue(); + + int virtioScsiIndex = controllerIndexMap.get(DiskInterface.VirtIO_SCSI); + int sPaprVscsiIndex = controllerIndexMap.get(DiskInterface.SPAPR_VSCSI); + + if (diskInterface == DiskInterface.VirtIO_SCSI) { + Map<VmDevice, Integer> vmDeviceUnitMap = VmInfoBuilder.getVmDeviceUnitMapForVirtioScsiDisks(getVm()); + return getAddressMapForScsiDisk(address, vmDeviceUnitMap, vmDevice, virtioScsiIndex, false); + } else if (diskInterface == DiskInterface.SPAPR_VSCSI) { + Map<VmDevice, Integer> vmDeviceUnitMap = VmInfoBuilder.getVmDeviceUnitMapForSpaprScsiDisks(getVm()); + return getAddressMapForScsiDisk(address, vmDeviceUnitMap, vmDevice, sPaprVscsiIndex, true); + } + } finally { + LockManagerFactory.getLockManager().releaseLock(vmDiskHotPlugEngineLock); + } + } + return null; + } + + private Map<String, String> getAddressMapForScsiDisk(String address, + Map<VmDevice, Integer> vmDeviceUnitMap, + VmDevice vmDevice, + int controllerIndex, + boolean reserveFirstAddress) { + Map<String, String> addressMap; + int availableUnit = VmInfoBuilder.getAvailableUnitForScsiDisk(vmDeviceUnitMap, reserveFirstAddress); + + // If address has been already set before, verify its uniqueness; + // Otherwise, set address according to the next available unit. + if (StringUtils.isNotBlank(address)) { + addressMap = XmlRpcStringUtils.string2Map(address); + int unit = Integer.valueOf(addressMap.get(VdsProperties.Unit)); + if (vmDeviceUnitMap.containsValue(unit)) { + addressMap = VmInfoBuilder.createAddressForScsiDisk(controllerIndex, availableUnit); + } + } else { + addressMap = VmInfoBuilder.createAddressForScsiDisk(controllerIndex, availableUnit); + } + + // Updating device's address immediately (instead of waiting to VmsMonitoring) + // to prevent a duplicate unit value (i.e. ensuring a unique unit value). + updateVmDeviceAddress(addressMap.toString(), vmDevice); + + return addressMap; + } + + protected void updateVmDeviceAddress(final String address, final VmDevice vmDevice) { + vmDevice.setAddress(address); + getCompensationContext().snapshotEntity(vmDevice); + getCompensationContext().stateChanged(); + getVmDeviceDao().update(vmDevice); + } + + protected EngineLock lockVmDiskHotPlugWithWait() { + EngineLock vmDiskHotPlugEngineLock = new EngineLock(); + vmDiskHotPlugEngineLock.setExclusiveLocks(Collections.singletonMap(getVmId().toString(), + LockMessagesMatchUtil.makeLockingPair(LockingGroup.VM_DISK_HOT_PLUG, + VdcBllMessages.ACTION_TYPE_FAILED_OBJECT_LOCKED))); + getLockManager().acquireLockWait(vmDiskHotPlugEngineLock); + return vmDiskHotPlugEngineLock; + } } diff --git a/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/locks/LockingGroup.java b/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/locks/LockingGroup.java index 83801a6..b251204 100644 --- a/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/locks/LockingGroup.java +++ b/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/locks/LockingGroup.java @@ -26,6 +26,6 @@ SYNC_LUNS, /** This group is used for indication that an operation is executed using the specified host */ VDS_EXECUTION, - VDS_POOL_AND_STORAGE_CONNECTIONS; - + VDS_POOL_AND_STORAGE_CONNECTIONS, + VM_DISK_HOT_PLUG } diff --git a/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/vdscommands/HotPlugDiskVDSParameters.java b/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/vdscommands/HotPlugDiskVDSParameters.java index fdb62fc..d618485 100644 --- a/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/vdscommands/HotPlugDiskVDSParameters.java +++ b/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/vdscommands/HotPlugDiskVDSParameters.java @@ -5,20 +5,24 @@ import org.ovirt.engine.core.common.businessentities.VmDevice; import org.ovirt.engine.core.compat.Guid; +import java.util.Map; + public class HotPlugDiskVDSParameters extends VdsAndVmIDVDSParametersBase { private Disk disk; private VmDevice vmDevice; private VM vm; + private Map<String, String> addressMap; public HotPlugDiskVDSParameters() { } - public HotPlugDiskVDSParameters(Guid vdsId, VM vm, Disk disk, VmDevice vmDevice) { + public HotPlugDiskVDSParameters(Guid vdsId, VM vm, Disk disk, VmDevice vmDevice, Map<String, String> addressMap) { super(vdsId, vm.getId()); this.disk = disk; this.vmDevice = vmDevice; this.vm = vm; + this.addressMap = addressMap; } public Disk getDisk() { @@ -45,8 +49,16 @@ this.vm = vm; } + public Map<String, String> getAddressMap() { + return addressMap; + } + + public void setAddressMap(Map<String, String> addressMap) { + this.addressMap = addressMap; + } + @Override public String toString() { - return String.format("%s, diskId = %s", super.toString(), disk.getId()); + return String.format("%s, diskId = %s, addressMap= %s", super.toString(), disk.getId(), addressMap.toString()); } } diff --git a/backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/vdsbroker/HotPlugDiskVDSCommand.java b/backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/vdsbroker/HotPlugDiskVDSCommand.java index e0f5966..72b06cf 100644 --- a/backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/vdsbroker/HotPlugDiskVDSCommand.java +++ b/backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/vdsbroker/HotPlugDiskVDSCommand.java @@ -3,7 +3,6 @@ import java.util.HashMap; import java.util.Map; -import org.apache.commons.lang.StringUtils; import org.ovirt.engine.core.common.FeatureSupported; import org.ovirt.engine.core.common.businessentities.Disk; import org.ovirt.engine.core.common.businessentities.Disk.DiskStorageType; @@ -11,16 +10,11 @@ import org.ovirt.engine.core.common.businessentities.DiskInterface; import org.ovirt.engine.core.common.businessentities.LunDisk; import org.ovirt.engine.core.common.businessentities.PropagateErrors; -import org.ovirt.engine.core.common.businessentities.VM; import org.ovirt.engine.core.common.businessentities.VmDevice; import org.ovirt.engine.core.common.businessentities.VolumeFormat; import org.ovirt.engine.core.common.utils.VmDeviceType; import org.ovirt.engine.core.common.vdscommands.HotPlugDiskVDSParameters; import org.ovirt.engine.core.compat.Guid; -import org.ovirt.engine.core.dal.dbbroker.DbFacade; -import org.ovirt.engine.core.utils.archstrategy.ArchStrategyFactory; -import org.ovirt.engine.core.vdsbroker.architecture.GetControllerIndices; -import org.ovirt.engine.core.vdsbroker.xmlrpc.XmlRpcStringUtils; public class HotPlugDiskVDSCommand<P extends HotPlugDiskVDSParameters> extends VdsBrokerCommand<P> { @@ -48,7 +42,7 @@ VmDevice vmDevice = getParameters().getVmDevice(); drive.put(VdsProperties.Type, VmDeviceType.DISK.getName()); - addAddress(drive, getParameters().getVmDevice().getAddress()); + drive.put(VdsProperties.Address, getParameters().getAddressMap()); drive.put(VdsProperties.INTERFACE, disk.getDiskInterface().getName()); drive.put(VdsProperties.Shareable, (vmDevice.getSnapshotId() != null && FeatureSupported.hotPlugDiskSnapshot(getParameters().getVm() @@ -100,56 +94,5 @@ } return drive; - } - - private void addAddress(Map<String, Object> map, String address) { - if (getParameters().getDisk().getDiskInterface() != DiskInterface.VirtIO_SCSI && - getParameters().getDisk().getDiskInterface() != DiskInterface.SPAPR_VSCSI) { - if (StringUtils.isNotBlank(address)) { - map.put("address", XmlRpcStringUtils.string2Map(address)); - } - } else { - VM vm = DbFacade.getInstance().getVmDao().get(getParameters().getVmId()); - - Map<DiskInterface, Integer> controllerIndexMap = - ArchStrategyFactory.getStrategy(vm.getClusterArch()).run(new GetControllerIndices()).returnValue(); - - int virtioScsiIndex = controllerIndexMap.get(DiskInterface.VirtIO_SCSI); - int sPaprVscsiIndex = controllerIndexMap.get(DiskInterface.SPAPR_VSCSI); - - if (getParameters().getDisk().getDiskInterface() == DiskInterface.VirtIO_SCSI) { - Map<VmDevice, Integer> vmDeviceUnitMap = VmInfoBuilder.getVmDeviceUnitMapForVirtioScsiDisks(getParameters().getVm()); - - addAddressForScsiDisk(map, address, vmDeviceUnitMap, virtioScsiIndex, false); - } else if (getParameters().getDisk().getDiskInterface() == DiskInterface.SPAPR_VSCSI) { - Map<VmDevice, Integer> vmDeviceUnitMap = VmInfoBuilder.getVmDeviceUnitMapForSpaprScsiDisks(getParameters().getVm()); - - addAddressForScsiDisk(map, address, vmDeviceUnitMap, sPaprVscsiIndex, true); - } - } - } - - private void addAddressForScsiDisk(Map<String, Object> map, - String address, - Map<VmDevice, Integer> vmDeviceUnitMap, - int controllerIndex, - boolean reserveFirstAddress) { - int availableUnit = VmInfoBuilder.getAvailableUnitForScsiDisk(vmDeviceUnitMap, reserveFirstAddress); - - // If address has been already set before, verify its uniqueness; - // Otherwise, set address according to the next available unit. - if (StringUtils.isNotBlank(address)) { - Map<String, String> addressMap = XmlRpcStringUtils.string2Map(address); - int unit = Integer.valueOf(addressMap.get(VdsProperties.Unit)); - if (!vmDeviceUnitMap.containsValue(unit)) { - map.put(VdsProperties.Address, addressMap); - } - else { - map.put(VdsProperties.Address, VmInfoBuilder.createAddressForScsiDisk(controllerIndex, availableUnit)); - } - } - else { - map.put(VdsProperties.Address, VmInfoBuilder.createAddressForScsiDisk(controllerIndex, availableUnit)); - } } } -- To view, visit https://gerrit.ovirt.org/42019 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: newchange Gerrit-Change-Id: I5e621618e8891fca52b48bff437cc4c2a1f695db Gerrit-PatchSet: 1 Gerrit-Project: ovirt-engine Gerrit-Branch: ovirt-engine-3.5 Gerrit-Owner: Daniel Erez <[email protected]> _______________________________________________ Engine-patches mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/engine-patches
