[Engine-devel] Using config values

[Itamar Heim]

On 12/01/2013 11:23 AM, Moti Asayag wrote:
>
>
> ----- 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.

lowering the cluster level is not supported.
there is a difference between the configs we expose to users, and the 
configs we prefer to not be changed, but are used to workaround issues 
in some cases.