Maor Lipchuk 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 done 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" done 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 dif 1.done 2. I will block this for now from the CDA 3. This is already described in the wiki, but I will add it. 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, removed 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 f It's not that relevant, for now we can also have message per VM 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 differ done 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 we can use the same message, it indicates Vms and Tempaltes in the message. I will change the list name accordingly 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 differ done 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 differ done 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 differ done 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 it is related, see removeEntitesFromStorageDomain it should remove the disks as well 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 no, because if I will split this transaction I might get data inconsistency on engine restart 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 no, because if I will split this transaction I might get data inconsistency on engine restart 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 we do this for templates also 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 1. done 2. I prefer not to be dependent on other stored procedures, that is why we have views 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 done 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 > ? will remove -- 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 <[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
