[Engine-devel] FeatureSupported and compatibility versions

[Mike Kolesnik]

----- 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.
I don't think "From, To, etc" is a good design, it's not a standard way and is very restrictive.

> 
> >>>>> Thoughts?
> >>>>>
> >>>>> Regards,
> >>>>> Shireesh