[Engine-devel] FeatureSupported and compatibility versions

Mike Kolesnik mkolesni at redhat.com
Tue Apr 2 09:17:31 UTC 2013


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

> On 03/27/2013 05:48 PM, Mike Kolesnik wrote:

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

> > > On 03/20/2013 08:20 PM, Yair Zaslavsky wrote:
> > 
> 

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

> > > > > From: "Shireesh Anjal" <sanjal at redhat.com> To: "Mike Kolesnik"
> > > > > <mkolesni at redhat.com> Cc: engine-devel at ovirt.org Sent: Wednesday,
> > > > > March
> > > > > 20,
> > > > > 2013 4:47:08 PM
> > > > 
> > > 
> > 
> 
> > > > > Subject: Re: [Engine-devel] FeatureSupported and compatibility
> > > > 
> > > 
> > 
> 
> > > > > versions
> > > > 
> > > 
> > 
> 

> > > > > On 03/18/2013 01:11 PM, Shireesh Anjal wrote:
> > > > 
> > > 
> > 
> 

> > > > > > On 03/18/2013 12:59 PM, Mike Kolesnik wrote:
> > > > > 
> > > > 
> > > 
> > 
> 

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

> > > > > > > > Hi all,
> > > > > > > 
> > > > > > 
> > > > > 
> > > > 
> > > 
> > 
> 

> > > > > > > > The current mechanism in oVirt to check whether a feature is
> > > > > > > 
> > > > > > 
> > > > > 
> > > > 
> > > 
> > 
> 
> > > > > > > > supported
> > > > > > > 
> > > > > > 
> > > > > 
> > > > 
> > > 
> > 
> 
> > > > > > > > in a particular compatibility version is to use the
> > > > > > > 
> > > > > > 
> > > > > 
> > > > 
> > > 
> > 
> 
> > > > > > > > FeatureSupported
> > > > > > > 
> > > > > > 
> > > > > 
> > > > 
> > > 
> > 
> 
> > > > > > > > class. e.g.
> > > > > > > 
> > > > > > 
> > > > > 
> > > > 
> > > 
> > 
> 

> > > > > > > > FeatureSupported.networkLinking(getVm().getVdsGroupCompatibilityVersion())
> > > > > > > 
> > > > > > 
> > > > > 
> > > > 
> > > 
> > 
> 

> > > > > > > > Checks whether the "network linking" feature is supported for
> > > > > > > 
> > > > > > 
> > > > > 
> > > > 
> > > 
> > 
> 
> > > > > > > > the
> > > > > > > 
> > > > > > 
> > > > > 
> > > > 
> > > 
> > 
> 
> > > > > > > > the
> > > > > > > 
> > > > > > 
> > > > > 
> > > > 
> > > 
> > 
> 
> > > > > > > > VM's cluster compatibility version. This internally checks
> > > > > > > 
> > > > > > 
> > > > > 
> > > > 
> > > 
> > 
> 
> > > > > > > > whether
> > > > > > > 
> > > > > > 
> > > > > 
> > > > 
> > > 
> > 
> 
> > > > > > > > the
> > > > > > > 
> > > > > > 
> > > > > 
> > > > 
> > > 
> > 
> 
> > > > > > > > value of the corresponding config (NetworkLinkingSupported) for
> > > > > > > 
> > > > > > 
> > > > > 
> > > > 
> > > 
> > 
> 
> > > > > > > > the
> > > > > > > 
> > > > > > 
> > > > > 
> > > > 
> > > 
> > 
> 
> > > > > > > > given compatibility version is true/false.
> > > > > > > 
> > > > > > 
> > > > > 
> > > > 
> > > 
> > 
> 

