[Engine-devel] Using config values

Sun Dec 1 09:23:24 UTC 2013

----- Original Message -----
> From: "Omer Frenkel" <ofrenkel at redhat.com>
> To: "Mike Kolesnik" <mkolesni at redhat.com>
> Cc: "engine-devel" <engine-devel at ovirt.org>
> Sent: Sunday, December 1, 2013 11:09:32 AM
> Subject: Re: [Engine-devel] Using config values
> 
> 
> 
> 
> 
> 
> From: "Mike Kolesnik" <mkolesni at redhat.com>
> To: "Omer Frenkel" <ofrenkel at redhat.com>
> Cc: "Kanagaraj" <kmayilsa at redhat.com>, "engine-devel"
> <engine-devel at ovirt.org>
> Sent: Sunday, December 1, 2013 11:01:50 AM
> Subject: Re: [Engine-devel] Using config values
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> From: "Mike Kolesnik" <mkolesni at redhat.com>
> To: "Kanagaraj" <kmayilsa at redhat.com>
> Cc: "engine-devel" <engine-devel at ovirt.org>
> Sent: Sunday, December 1, 2013 8:08:42 AM
> Subject: Re: [Engine-devel] Using config values
> 
> 
> 
> 
> Hi All,
> 
> Hi Kanagaraj,
> 
> 
> 
> 
> The are some issues arising in configurations whenever we move up on the
> versions(3.3 => 3.4), because of the way we store and interpret them.
> 
> Whenever there is a new cluster level, you will need to add a new entry for
> all(most) of the configuration. Mostly a copy paste if you see from 3.2 to
> 3.3, except some CPU/PM type related configurations.
> Better option would be to have the defaul config value in ConfigValues.java
> and the overrides will go to config.sql. In this approach you don't need a
> new entries to config.sql when there is a new cluster level.
> 
> Lets take an exmaple, "SupportForceCreateVG" - This is supported from 3.1
> onwards,
> 
> If you look at config.sql, you will see following entries
> select fn_db_add_config_value('SupportForceCreateVG','false','3.0');
> select fn_db_add_config_value('SupportForceCreateVG','true','3.1');
> select fn_db_add_config_value('SupportForceCreateVG','true','3.2');
> select fn_db_add_config_value('SupportForceCreateVG','true','3.3');
> 
> And in ConfigValues.java
> 
> @TypeConverterAttribute(Boolean.class)
> @DefaultValueAttribute("false")
> SupportForceCreateVG,
> 
> Now if there is 3.4 and 3.5, the user needs to add 2 more entries, which i
> feel is redundant.
> 
> Instead we can make
> 
> @TypeConverterAttribute(Boolean.class)
> @DefaultValueAttribute("true")
> SupportForceCreateVG,
> 
> and have only the following in config.sql
> select fn_db_add_config_value('SupportForceCreateVG','false','3.0');
> 
> if a particular value(for a specific cluster level) is not found in
> Config.sql, the fallback is to use the value available in ConfigValues.java.
> 
> This has already been implemented, there are many "feature supported"
> configurations working like this (for example GlusterSupport).
> 
> I think a more interesting approach would be to move these out of the DB
> since these values don't really hav e a reson to be there.
> Since the entire thing is abstracted by "Gluster/FeatureSupported" classes
> then we can easily change mechanism (of course whatever code is not using it
> can be easily converted to use the mechanism)
> 
> For example a simple enum could do the trick:
> ------------------------------------- EXAMPLE
> -------------------------------------
> /**
> * Convenience class to check if a gluster feature is supported or not in any
> given version.<br>
> * Methods should be named by feature and accept version to check against.
> */
> public class GlusterFeatureSupported {
> /**
> * @param version
> * Compatibility version to check for.
> * @return <code>true</code> if gluster support is enabled, <code>false</code>
> if it's not.
> */
> public static boolean gluster(Version version) {
> return SupportedFeatures.GLUSTER.isSupportedOn(version);
> }
> 
> /**
> * @param version
> * Compatibility version to check for.
> * @return <code>true</code> if gluster heavyweight refresh is enabled,
> <code>false</code> if it's not.
> */
> public static boolean refreshHeavyWeight(Version version) {
> return SupportedFeatures.REFRESH_HEAVYWEIGHT.isSupportedOn(version);
> }
> 
> /* More methods... */
> 
> enum SupportedFeatures {
> GLUSTER(Version.v3_0),
> REFRESH_HEAVYWEIGHT(Version.v3_0, Version.v3_1),
> /* More members */;
> 
> private Set<Version> unsupportedVersions = new HashSet<Version>();
> 
> private SupportedFeatures(Version... versions) {
> unsupportedVersions.addAll(Arrays.asList(versions));
> }
> 
> public boolean isSupportedOn(Version version) {
> return !unsupportedVersions.contains(version);
> }
> }
> ------------------------------------- END EXAMPLE
> -------------------------------------
> 
> Thoughts?
> 
> unless i didn't understand something, this is not good,
> this should stay configurable by the users,
> for example if some user experience some issues with a feature and want to
> turn it off/change the values..
> (not all version configuration are boolean, some are different values to
> different versions, like cpu-list)
> 
> This is for API level compatibility.
> If VDSM doesn't support for example hot plug in 3.1 then the user can't just
> decide that it does and change it.
> Also, this is not changeable by user since it's not exposed by engine-config
> (nor should it be).
> some are exposed
> 
> 
> 
> This is strictly for the engine-VDSM API compatibility, not for other configs
> which are version specific.
> right, but still user should be able to turn features off in case of
> problems,
> or change in some cases (for example it is possible to add support for more
> power management devices, i know it was done by users)
> no reason to block this
> 

In this case you add additional protection level to block user from enabling features
for unsupported cluster levels.

However it sounds more reasonable than requiring the user to use a lower cluster level
which enforces him to give up on other features.

> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> Regards,
> Mike
> 
> 
> 
> Please share your thoughts on this.
> 
> Thanks,
> Kanagaraj
> 
> 
> _______________________________________________
> Engine-devel mailing list
> Engine-devel at ovirt.org
> http://lists.ovirt.org/mailman/listinfo/engine-devel
> 
> 
> 
> 
> _______________________________________________
> Engine-devel mailing list
> Engine-devel at ovirt.org
> http://lists.ovirt.org/mailman/listinfo/engine-devel
> 