[Engine-devel] Using config values

[Mike Kolesnik]

Regards, 
Mike 

----- Original Message -----

> ----- Original Message -----

> > 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
> 

> > ----- Original Message -----
> 

> > > ----- Original Message -----
> > 
> 

> > > > 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
> > > 
> > 
> 

> > > > ----- Original Message -----
> > > 
> > 
> 

> > > > > 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

If there is such a thing then its a design flaw. 

The only one I see in FeatureSupported class is EnableMACAntiSpoofingFilterRules 
This should be split into 2: 
MACAntiSpoofingFilterRulesSupported - which is used to determine API compatibility 
EnableMACAntiSpoofingFilterRules - which the user can set to determine system behaviour 

I see no reason why to allow user to set EnableMACAntiSpoofingFilterRules=true for 3.0 since it will not work so it's just confusing to him 

> > 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

Again not talking about all the config values, strictly the ones that are (or should be) in FeatureSupported class. 

In case we want to allow user to tweak behavior there, we can always do as I suggested for EnableMACAntiSpoofingFilterRules 

> > > > 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
> > > 
> > 
> 
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.ovirt.org/pipermail/devel/attachments/20131201/a35edfb0/attachment-0001.html>