Moti Asayag has posted comments on this change. Change subject: <WIP>engine: add feature comptability check for VDS ......................................................................
Patch Set 6: (15 comments) https://gerrit.ovirt.org/#/c/39756/6/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddVdsGroupCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddVdsGroupCommand.java: Line 53: } Line 54: Line 55: @Override Line 56: protected void executeCommand() { Line 57: VDSGroup vdsGroup = getVdsGroup(); s/vdsGroup/cluster Line 58: vdsGroup.setArchitecture(getArchitecture()); Line 59: Line 60: checkMaxMemoryOverCommitValue(); Line 61: vdsGroup.setDetectEmulatedMachine(true); Line 73: getCpuProfileDao().save(CpuProfileHelper.createCpuProfile(getParameters().getVdsGroup().getId(), Line 74: getParameters().getVdsGroup().getName())); Line 75: } Line 76: Line 77: if (vdsGroup.getFeaturesSupported() != null && !vdsGroup.getFeaturesSupported().isEmpty()) { is those 2 are repeated called together, please add another method to VdsGroup and use it instead: public boolean hasFeaturesSupported() { return getFeaturesSupported() != null && !getFeatureSupported().isEmpty()) } Line 78: for(ClusterFeature feature:vdsGroup.getFeaturesSupported()){ Line 79: feature.setVdsGroupId(vdsGroup.getId()); Line 80: } Line 81: getClusterFeatureDao().saveAll(vdsGroup.getFeaturesSupported()); Line 74: getParameters().getVdsGroup().getName())); Line 75: } Line 76: Line 77: if (vdsGroup.getFeaturesSupported() != null && !vdsGroup.getFeaturesSupported().isEmpty()) { Line 78: for(ClusterFeature feature:vdsGroup.getFeaturesSupported()){ please format the code. the eclipse formatter suggests more spaces in that statement. Line 79: feature.setVdsGroupId(vdsGroup.getId()); Line 80: } Line 81: getClusterFeatureDao().saveAll(vdsGroup.getFeaturesSupported()); Line 82: } Line 77: if (vdsGroup.getFeaturesSupported() != null && !vdsGroup.getFeaturesSupported().isEmpty()) { Line 78: for(ClusterFeature feature:vdsGroup.getFeaturesSupported()){ Line 79: feature.setVdsGroupId(vdsGroup.getId()); Line 80: } Line 81: getClusterFeatureDao().saveAll(vdsGroup.getFeaturesSupported()); please use injection instead. Line 82: } Line 83: Line 84: setActionReturnValue(vdsGroup.getId()); Line 85: setSucceeded(true); Line 78: for(ClusterFeature feature:vdsGroup.getFeaturesSupported()){ Line 79: feature.setVdsGroupId(vdsGroup.getId()); Line 80: } Line 81: getClusterFeatureDao().saveAll(vdsGroup.getFeaturesSupported()); Line 82: } i'm not sure why the engine should expose that functionality to the user, and not to detect it itself. In the following patch there is some business logic on the UI side. This isn't the place for it on the same manner you wouldn't expose it in the restapi. I think that we should have that logic to determine the supported features here. In addition, what about UpdateVdsGroupCommand ? what if cluster is moved between dc's ? no need to update the cluster supported features ? Line 83: Line 84: setActionReturnValue(vdsGroup.getId()); Line 85: setSucceeded(true); Line 86: } https://gerrit.ovirt.org/#/c/39756/6/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/ClusterFeature.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/ClusterFeature.java: Line 1: package org.ovirt.engine.core.common.businessentities; Line 2: Line 3: import java.io.Serializable; Line 4: Line 5: import org.ovirt.engine.core.common.utils.ObjectUtils; ObjectUtils.objectsEqual is redundant. you should use java.util.Objects.equals() Line 6: import org.ovirt.engine.core.compat.Guid; Line 7: Line 8: public class ClusterFeature implements Serializable { Line 9: Line 9: Line 10: private static final long serialVersionUID = -8387930346405670858L; Line 11: private String feature; Line 12: private boolean enabled;; Line 13: private Guid vdsGroupId; s/vdsGroupId/clusterId and same for the getter/setter. Line 14: Line 15: public ClusterFeature() { Line 16: } Line 17: Line 15: public ClusterFeature() { Line 16: } Line 17: Line 18: public ClusterFeature(String feature, boolean enabled, Guid vdsGroupId) { Line 19: super(); the call to super is redundant. Line 20: this.feature = feature; Line 21: this.enabled = enabled; Line 22: this.vdsGroupId = vdsGroupId; Line 23: } Line 65: return true; Line 66: } Line 67: } Line 68: return false; Line 69: } please implement toString using ToStringBuilder which simplifies debug. https://gerrit.ovirt.org/#/c/39756/6/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/VDSGroup.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/VDSGroup.java: Line 86: private Guid clusterPolicyId; Line 87: Line 88: private String clusterPolicyName; Line 89: Line 90: private List<ClusterFeature> featuresSupported; Set would be more appropriate Line 91: Line 92: @ValidUri(message = "VALIDATION.VDS_GROUP.SPICE_PROXY.HOSTNAME_OR_IP", Line 93: groups = { CreateEntity.class, UpdateEntity.class }) Line 94: @Size(max = BusinessEntitiesDefinitions.SPICE_PROXY_ADDR_SIZE) https://gerrit.ovirt.org/#/c/39756/6/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/VdsDynamic.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/VdsDynamic.java: Line 162: Line 163: private String maintenanceReason; Line 164: Line 165: // Comma separated values of all the features supported Line 166: private String supportedFeatures; why not Set as well ? Line 167: Line 168: public VdsDynamic() { Line 169: rpmVersion = new RpmVersion(); Line 170: libvirtVersion = new RpmVersion(); https://gerrit.ovirt.org/#/c/39756/6/backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/ClusterFeatureDao.java File backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/ClusterFeatureDao.java: Line 20: * Line 21: * @param vdsGroupId Line 22: * @return Line 23: */ Line 24: public List<ClusterFeature> getByVdsGroupId(Guid vdsGroupId); s/getByVdsGroupId/getByClusterId Line 25: Line 26: /** Line 27: * Update the ClusterFeature. Line 28: * https://gerrit.ovirt.org/#/c/39756/6/backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/ClusterFeatureDaoImpl.java File backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/ClusterFeatureDaoImpl.java: Line 20: private static final RowMapper<ClusterFeature> clusterFeatureRowMapper = new ClusterFeatureRowMapper(); Line 21: Line 22: private static final class ClusterFeatureRowMapper implements RowMapper<ClusterFeature> { Line 23: @Override Line 24: public ClusterFeature mapRow(ResultSet rs, int rowNum) please merge lines Line 25: throws SQLException { Line 26: ClusterFeature feature = new ClusterFeature(); Line 27: feature.setVdsGroupId(getGuidDefaultEmpty(rs, "vds_group_id")); Line 28: feature.setEnabled(rs.getBoolean("is_enabled")); https://gerrit.ovirt.org/#/c/39756/6/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 708: } else { Line 709: vds.setLiveMergeSupport(false); Line 710: } Line 711: Line 712: // Its an work around until the supportedFeatures field is supported by VDSM for this specific case, wouldn't we like to persist this info only if this host resides on gluster cluster ? Line 713: if (RpmVersionUtils.compareRpmParts(vds.getGlusterVersion().getMajor() + "." Line 714: + vds.getGlusterVersion().getMinor(), "3.7") >= 0) { Line 715: vds.setSupportedFeatures("gluster37_features"); Line 716: } https://gerrit.ovirt.org/#/c/39756/6/packaging/dbscripts/upgrade/03_06_1250_add_cluster_features_table.sql File packaging/dbscripts/upgrade/03_06_1250_add_cluster_features_table.sql: Line 2: -- table cluster_feature_mappings Line 3: -- Maintans the list of features required from vdsm to add the host to the specific vds_gorup Line 4: -- ---------------------------------------------------------------------- Line 5: Line 6: CREATE TABLE cluster_features > please rename to gluster_cluster_features why do we need to distinguish between gluster to non-gluster features ? Line 7: ( Line 8: vds_group_id UUID NOT NULL, Line 9: feature varchar(256) NOT NULL, Line 10: is_enabled BOOLEAN, -- 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: 6 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Ramesh N <[email protected]> Gerrit-Reviewer: Eli Mesika <[email protected]> Gerrit-Reviewer: Kanagaraj M <[email protected]> Gerrit-Reviewer: Moti Asayag <[email protected]> Gerrit-Reviewer: Oved Ourfali <[email protected]> Gerrit-Reviewer: Ramesh N <[email protected]> Gerrit-Reviewer: Sahina Bose <[email protected]> Gerrit-Reviewer: Shubhendu Tripathi <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/engine-patches