> > > > > > > > I'm not sure if this is a good idea, since a feature is
> > > > > > > 
> > > > > > 
> > > > > 
> > > > 
> > > 
> > 
> 
> > > > > > > > typically
> > > > > > > 
> > > > > > 
> > > > > 
> > > > 
> > > 
> > 
> 
> > > > > > > > supported "from" a particular version. E.g. Gluster support was
> > > > > > > 
> > > > > > 
> > > > > 
> > > > 
> > > 
> > 
> 
> > > > > > > > introduced in 3.1, and it continues to be available in all
> > > > > > > 
> > > > > > 
> > > > > 
> > > > 
> > > 
> > 
> 
> > > > > > > > subsequent
> > > > > > > 
> > > > > > 
> > > > > 
> > > > 
> > > 
> > 
> 
> > > > > > > > versions. So I see no point in adding configuration for every
> > > > > > > 
> > > > > > 
> > > > > 
> > > > 
> > > 
> > 
> 
> > > > > > > > version
> > > > > > > 
> > > > > > 
> > > > > 
> > > > 
> > > 
> > 
> 
> > > > > > > > indicating whether the feature is supported in that version or
> > > > > > > 
> > > > > > 
> > > > > 
> > > > 
> > > 
> > 
> 
> > > > > > > > not. I
> > > > > > > 
> > > > > > 
> > > > > 
> > > > 
> > > 
> > 
> 
> > > > > > > > suggest to use either of the following options:
> > > > > > > 
> > > > > > 
> > > > > 
> > > > 
> > > 
> > 
> 

> > > > > > > You can "merge" the configs into a single config when older
> > > > > > 
> > > > > 
> > > > 
> > > 
> > 
> 
> > > > > > > versions
> > > > > > 
> > > > > 
> > > > 
> > > 
> > 
> 
> > > > > > > go out of the supported versions for the system.
> > > > > > 
> > > > > 
> > > > 
> > > 
> > 
> 

> > > > > > > i.e. in 4.0 you can have upgrade script that merges all
> > > > > > 
> > > > > 
> > > > 
> > > 
> > 
> 
> > > > > > > GlusterFeatureSupported to one entry instead of several.
> > > > > > 
> > > > > 
> > > > 
> > > 
> > 
> 

> > > > > Why are we even storing this information in config? Is this
> > > > 
> > > 
> > 
> 
> > > > > something
> > > > 
> > > 
> > 
> 
> > > > > that can be "configured" at customer site?
> > > > 
> > > 
> > 
> 

> > > > As previously explained (but off list :) ) , Config gives you the
> > > 
> > 
> 
> > > > ability to have a cachable "map" of entry (i.e - "feature name")
> > > 
> > 
> 
> > > > per version and value.
> > > 
> > 
> 
> > > > I guess it was convinient for the developers to use that.
> > > 
> > 
> 
> > > > I also mentioned that customers/oVirt users should config the
> > > 
> > 
> 
> > > > entries of vdc_options using engine-config tool only.
> > > 
> > 
> 
> > > > Not all entries are exposed via engine-config.properties (and no,
> > > 
> > 
> 
> > > > not just "is feature supported" entries are hidden).
> > > 
> > 
> 

> > > > > > > > 1) Instead of using a boolean config for each version, use a
> > > > > > > 
> > > > > > 
> > > > > 
> > > > 
> > > 
> > 
> 
> > > > > > > > single
> > > > > > > 
> > > > > > 
> > > > > 
> > > > 
> > > 
> > 
> 
> > > > > > > > string config that indicates the "supported from" version e.g.
> > > > > > > 
> > > > > > 
> > > > > 
> > > > 
> > > 
> > 
> 
> > > > > > > > GlusterSupportedFrom = 3.1. There could be rare  cases where a
> > > > > > > 
> > > > > > 
> > > > > 
> > > > 
> > > 
> > 
> 
> > > > > > > > feature,
> > > > > > > 
> > > > > > 
> > > > > 
> > > > 
> > > 
> > 
> 
> > > > > > > > for some reason, is removed in some release. In such cases, we
> > > > > > > 
> > > > > > 
> > > > > 
> > > > 
> > > 
> > 
> 
> > > > > > > > could
> > > > > > > 
> > > > > > 
> > > > > 
> > > > 
> > > 
> > 
> 
> > > > > > > > use
> > > > > > > 
> > > > > > 
> > > > > 
> > > > 
> > > 
> > 
> 
> > > > > > > > one additional config for the "supported to" version.
> > > > > > > 
> > > > > > 
> > > > > 
> > > > 
> > > 
> > 
> 

