Moti Asayag has posted comments on this change. Change subject: engine: add feature comptability check for VDS ......................................................................
Patch Set 10: (25 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. 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().getClusterFeatureDao(). 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. 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 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 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 be executed for it. 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. 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. 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 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 setters, later followed by another pair of getter-setter. Usually it easier to follow when getter is next to its setter. 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. 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 single parameter). 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() 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 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 to it. 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 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 implementation into its own DAO class. 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 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 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 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 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 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 already too long: please extract it into a method named "updateHostFeatures(...)" 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. and use Version.compareTo() for this condition instead creating the version string. 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 Line 716: } Line 717: } Line 718: Line 719: private static void setRngSupportedSourcesToVds(VDS vds, Map<String, Object> xmlRpcStruct) { -- 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
