Liron Ar has uploaded a new change for review. Change subject: core: GetAllDisksByVmIdQuery - improved performance and avoid NPE ......................................................................
core: GetAllDisksByVmIdQuery - improved performance and avoid NPE Currently when executing GetAllDisksByVmIdQuery, all the vm disks are queried using the GetDisksVmGuid stored procedure and then the vm device for each disk is being loaded to complete to queried data. This can cause to few issues: 1. NPE in case that we have a race between the query execution and removal/detachment of a disk from the VM. In that case the loaded device won't exist and the query will fail with NPE. 2. Mass of loads that could be prevented, GetDisksVmGuid already performs join with vm_device table so that records could be just returned. Change-Id: I91d12ee535563204b5d6aaeb64ec6a7c23ea1a81 Signed-off-by: Liron Aravot <[email protected]> --- M backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/GetAllDisksByVmIdQuery.java M backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/DiskDaoDbFacadeImpl.java M backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/VmDeviceDAODbFacadeImpl.java M packaging/dbscripts/all_disks_sp.sql M packaging/dbscripts/create_views.sql 5 files changed, 37 insertions(+), 40 deletions(-) git pull ssh://gerrit.ovirt.org:29418/ovirt-engine refs/changes/06/22506/1 diff --git a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/GetAllDisksByVmIdQuery.java b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/GetAllDisksByVmIdQuery.java index da85528..fd8adf0 100644 --- a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/GetAllDisksByVmIdQuery.java +++ b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/GetAllDisksByVmIdQuery.java @@ -1,19 +1,12 @@ package org.ovirt.engine.core.bll; import java.util.ArrayList; -import java.util.Collections; -import java.util.HashMap; import java.util.List; -import java.util.Map; import org.ovirt.engine.core.common.businessentities.Disk; import org.ovirt.engine.core.common.businessentities.Disk.DiskStorageType; import org.ovirt.engine.core.common.businessentities.DiskImage; -import org.ovirt.engine.core.common.businessentities.VmDevice; -import org.ovirt.engine.core.common.businessentities.VmDeviceGeneralType; import org.ovirt.engine.core.common.queries.IdQueryParameters; -import org.ovirt.engine.core.common.utils.VmDeviceType; -import org.ovirt.engine.core.compat.Guid; public class GetAllDisksByVmIdQuery<P extends IdQueryParameters> extends QueriesCommandBase<P> { public GetAllDisksByVmIdQuery(P parameters) { @@ -25,12 +18,8 @@ List<Disk> allDisks = getDbFacade().getDiskDao().getAllForVm (getParameters().getId(), getUserID(), getParameters().isFiltered()); - Map<Guid, VmDevice> disksVmDevices = getDisksVmDeviceMap(); List<Disk> disks = new ArrayList<Disk>(allDisks); for (Disk disk : allDisks) { - VmDevice diskDevice = disksVmDevices.get(disk.getId()); - disk.setPlugged(diskDevice.getIsPlugged()); - disk.setReadOnly(diskDevice.getIsReadOnly()); if (disk.getDiskStorageType() == DiskStorageType.IMAGE) { DiskImage diskImage = (DiskImage) disk; diskImage.getSnapshots().addAll(getAllImageSnapshots(diskImage)); @@ -41,25 +30,5 @@ protected List<DiskImage> getAllImageSnapshots(DiskImage diskImage) { return ImagesHandler.getAllImageSnapshots(diskImage.getImageId(), diskImage.getImageTemplateId()); - } - - private Map<Guid, VmDevice> getDisksVmDeviceMap() { - List<VmDevice> disksVmDevices = - getDbFacade().getVmDeviceDao() - .getVmDeviceByVmIdTypeAndDevice(getParameters().getId(), - VmDeviceGeneralType.DISK, - VmDeviceType.DISK.getName(), - getUserID(), - getParameters().isFiltered()); - - if (disksVmDevices.isEmpty()) { - return Collections.emptyMap(); - } - - Map<Guid, VmDevice> toReturn = new HashMap<>(); - for (VmDevice diskVmDevice : disksVmDevices) { - toReturn.put(diskVmDevice.getDeviceId(), diskVmDevice); - } - return toReturn; } } 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 d41dc52..84bb686 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 @@ -48,7 +48,7 @@ .addValue("only_plugged", onlyPluggedDisks) .addValue("user_id", userID) .addValue("is_filtered", isFiltered); - return getCallsHandler().executeReadList("GetDisksVmGuid", DiskRowMapper.instance, parameterSource); + return getCallsHandler().executeReadList("GetDisksVmGuid", DiskForVmRowMapper.instance, parameterSource); } @Override @@ -129,6 +129,22 @@ } } + private static class DiskForVmRowMapper implements RowMapper<Disk> { + + public static DiskForVmRowMapper instance = new DiskForVmRowMapper(); + + private DiskForVmRowMapper() { + } + + @Override + public Disk mapRow(ResultSet rs, int rowNum) throws SQLException { + Disk disk = DiskRowMapper.instance.mapRow(rs, rowNum); + disk.setPlugged(rs.getBoolean("is_plugged")); + disk.setReadOnly(rs.getBoolean("is_readonly")); + return disk; + } + } + private static class DiskBasicViewRowMapper implements RowMapper<Disk> { public static DiskBasicViewRowMapper instance = new DiskBasicViewRowMapper(); 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 fecee4a..a4fd4ab 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 @@ -150,6 +150,8 @@ vmDevice.setSpecParams(SerializationFactory.getDeserializer() .deserializeOrCreateNew(rs.getString("spec_params"), HashMap.class)); vmDevice.setIsManaged(rs.getBoolean("is_managed")); + // note - those columns are being used also in DiskRowMapper, therefore any related + // change should be done there as well. vmDevice.setIsPlugged(rs.getBoolean("is_plugged")); vmDevice.setIsReadOnly(rs.getBoolean("is_readonly")); vmDevice.setAlias(rs.getString("alias")); diff --git a/packaging/dbscripts/all_disks_sp.sql b/packaging/dbscripts/all_disks_sp.sql index 6130131..112b4f7 100644 --- a/packaging/dbscripts/all_disks_sp.sql +++ b/packaging/dbscripts/all_disks_sp.sql @@ -38,22 +38,23 @@ 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_including_snapshots STABLE +RETURNS SETOF all_disks_for_vms STABLE AS $procedure$ BEGIN - RETURN QUERY SELECT all_disks_including_snapshots.* - FROM all_disks_including_snapshots - LEFT JOIN vm_device ON vm_device.device_id = all_disks_including_snapshots.image_group_id AND (NOT v_only_plugged OR is_plugged) - WHERE vm_device.vm_id = v_vm_guid - AND ((vm_device.snapshot_id IS NULL AND all_disks_including_snapshots.active IS NOT FALSE) - OR vm_device.snapshot_id = all_disks_including_snapshots.vm_snapshot_id) + RETURN QUERY SELECT * + FROM all_disks_for_vms + WHERE vm_id = v_vm_guid AND (NOT v_only_plugged OR is_plugged) AND (NOT v_is_filtered OR EXISTS (SELECT 1 FROM user_disk_permissions_view - WHERE user_id = v_user_id AND entity_id = all_disks_including_snapshots.disk_id)); + WHERE user_id = v_user_id AND entity_id = disk_id)); END; $procedure$ LANGUAGE plpgsql; +DROP TYPE IF EXISTS disks_basic_rs2 CASCADE; +CREATE TYPE disks_basic_rs2 AS (disk_info all_disks_including_snapshots,is_plugged BOOLEAN,is_readonly BOOLEAN); + + DROP TYPE IF EXISTS disks_basic_rs CASCADE; CREATE TYPE disks_basic_rs AS (disk_id UUID,disk_alias varchar(255),size BIGINT); diff --git a/packaging/dbscripts/create_views.sql b/packaging/dbscripts/create_views.sql index ebcc883..0d4fc40 100644 --- a/packaging/dbscripts/create_views.sql +++ b/packaging/dbscripts/create_views.sql @@ -257,6 +257,15 @@ WHERE active IS NULL OR active = TRUE; +CREATE OR REPLACE VIEW all_disks_for_vms +AS +SELECT all_disks_including_snapshots.*, vm_device.is_plugged, vm_device.is_readonly, vm_device.vm_id +FROM all_disks_including_snapshots +JOIN vm_device ON vm_device.device_id = all_disks_including_snapshots.image_group_id +WHERE ((vm_device.snapshot_id IS NULL AND all_disks_including_snapshots.active IS NOT FALSE) +OR vm_device.snapshot_id = all_disks_including_snapshots.vm_snapshot_id); + + CREATE OR REPLACE VIEW storage_domains AS SELECT -- To view, visit http://gerrit.ovirt.org/22506 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: newchange Gerrit-Change-Id: I91d12ee535563204b5d6aaeb64ec6a7c23ea1a81 Gerrit-PatchSet: 1 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Liron Ar <[email protected]> _______________________________________________ Engine-patches mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/engine-patches