> > > > > > > > 2) Continue with the boolean approach, but do not have entries
> > > > > > > 
> > > > > > 
> > > > > 
> > > > 
> > > 
> > 
> 
> > > > > > > > for
> > > > > > > 
> > > > > > 
> > > > > 
> > > > 
> > > 
> > 
> 
> > > > > > > > every
> > > > > > > 
> > > > > > 
> > > > > 
> > > > 
> > > 
> > 
> 
> > > > > > > > version; rather make use of the "default value" for majority of
> > > > > > > 
> > > > > > 
> > > > > 
> > > > 
> > > 
> > 
> 
> > > > > > > > cases,
> > > > > > > 
> > > > > > 
> > > > > 
> > > > 
> > > 
> > 
> 
> > > > > > > > and add the explicit version mapping for the minority e.g.
> > > > > > > 
> > > > > > 
> > > > > 
> > > > 
> > > 
> > 
> 
> > > > > > > > GlusterSupported = true by default, and false in case of 3.0
> > > > > > > 
> > > > > > 
> > > > > 
> > > > 
> > > 
> > 
> 
> > > > > > > > (only
> > > > > > > 
> > > > > > 
> > > > > 
> > > > 
> > > 
> > 
> 
> > > > > > > > one
> > > > > > > 
> > > > > > 
> > > > > 
> > > > 
> > > 
> > 
> 
> > > > > > > > config required for 3.0)
> > > > > > > 
> > > > > > 
> > > > > 
> > > > 
> > > 
> > 
> 

> > > > > > > I'm not sure why we would want to complicate this simple
> > > > > > 
> > > > > 
> > > > 
> > > 
> > 
> 
> > > > > > > mechanism?
> > > > > > 
> > > > > 
> > > > 
> > > 
> > 
> 

> > > > > > > Is there much to gain?
> > > > > > 
> > > > > 
> > > > 
> > > 
> > 
> 

> > > > > > I think option 1 suggested above is simpler - to implement as
> > > > > 
> > > > 
> > > 
> > 
> 
> > > > > > well
> > > > > 
> > > > 
> > > 
> > 
> 
> > > > > > as
> > > > > 
> > > > 
> > > 
> > 
> 
> > > > > > to understand.
> > > > > 
> > > > 
> > > 
> > 
> 

> > > > > > Let me give you an example of why I don't like current mechanism.
> > > > > 
> > > > 
> > > 
> > 
> 
> > > > > > I
> > > > > 
> > > > 
> > > 
> > 
> 
> > > > > > introduce a version check for a feature that was introduced in
> > > > > 
> > > > 
> > > 
> > 
> 
> > > > > > 3.1.
> > > > > 
> > > > 
> > > 
> > 
> 
> > > > > > I'm being asked now to add three entries in config
> > > > > 
> > > > 
> > > 
> > 
> 

> > > > > > 3.0 - false
> > > > > 
> > > > 
> > > 
> > 
> 
> > > > > > 3.1 - true
> > > > > 
> > > > 
> > > 
> > 
> 
> > > > > > 3.2 - true
> > > > > 
> > > > 
> > > 
> > 
> 

