Ramesh N has posted comments on this change. Change subject: <WIP>engine: add feature comptability check for VDS ......................................................................
Patch Set 6: (3 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 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, a While creating cluster, user should be able to select the features he wants in the cluster. Its his personal choice while creating the cluster. He may not want this feature in the cluster. So he will uncheck the check box for this feature and he can add nodes which doesn't supports this feature also to the cluster. Thats the reason why this field is exposed to the UI. Regarding the logic in the UI side. This gluster37_support feature is applicable only in case of 3.5 cluster. Its not supported in 3.4 cluster and its available by default in 3.6 cluster. We can have some thing like master list of features and maintain it in a table. We can retrieve this feature set from table and show it in the UI. Do u have any other suggestion to achieve this? Regarding updateVdsGroup, Yes I have to add this. Line 83: Line 84: setActionReturnValue(vdsGroup.getId()); Line 85: setSucceeded(true); Line 86: } 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 This is a temp code to test the feature. It will be replaced with more generic code using supportedFeatures fields from VDSM. We won't merge this patch with this temp fix. 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 6: CREATE TABLE cluster_features Line 7: ( Line 8: vds_group_id UUID NOT NULL, Line 9: feature varchar(256) NOT NULL, Line 10: is_enabled BOOLEAN, > is there a chance that a feature which isn't enabled for this cluster shoul Yes. Assume a case where a feature is enabled for a particular cluster during cluster creation. After that user disables the same using edit cluster dialog. In this case we can set this flag as enabled=false. I agree that we can remove the entry instead of marking it as disabled. Does this makes sense?. Line 11: CONSTRAINT PK_cluster_features PRIMARY KEY (vds_group_id, feature), Line 12: FOREIGN KEY (vds_group_id) REFERENCES vds_groups(vds_group_id) ON DELETE CASCADE Line 13: ) ; Line 14: -- 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
