Ramesh N has posted comments on this change. Change subject: engine: add feature comptability check for VDS ......................................................................
Patch Set 10: (27 comments) https://gerrit.ovirt.org/#/c/39756/10/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/GetClusterFeaturesByVersionAndCategoryQuery.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/GetClusterFeaturesByVersionAndCategoryQuery.java: Line 15: clusterFeaturesList > the List shouldn't be part of the name. clusterFeatures is enough. Done Line 12: Line 13: @Override Line 14: protected void executeQueryCommand() { Line 15: Set<AdditionalClusterFeature> clusterFeaturesList = Line 16: getDbFacade().getClusterFeatureDao() > you should be able to use injection here instead of getDbFacade().getCluste Done Line 17: .getClusterFeaturesForVersionAndCategory(getParameters().getVersion().getValue(), Line 18: getParameters().getCategory()); Line 19: getQueryReturnValue().setReturnValue(clusterFeaturesList); Line 20: } https://gerrit.ovirt.org/#/c/39756/10/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/HandleVdsVersionCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/HandleVdsVersionCommand.java: Line 87: } Line 88: Line 89: private void checkClusterAdditionalFeaturesSupported(VDSGroup cluster, VDS vds) { Line 90: ClusterFeatureDao clusterFeatureDao = getClusterFeatureDao(); Line 91: VdsDynamicDAO vdsDynamicDao = getVdsDynamicDao(); > please replace the 2 above with injection. Done Line 92: Set<SupportedAdditionalClusterFeature> featuresSupportedByCluster = Line 93: clusterFeatureDao.getSupportedFeaturesByClusterId(cluster.getId()); Line 94: Set<String> featuresSupportedByVds = Line 95: vdsDynamicDao.getSupportedHostFeaturesByVdsId(vds.getId()); Line 88: Line 89: private void checkClusterAdditionalFeaturesSupported(VDSGroup cluster, VDS vds) { Line 90: ClusterFeatureDao clusterFeatureDao = getClusterFeatureDao(); Line 91: VdsDynamicDAO vdsDynamicDao = getVdsDynamicDao(); Line 92: Set<SupportedAdditionalClusterFeature> featuresSupportedByCluster = > i'd name it clusterSupportedFeatures Done Line 93: clusterFeatureDao.getSupportedFeaturesByClusterId(cluster.getId()); Line 94: Set<String> featuresSupportedByVds = Line 95: vdsDynamicDao.getSupportedHostFeaturesByVdsId(vds.getId()); Line 96: for (SupportedAdditionalClusterFeature feature : featuresSupportedByCluster) { Line 90: ClusterFeatureDao clusterFeatureDao = getClusterFeatureDao(); Line 91: VdsDynamicDAO vdsDynamicDao = getVdsDynamicDao(); Line 92: Set<SupportedAdditionalClusterFeature> featuresSupportedByCluster = Line 93: clusterFeatureDao.getSupportedFeaturesByClusterId(cluster.getId()); Line 94: Set<String> featuresSupportedByVds = > and this hostSupportedFeatures Done Line 95: vdsDynamicDao.getSupportedHostFeaturesByVdsId(vds.getId()); Line 96: for (SupportedAdditionalClusterFeature feature : featuresSupportedByCluster) { Line 97: if (feature.isEnabled() && !featuresSupportedByVds.contains(feature.getFeature().getName())) { Line 98: Map<String, String> customLogValues = new HashMap<>(); https://gerrit.ovirt.org/#/c/39756/10/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/UpdateVdsGroupCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/UpdateVdsGroupCommand.java: Line 154: Set<SupportedAdditionalClusterFeature> supportedFeatures = Line 155: clusterFeatureDao.getSupportedFeaturesByClusterId(getVdsGroup().getId()); Line 156: supportedFeatures.removeAll(getVdsGroup().getAddtionalFeaturesSupported()); Line 157: // Disable the features which are not selected in update cluster Line 158: if (!supportedFeatures.isEmpty()) { > the surrounding if is not needed. if supportedFeatures, no iteration will b Done Line 159: for (SupportedAdditionalClusterFeature feature : supportedFeatures) { Line 160: if (feature.isEnabled()) { Line 161: feature.setEnabled(false); Line 162: clusterFeatureDao.updateSupportedClusterFeature(feature); Line 462: Line 463: private Set<SupportedAdditionalClusterFeature> getAdditionalClusterFeaturesAdded() { Line 464: // Lets not modify the existing collection. Hence creating a new hashset. Line 465: Set<SupportedAdditionalClusterFeature> featuresSupported = Line 466: new HashSet<SupportedAdditionalClusterFeature>(getVdsGroup().getAddtionalFeaturesSupported()); > i don't think redeclaring the type on the right side is needed. Done Line 467: featuresSupported.removeAll(clusterFeatureDao.getSupportedFeaturesByClusterId(getVdsGroup().getId())); Line 468: return featuresSupported; Line 469: } Line 470: Line 483: } Line 484: } Line 485: Line 486: return true; Line 487: > please remove this empty line. Done Line 488: } Line 489: Line 490: private boolean validateManagementNetworkAttachement() { Line 491: final Network managementNetwork; https://gerrit.ovirt.org/#/c/39756/10/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/AuditLogType.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/AuditLogType.java: Line 447: VDS > s/VDS/HOST Done https://gerrit.ovirt.org/#/c/39756/10/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/AdditionalClusterFeature.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/AdditionalClusterFeature.java: Line 30: this.version = version; Line 31: this.description = description; Line 32: this.category = category; Line 33: } Line 34: > this is a bit un-organized: there are first 4 getters, followed by 4 setter Done Line 35: public Guid getId() { Line 36: return id; Line 37: } Line 38: Line 75: @Override Line 76: public int hashCode() { Line 77: final int prime = 31; Line 78: int result = 1; Line 79: result = prime * result + ((id == null) ? 0 : id.hashCode()); > please use java.util.Objects.hash() which simplifies this method. That simplifies a lot. Thank you for the suggestion. Line 80: result = prime * result + ((name == null) ? 0 : name.hashCode()); Line 81: result = prime * result + ((version == null) ? 0 : version.hashCode()); Line 82: result = prime * result + ((description == null) ? 0 : description.hashCode()); Line 83: result = prime * result + ((category == null) ? 0 : category.hashCode()); https://gerrit.ovirt.org/#/c/39756/10/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/SupportedAdditionalClusterFeature.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/SupportedAdditionalClusterFeature.java: Line 15: Line 16: public SupportedAdditionalClusterFeature() { Line 17: } Line 18: Line 19: public SupportedAdditionalClusterFeature(Guid clusterId, > please format (if line is longer than 120, each line should contain a singl Done Line 20: boolean enabled, AdditionalClusterFeature feature) { Line 21: this.clusterId = clusterId; Line 22: this.setEnabled(enabled); Line 23: this.feature = feature; Line 50: @Override Line 51: public int hashCode() { Line 52: final int prime = 31; Line 53: int result = 1; Line 54: result = prime * result + ((clusterId == null) ? 0 : clusterId.hashCode()); > see previous file's comment regarding usage of Objects.hash() Done Line 55: result = prime * result + ((feature == null) ? 0 : feature.hashCode()); Line 56: result = prime * result + (enabled ? 0 : 1); Line 57: return result; Line 58: } https://gerrit.ovirt.org/#/c/39756/10/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/queries/GetClusterFeaturesByVersionAndCategoryParameters.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/queries/GetClusterFeaturesByVersionAndCategoryParameters.java: Line 19: public void setVersion(Version version) { Line 20: this.version = version; Line 21: } Line 22: Line 23: public GetClusterFeaturesByVersionAndCategoryParameters(Version version, ApplicationMode category) { > please move the c'tor next to the other c'tor Done Line 24: this.version = version; Line 25: this.category = category; Line 26: setRefresh(false); Line 27: } https://gerrit.ovirt.org/#/c/39756/10/backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/VdsDynamicDAO.java File backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/VdsDynamicDAO.java: Line 42: * @param id Line 43: * @param cpuFlags Line 44: */ Line 45: void updateCpuFlags(Guid id, String cpuFlags); Line 46: > please name the id field hostId where capable below and align the java doc Done Line 47: /** Line 48: * Add the given feature to the supported_host_features table. Line 49: * Line 50: * @param feature Line 51: vdsId > please fix Done Line 48: * Add the given feature to the supported_host_features table. Line 49: * Line 50: * @param feature Line 51: * @param vdsId Line 52: */ > please move the feature related apis into a new DAO interface and its imple Moved all the new APIs into SupportedHostFeatureDao Line 53: void addSupportedHostFeature(Guid id, String feature); Line 54: Line 55: /** Line 56: * Add all the given features to the supported_host_features table. Line 55: /** Line 56: * Add all the given features to the supported_host_features table. Line 57: * Line 58: * @param features Line 59: * @param vdsId > please fix Done Line 60: */ Line 61: void addAllSupportedHostFeature(Guid id, Set<String> features); Line 62: Line 63: /** Line 73: * Line 74: * @param id Line 75: * @return Line 76: */ Line 77: Set<String> getSupportedHostFeaturesByVdsId(Guid id); > s/getSupportedHostFeaturesByVdsId/getSupportedHostFeaturesByHostId Done https://gerrit.ovirt.org/#/c/39756/10/backend/manager/modules/dal/src/main/resources/bundles/AppErrors.properties File backend/manager/modules/dal/src/main/resources/bundles/AppErrors.properties: Line 470: Hosts which doesn't > s/Hosts which doesn't/hosts which don't Done Line 467: -Please move Hosts with lower CPU to maintenance first. Line 468: VDS_GROUP_CANNOT_UPDATE_COMPATIBILITY_VERSION_WITH_LOWER_HOSTS=Cannot change Cluster Compatibility Version to higher version when there are active Hosts with lower version.\n\ Line 469: -Please move Hosts with lower version to maintenance first. Line 470: VDS_GROUP_CANNOT_UPDATE_SUPPORTED_FEATURES_WITH_LOWER_HOSTS=Cannot enable new Cluster feature when there are active Hosts which doesn't support the selected feature.\n\ Line 471: -Please move Hosts with lower version to maintenance first. > s/Hosts/hosts Done Line 472: VDS_GROUP_CANNOT_UPDATE_VDS_UP=Cannot change Cluster.Trying to connect Cluster to Data Center with Hosts that are up. Line 473: VDS_GROUP_CANNOT_ADD_COMPATIBILITY_VERSION_WITH_LOWER_STORAGE_POOL=Cannot add Cluster with Compatibility Version that is lower than the Data Center Compatibility Version.\n\ Line 474: -Please upgrade your Cluster to a later Compatibility version first. Line 475: VDS_GROUP_CPU_TYPE_CANNOT_BE_NULL=Cannot add Cluster. CPU type must be specified https://gerrit.ovirt.org/#/c/39756/10/backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/VdsManager.java File backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/VdsManager.java: Line 657: processVdsFeaturesReported > processHostFeaturesReported Done https://gerrit.ovirt.org/#/c/39756/10/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 707: vds.setLiveMergeSupport(AssignBoolValue(xmlRpcStruct, VdsProperties.liveMergeSupport)); Line 708: } else { Line 709: vds.setLiveMergeSupport(false); Line 710: } Line 711: > since we expect this to grow, and due to the fact that this method is alrea I will change this with actual code where we will look for the field 'additionalFeatures' and all the entries in the field. Line 712: // Its an work around until the supportedFeatures field is supported by VDSM Line 713: if (RpmVersionUtils.compareRpmParts(vds.getGlusterVersion().getMajor() + "." Line 714: + vds.getGlusterVersion().getMinor(), "3.7") >= 0) { Line 715: vds.getSupportedFeatures().add("gluster37_features"); Line 710: } Line 711: Line 712: // Its an work around until the supportedFeatures field is supported by VDSM Line 713: if (RpmVersionUtils.compareRpmParts(vds.getGlusterVersion().getMajor() + "." Line 714: + vds.getGlusterVersion().getMinor(), "3.7") >= 0) { > please move "3.7" into a constant in Version class. Done Line 715: vds.getSupportedFeatures().add("gluster37_features"); Line 716: } Line 717: } Line 718: Line 711: Line 712: // Its an work around until the supportedFeatures field is supported by VDSM Line 713: if (RpmVersionUtils.compareRpmParts(vds.getGlusterVersion().getMajor() + "." Line 714: + vds.getGlusterVersion().getMinor(), "3.7") >= 0) { Line 715: vds.getSupportedFeatures().add("gluster37_features"); > please replace hard-coded string with VdsProperties.GLUSTER37 Done Line 716: } Line 717: } Line 718: Line 719: private static void setRngSupportedSourcesToVds(VDS vds, Map<String, Object> xmlRpcStruct) { https://gerrit.ovirt.org/#/c/39756/10/packaging/dbscripts/upgrade/03_06_1290_add_cluster_features_table.sql File packaging/dbscripts/upgrade/03_06_1290_add_cluster_features_table.sql: Line 12: -- 3. All 1111 1111 Line 13: Line 14: CREATE TABLE cluster_features Line 15: ( Line 16: feature_id UUID NOT NULL, > this should be aligned with the next field, so please either: Done Line 17: name VARCHAR(256) NOT NULL, Line 18: version VARCHAR(40), Line 19: category INTEGER NOT NULL, Line 20: description TEXT, Line 37: Line 38: CREATE TABLE supported_host_features Line 39: ( Line 40: host_id UUID NOT NULL, Line 41: feature VARCHAR(256) NOT NULL, > s/feature/feature_name Done Line 42: CONSTRAINT PK_supported_host_features PRIMARY KEY (host_id, feature), Line 43: FOREIGN KEY (host_id) REFERENCES vds_static(vds_id) ON DELETE CASCADE Line 44: ) ; Line 45: -- To view, visit https://gerrit.ovirt.org/39756 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Icba02b189a169bc676e0c5f47f7aaf394f0b49a6 Gerrit-PatchSet: 10 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Ramesh N <rnach...@redhat.com> Gerrit-Reviewer: Eli Mesika <emes...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Kanagaraj M <kmayi...@redhat.com> Gerrit-Reviewer: Moti Asayag <masa...@redhat.com> Gerrit-Reviewer: Oved Ourfali <oourf...@redhat.com> Gerrit-Reviewer: Ramesh N <rnach...@redhat.com> Gerrit-Reviewer: Sahina Bose <sab...@redhat.com> Gerrit-Reviewer: Shubhendu Tripathi <shtri...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
