Liron Ar has posted comments on this change. Change subject: core: Support detach Storage Domain containing entities. ......................................................................
Patch Set 26: (8 comments) http://gerrit.ovirt.org/#/c/24286/26/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 84: } Line 85: Line 86: @Override Line 87: protected boolean canDoAction() { Line 88: return canDetachStorageDomainWithVmsAndDisks(getStorageDomain()) && in patch 21 you wrote that you moved this check to "canDetachDomain" as i requested, is it intentionally missing? Line 89: canDetachDomain(getParameters().getDestroyingPool(), Line 90: getParameters().getRemoveLast(), Line 91: isInternalExecution()); Line 92: } http://gerrit.ovirt.org/#/c/24286/26/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 130: } Line 131: return templatesForStorageDomain; Line 132: } Line 133: Line 134: protected List<DiskImage> getdiskImagesForStorageDomain(Guid storageDomainId) { /getdiskImagesForStorageDomain/getDiskImagesForStorageDomain Line 135: if (diskImagesForStorageDomain == null) { Line 136: diskImagesForStorageDomain = getDiskImageDao().getAllForStorageDomain(storageDomainId); Line 137: } Line 138: return diskImagesForStorageDomain; Line 174: if (!validate(new StorageDomainValidator(storageDomain).hasDisksOnRelatedStorageDomains())) { Line 175: return false; Line 176: } Line 177: Line 178: List<VM> vmRelatedToDomain = getVMsForStorageDomain(storageDomain.getId()); in patchset 21 i commented about that - " 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. " was it handled or somehow missed? Line 179: SnapshotsValidator snapshotsValidator = new SnapshotsValidator(); Line 180: boolean succeeded = true; Line 181: List<DiskImage> imagesRelatedToDomain = getdiskImagesForStorageDomain(storageDomain.getId()); Line 182: for (DiskImage diskImage : imagesRelatedToDomain) { Line 179: SnapshotsValidator snapshotsValidator = new SnapshotsValidator(); Line 180: boolean succeeded = true; Line 181: List<DiskImage> imagesRelatedToDomain = getdiskImagesForStorageDomain(storageDomain.getId()); Line 182: for (DiskImage diskImage : imagesRelatedToDomain) { Line 183: if (!diskImage.getActive()) { 1. seems that there's a mistake here - the check is if the image isn't active, while the returned message is for active image. 2. seems like getdiskImagesForStorageDomain() will always return only the active disks for the domain, so it seems like this check is redundant , can you elaborate why it's needed? Line 184: addCanDoActionMessage(VdcBllMessages.ACTION_TYPE_FAILED_CANNOT_REMOVE_ACTIVE_IMAGE); Line 185: succeeded = false; Line 186: } Line 187: } Line 194: } Line 195: if (vm.getVmPoolId() != null) { Line 196: vmsInPool.add(vm.getName()); Line 197: } Line 198: if (!validate(snapshotsValidator.vmNotInPreview(vm.getId()))) { i don't think that this message is good enough as it doesn't include the vm id - if there are 50 vms in preview you'll get 50 "VM is in preview mode" messages which won't help much. Line 199: succeeded = false; Line 200: } Line 201: } Line 202: Line 224: // Check if we have entities related to the Storage Domain. Line 225: List<VM> vmsForStorageDomain = getVMsForStorageDomain(storageDomain.getId()); Line 226: List<VmTemplate> vmTemplatesForStorageDomain = getTemplatesForStorageDomain(storageDomain.getId()); Line 227: List<DiskImage> disksForStorageDomain = getDiskImageDao().getAllForStorageDomain(storageDomain.getId()); Line 228: if (!vmsForStorageDomain.isEmpty() || !vmTemplatesForStorageDomain.isEmpty() || !disksForStorageDomain.isEmpty()) { i'd suggest to move this if inside removeEntitiesFromStorageDomain, so if it will be ever called from somewhere else it won't start the transaction if there's nothing to do. Line 229: removeEntitiesFromStorageDomain(vmsForStorageDomain, vmTemplatesForStorageDomain, storageDomain.getId()); Line 230: } Line 231: } Line 232: http://gerrit.ovirt.org/#/c/24286/26/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 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() { 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()); /s/hasDisksOnRelatedStorageDomains/hasVmsWithDisksOnOtherStorageDomains where do we check it for templates? the method should be here as well Line 108: if (!vms.isEmpty()) { Line 109: List<String> vmNames = new ArrayList<>(); Line 110: for (VM vm : vms) { Line 111: vmNames.add(vm.getName()); http://gerrit.ovirt.org/#/c/24286/26/backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/StorageDomainDAO.java File backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/StorageDomainDAO.java: Line 164: * Line 165: * @param id Line 166: * the storage domain Line 167: */ Line 168: void removeEntitesFromStorageDomain(Guid id); if you can please add a test. Line 169: Line 170: /** Line 171: * Retrieves all storage domains for the specified connection. Line 172: * @param storagePoolId -- 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: 26 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
