Yair Zaslavsky has uploaded a new change for review. Change subject: [WIP] core: Prevent dead lock on vm device (#852451) ......................................................................
[WIP] core: Prevent dead lock on vm device (#852451) https://bugzilla.redhat.com/852451 This patch is initial draft as it presents some ideas, and should be tested more! The following patch solved a dead lock on vm device: Both VdsUpdateRuntimeInfo and HotPlugDisk update a collection of VmDevices. The devices are un oredered, so a dead lock may occur. This patch solves the issue the following way: a. Presents ascending ordering of vm devices (both at stored procedure and at code , where needed) b. Changes HotplugDisk to perform update for both plugged flag and boot order in the same area of code c. Changes hotplugDisk to update only boot order and plugged fields of vm device d. Changes VdsUpdateRuntime info to update only the alias and address fields c and d are performed in order to avoid data races between the two concurrent flows We have discussed about using an optimistic lock mechanism at first, but using c and d , lets us have a KISS apporach to the solution Change-Id: If316259f9760777c5f85db0880a157021c61a6ba Signed-off-by: Yair Zaslavsky <yzasl...@redhat.com> --- M backend/manager/dbscripts/vm_device_sp.sql M backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AttachDiskToVmCommand.java M backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/DetachDiskFromVmCommand.java 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/UpdateVmDiskCommand.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/MassOperationsDao.java M backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/MassOperationsGenericDaoDbFacade.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 backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/VdsUpdateRunTimeInfo.java 12 files changed, 225 insertions(+), 33 deletions(-) git pull ssh://gerrit.ovirt.org:29418/ovirt-engine refs/changes/41/7641/1 diff --git a/backend/manager/dbscripts/vm_device_sp.sql b/backend/manager/dbscripts/vm_device_sp.sql index 594582b..0cfaf90 100644 --- a/backend/manager/dbscripts/vm_device_sp.sql +++ b/backend/manager/dbscripts/vm_device_sp.sql @@ -74,6 +74,43 @@ END; $procedure$ LANGUAGE plpgsql; +Create or replace FUNCTION UpdateVmDeviceRuntimeInfo( + v_device_id UUID, + v_vm_id UUID, + v_address varchar(255), + v_alias varchar(255)) +RETURNS VOID +AS $procedure$ +BEGIN + UPDATE vm_device + SET + address = v_address, + alias = v_alias, + _update_date = current_timestamp + WHERE device_id = v_device_id and vm_id = v_vm_id; +END; $procedure$ +LANGUAGE plpgsql; + + +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; +END; $procedure$ +LANGUAGE plpgsql; + + + Create or replace FUNCTION DeleteVmDevice(v_device_id UUID, v_vm_id UUID) RETURNS VOID AS $procedure$ @@ -113,7 +150,7 @@ RETURN QUERY SELECT * FROM vm_device_view - WHERE vm_id = v_vm_id; + WHERE vm_id = v_vm_id order by device_id; END; $procedure$ LANGUAGE plpgsql; diff --git a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AttachDiskToVmCommand.java b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AttachDiskToVmCommand.java index 2548377..0938efd 100644 --- a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AttachDiskToVmCommand.java +++ b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AttachDiskToVmCommand.java @@ -129,7 +129,7 @@ } protected void updateBootOrderInVmDevice() { - VmDeviceUtils.updateBootOrderInVmDevice(getVm().getStaticData()); + VmDeviceUtils.updateBootOrderInVmDeviceAndStoreToDB(getVm().getStaticData()); } private void updateDiskVmSnapshotId() { diff --git a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/DetachDiskFromVmCommand.java b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/DetachDiskFromVmCommand.java index 7a527c8..07f80ea 100644 --- a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/DetachDiskFromVmCommand.java +++ b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/DetachDiskFromVmCommand.java @@ -77,7 +77,7 @@ // update cached image VmHandler.updateDisksFromDb(getVm()); // update vm device boot order - VmDeviceUtils.updateBootOrderInVmDevice(getVm().getStaticData()); + VmDeviceUtils.updateBootOrderInVmDeviceAndStoreToDB(getVm().getStaticData()); setSucceeded(true); } 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 64cf251..63fc7c9 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,5 +1,7 @@ package org.ovirt.engine.core.bll; +import java.util.List; + import org.ovirt.engine.core.bll.utils.VmDeviceUtils; import org.ovirt.engine.core.common.AuditLogType; import org.ovirt.engine.core.common.action.HotPlugDiskToVmParameters; @@ -68,15 +70,22 @@ if (getVm().getstatus() == VMStatus.Up) { performPlugCommnad(getPlugAction(), disk, oldVmDevice); } - oldVmDevice.setIsPlugged(!oldVmDevice.getIsPlugged()); + + //Update boot order and + final List<VmDevice> devices = VmDeviceUtils.updateBootOrderInVmDevice(getVm().getStaticData()); + for (VmDevice device:devices) { + if (device.getDeviceId().equals(oldVmDevice.getDeviceId())) { + device.setIsPlugged(!oldVmDevice.getIsPlugged()); + break; + } + } + TransactionSupport.executeInNewTransaction(new TransactionMethod<Void>() { @Override public Void runInTransaction() { - getVmDeviceDao().update(oldVmDevice); - // update cached image + + getVmDeviceDao().updateAll("UpdateVmDeviceForHotPlugDisk",devices); VmHandler.updateDisksFromDb(getVm()); - // update vm device boot order - VmDeviceUtils.updateBootOrderInVmDevice(getVm().getStaticData()); return null; } }); diff --git a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/UpdateVmDiskCommand.java b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/UpdateVmDiskCommand.java index 74de729..906a1fd 100644 --- a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/UpdateVmDiskCommand.java +++ b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/UpdateVmDiskCommand.java @@ -242,7 +242,7 @@ // update cached image VmHandler.updateDisksFromDb(getVm()); // update vm device boot order - VmDeviceUtils.updateBootOrderInVmDevice(getVm().getStaticData()); + VmDeviceUtils.updateBootOrderInVmDeviceAndStoreToDB(getVm().getStaticData()); setSucceeded(UpdateVmInSpm(getVm().getstorage_pool_id(), Arrays.asList(getVm()))); 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 b491421..d26d165 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 @@ -44,7 +44,6 @@ private final static int SLOTS_PER_CONTROLLER = 6; private final static int COMPANION_USB_CONTROLLERS = 3; - /** * Update the vm devices according to changes made in vm static for existing VM */ @@ -54,7 +53,7 @@ updateCdInVmDevice(oldVmBase, entity); if (oldVmBase.getdefault_boot_sequence() != entity .getdefault_boot_sequence()) { - updateBootOrderInVmDevice(entity); + updateBootOrderInVmDeviceAndStoreToDB(entity); } // if the console type has changed, recreate Video devices @@ -98,7 +97,7 @@ public static <T extends VmBase> void updateVmDevices(T entity, Guid newId) { if (entity != null) { updateCdInVmDevice(entity); - updateBootOrderInVmDevice(entity); + updateBootOrderInVmDeviceAndStoreToDB(entity); updateNumOfMonitorsInVmDevice(null, entity); updateUSBSlots(null, entity); updateMemoryBalloon(null, entity, true); @@ -190,7 +189,7 @@ if (isVm) { // update devices boot order - updateBootOrderInVmDevice(vmBase); + updateBootOrderInVmDeviceAndStoreToDB(vmBase); // create sound card for a desktop VM if not exists if (vmBase.getvm_type() == VmType.Desktop) { @@ -277,7 +276,7 @@ if (type.equals(VmDeviceType.DISK) || type.equals(VmDeviceType.INTERFACE )) { // recalculate boot sequence VmBase vmBase = DbFacade.getInstance().getVmStaticDAO().get(id.getVmId()); - updateBootOrderInVmDevice(vmBase); + updateBootOrderInVmDeviceAndStoreToDB(vmBase); } return managedDevice; } @@ -314,10 +313,24 @@ /** * Updates VM boot order in vm device according to the BootSequence enum value. + * Stores the updated devices in DB * @param vmBase */ - public static void updateBootOrderInVmDevice(VmBase vmBase) { + public static void updateBootOrderInVmDeviceAndStoreToDB(VmBase vmBase) { + List<VmDevice> devices = updateBootOrderInVmDevice(vmBase); + for (VmDevice device : devices) { + dao.update(device); + } + } + + /** + * Updates boot order in vm device according to the BootSequence enum value. + * @param vmBase + * @return the updated VmDevice list + */ + public static List<VmDevice> updateBootOrderInVmDevice(VmBase vmBase) { if (vmBase instanceof VmStatic) { + //Returns the devices sorted in ascending manner List<VmDevice> devices = dao.getVmDeviceByVmId(vmBase.getId()); // reset current boot order for (VmDevice device: devices) { @@ -327,11 +340,9 @@ VmHandler.updateDisksForVm(vm, DbFacade.getInstance().getDiskDao().getAllForVm(vm.getId())); boolean isOldCluster = VmDeviceCommonUtils.isOldClusterVersion(vm.getvds_group_compatibility_version()); VmDeviceCommonUtils.updateVmDevicesBootOrder(vm, devices, isOldCluster); - // update boot order in vm device - for (VmDevice device : devices) { - dao.update(device); - } + return devices; } + return Collections.emptyList(); } /** diff --git a/backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/MassOperationsDao.java b/backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/MassOperationsDao.java index 3825d3b..79359da 100644 --- a/backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/MassOperationsDao.java +++ b/backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/MassOperationsDao.java @@ -14,10 +14,19 @@ /** * Updates the given entities using a more efficient method to update all of them at once, rather than each at a - * time. + * time. The procedure name to be used is "UpdateEntityName> where EntityName stands for the name of the entity * * @param entities * The entities to update. */ void updateAll(Collection<T> entities); + + + /** + * Updates the given entities using a more efficient method to update all of them at once, rather than each at a + * time. + * @param procedureName procedure name for update + * @param entities + */ + void updateAll(String procedureName, Collection<T> entities); } diff --git a/backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/MassOperationsGenericDaoDbFacade.java b/backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/MassOperationsGenericDaoDbFacade.java index 197c361..14feafb 100644 --- a/backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/MassOperationsGenericDaoDbFacade.java +++ b/backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/MassOperationsGenericDaoDbFacade.java @@ -24,9 +24,21 @@ @Override public void updateAll(Collection<T> entities) { + updateAll(getProcedureNameForUpdate(),entities); for (T entity : entities) { getCallsHandler().executeModification(getProcedureNameForUpdate(), createFullParametersMapper(entity)); } } + + @Override + public void updateAll(String procedureName, Collection<T> entities) { + if (procedureName == null) { + procedureName = getProcedureNameForUpdate(); + } + for (T entity : entities) { + getCallsHandler().executeModification(procedureName, + createFullParametersMapper(entity)); + } + } } 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 36963e0..e056b28 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 @@ -41,4 +41,19 @@ * @param device */ void clearDeviceAddress(Guid deviceId); + + /** + * Runs an update for the device according to fields that were updated during + * VdsUpdateRuntimeInfo periodic call + * @param vmDevice + */ + void updateRuntimeInfo(VmDevice vmDevice); + + /** + * Runs an update for the device according to fields that were updated during + * HotPlugDisk + * @param vmDevice + */ + void updateHotPlugDisk(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 b84488a..d325adf 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 @@ -175,4 +175,31 @@ getCallsHandler().executeModification("clearVmDeviceAddress", parameterSource); } + + @Override + public void updateRuntimeInfo(VmDevice vmDevice) { + MapSqlParameterSource paramsForUpdate = createParameterSourceForUpdate(vmDevice) + .addValue("address", vmDevice.getAddress()) + .addValue("alias",vmDevice.getAlias()); + + getCallsHandler().executeModification("UpdateVmDeviceRuntimeInfo", paramsForUpdate); + } + + private MapSqlParameterSource createParameterSourceForUpdate(VmDevice vmDevice) { + MapSqlParameterSource parameterSource = getCustomMapSqlParameterSource() + .addValue("vm_id", vmDevice.getVmId()) + .addValue("device_id",vmDevice.getDeviceId()); + 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); + + } + } 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 a203aca..f7763de 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 @@ -146,4 +146,33 @@ Assert.assertTrue(StringUtils.isBlank(dao.get(getExistingEntityId()).getAddress())); } + + @Test + public void testUpdateDeviceRuntimeInfo() { + VmDevice vmDevice = dao.get(getExistingEntityId()); + Assert.assertTrue(StringUtils.isNotBlank(vmDevice.getAddress())); + String newAddressValue = "newaddr"; + vmDevice.setAddress(newAddressValue); + String newAlias = "newalias"; + vmDevice.setAlias(newAlias); + dao.updateRuntimeInfo(vmDevice); + dao.get(getExistingEntityId()); + assertEquals(vmDevice.getAddress(), newAddressValue); + assertEquals(vmDevice.getAlias(), newAlias); + } + + @Test + 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); + } + } diff --git a/backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/VdsUpdateRunTimeInfo.java b/backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/VdsUpdateRunTimeInfo.java index b1a4f92..6371236 100644 --- a/backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/VdsUpdateRunTimeInfo.java +++ b/backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/VdsUpdateRunTimeInfo.java @@ -3,6 +3,7 @@ import java.util.ArrayList; import java.util.Collection; import java.util.Collections; +import java.util.Comparator; import java.util.HashMap; import java.util.HashSet; import java.util.LinkedList; @@ -56,6 +57,7 @@ import org.ovirt.engine.core.utils.NetworkUtils; import org.ovirt.engine.core.utils.ObjectIdentityChecker; import org.ovirt.engine.core.utils.Pair; +import org.ovirt.engine.core.utils.comparator.NGuidComparator; import org.ovirt.engine.core.utils.log.Log; import org.ovirt.engine.core.utils.log.LogFactory; import org.ovirt.engine.core.utils.transaction.TransactionMethod; @@ -108,6 +110,20 @@ private static Map<Guid, Long> hostDownTimes = new HashMap<Guid, Long>(); private int runningVmsInTransition = 0; + private static final Comparator<VmDevice> vmDeviceComparator = new Comparator<VmDevice>() { + + @Override + public int compare(VmDevice o1, VmDevice o2) { + //As primary key at db for Vm device is defined both using + //vm device Id and vm Id, these two values are used for comparison + NGuidComparator comparator = new NGuidComparator(); + int vmIdComparison = comparator.compare(o1.getVmId(),o2.getVmId()); + return (vmIdComparison == 0)?0: + comparator.compare(o1.getDeviceId(), o2.getDeviceId()); + } + }; + + public VM GetVmFromDictionary(Guid id) { VM vm = null; vm = _vmDict.get(id); @@ -156,9 +172,16 @@ saveVmDevicesToDb(); } - private void saveVmDevicesToDb() { + private void sortVmDeviceList(List<VmDevice> vmDevices) { + Collections.sort(vmDevices,vmDeviceComparator); + } - updateAllInTransaction(vmDeviceToSave.values(), getDbFacade().getVmDeviceDAO()); + private void saveVmDevicesToDb() { + //Sort VM devices in ascending order + List<VmDevice> sortedVmDevices = new ArrayList<VmDevice>(); + sortedVmDevices.addAll(vmDeviceToSave.values()); + sortVmDeviceList(sortedVmDevices); + updateAllInTransaction(sortedVmDevices, getDbFacade().getVmDeviceDAO()); if (!removedDeviceIds.isEmpty()) { TransactionSupport.executeInScope(TransactionScopeOption.Required, @@ -197,20 +220,40 @@ */ private static <T extends BusinessEntity<?>> void updateAllInTransaction (final Collection<T> entities, final MassOperationsDao<T> dao) { - if (!entities.isEmpty()) { - TransactionSupport.executeInScope(TransactionScopeOption.Required, - new TransactionMethod<Void>() { - - @Override - public Void runInTransaction() { - dao.updateAll(entities); - return null; - } - }); - } + updateAllInTransaction(null, entities,dao); } /** + * Update all the given entities in a transaction, so that a new connection/transaction won't be opened for each + * entity update. + * + * @param <T> + * The type of entity. + * @param procieudreName + * The name of stored procedure to use for update + * @param entities + * The entities to update. + * @param dao + * The DAO used for updating. + */ + + private static <T extends BusinessEntity<?>> void updateAllInTransaction + (final String procedureName, final Collection<T> entities, final MassOperationsDao<T> dao) { + if (!entities.isEmpty()) { + TransactionSupport.executeInScope(TransactionScopeOption.Required, + new TransactionMethod<Void>() { + + @Override + public Void runInTransaction() { + dao.updateAll(procedureName,entities); + return null; + } + }); + } +} + + + /** * check if value is less than configurable threshold , if yes , generated event log message * * @param stat -- To view, visit http://gerrit.ovirt.org/7641 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: newchange Gerrit-Change-Id: If316259f9760777c5f85db0880a157021c61a6ba Gerrit-PatchSet: 1 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Yair Zaslavsky <yzasl...@redhat.com> _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
