Liron Ar has posted comments on this change. Change subject: core: Support detach Storage Domain containing entities. ......................................................................
Patch Set 21: (17 comments) http://gerrit.ovirt.org/#/c/24286/21/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 59: log.info(" Detach storage domain: after detach in vds"); Line 60: disconnectAllHostsInPool(); Line 61: Line 62: log.info(" Detach storage domain: after disconnect storage"); Line 63: detachStorageDomainWithEntities(getStorageDomain()); this should be done in the transaction below in line 64 otherwise if you crush between, you'll be left with the storage domain still attached but the entities would be already "removed" from the regular tables Line 64: TransactionSupport.executeInNewTransaction(new TransactionMethod<Object>() { Line 65: @Override Line 66: public Object runInTransaction() { Line 67: StoragePoolIsoMap mapToRemove = getStorageDomain().getStoragePoolIsoMapData(); Line 90: } Line 91: Line 92: @Override Line 93: protected boolean canDoAction() { Line 94: return canDetachStorageDomainWithVmsAndDisks(getStorageDomain()) this check should be inside "canDetachDomain" Line 95: && canDetachDomain(getParameters().getDestroyingPool(), Line 96: getParameters().getRemoveLast(), Line 97: isInternalExecution()); Line 98: } http://gerrit.ovirt.org/#/c/24286/21/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/StorageHandlingCommandBase.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/StorageHandlingCommandBase.java: Line 147: protected boolean canDetachStorageDomainWithVmsAndDisks(StorageDomain storageDomain) { Line 148: if (!validate(new StorageDomainValidator(storageDomain).hasDisksOnRelatedStorageDomains())) { Line 149: return false; Line 150: } Line 151: List<VM> vmRelatedToDomain = getVmDAO().getAllForStorageDomain(storageDomain.getId()); 1. please have this in a lazy getter, it's loaded multiple times in the different methods here. 2. there's an issue here, if there's a VM with disk snapshot that reside on the domain for detach, it won't be returned on that list. this means that eventually we'll still have the disk snapshot attached to this vm, while we can attach the domain to other storage pool. we should describe in the commit message the behavior for disk snapshots/shareable disks along this feature (as they aren't contained in the ovf). Line 152: SnapshotsValidator snapshotsValidator = new SnapshotsValidator(); Line 153: boolean succeeded = true; Line 154: List<String> vmsDeleteProtected = new ArrayList<>(); Line 155: List<String> vmsNotDown = new ArrayList<>(); Line 159: if (vm.isDeleteProtected()) { Line 160: vmsDeleteProtected.add(vm.getName()); Line 161: } Line 162: // TODO : should change the message, Add also if disk is not active Line 163: if (vm.getStatus() != VMStatus.Down) { this check seems to be redundant, if the storage domain is on maintenance (so we can detach it), we can't a have vm which is not down with the disk plugged to it (we won't be able to run it) Line 164: vmsNotDown.add(vm.getName()); Line 165: } Line 166: if (vm.getVmPoolId() != null) { Line 167: vmsInPool.add(vm.getName()); Line 166: if (vm.getVmPoolId() != null) { Line 167: vmsInPool.add(vm.getName()); Line 168: } Line 169: if (!validate(snapshotsValidator.vmNotInPreview(vm.getId()))) { Line 170: succeeded = false; let's have a message for all of those vms same as the other messages here for consistency Line 171: } Line 172: } Line 173: Line 174: List<VmTemplate> templatesRelatedToDomain = getVmTemplateDAO().getAllForStorageDomain(storageDomain.getId()); Line 170: succeeded = false; Line 171: } Line 172: } Line 173: Line 174: List<VmTemplate> templatesRelatedToDomain = getVmTemplateDAO().getAllForStorageDomain(storageDomain.getId()); please have this in a lazy getter, it's loaded multiple times in the different methods here. Line 175: for (VmTemplate vmTemplate : templatesRelatedToDomain) { Line 176: if (vmTemplate.isDeleteProtected()) { Line 177: vmsDeleteProtected.add(vmTemplate.getName()); Line 178: } Line 173: Line 174: List<VmTemplate> templatesRelatedToDomain = getVmTemplateDAO().getAllForStorageDomain(storageDomain.getId()); Line 175: for (VmTemplate vmTemplate : templatesRelatedToDomain) { Line 176: if (vmTemplate.isDeleteProtected()) { Line 177: vmsDeleteProtected.add(vmTemplate.getName()); this should be on a different list with a different message Line 178: } Line 179: } Line 180: Line 181: if (!vmsDeleteProtected.isEmpty()) { Line 197: } Line 198: Line 199: protected void detachStorageDomainWithEntities(StorageDomain storageDomain) { Line 200: // Check if we have entities related to the Storage Domain. Line 201: List<VM> vmsForStorageDomain = getVmDAO().getAllForStorageDomain(storageDomain.getId()); please have this in a lazy getter, it's loaded multiple times in the different methods here. Line 202: List<VmTemplate> vmTemplatesForStorageDomain = getVmTemplateDAO().getAllForStorageDomain(storageDomain.getId()); Line 203: List<DiskImage> disksForStorageDomain = getDiskImageDao().getAllForStorageDomain(storageDomain.getId()); Line 204: if (!vmsForStorageDomain.isEmpty() || !vmTemplatesForStorageDomain.isEmpty() || !disksForStorageDomain.isEmpty()) { Line 205: removeEntitiesFromStorageDomain(vmsForStorageDomain, vmTemplatesForStorageDomain, storageDomain.getId()); Line 198: Line 199: protected void detachStorageDomainWithEntities(StorageDomain storageDomain) { Line 200: // Check if we have entities related to the Storage Domain. Line 201: List<VM> vmsForStorageDomain = getVmDAO().getAllForStorageDomain(storageDomain.getId()); Line 202: List<VmTemplate> vmTemplatesForStorageDomain = getVmTemplateDAO().getAllForStorageDomain(storageDomain.getId()); please have this in a lazy getter, it's loaded multiple times in the different methods here. Line 203: List<DiskImage> disksForStorageDomain = getDiskImageDao().getAllForStorageDomain(storageDomain.getId()); Line 204: if (!vmsForStorageDomain.isEmpty() || !vmTemplatesForStorageDomain.isEmpty() || !disksForStorageDomain.isEmpty()) { Line 205: removeEntitiesFromStorageDomain(vmsForStorageDomain, vmTemplatesForStorageDomain, storageDomain.getId()); Line 206: } Line 199: protected void detachStorageDomainWithEntities(StorageDomain storageDomain) { Line 200: // Check if we have entities related to the Storage Domain. Line 201: List<VM> vmsForStorageDomain = getVmDAO().getAllForStorageDomain(storageDomain.getId()); Line 202: List<VmTemplate> vmTemplatesForStorageDomain = getVmTemplateDAO().getAllForStorageDomain(storageDomain.getId()); Line 203: List<DiskImage> disksForStorageDomain = getDiskImageDao().getAllForStorageDomain(storageDomain.getId()); please have this in a lazy getter, it's loaded multiple times in the different methods here. Line 204: if (!vmsForStorageDomain.isEmpty() || !vmTemplatesForStorageDomain.isEmpty() || !disksForStorageDomain.isEmpty()) { Line 205: removeEntitiesFromStorageDomain(vmsForStorageDomain, vmTemplatesForStorageDomain, storageDomain.getId()); Line 206: } Line 207: } Line 200: // Check if we have entities related to the Storage Domain. Line 201: List<VM> vmsForStorageDomain = getVmDAO().getAllForStorageDomain(storageDomain.getId()); Line 202: List<VmTemplate> vmTemplatesForStorageDomain = getVmTemplateDAO().getAllForStorageDomain(storageDomain.getId()); Line 203: List<DiskImage> disksForStorageDomain = getDiskImageDao().getAllForStorageDomain(storageDomain.getId()); Line 204: if (!vmsForStorageDomain.isEmpty() || !vmTemplatesForStorageDomain.isEmpty() || !disksForStorageDomain.isEmpty()) { the removeEntitiesFromStorageDomain() method isn't related to disks on the storage domain, so you can remove it from the if and to not load it at all if it's used only here Line 205: removeEntitiesFromStorageDomain(vmsForStorageDomain, vmTemplatesForStorageDomain, storageDomain.getId()); Line 206: } Line 207: } Line 208: Line 211: */ Line 212: private void removeEntitiesFromStorageDomain(final List<VM> vmsForStorageDomain, Line 213: final List<VmTemplate> vmTemplatesForStorageDomain, Line 214: final Guid storageDomainId) { Line 215: TransactionSupport.executeInNewTransaction(new TransactionMethod<Object>() { we should move the check from line 204 here and start this transaction only if vmsForStorageDomain isn't empty Line 216: @Override Line 217: public Object runInTransaction() { Line 218: for (VM vm : vmsForStorageDomain) { Line 219: getUnregisteredOVFDataDao().insertOVFDataForEntities( Line 230: vm.getVdsGroupCompatibilityVersion().getValue(), Line 231: storageDomainId, Line 232: null); Line 233: } Line 234: we should move the check from line 204 here and start this transaction only if vmTemplatesForStorageDomain isn't empty Line 235: for (VmTemplate vmTemplate : vmTemplatesForStorageDomain) { Line 236: getUnregisteredOVFDataDao().insertOVFDataForEntities( Line 237: vmTemplate.getId(), Line 238: vmTemplate.getName(), http://gerrit.ovirt.org/#/c/24286/21/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/validator/StorageDomainValidator.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/validator/StorageDomainValidator.java: Line 101: } Line 102: Line 103: // TODO: Validation should be removed once we support detach of storage domain with VMs containing multiple disks Line 104: // resides on different storage domains. Line 105: public ValidationResult hasDisksOnRelatedStorageDomains() { /s/hasDisksOnRelatedStorageDomains/hasVmsWithDisksOnOtherStorageDomains what about templates with disks on multiple domains? Line 106: // If there are VMs with disks on other storage domain we should block the operation. Line 107: List<VM> vms = getDbFacade().getVmDao().getAllVMsWithDisksOnOtherStorageDomain(storageDomain.getId()); Line 108: if (!vms.isEmpty()) { Line 109: List<String> vmNames = new ArrayList<>(); http://gerrit.ovirt.org/#/c/24286/21/packaging/dbscripts/disk_images_sp.sql File packaging/dbscripts/disk_images_sp.sql: Line 159: AS $procedure$ Line 160: BEGIN Line 161: RETURN QUERY SELECT image_storage_view.* Line 162: FROM images_storage_domain_view image_storage_view INNER JOIN storage_for_image_view ON image_storage_view.image_guid = storage_for_image_view.image_id Line 163: WHERE active AND image_storage_view.storage_id = v_storage_domain_id; 1. there's no need for the join here 2. please make use of GetSnapshotsByStorageDomainId() stored procedure which already gives you that needed functionallity (return from it just the active records) Line 164: END; $procedure$ Line 165: LANGUAGE plpgsql; Line 166: Line 167: http://gerrit.ovirt.org/#/c/24286/21/packaging/dbscripts/storages_sp.sql File packaging/dbscripts/storages_sp.sql: Line 691: delete FROM image_storage_domain_map where storage_domain_id = v_storage_domain_id; Line 692: delete FROM images where image_guid in (select image_id from STORAGE_DOMAIN_MAP_TABLE); Line 693: delete FROM vm_interface where vmt_guid in(select vm_guid from TEMPLATES_IDS_TEMPORARY_TABLE); Line 694: delete FROM permissions where object_id in (select vm_guid from TEMPLATES_IDS_TEMPORARY_TABLE); Line 695: delete FROM permissions where object_id = v_storage_domain_id; please move this also to Force_Delete_storage_domain stored procedure Line 696: delete FROM vm_static where vm_guid in(select vm_id as vm_guid from VM_IDS_TEMPORARY_TABLE where entity_type <> 'TEMPLATE'); Line 697: Line 698: -- Delete pools and snapshots of pools based on templates from the storage domain to be removed Line 699: delete FROM snapshots where vm_id in (select vm_guid FROM vm_static where vmt_guid in (select vm_guid from TEMPLATES_IDS_TEMPORARY_TABLE)); http://gerrit.ovirt.org/#/c/24286/21/packaging/dbscripts/vms_sp.sql File packaging/dbscripts/vms_sp.sql: Line 1028 Line 1029 Line 1030 Line 1031 Line 1032 ? -- 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: 21 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Maor Lipchuk <mlipc...@redhat.com> Gerrit-Reviewer: Allon Mureinik <amure...@redhat.com> Gerrit-Reviewer: Daniel Erez <de...@redhat.com> Gerrit-Reviewer: Eli Mesika <emes...@redhat.com> Gerrit-Reviewer: Liran Zelkha <lzel...@redhat.com> Gerrit-Reviewer: Liron Ar <lara...@redhat.com> Gerrit-Reviewer: Maor Lipchuk <mlipc...@redhat.com> Gerrit-Reviewer: Sergey Gotliv <sgot...@redhat.com> Gerrit-Reviewer: Tal Nisan <tni...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
