[Engine-devel] FeatureSupported and compatibility versions

[Shireesh Anjal]

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

>>>>> Thoughts?
>>>>>
>>>>> Regards,
>>>>> Shireesh
>>>>> _______________________________________________
>>>>> Engine-devel mailing list
>>>>> Engine-devel at ovirt.org
>>>>> http://lists.ovirt.org/mailman/listinfo/engine-devel
>>>>>
>>> _______________________________________________
>>> Engine-devel mailing list
>>> Engine-devel at ovirt.org
>>> http://lists.ovirt.org/mailman/listinfo/engine-devel
>> _______________________________________________
>> Engine-devel mailing list
>> Engine-devel at ovirt.org
>> http://lists.ovirt.org/mailman/listinfo/engine-devel
>>
> _______________________________________________
> Engine-devel mailing list
> Engine-devel at ovirt.org
> http://lists.ovirt.org/mailman/listinfo/engine-devel