Allon Mureinik has posted comments on this change. Change subject: core: Limitation addition for downgrading DC's compatible version. ......................................................................
Patch Set 4: Code-Review-1 (8 comments) http://gerrit.ovirt.org/#/c/37478/4/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 329: } Line 330: return returnValue; Line 331: } Line 332: Line 333: protected boolean isStorageDomainCompatibleWithDC(StorageDomainStatic domainStatic, boolean doValidate) { I don't like the C-style passing of a boolean to change behavior. I'd suggest to just return the validation result, and the caller can decide whether to do validate() or not. Line 334: StoragePoolValidator spv = new StoragePoolValidator(getStoragePool()); Line 335: if (domainStatic.getStorageType() == StorageType.GLUSTERFS) { Line 336: ValidationResult result = spv.isGlusterSupportedInDC(); Line 337: return (doValidate==true) ? validate(result) : result.isValid(); Line 344: Line 345: return true; Line 346: } Line 347: Line 348: protected boolean isStorageDomainCompatibleWithDC(StorageDomain storageDomain, boolean doValidate) { This looks redundant - why can't the called just call .getStorageDomainStaticData() himself? Line 349: return isStorageDomainCompatibleWithDC(storageDomain.getStorageStaticData(), doValidate); Line 350: } Line 351: Line 352: protected boolean checkDomainCanBeAttached(StorageDomain storageDomain) { Line 398: } Line 399: return false; Line 400: } Line 401: Line 402: public boolean isMixedTypeDC(StorageDomain storageDomain) { Here too - why can't the caller do this? Line 403: return isMixedTypeDC(storageDomain.getStorageStaticData()); Line 404: } Line 405: Line 406: Line 546: if (domainStatic.getStorageDomainType().isIsoOrImportExportDomain()) { Line 547: return true; Line 548: } Line 549: Line 550: if (storagePool != null) { > I'm not sure if the scenario is valid, but before using the VersionStorageF Preferred == the version all the domains will be upgraded to. There is no such thing as a v2 file domain (or a v1 block domain, for that matter), so I'm fine with failing it, instead of sending it all the way to VDSM and having it fail there. Line 551: if (VersionStorageFormatUtil.getPreferredForVersion(storagePool.getcompatibility_version(), Line 552: domainStatic.getStorageType()).compareTo(domainStatic.getStorageFormat()) < 0) { Line 553: return false; Line 554: } Line 555: } Line 556: return true; Line 557: } Line 558: Line 559: protected boolean isStorageDomainFormatCorrectForDC(StorageDomain storageDomain, StoragePool storagePool) { Here too - why doesn't the caller do this? Line 560: return isStorageDomainFormatCorrectForDC(storageDomain.getStorageStaticData(), storagePool); Line 561: } Line 562: Line 563: protected Set<StorageFormatType> getSupportedStorageFormatSet(Version version) { http://gerrit.ovirt.org/#/c/37478/4/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/storage/StorageHandlingCommandBaseTest.java File backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/storage/StorageHandlingCommandBaseTest.java: Line 143: public void testAttachOnValidDomain() { Line 144: StorageDomain storageDomain = createValidStorageDomain(); Line 145: assertTrue("Attaching a valid domain to attach was failed", cmd.checkDomainCanBeAttached(storageDomain)); Line 146: } Line 147: Please don't mix cleanups with logic changes Line 148: /** Line 149: * Mixed types are not allowed on version lower than V3.4, test that attempting to attach a domain of different type Line 150: * than what already exists in the data center will fail for versions 3.0 to 3.3 inclusive Line 151: */ Line 177: else { Line 178: assertFalse("Attaching an ISCSI domain to a pool with NFS domain with no mixed type allowed succeeded, version: " + version, cmd.checkDomainCanBeAttached(domainToAttach)); Line 179: CanDoActionTestUtils.assertCanDoActionMessages("Attaching an ISCSI domain to a pool with NFS domain with no mixed type failed with the wrong message", cmd, Line 180: VdcBllMessages.ACTION_TYPE_FAILED_MIXED_STORAGE_TYPES_NOT_ALLOWED); Line 181: } here too Line 182: } Line 183: Line 184: @Test Line 185: public void testAttachPosixCompatibility() { http://gerrit.ovirt.org/#/c/37478/4/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/storage/UpdateStoragePoolCommandTest.java File backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/storage/UpdateStoragePoolCommandTest.java: Line 176: addManagementNetworkToPool(); Line 177: setupNetworkValidator(true); Line 178: assertTrue(cmd.canDoAction()); Line 179: } Line 180: please separate cleanups from logic changes Line 181: @Test Line 182: public void lowerVersionMgmtNetworkNonSupportedFeatures() { Line 183: storagePoolWithLowerVersion(); Line 184: addNonDefaultClusterToPool(); -- To view, visit http://gerrit.ovirt.org/37478 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ib97488c705a32fe73b390ef226b1ad19a3784b76 Gerrit-PatchSet: 4 Gerrit-Project: ovirt-engine Gerrit-Branch: ovirt-engine-3.5 Gerrit-Owner: Amit Aviram <aavi...@redhat.com> Gerrit-Reviewer: Allon Mureinik <amure...@redhat.com> Gerrit-Reviewer: Liron Aravot <lara...@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
