Hello Sergey Gotliv,
I'd like you to do a code review. Please visit
http://gerrit.ovirt.org/19521
to review the following change.
Change subject: engine: Split update of 'isPlugged' and 'bootOrder'
properties...
......................................................................
engine: Split update of 'isPlugged' and 'bootOrder' properties...
HotPlugDiskToVmCommand updates 'isPlugged' and 'bootOrder' of all
devices attached to any particular VM. It should only update 'isPlugged'
for the device that was plugged by this command, otherwise it can cause a
race with another thread which handles the hot plug of another disk for
the same VM.
Change-Id: I1359a34a48a6261e22631ff1640d81d735e8c490
Bug-Url: https://bugzilla.redhat.com/1003649
Signed-off-by: Sergey Gotliv <[email protected]>
---
M
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/HotPlugDiskToVmCommand.java
M
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/utils/VmDeviceUtils.java
M
backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/VmDeviceDAO.java
M
backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/VmDeviceDAODbFacadeImpl.java
M
backend/manager/modules/dal/src/test/java/org/ovirt/engine/core/dao/VmDeviceDAOTest.java
M packaging/dbscripts/vm_device_sp.sql
6 files changed, 89 insertions(+), 31 deletions(-)
git pull ssh://gerrit.ovirt.org:29418/ovirt-engine refs/changes/21/19521/1
diff --git
a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/HotPlugDiskToVmCommand.java
b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/HotPlugDiskToVmCommand.java
index 00096dc..678bb19 100644
---
a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/HotPlugDiskToVmCommand.java
+++
b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/HotPlugDiskToVmCommand.java
@@ -1,7 +1,7 @@
package org.ovirt.engine.core.bll;
import java.util.Collections;
-import java.util.List;
+import java.util.HashMap;
import java.util.Map;
import org.ovirt.engine.core.bll.utils.VmDeviceUtils;
@@ -40,13 +40,10 @@
@Override
protected boolean canDoAction() {
- disk = getDiskDao().get(getParameters().getDiskId());
-
- return
- isVmExist() &&
+ return isVmExist() &&
isVmInUpPausedDownStatus() &&
canRunActionOnNonManagedVm() &&
- isDiskExist(disk) &&
+ isDiskExist(getDisk()) &&
checkCanPerformPlugUnPlugDisk() &&
isVmNotInPreviewSnapshot() &&
imageStorageValidation();
@@ -56,10 +53,10 @@
// If the VM is not an image then it does not use the storage domain.
// If the VM is not in UP or PAUSED status, then we know that there is
no running qemu process,
// so we don't need to check the storage domain activity.
- if (disk.getDiskStorageType() != DiskStorageType.IMAGE ||
!getVm().getStatus().isRunningOrPaused()) {
+ if (getDisk().getDiskStorageType() != DiskStorageType.IMAGE ||
!getVm().getStatus().isRunningOrPaused()) {
return true;
}
- DiskImage diskImage = (DiskImage) disk;
+ DiskImage diskImage = (DiskImage) getDisk();
StorageDomain storageDomain = getStorageDomainDAO().getForStoragePool(
diskImage.getStorageIds().get(0),
diskImage.getStoragePoolId());
StorageDomainValidator storageDomainValidator =
getStorageDomainValidator(storageDomain);
@@ -75,9 +72,14 @@
boolean returnValue = true;
if (getVm().getStatus().isUpOrPaused()) {
setVdsId(getVm().getRunOnVds());
- returnValue =
- isHotPlugSupported() && isOsSupportingHotPlug()
- && isInterfaceSupportedForPlugUnPlug(disk);
+ if (!canPerformHotPlug() ||
!isInterfaceSupportedForPlugUnPlug(disk)) {
+ return false;
+ }
+ }
+
+ oldVmDevice = getVmDeviceDao().get(new VmDeviceId(getDisk().getId(),
getVmId()));
+ if (getPlugAction() == VDSCommandType.HotPlugDisk &&
oldVmDevice.getIsPlugged()) {
+ return
failCanDoAction(VdcBllMessages.HOT_PLUG_DISK_IS_NOT_UNPLUGGED);
}
if (returnValue) {
oldVmDevice =
@@ -101,34 +103,59 @@
@Override
protected void executeVmCommand() {
if (getVm().getStatus().isUpOrPaused()) {
- performPlugCommand(getPlugAction(), disk, oldVmDevice);
+ performPlugCommand(getPlugAction(), getDisk(), oldVmDevice);
}
- //Update boot order and isPlugged fields
- final List<VmDevice> devices =
VmDeviceUtils.updateBootOrderInVmDevice(getVm().getStaticData());
- for (VmDevice device:devices) {
- if (device.getDeviceId().equals(oldVmDevice.getDeviceId())) {
- device.setIsPlugged(!oldVmDevice.getIsPlugged());
- break;
- }
+ // At this point disk is already plugged to or unplugged from VM
(depends on the command),
+ // so device's 'isPlugged' property should be updated accordingly in DB
+ updateDeviceIsPluggedProperty();
+
+ // Now after updating 'isPlugged' property of the plugged/unplugged
device, its time to
+ // update the boot order for all VM devices. Failure to do that
doesn't change the fact that
+ // device is already plugged to or unplugged from VM.
+ if (disk.isBoot()) {
+ updateBootOrder();
}
+ getVmStaticDAO().incrementDbGeneration(getVm().getId());
+ setSucceeded(true);
+ }
+
+ private void updateDeviceIsPluggedProperty() {
+ VmDevice device = getVmDeviceDao().get(oldVmDevice.getId());
+ device.setIsPlugged(!oldVmDevice.getIsPlugged());
+ getVmDeviceDao().updateHotPlugDisk(device);
+ }
+
+ private void updateBootOrder() {
TransactionSupport.executeInNewTransaction(new
TransactionMethod<Void>() {
@Override
public Void runInTransaction() {
- getVmStaticDAO().incrementDbGeneration(getVm().getId());
- getVmDeviceDao().updateAll("UpdateVmDeviceForHotPlugDisk",
devices);
- VmHandler.updateDisksFromDb(getVm());
+
VmDeviceUtils.updateBootOrderInVmDeviceAndStoreToDB(getVm().getStaticData());
return null;
}
});
- setSucceeded(true);
}
@Override
protected Map<String, Pair<String, String>> getSharedLocks() {
return Collections.singletonMap(getVmId().toString(),
LockMessagesMatchUtil.makeLockingPair(LockingGroup.VM,
VdcBllMessages.ACTION_TYPE_FAILED_OBJECT_LOCKED));
+ }
+
+ @Override
+ protected Map<String, Pair<String, String>> getExclusiveLocks() {
+ Map<String, Pair<String, String>> exclusiveLock = new HashMap<>();
+ exclusiveLock.put(getDisk().getId().toString(),
+ LockMessagesMatchUtil.makeLockingPair(LockingGroup.DISK,
+ VdcBllMessages.ACTION_TYPE_FAILED_DISKS_LOCKED));
+ if (getDisk().isBoot()) {
+ exclusiveLock.put(getVmId().toString(),
+
LockMessagesMatchUtil.makeLockingPair(LockingGroup.VM_DISK_BOOT,
+ VdcBllMessages.ACTION_TYPE_FAILED_OBJECT_LOCKED));
+
+ }
+ return exclusiveLock;
}
@Override
@@ -141,4 +168,10 @@
return disk.getDiskAlias();
}
+ protected Disk getDisk() {
+ if (disk == null) {
+ disk = getDiskDao().get(getParameters().getDiskId());
+ }
+ return disk;
+ }
}
diff --git
a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/utils/VmDeviceUtils.java
b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/utils/VmDeviceUtils.java
index 0507ee1..0274c65 100644
---
a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/utils/VmDeviceUtils.java
+++
b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/utils/VmDeviceUtils.java
@@ -542,7 +542,7 @@
public static void updateBootOrderInVmDeviceAndStoreToDB(VmBase vmBase) {
List<VmDevice> devices = updateBootOrderInVmDevice(vmBase);
for (VmDevice device : devices) {
- dao.update(device);
+ dao.updateBootOrder(device);
}
}
diff --git
a/backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/VmDeviceDAO.java
b/backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/VmDeviceDAO.java
index 075b8c3..153e6ba 100644
---
a/backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/VmDeviceDAO.java
+++
b/backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/VmDeviceDAO.java
@@ -61,4 +61,5 @@
*/
void updateHotPlugDisk(VmDevice vmDevice);
+ void updateBootOrder(VmDevice vmDevice);
}
diff --git
a/backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/VmDeviceDAODbFacadeImpl.java
b/backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/VmDeviceDAODbFacadeImpl.java
index 5840a7a..75f7c1f 100644
---
a/backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/VmDeviceDAODbFacadeImpl.java
+++
b/backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/VmDeviceDAODbFacadeImpl.java
@@ -187,14 +187,18 @@
return parameterSource;
}
-
@Override
public void updateHotPlugDisk(VmDevice vmDevice) {
MapSqlParameterSource paramsForUpdate =
createParameterSourceForUpdate(vmDevice)
- .addValue("boot_order",vmDevice.getBootOrder())
.addValue("is_plugged",vmDevice.getIsPlugged());
getCallsHandler().executeModification("UpdateVmDeviceForHotPlugDisk" ,
paramsForUpdate);
+ }
+ @Override
+ public void updateBootOrder(VmDevice vmDevice) {
+ MapSqlParameterSource paramsForUpdate =
createParameterSourceForUpdate(vmDevice)
+ .addValue("boot_order",vmDevice.getBootOrder());
+ getCallsHandler().executeModification("UpdateVmDeviceBootOrder" ,
paramsForUpdate);
}
@Override
diff --git
a/backend/manager/modules/dal/src/test/java/org/ovirt/engine/core/dao/VmDeviceDAOTest.java
b/backend/manager/modules/dal/src/test/java/org/ovirt/engine/core/dao/VmDeviceDAOTest.java
index 80c5aa4..61d5378 100644
---
a/backend/manager/modules/dal/src/test/java/org/ovirt/engine/core/dao/VmDeviceDAOTest.java
+++
b/backend/manager/modules/dal/src/test/java/org/ovirt/engine/core/dao/VmDeviceDAOTest.java
@@ -183,14 +183,21 @@
public void testUpdateHotPlugDisk() {
VmDevice vmDevice = dao.get(getExistingEntityId());
boolean newPluggedValue = !vmDevice.getIsPlugged();
- int newBootOrderValue = vmDevice.getBootOrder() + 1;
Assert.assertTrue(StringUtils.isNotBlank(vmDevice.getAddress()));
- vmDevice.setBootOrder(newBootOrderValue);
vmDevice.setIsPlugged(newPluggedValue);
dao.updateHotPlugDisk(vmDevice);
dao.get(getExistingEntityId());
assertEquals(vmDevice.getIsPlugged(), newPluggedValue);
- assertEquals(vmDevice.getBootOrder(),newBootOrderValue);
}
+ @Test
+ public void testUpdateBootOrder() {
+ VmDevice vmDevice = dao.get(getExistingEntityId());
+ int newBootOrderValue = vmDevice.getBootOrder() + 1;
+ Assert.assertTrue(StringUtils.isNotBlank(vmDevice.getAddress()));
+ vmDevice.setBootOrder(newBootOrderValue);
+ dao.updateHotPlugDisk(vmDevice);
+ dao.get(getExistingEntityId());
+ assertEquals(vmDevice.getBootOrder(),newBootOrderValue);
+ }
}
diff --git a/packaging/dbscripts/vm_device_sp.sql
b/packaging/dbscripts/vm_device_sp.sql
index c6fd3bd..a5dbd1b 100644
--- a/packaging/dbscripts/vm_device_sp.sql
+++ b/packaging/dbscripts/vm_device_sp.sql
@@ -100,14 +100,12 @@
Create or replace FUNCTION UpdateVmDeviceForHotPlugDisk(
v_device_id UUID,
v_vm_id UUID,
- v_boot_order int,
v_is_plugged boolean)
RETURNS VOID
AS $procedure$
BEGIN
UPDATE vm_device
SET
- boot_order = v_boot_order,
is_plugged = v_is_plugged,
_update_date = current_timestamp
WHERE device_id = v_device_id and vm_id = v_vm_id;
@@ -115,6 +113,21 @@
LANGUAGE plpgsql;
+Create or replace FUNCTION UpdateVmDeviceBootOrder(
+ v_device_id UUID,
+ v_vm_id UUID,
+ v_boot_order int)
+RETURNS VOID
+AS $procedure$
+BEGIN
+ UPDATE vm_device
+ SET
+ boot_order = v_boot_order,
+ _update_date = current_timestamp
+ WHERE device_id = v_device_id and vm_id = v_vm_id;
+END; $procedure$
+LANGUAGE plpgsql;
+
Create or replace FUNCTION DeleteVmDevice(v_device_id UUID, v_vm_id UUID)
RETURNS VOID
--
To view, visit http://gerrit.ovirt.org/19521
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I1359a34a48a6261e22631ff1640d81d735e8c490
Gerrit-PatchSet: 1
Gerrit-Project: ovirt-engine
Gerrit-Branch: ovirt-engine-3.3
Gerrit-Owner: Tal Nisan <[email protected]>
Gerrit-Reviewer: Sergey Gotliv <[email protected]>
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
