Allon Mureinik has uploaded a new change for review. Change subject: core: lock disks when migrating VMs ......................................................................
core: lock disks when migrating VMs This patch solves a race between Live Migration and Live Storage Migration by having the VM migration commands take a (shared) lock on all the VM's plugged disks and an exclusive lock on the VM itself. This new behaviour will also have the positive effect of solving a bunch of (unreported) similar races such as Live Snapshot, the hotplug commands, etc. This patch contains: 1. Added functionality to DiskDao.getForVm to retrieve only the plugged disks 2. Adding the relevant locks to the migration commands 3. Adding canDoAction checks for locked disks, for backwards compatibility with other storage related commands. Change-Id: Ie9a335c3269400b355acd566c8f39e4a50fa237b Bug-Url: https://bugzilla.redhat.com/878131 Signed-off-by: Allon Mureinik <[email protected]> --- M backend/manager/dbscripts/all_disks_sp.sql M backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/MigrateVmCommand.java M backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/MigrateVmToServerCommand.java M backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/DiskDao.java M backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/DiskDaoDbFacadeImpl.java M backend/manager/modules/dal/src/test/java/org/ovirt/engine/core/dao/BaseDiskDaoTest.java M backend/manager/modules/dal/src/test/java/org/ovirt/engine/core/dao/DiskDaoTest.java M backend/manager/modules/dal/src/test/java/org/ovirt/engine/core/dao/DiskImageDAOTest.java M backend/manager/modules/dal/src/test/java/org/ovirt/engine/core/dao/DiskLunMapDaoTest.java M backend/manager/modules/dal/src/test/java/org/ovirt/engine/core/dao/VmDeviceDAOTest.java M backend/manager/modules/dal/src/test/resources/fixtures.xml 11 files changed, 167 insertions(+), 26 deletions(-) git pull ssh://gerrit.ovirt.org:29418/ovirt-engine refs/changes/63/13263/1 diff --git a/backend/manager/dbscripts/all_disks_sp.sql b/backend/manager/dbscripts/all_disks_sp.sql index 4dc843b..11f0038 100644 --- a/backend/manager/dbscripts/all_disks_sp.sql +++ b/backend/manager/dbscripts/all_disks_sp.sql @@ -37,13 +37,13 @@ LANGUAGE plpgsql; -Create or replace FUNCTION GetDisksVmGuid(v_vm_guid UUID, v_user_id UUID, v_is_filtered BOOLEAN) +Create or replace FUNCTION GetDisksVmGuid(v_vm_guid UUID, v_only_plugged BOOLEAN, v_user_id UUID, v_is_filtered BOOLEAN) RETURNS SETOF all_disks AS $procedure$ BEGIN RETURN QUERY SELECT all_disks.* FROM all_disks - LEFT JOIN vm_device on vm_device.device_id = all_disks.image_group_id + LEFT JOIN vm_device ON vm_device.device_id = all_disks.image_group_id AND (NOT v_only_plugged OR is_plugged) WHERE vm_device.vm_id = v_vm_guid AND (NOT v_is_filtered OR EXISTS (SELECT 1 FROM user_disk_permissions_view diff --git a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/MigrateVmCommand.java b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/MigrateVmCommand.java index 45ea7c4..443abe2 100644 --- a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/MigrateVmCommand.java +++ b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/MigrateVmCommand.java @@ -1,12 +1,16 @@ package org.ovirt.engine.core.bll; import java.util.ArrayList; +import java.util.HashMap; +import java.util.List; +import java.util.Map; import org.ovirt.engine.core.bll.job.ExecutionHandler; import org.ovirt.engine.core.bll.snapshots.SnapshotsValidator; import org.ovirt.engine.core.common.AuditLogType; import org.ovirt.engine.core.common.FeatureSupported; import org.ovirt.engine.core.common.action.MigrateVmParameters; +import org.ovirt.engine.core.common.businessentities.Disk; import org.ovirt.engine.core.common.businessentities.MigrationMethod; import org.ovirt.engine.core.common.businessentities.MigrationSupport; import org.ovirt.engine.core.common.businessentities.VDS; @@ -15,6 +19,7 @@ import org.ovirt.engine.core.common.businessentities.VMStatus; import org.ovirt.engine.core.common.errors.VdcBLLException; import org.ovirt.engine.core.common.errors.VdcBllErrors; +import org.ovirt.engine.core.common.utils.Pair; import org.ovirt.engine.core.common.vdscommands.MigrateStatusVDSCommandParameters; import org.ovirt.engine.core.common.vdscommands.MigrateVDSCommandParameters; import org.ovirt.engine.core.common.vdscommands.VDSCommandType; @@ -25,11 +30,14 @@ import org.ovirt.engine.core.dal.dbbroker.auditloghandling.CustomLogFields; @CustomLogFields({ @CustomLogField("VdsDestination"), @CustomLogField("DueToMigrationError") }) +@LockIdNameAttribute public class MigrateVmCommand<T extends MigrateVmParameters> extends RunVmCommandBase<T> { private static final long serialVersionUID = -89419649366187512L; private Guid _vdsDestinationId; protected boolean forcedMigrationForNonMigratableVM; + private List<Disk> pluggedVmDisks; + private Map<String, Pair<String, String>> sharedLockMap; /** * Used to log the migration error. @@ -213,7 +221,19 @@ reasons.add(VdcBllMessages.ACTION_TYPE_FAILED_VDS_STATUS_ILLEGAL.name()); } - retValue = retValue && validate(new SnapshotsValidator().vmNotDuringSnapshot(vm.getId())) + retValue = + retValue + && ImagesHandler.PerformImagesChecks(reasons, + getStoragePoolId().getValue(), + Guid.Empty, + false, + true, + false, + false, + false, + false, + getVmPluggedDisks()) + && validate(new SnapshotsValidator().vmNotDuringSnapshot(vm.getId())) && getVdsSelector().canFindVdsToRunOn(reasons, true); } @@ -222,6 +242,13 @@ reasons.add(VdcBllMessages.VAR__TYPE__VM.toString()); } return retValue; + } + + private List<Disk> getVmPluggedDisks() { + if (pluggedVmDisks == null) { + pluggedVmDisks = getDiskDao().getAllForVm(getVmId(), true); + } + return pluggedVmDisks; } @Override @@ -271,4 +298,19 @@ return super.getCurrentVdsId(); } } + + @Override + protected Map<String, Pair<String, String>> getSharedLocks() { + if (sharedLockMap == null) { + List<Disk> disksToLock = getVmPluggedDisks(); + if (!disksToLock.isEmpty()) { + sharedLockMap = new HashMap<String, Pair<String, String>>(disksToLock.size()); + for (Disk disk : disksToLock) { + sharedLockMap.put(disk.getId().toString(), LockMessagesMatchUtil.DISK); + } + } + } + return sharedLockMap; + } + } diff --git a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/MigrateVmToServerCommand.java b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/MigrateVmToServerCommand.java index 723b51d..d6b230a 100644 --- a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/MigrateVmToServerCommand.java +++ b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/MigrateVmToServerCommand.java @@ -7,6 +7,7 @@ import org.ovirt.engine.core.dal.VdcBllMessages; import org.ovirt.engine.core.dal.dbbroker.DbFacade; +@LockIdNameAttribute public class MigrateVmToServerCommand<T extends MigrateVmToServerParameters> extends MigrateVmCommand<T> { public MigrateVmToServerCommand(T parameters) { super(parameters); diff --git a/backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/DiskDao.java b/backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/DiskDao.java index 645fb9e..ad54fb0 100644 --- a/backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/DiskDao.java +++ b/backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/DiskDao.java @@ -17,6 +17,17 @@ List<Disk> getAllForVm(Guid id); /** + * Retrieves all disks for the specified virtual machine id. + * + * @param id + * the VM id + * @param onlyPluggedDisks + * whether to returned only the disks plugged to the VM or not + * @return the list of disks + */ + List<Disk> getAllForVm(Guid id, boolean onlyPluggedDisks); + + /** * Retrieves all disks for the specified virtual machine id, * with optional filtering * @@ -32,6 +43,22 @@ List<Disk> getAllForVm(Guid id, Guid userID, boolean isFiltered); /** + * Retrieves all disks for the specified virtual machine id, with optional filtering + * + * @param id + * the VM id + * @param onlyPluggedDisks + * whether to returned only the disks plugged to the VM or not + * @param userID + * the ID of the user requesting the information + * @param isFiltered + * Whether the results should be filtered according to the user's permissions + * + * @return the list of disks + */ + List<Disk> getAllForVm(Guid id, boolean onlyPluggedDisks, Guid userID, boolean isFiltered); + + /** * Retrieves all disks for the specified user * with optional filtering * diff --git a/backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/DiskDaoDbFacadeImpl.java b/backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/DiskDaoDbFacadeImpl.java index 83c2f5b..8d83684 100644 --- a/backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/DiskDaoDbFacadeImpl.java +++ b/backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/DiskDaoDbFacadeImpl.java @@ -28,19 +28,32 @@ @Override public List<Disk> getAllForVm(Guid id) { - return getAllForVm(id, null, false); + return getAllForVm(id, false); + } + + @Override + public List<Disk> getAllForVm(Guid id, boolean onlyPluggedDisks) { + return getAllForVm(id, onlyPluggedDisks, null, false); + } + + @Override + public List<Disk> getAllForVm(Guid id, Guid userID, boolean isFiltered) { + return getAllForVm(id, false, userID, isFiltered); + } + + @Override + public List<Disk> getAllForVm(Guid id, boolean onlyPluggedDisks, Guid userID, boolean isFiltered) { + MapSqlParameterSource parameterSource = getCustomMapSqlParameterSource() + .addValue("vm_guid", id) + .addValue("only_plugged", onlyPluggedDisks) + .addValue("user_id", userID) + .addValue("is_filtered", isFiltered); + return getCallsHandler().executeReadList("GetDisksVmGuid", DiskRowMapper.instance, parameterSource); } @Override public List<Disk> getAll() { return getAll(null, false); - } - - @Override - public List<Disk> getAllForVm(Guid id, Guid userID, boolean isFiltered) { - MapSqlParameterSource parameterSource = getCustomMapSqlParameterSource() - .addValue("vm_guid", id).addValue("user_id", userID).addValue("is_filtered", isFiltered); - return getCallsHandler().executeReadList("GetDisksVmGuid", DiskRowMapper.instance, parameterSource); } @Override diff --git a/backend/manager/modules/dal/src/test/java/org/ovirt/engine/core/dao/BaseDiskDaoTest.java b/backend/manager/modules/dal/src/test/java/org/ovirt/engine/core/dao/BaseDiskDaoTest.java index fa005a3..b1f27cb 100644 --- a/backend/manager/modules/dal/src/test/java/org/ovirt/engine/core/dao/BaseDiskDaoTest.java +++ b/backend/manager/modules/dal/src/test/java/org/ovirt/engine/core/dao/BaseDiskDaoTest.java @@ -15,7 +15,7 @@ public class BaseDiskDaoTest extends BaseGenericDaoTestCase<Guid, BaseDisk, BaseDiskDao> { private static final Guid EXISTING_DISK_ID = new Guid("1b26a52b-b60f-44cb-9f46-3ef333b04a34"); - private static final int TOTAL_DISKS = 5; + private static final int TOTAL_DISKS = 6; @Override protected Guid generateNonExistingId() { diff --git a/backend/manager/modules/dal/src/test/java/org/ovirt/engine/core/dao/DiskDaoTest.java b/backend/manager/modules/dal/src/test/java/org/ovirt/engine/core/dao/DiskDaoTest.java index 42a01b6..adcfdb0 100644 --- a/backend/manager/modules/dal/src/test/java/org/ovirt/engine/core/dao/DiskDaoTest.java +++ b/backend/manager/modules/dal/src/test/java/org/ovirt/engine/core/dao/DiskDaoTest.java @@ -1,7 +1,6 @@ package org.ovirt.engine.core.dao; import static org.junit.Assert.assertEquals; -import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertNotNull; import static org.junit.Assert.assertNull; import static org.junit.Assert.assertTrue; @@ -39,14 +38,7 @@ return TOTAL_DISK_IMAGES + DiskLunMapDaoTest.TOTAL_DISK_LUN_MAPS; } - @Test - public void testGetAllForVm() { - List<Disk> result = dao.getAllForVm(FixturesTool.VM_TEMPLATE_RHEL5); - - assertNotNull(result); - assertFalse(result.isEmpty()); - } - + @Override @Test public void testGet() { Disk result = dao.get(getExistingEntityId()); @@ -108,6 +100,27 @@ } @Test + public void testGetPluggedForVMFilteredWithPermissions() { + // test user 3 - has permissions + List<Disk> disks = dao.getAllForVm(FixturesTool.VM_RHEL5_POOL_57, true, PRIVILEGED_USER_ID, true); + assertPluggedGetAllForVMResult(disks); + } + + @Test + public void testGetPluggedForVMFilteredWithPermissionsNoPermissions() { + // test user 2 - hasn't got permissions + List<Disk> disks = dao.getAllForVm(FixturesTool.VM_RHEL5_POOL_57, true, UNPRIVILEGED_USER_ID, true); + assertTrue("VM should have no disks viewable to the user", disks.isEmpty()); + } + + @Test + public void testGetPluggedForVMFilteredWithPermissionsNoPermissionsAndNoFilter() { + // test user 2 - hasn't got permissions, but no filtering was requested + List<Disk> disks = dao.getAllForVm(FixturesTool.VM_RHEL5_POOL_57, true, UNPRIVILEGED_USER_ID, false); + assertPluggedGetAllForVMResult(disks); + } + + @Test public void testGetAllForVM() { List<Disk> disks = dao.getAllForVm(FixturesTool.VM_RHEL5_POOL_57); assertFullGetAllForVMResult(disks); @@ -162,7 +175,16 @@ * The result to check */ private static void assertFullGetAllForVMResult(List<Disk> disks) { - assertEquals("VM should have three disks", 3, disks.size()); + assertEquals("VM should have three disks", 4, disks.size()); + } + + /** + * Asserts the result of {@link DiskImageDAO#getAllForVm(Guid)} contains the correct plugged disks. + * @param disks + * The result to check + */ + private static void assertPluggedGetAllForVMResult(List<Disk> disks) { + assertEquals("VM should have only one plugged disk", 1, disks.size()); } /** diff --git a/backend/manager/modules/dal/src/test/java/org/ovirt/engine/core/dao/DiskImageDAOTest.java b/backend/manager/modules/dal/src/test/java/org/ovirt/engine/core/dao/DiskImageDAOTest.java index 015e08a..7a47816 100644 --- a/backend/manager/modules/dal/src/test/java/org/ovirt/engine/core/dao/DiskImageDAOTest.java +++ b/backend/manager/modules/dal/src/test/java/org/ovirt/engine/core/dao/DiskImageDAOTest.java @@ -22,7 +22,7 @@ private DiskImage existingTemplate; - private static final int TOTAL_DISK_IMAGES = 7; + protected static final int TOTAL_DISK_IMAGES = 7; private BaseDiskDao diskDao; @Override diff --git a/backend/manager/modules/dal/src/test/java/org/ovirt/engine/core/dao/DiskLunMapDaoTest.java b/backend/manager/modules/dal/src/test/java/org/ovirt/engine/core/dao/DiskLunMapDaoTest.java index 06f7959..0bf7a98 100644 --- a/backend/manager/modules/dal/src/test/java/org/ovirt/engine/core/dao/DiskLunMapDaoTest.java +++ b/backend/manager/modules/dal/src/test/java/org/ovirt/engine/core/dao/DiskLunMapDaoTest.java @@ -15,7 +15,7 @@ private static final DiskLunMapId EXISTING_DISK_LUN_MAP_ID = new DiskLunMapId(new Guid("1b26a52b-b60f-44cb-9f46-3ef333b04a35"), "1IET_00180002"); - protected static final int TOTAL_DISK_LUN_MAPS = 2; + protected static final int TOTAL_DISK_LUN_MAPS = 3; @Override protected DiskLunMapId generateNonExistingId() { 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 2c81121..7e04af3 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 @@ -23,8 +23,8 @@ private static final Guid EXISTING_VM_ID = new Guid("77296e00-0cad-4e5a-9299-008a7b6f4355"); private static final Guid EXISTING_DEVICE_ID = new Guid("e14ed6f0-3b12-11e1-b614-63d00126418d"); - private static final int TOTAL_DEVICES = 9; - private static final int TOTAL_DEVICES_FOR_EXISTING_VM = 2; + private static final int TOTAL_DEVICES = 10; + private static final int TOTAL_DEVICES_FOR_EXISTING_VM = 3; @Override diff --git a/backend/manager/modules/dal/src/test/resources/fixtures.xml b/backend/manager/modules/dal/src/test/resources/fixtures.xml index 3f858bc..2dfbb82 100644 --- a/backend/manager/modules/dal/src/test/resources/fixtures.xml +++ b/backend/manager/modules/dal/src/test/resources/fixtures.xml @@ -263,6 +263,15 @@ <value>IET</value> <value>VIRTUAL-DISK</value> </row> + <row> + <value></value> + <value>1IET_00180004</value> + <value>fDMzhE-wx3s-zo3q-Qcxd-T0li-yoYU-QvVePk</value> + <value>SOME-LUN-DISK</value> + <value>1</value> + <value>IET</value> + <value>VIRTUAL-DISK</value> + </row> </table> <table name="LUN_storage_server_connection_map"> @@ -2648,6 +2657,16 @@ <value>true</value> <value>false</value> </row> + <row> + <value>1b26a52b-b60f-44cb-9f46-3ef333b04a40</value> + <value>VirtIO</value> + <value>4</value> + <value>Off</value> + <value>false</value> + <value>New Description</value> + <value>false</value> + <value>false</value> + </row> </table> <table name="disk_lun_map"> @@ -2660,6 +2679,10 @@ <row> <value>1b26a52b-b60f-44cb-9f46-3ef333b04a37</value> <value>1IET_00180003</value> + </row> + <row> + <value>1b26a52b-b60f-44cb-9f46-3ef333b04a40</value> + <value>1IET_00180004</value> </row> </table> @@ -4046,6 +4069,19 @@ <value>false</value> <value>alias</value> </row> + <row> + <value>1b26a52b-b60f-44cb-9f46-3ef333b04a40</value> + <value>77296e00-0cad-4e5a-9299-008a7b6f4355</value> + <value>disk</value> + <value>disk</value> + <value>sample</value> + <value>1</value> + <value>{}</value> + <value>true</value> + <value>true</value> + <value>false</value> + <value>alias</value> + </row> </table> <table name="job"> -- To view, visit http://gerrit.ovirt.org/13263 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: newchange Gerrit-Change-Id: Ie9a335c3269400b355acd566c8f39e4a50fa237b Gerrit-PatchSet: 1 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Allon Mureinik <[email protected]> _______________________________________________ Engine-patches mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/engine-patches
