[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/engine-devel/attachments/20130402/b17cf7e1/attachment.html>
More information about the Engine-devel
mailing list