Maor Lipchuk 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 r yes, I forgot to submit another comment of mine. I prefer to keep this separated from the canDetachDomain for now, and I will integrate this into the canDetachDomain in the second patch set, since part of the validation done in the canDetachStorageDomainWithVmsAndDisks will be irrelevant. 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 done 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 - After talked c2p the validation is not relevant and line 181 should be removed. 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 acti After talked p2p, All this validation should be removed 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 ok 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 i done 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 included template 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. done 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
