Maor Lipchuk has posted comments on this change. Change subject: core[WIP]: Support detach Storage Domain with disks. ......................................................................
Patch Set 6: (4 comments) http://gerrit.ovirt.org/#/c/24286/6/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/DetachStorageDomainFromPoolCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/DetachStorageDomainFromPoolCommand.java: Line 105: // Check if we have entities related to the Storage Domain. Line 106: List<VM> vmsForStorageDomain = getVmDAO().getAllForStorageDomain(getStorageDomain().getId()); Line 107: List<VmTemplate> vmTemplatesForStorageDomain = getVmTemplateDAO().getAllForStorageDomain(getStorageDomain().getId()); Line 108: List<DiskImage> disksForStorageDomain = getDiskImageDao().getAllForStorageDomain(getStorageDomain().getId()); Line 109: if (!disksForStorageDomain.isEmpty() || vmsForStorageDomain.isEmpty() || vmTemplatesForStorageDomain.isEmpty()) { > IMHO we should have the check after each query, and before the next query. since we are in the execute phase the fetching from the DB should not have any meaningful influence. I tried to avoid nested/multiple if conditions, since I think it will make the code be less readable that way. I don't mind changing this though, so I will fixed that in the next patch. Line 110: // TODO : Save the OVF of vms/Tempaltes in a table. Line 111: // Remove all related entities of the Storage Domain from the DB. Line 112: TransactionSupport.executeInNewTransaction(new TransactionMethod<Object>() { Line 113: @Override Line 146: } Line 147: log.errorFormat("Failed to remove Storage Domain {0} since there are vms which contain disks related to other storage domains {1}.", Line 148: getStorageDomain().getId(), Line 149: StringUtils.join(vmNames, ",")); Line 150: addCanDoActionMessage(String.format("$vms %1$s", StringUtils.join(vmNames, ","))); > If I see correctly, the only use of vmNames is this join. Wouldn't it be be The string utils has already implementation for using comma separations for list of names. I want to avoid handling a logic for putting the comma for one VM or multiple VMs. Line 151: addCanDoActionMessage(VdcBllMessages.VDS_GROUP_CANNOT_DETACH_DATA_DOMAIN_FROM_LOCAL_STORAGE); Line 152: return false; Line 153: } Line 154: http://gerrit.ovirt.org/#/c/24286/6/packaging/dbscripts/create_views.sql File packaging/dbscripts/create_views.sql: Line 625: vm_static.serial_number_policy as serial_number_policy, vm_static.custom_serial_number as custom_serial_number Line 626: FROM vm_static INNER JOIN Line 627: vm_dynamic ON vm_static.vm_guid = vm_dynamic.vm_guid INNER JOIN Line 628: vm_static AS vm_templates ON vm_static.vmt_guid = vm_templates.vm_guid INNER JOIN Line 629: vm_statistics ON vm_static.vm_guid = vm_statistics.vm_guid LEFT OUTER JOIN > Why? Will we have VMs with no VDS_Groups? This was the implementation which was introduced in the first patch set, it should be changed now. removed it Line 630: vds_groups ON vm_static.vds_group_id = vds_groups.vds_group_id LEFT OUTER JOIN Line 631: storage_pool ON vm_static.vds_group_id = vds_groups.vds_group_id Line 632: and vds_groups.storage_pool_id = storage_pool.id LEFT OUTER JOIN Line 633: quota ON vm_static.quota_id = quota.id LEFT OUTER JOIN http://gerrit.ovirt.org/#/c/24286/6/packaging/dbscripts/vms_sp.sql File packaging/dbscripts/vms_sp.sql: Line 1034: ON i.image_guid = ISD.image_id) vms_with_disks_on_storage_domain ON vms.vm_guid = vms_with_disks_on_storage_domain.vm_guid Line 1035: INNER JOIN vm_device vd ON vd.vm_id = vms.vm_guid Line 1036: INNER JOIN images i ON i.image_group_id = vd.device_id Line 1037: INNER JOIN image_storage_domain_map on i.image_guid = image_storage_domain_map.image_id Line 1038: WHERE image_storage_domain_map.storage_domain_id != v_storage_domain_id; > Seems a very complicated and time-consuming query, can you explain please w sure, when detaching a storage domain which contains VMs, we should block it if there are VMs which contains disks on other storage domains. this Stored procedure should return the VMs which has disks on multiple Storage Domain, so we can use those vm names in the CDA message If you can suggest of a better way to implement it, I will change it. Line 1039: END; $procedure$ Line 1040: LANGUAGE plpgsql; Line 1041: Line 1042: Create or replace FUNCTION updateClusterForDetachedVMs(v_storage_domain_id UUID) -- To view, visit http://gerrit.ovirt.org/24286 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I971fe6acd4a2667a09487c5e1108cf7c759587f1 Gerrit-PatchSet: 6 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Maor Lipchuk <[email protected]> Gerrit-Reviewer: Allon Mureinik <[email protected]> Gerrit-Reviewer: Daniel Erez <[email protected]> Gerrit-Reviewer: Eli Mesika <[email protected]> Gerrit-Reviewer: Liran Zelkha <[email protected]> Gerrit-Reviewer: Liron Ar <[email protected]> Gerrit-Reviewer: Maor Lipchuk <[email protected]> Gerrit-Reviewer: Sergey Gotliv <[email protected]> Gerrit-Reviewer: Tal Nisan <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/engine-patches
