[Engine-devel] FeatureSupported and compatibility versions

[Shireesh Anjal]

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

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

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

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.ovirt.org/pipermail/engine-devel/attachments/20130328/65fffd9f/attachment.html>