Ayal Baron has posted comments on this change. Change subject: core,webadmin: Remove of storage pool type ......................................................................
Patch Set 1: (17 comments) http://gerrit.ovirt.org/#/c/23402/1/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/ISCSIStorageHelper.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/ISCSIStorageHelper.java: Line 219: storagePoolId); Line 220: } Line 221: Line 222: @Override Line 223: public boolean disconnectStorageFromLunByVdsId(StorageDomain storageDomain, Guid vdsId, LUNs lun) { what's the extra space here for? Line 224: return runConnectionStorageToDomain(storageDomain, vdsId, VDSCommandType.DisconnectStorageServer.getValue(), Line 225: lun, Guid.Empty); Line 226: } Line 227: http://gerrit.ovirt.org/#/c/23402/1/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 156: return returnValue; Line 157: } Line 158: Line 159: protected boolean isStorageDomainTypeCorrect(StorageDomain storageDomain) { Line 160: return storageDomain.isLocal() == getStoragePool().isLocal(); how is this check not a compatibility check? i.e. why is this not in isStorageDomainTypeCompatibleWithPool? Line 161: } Line 162: Line 163: protected boolean isStorageDomainNotInPool(StorageDomain storageDomain) { Line 164: boolean returnValue = false; Line 171: } Line 172: return returnValue; Line 173: } Line 174: Line 175: protected boolean isStorageDomainTypeCompatibleWithPool(StorageDomain storageDomain) { No need for the word 'Type'. Also, the check is compatibility of storage domain with DC. Name should be: 'isStorageDomainCompatibleWithDC' Line 176: StoragePoolValidator spv = new StoragePoolValidator(getStoragePool()); Line 177: if (storageDomain.getStorageType() == StorageType.GLUSTERFS) { Line 178: return spv.isDcMatchingGlusterCompatiblityVersion().isValid(); Line 179: } Line 191: && checkStorageDomainSharedStatusNotLocked(storageDomain) Line 192: && ((storageDomain.getStorageDomainType() == StorageDomainType.ISO || storageDomain.getStorageDomainType() == Line 193: StorageDomainType.ImportExport) || isStorageDomainNotInPool(storageDomain)) Line 194: && isStorageDomainTypeCorrect(storageDomain) && isStorageDomainTypeCompatibleWithPool(storageDomain) Line 195: && (isMixedTypesAllowedOnPool() || !isStoragePoolContainsOtherTypes(storageDomain)); this condition is impossible to follow. Please split into separate 'if's as you did in isStorageDomainTypeCompatibleWithPool Line 196: } Line 197: Line 198: Line 199: // \TODO: Should be removed when 3.0 compatibility will not be supported, for now we are blocking the possibility Line 195: && (isMixedTypesAllowedOnPool() || !isStoragePoolContainsOtherTypes(storageDomain)); Line 196: } Line 197: Line 198: Line 199: // \TODO: Should be removed when 3.0 compatibility will not be supported, for now we are blocking the possibility s/\\// Line 200: // to mix NFS domains with block domains on 3.0 pools since block domains on 3.0 pools can be in V2 format while NFS Line 201: // domains on 3.0 can only be in V1 format Line 202: protected boolean isMixedTypesAllowedOnPool() { Line 203: return getStoragePool().getcompatibility_version().compareTo(Version.v3_0) > 0; Line 198: Line 199: // \TODO: Should be removed when 3.0 compatibility will not be supported, for now we are blocking the possibility Line 200: // to mix NFS domains with block domains on 3.0 pools since block domains on 3.0 pools can be in V2 format while NFS Line 201: // domains on 3.0 can only be in V1 format Line 202: protected boolean isMixedTypesAllowedOnPool() { s/isMixedTypesAllowedOnPool/isMixedTypesAllowedOnPoolInPool/' i.e. in vs. on However, I'd call it 'poolSupportsMixedTypes', but there might be a convention about using the to be verb or something. Also, please change the use of the word 'pool' with DC all over the patch. Line 203: return getStoragePool().getcompatibility_version().compareTo(Version.v3_0) > 0; Line 204: } Line 205: Line 206: public boolean isStoragePoolContainsOtherTypes(StorageDomain storageDomain) { Line 202: protected boolean isMixedTypesAllowedOnPool() { Line 203: return getStoragePool().getcompatibility_version().compareTo(Version.v3_0) > 0; Line 204: } Line 205: Line 206: public boolean isStoragePoolContainsOtherTypes(StorageDomain storageDomain) { s/isStoragePoolContainsOtherTypes/isMixedTypeDC/ Line 207: boolean isBlockDomain = storageDomain.getStorageType().isBlockDomain(); Line 208: Line 209: List<StorageDomain> poolDomains = getStorageDomainDAO().getAllForStoragePool(getStoragePoolId()); Line 210: for (StorageDomain currSD : poolDomains) { http://gerrit.ovirt.org/#/c/23402/1/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/StoragePoolValidator.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/StoragePoolValidator.java: Line 22: this.storagePool = storagePool; Line 23: } Line 24: Line 25: /** Line 26: * Checks in case the DC compatibility version is compatible with Posix type. s/in case/that/ s/compatible with Posix type/supports Posix domains/ Line 27: * In case there is mismatch, a proper canDoAction message will be added Line 28: * Line 29: * @return The result of the validation Line 30: */ Line 27: * In case there is mismatch, a proper canDoAction message will be added Line 28: * Line 29: * @return The result of the validation Line 30: */ Line 31: public ValidationResult isDcMatchingPosixCompatiblityVersion() { s/isDcMatchingPosixCompatibilityVersion/isPosixSupportedInDc/ Line 32: if (!storagePool.isLocal() && Line 33: !Config.<Boolean> getValue(ConfigValues.PosixStorageEnabled, storagePool.getcompatibility_version().toString())) { Line 34: return new ValidationResult(VdcBllMessages.DATA_CENTER_POSIX_STORAGE_NOT_SUPPORTED_IN_CURRENT_VERSION); Line 35: } Line 36: return ValidationResult.VALID; Line 37: } Line 38: Line 39: /** Line 40: * Checks in case the DC compatibility version is compatible with Gluster type. same Line 41: * In case there is mismatch, a proper canDoAction message will be added Line 42: * Line 43: * @return true if the version matches Line 44: */ Line 41: * In case there is mismatch, a proper canDoAction message will be added Line 42: * Line 43: * @return true if the version matches Line 44: */ Line 45: public ValidationResult isDcMatchingGlusterCompatiblityVersion() { same Line 46: if (!storagePool.isLocal() && Line 47: !Config.<Boolean> getValue(ConfigValues.GlusterFsStorageEnabled, storagePool.getcompatibility_version().toString())) { Line 48: return new ValidationResult(VdcBllMessages.DATA_CENTER_GLUSTER_STORAGE_NOT_SUPPORTED_IN_CURRENT_VERSION); Line 49: } http://gerrit.ovirt.org/#/c/23402/1/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/UpdateStoragePoolCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/UpdateStoragePoolCommand.java: Line 55: Line 56: @Override Line 57: protected void executeCommand() { Line 58: updateQuotaCache(); Line 59: if (_oldStoragePool.getStatus() == StoragePoolStatus.Up && getMasterDomain() != null) { this is an unrelated bug fix. it should be in a separate patch Line 60: if (!StringUtils.equals(_oldStoragePool.getName(), getStoragePool().getName())) { Line 61: runVdsCommand(VDSCommandType.SetStoragePoolDescription, Line 62: new SetStoragePoolDescriptionVDSCommandParameters( Line 63: getStoragePool().getId(), getStoragePool().getName()) Line 106: TransactionSupport.executeInScope(TransactionScopeOption.RequiresNew, Line 107: new TransactionMethod<Object>() { Line 108: @Override Line 109: public Object runInTransaction() { Line 110: getStoragePoolDAO().updatePartial(storagePool); this change is also not related Line 111: if (getMasterDomain() != null) { Line 112: updateMemberDomainsFormat(targetFormat); Line 113: } Line 114: return null; Line 114: return null; Line 115: } Line 116: }); Line 117: Line 118: if (_oldStoragePool.getStatus() == StoragePoolStatus.Up && getMasterDomain() != null) { same Line 119: try { Line 120: // No need to worry about "reupgrading" as VDSM will silently ignore Line 121: // the request. Line 122: runVdsCommand(VDSCommandType.UpgradeStoragePool, http://gerrit.ovirt.org/#/c/23402/1/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 187: return versions; Line 188: } Line 189: Line 190: private static StoragePool createNewStoragePool() { Line 191: StoragePool pool = createBasicPool(); no need to: pool.setIsLocal(false); ? Line 192: pool.setcompatibility_version(VERSION_1_1); Line 193: return pool; Line 194: } Line 195: http://gerrit.ovirt.org/#/c/23402/1/backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/StoragePoolDAODbFacadeImpl.java File backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/StoragePoolDAODbFacadeImpl.java: Line 130: MapSqlParameterSource parameterSource = getCustomMapSqlParameterSource() Line 131: .addValue("description", pool.getdescription()) Line 132: .addValue("free_text_comment", pool.getComment()) Line 133: .addValue("id", pool.getId()) Line 134: .addValue("is_local", pool.isLocal()) why not keep the same order as before? (which was consistent between save and update) Line 135: .addValue("name", pool.getName()) Line 136: .addValue("status", pool.getStatus()) Line 137: .addValue("master_domain_version", Line 138: pool.getmaster_domain_version()) http://gerrit.ovirt.org/#/c/23402/1/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/dataprovider/AsyncDataProvider.java File frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/dataprovider/AsyncDataProvider.java: Line 3066: if (isLocalType) { Line 3067: return version.compareTo(new Version(3, 0)) >= 0; Line 3068: } Line 3069: else { Line 3070: return version.compareTo(new Version(3, 0)) >= 0; what's the difference between the two checks? they look exactly the same to me Line 3071: } Line 3072: } Line 3073: Line 3074: public static int getClusterDefaultMemoryOverCommit() { -- To view, visit http://gerrit.ovirt.org/23402 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: If29a4ecb9aa284b57e9f5218ca50cf4287452e3e Gerrit-PatchSet: 1 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Tal Nisan <[email protected]> Gerrit-Reviewer: Allon Mureinik <[email protected]> Gerrit-Reviewer: Ayal Baron <[email protected]> Gerrit-Reviewer: Daniel Erez <[email protected]> Gerrit-Reviewer: Federico Simoncelli <[email protected]> Gerrit-Reviewer: Liron Ar <[email protected]> Gerrit-Reviewer: Maor Lipchuk <[email protected]> Gerrit-Reviewer: Sergey Gotliv <[email protected]> Gerrit-Reviewer: Vered Volansky <[email protected]> Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/engine-patches
