Tal Nisan has posted comments on this change. Change subject: core,webadmin: Remove of storage pool type ......................................................................
Patch Set 1: (30 comments) http://gerrit.ovirt.org/#/c/23402/1/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/AddEmptyStoragePoolCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/AddEmptyStoragePoolCommand.java: Line 87 Line 88 Line 89 Line 90 Line 91 > You'd still want to check isDcMatchingGlusterCompatiblityVersion and isDcM You're adding an empty pool which is typeless, this part was used to a case where you add a pool of type gluster with a version that does not support it, instead the checks are now performed when attaching a gluster/posix domain to a pool 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) { > huh? By mistake guys, don't be so dramatic.. ;) 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 155: } Line 156: return returnValue; Line 157: } Line 158: Line 159: protected boolean isStorageDomainTypeCorrect(StorageDomain storageDomain) { > Not sure if I'd do it in this patch or not, but this is a HORRIBLE name, th After the feature is merged along with a nice big test for that class Line 160: return storageDomain.isLocal() == getStoragePool().isLocal(); Line 161: } Line 162: Line 163: protected boolean isStorageDomainNotInPool(StorageDomain storageDomain) { 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? This class is complicated enough without me changing the methods, will refactor after the feature is merged and also add a nice thorough test as it definitely lacks one 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) { > Better to return a ValidationResult, so you could add the messages to the C Done Line 176: StoragePoolValidator spv = new StoragePoolValidator(getStoragePool()); Line 177: if (storageDomain.getStorageType() == StorageType.GLUSTERFS) { Line 178: return spv.isDcMatchingGlusterCompatiblityVersion().isValid(); Line 179: } Line 171: } Line 172: return returnValue; Line 173: } Line 174: Line 175: protected boolean isStorageDomainTypeCompatibleWithPool(StorageDomain storageDomain) { > No need for the word 'Type'. Done 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 a Done 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 > please remove the "\" Done 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/' Done 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/ Done 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/ Not my words to begin with ;) but done 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: */ 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: It's false by default but I'll add it just for readability's sake 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/common/src/main/java/org/ovirt/engine/core/common/businessentities/AuditLog.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/AuditLog.java: Line 41: private int customEventId; Line 42: private int eventFloodInSec; Line 43: private String customData; Line 44: private boolean external; Line 45: private boolean deleted; > Should also be removed. Done Line 46: private String storagePoolType; Line 47: private String compatibilityVersion; Line 48: private String quotaEnforcementType; Line 49: private String callStack; http://gerrit.ovirt.org/#/c/23402/1/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/StorageDomain.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/StorageDomain.java: Line 247: getStorageStaticData().setStorageType(storageType); Line 248: } Line 249: Line 250: public boolean isLocal() { Line 251: return getStorageType() == StorageType.LOCALFS; > I'd add an isLocal() method to the StorageType enum and have this return ge Done Line 252: } Line 253: Line 254: private StorageDomainSharedStatus storageDomainSharedStatus; Line 255: http://gerrit.ovirt.org/#/c/23402/1/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/StoragePool.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/StoragePool.java: Line 192: result = prime * result + masterDomainVersion; Line 193: result = prime * result + ((name == null) ? 0 : name.hashCode()); Line 194: result = prime * result + ((recovery_mode == null) ? 0 : recovery_mode.hashCode()); Line 195: result = prime * result + ((spmVdsId == null) ? 0 : spmVdsId.hashCode()); Line 196: result = prime * result + ((status == null) ? 0 : status.hashCode()); > You should consider the isLocal property too. Done Line 197: result = prime * result + ((storagePoolFormatType == null) ? 0 : storagePoolFormatType.hashCode()); Line 198: result = prime * result + ((quotaEnforcementType == null) ? 0 : quotaEnforcementType.hashCode()); Line 199: return result; Line 200: } Line 219: && masterDomainVersion == other.masterDomainVersion Line 220: && ObjectUtils.objectsEqual(name, other.name) Line 221: && recovery_mode == other.recovery_mode Line 222: && ObjectUtils.objectsEqual(spmVdsId, other.spmVdsId) Line 223: && status == other.status > You should consider the isLocal property too. Done Line 224: && ObjectUtils.objectsEqual(storagePoolFormatType, other.storagePoolFormatType) Line 225: && quotaEnforcementType == other.quotaEnforcementType); Line 226: } Line 227: http://gerrit.ovirt.org/#/c/23402/1/backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dal/dbbroker/auditloghandling/AuditLogableBase.java File backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dal/dbbroker/auditloghandling/AuditLogableBase.java: Line 79: private String origin = "oVirt"; Line 80: private int customEventId = -1; Line 81: private int eventFloodInSec = 30; Line 82: private String customData = ""; Line 83: private boolean external = false; > This should also be removed. Done Line 84: private String storagePoolType; Line 85: private String compatibilityVersion; Line 86: private String quotaEnforcementType; Line 87: private Guid quotaIdForLog; 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 a The order doesn't really mean anything but there was no reason to change it in the first place indeed so fixed 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/backend/manager/modules/dal/src/test/resources/fixtures.xml File backend/manager/modules/dal/src/test/resources/fixtures.xml: Line 446: <row> Line 447: <value>6d849ebf-755f-4552-ad09-9a090cda105d</value> Line 448: <value>rhel6.iscsi</value> Line 449: <value></value> Line 450: <value>false</value> > Should also be applied to the rest of the entries... Done Line 451: <value>1</value> Line 452: <value>1127</value> Line 453: <value>787c5473-9a82-4191-930b-814db3451531</value> Line 454: <value>2.3</value> http://gerrit.ovirt.org/#/c/23402/1/backend/manager/modules/restapi/jaxrs/src/main/java/org/ovirt/engine/api/restapi/resource/BackendDataCentersResource.java File backend/manager/modules/restapi/jaxrs/src/main/java/org/ovirt/engine/api/restapi/resource/BackendDataCentersResource.java: Line 54: validateEnum(StorageType.class, dataCenter.getStorageType().toUpperCase()); Line 55: } Line 56: else if(!dataCenter.isSetLocal()) { Line 57: validateParameters(dataCenter, "local"); Line 58: } > I'm missing the logic to treat legacy scripts that set type=NFS in a way t It's in DataCenterMapper.java Line 59: StoragePool entity = map(dataCenter); Line 60: return performCreate(VdcActionType.AddEmptyStoragePool, Line 61: new StoragePoolManagementParameter(entity), Line 62: new QueryIdResolver<Guid>(VdcQueryType.GetStoragePoolById, IdQueryParameters.class)); http://gerrit.ovirt.org/#/c/23402/1/backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/vdsbroker/VdsBrokerObjectsBuilder.java File backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/vdsbroker/VdsBrokerObjectsBuilder.java: Line 80 Line 81 Line 82 Line 83 Line 84 > wouldn't you want to use the type property to do set the isLocal property? Not sure that it actually returns something, but I added it just in case http://gerrit.ovirt.org/#/c/23402/1/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/Cloner.java File frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/Cloner.java: Line 265: Line 266: obj.setdescription(instance.getdescription()); Line 267: obj.setComment(instance.getComment()); Line 268: obj.setId(instance.getId()); Line 269: obj.setName(instance.getName()); > you should also clone the isLocal property Done Line 270: obj.setStatus(instance.getStatus()); Line 271: Line 272: obj.setmaster_domain_version(instance.getmaster_domain_version()); Line 273: obj.setLVER(instance.getLVER()); 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 3067: return version.compareTo(new Version(3, 0)) >= 0; Line 3068: } Line 3069: else { Line 3070: return version.compareTo(new Version(3, 0)) >= 0; Line 3071: } > This is not true. We don't support only support POSIX for 3.1 and above, an We do not support adding a gluster/posix storage domain by the version, the check now is for shared/local by the pool type, both checks are the same to make it easier to change and to distinguish between local/shared supported versions Line 3072: } Line 3073: Line 3074: public static int getClusterDefaultMemoryOverCommit() { Line 3075: return 100; http://gerrit.ovirt.org/#/c/23402/1/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/hosts/ConfigureLocalStorageModel.java File frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/hosts/ConfigureLocalStorageModel.java: Line 148: }); Line 149: Line 150: // Set the storage type to be Local. Line 151: for (Boolean bool : getDataCenter().getStoragePoolType().getItems()) { Line 152: if (bool == Boolean.TRUE) { > Boolean is (unfortunately) not an Enum - three is now guarantee that two in Correct, nice catch, just used if (bool), that does the trick Btw, if I recall correctly it worked since in the Javascript code which it translates to they are equal though they are objects Line 153: getDataCenter().getStoragePoolType().setSelectedItem(bool); Line 154: break; Line 155: } Line 156: } http://gerrit.ovirt.org/#/c/23402/1/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/storage/NewEditStorageModelBehavior.java File frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/storage/NewEditStorageModelBehavior.java: Line 102: dataCenter.getStatus() != StoragePoolStatus.Uninitialized; Line 103: Line 104: return isExportDomain && canAttachExportDomain || Line 105: isIsoDomain && canAttachIsoDomain || Line 106: isDataDomain; > shouldn't you check it's not a LOCAL domain being attached to a non-local D It's already checked in line 85 Line 107: Line 108: } Line 109: } http://gerrit.ovirt.org/#/c/23402/1/packaging/dbscripts/create_views.sql File packaging/dbscripts/create_views.sql: Line 937: ---------------------------------------------- Line 938: CREATE OR REPLACE VIEW storage_pool_with_storage_domain Line 939: Line 940: AS Line 941: SELECT storage_pool.id as id, storage_pool.name as name, storage_pool.description as description, storage_pool.free_text_comment as free_text_comment, storage_pool.status as status, > You're missing some isLocal property here Done Line 942: storage_pool.master_domain_version as master_domain_version, storage_pool.spm_vds_id as spm_vds_id, storage_pool.compatibility_version as compatibility_version, storage_pool._create_date as _create_date, Line 943: storage_pool._update_date as _update_date, storage_pool_iso_map.storage_id as storage_id, storage_pool_iso_map.storage_pool_id as storage_pool_id, Line 944: storage_domain_static.storage_type as storage_type, storage_domain_static.storage_domain_type as storage_domain_type, Line 945: storage_domain_static.storage_domain_format_type as storage_domain_format_type, http://gerrit.ovirt.org/#/c/23402/1/packaging/dbscripts/storages_sp.sql File packaging/dbscripts/storages_sp.sql: Line 547 Line 548 Line 549 Line 550 Line 551 > Should be part of a previous patch in the series - see comments on that pat Yeah, I just moved it backwards ;) http://gerrit.ovirt.org/#/c/23402/1/packaging/dbscripts/upgrade/03_04_0470_remove_storage_pool_type.sql File packaging/dbscripts/upgrade/03_04_0470_remove_storage_pool_type.sql: Line 1: select fn_db_add_column('storage_pool', 'is_local', 'boolean'); Line 2: Line 3: update storage_pool set is_local = false; > This is redundant - remove it. Done Line 4: Line 5: create or replace function __temp_update_storage_domain_is_local() returns void Line 6: as $function$ Line 7: begin Line 2: Line 3: update storage_pool set is_local = false; Line 4: Line 5: create or replace function __temp_update_storage_domain_is_local() returns void Line 6: as $function$ > Not quite sure why you need this function It didn't work otherwise, doesn't really add too much anyway.. Line 7: begin Line 8: if (exists (select 1 from information_schema.columns where table_name ilike 'storage_pool' and column_name ilike 'storage_pool_type')) then Line 9: update storage_pool set is_local=true where storage_pool_type=4; Line 10: end if; Line 5: create or replace function __temp_update_storage_domain_is_local() returns void Line 6: as $function$ Line 7: begin Line 8: if (exists (select 1 from information_schema.columns where table_name ilike 'storage_pool' and column_name ilike 'storage_pool_type')) then Line 9: update storage_pool set is_local=true where storage_pool_type=4; > UPDATE storage_pool Hehe, DBAs... Done Line 10: end if; Line 11: end; $function$ Line 12: language plpgsql; Line 13: -- 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: Tal Nisan <[email protected]> Gerrit-Reviewer: Vered Volansky <[email protected]> Gerrit-Reviewer: Yaniv Dary <[email protected]> Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/engine-patches
