Allon Mureinik has posted comments on this change. Change subject: core,webadmin: Remove of storage pool type ......................................................................
Patch Set 1: Code-Review-1 (23 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 isDcMatchingPosixCompatiblityVersion - the DC you're creating may not be of a sufficiently advanced version. 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? why? 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, that deserves fixing. Line 160: return storageDomain.isLocal() == getStoragePool().isLocal(); Line 161: } Line 162: Line 163: protected boolean isStorageDomainNotInPool(StorageDomain storageDomain) { 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 CDA failure. Line 176: StoragePoolValidator spv = new StoragePoolValidator(getStoragePool()); Line 177: if (storageDomain.getStorageType() == StorageType.GLUSTERFS) { Line 178: return spv.isDcMatchingGlusterCompatiblityVersion().isValid(); Line 179: } 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 "\" 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; 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. 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 getStorageType.isLocal() 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. 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. 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. 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/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... 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 that you'd get isLocal=false. 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? Something like this, probably: if (xmlRpcStruct.containsKey("type")) { sPool.setIIsLocal(StorageType.valueOf(xmlRpcStruct.get("type").toString() != StorageType.LOCAL)); } 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 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, and GLUSTER for 3.3 and above. 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 instances of Boolean.TRUE would be the same object - just use if(bool.booleanValue()). If bool can be null, you should use Boolean.TRUE.equals(bool) 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 DC (or vice versa)? Line 107: Line 108: } Line 109: } http://gerrit.ovirt.org/#/c/23402/1/packaging/dbscripts/create_dwh_views.sql File packaging/dbscripts/create_dwh_views.sql: Line 12 Line 13 Line 14 Line 15 Line 16 You should probably have an isLocal boolean here or something to that effect. Yaniv/Eli? 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 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 patch. 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. 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 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 SET is_local = (storage_pool_type=4) 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: Vered Volansky <[email protected]> Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/engine-patches