> > > > > > It will also mean that when 3.3 goes out, someone has to make
> > > > > 
> > > > 
> > > 
> > 
> 
> > > > > > sure
> > > > > 
> > > > 
> > > 
> > 
> 
> > > > > > that another entry is added for 3.3-true. I think it is not
> > > > > 
> > > > 
> > > 
> > 
> 
> > > > > > logical
> > > > > 
> > > > 
> > > 
> > 
> 
> > > > > > as
> > > > > 
> > > > 
> > > 
> > 
> 
> > > > > > well as scalable if you have more versions. And it sounds far
> > > > > 
> > > > 
> > > 
> > 
> 
> > > > > > more
> > > > > 
> > > > 
> > > 
> > 
> 
> > > > > > complex (to maintain) than just having
> > > > > 
> > > > 
> > > 
> > 
> 

> > > > > > <Feature>SupportedFrom = 3.1
> > > > > 
> > > > 
> > > 
> > 
> 

> > > > > > So I would like to know if there are any objections to my
> > > > > 
> > > > 
> > > 
> > 
> 
> > > > > > proposal.
> > > > > 
> > > > 
> > > 
> > 
> 
> > > > > > I
> > > > > 
> > > > 
> > > 
> > 
> 
> > > > > > intend to use this for at least the gluster related features.
> > > > > 
> > > > 
> > > 
> > 
> 

> > > I've sent a patch ( http://gerrit.ovirt.org/12970 ) with following
> > 
> 
> > > changes:
> > 
> 

> > > 1) Introduced CompatibilityUtils that provides utility methods for
> > 
> 
> > > checking if a given feature is supported in the config. One method to
> > 
> 
> > > check based on boolean values (as is being done today for virt
> > 
> 
> > > features), and nother to check based on a range (from, to) which I
> > 
> 
> > > would
> > 
> 
> > > like to use for gluster features.
> > 
> 
> > > 2) Renamed FeatureSupported to VirtFeatureSupported, and made it use
> > 
> 
> > > the
> > 
> 
> > > first utility method from CompatibilityUtils
> > 
> 
> > > 3) Introduced GlusterFeatureSupported for gluster features, which
> > 
> 
> > > uses
> > 
> 
> > > the second utility method from CompatibilityUtils
> > 
> 

> > > Key advantage here is that
> > 
> 
> > > - we don't have to touch any virt specifc source for adding
> > 
> 
> > > compatibility checks for gluster features
> > 
> 
> > > - virt features continue to use the existing boolean config check
> > 
> 

> > > Any comments / suggestions / reviews will be highly appreciated :)
> > 
> 

> > I think splitting to two classes is OK, but the underlying mechanism IMO
> > should be as Omer suggested:
> 
> > Use the default value from the java config file, and if in the DB there is
> > a
> > version specific value then use it for that version only.
> 

> Review comments here are on the contrary:
> http://gerrit.ovirt.org/#/c/12970/5/backend/manager/dbscripts/upgrade/pre_upgrade/0000_config.sql

The comment in the review simply states that the mechanism is probably broken, not that that's the way it has to be done. 

> > I don't think "From, To, etc" is a good design, it's not a standard way and
> > is very restrictive.
> 

> Can you please explain in what way is it restrictive?

> Also, what is the "etc" you are referring to?
What if for certain version it is not supported, you add "except"? Or do you specify 2 ranges? 

Starting to add from/to creates a limited design of one range, which would be difficult to tune if necessary. 

I think the design generally for config values is very simple and suits us well - use the default value, unless a specific version is configured differently. 
This way you can specify the feature is supported, and disable it for specific versions. 
I think this direction gives us the flexibility that we would like to have. 

Currently it doesn't work that way, but I think it's not impossible to change, and more worthwhile than introducing a new mechanism. 

> > > > > > > > Thoughts?
> > > > > > > 
> > > > > > 
> > > > > 
> > > > 
> > > 
> > 
> 

> > > > > > > > Regards,
> > > > > > > 
> > > > > > 
> > > > > 
> > > > 
> > > 
> > 
> 
> > > > > > > > Shireesh
> > > > > > > 
> > > > > > 
> > > > > 
> > > > 
> > > 
> > 
> 
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.ovirt.org/pipermail/devel/attachments/20130402/b17cf7e1/attachment-0001.html>


More information about the Devel mailing list