[Engine-devel] FeatureSupported and compatibility versions

Sahina Bose sabose at redhat.com
Thu Apr 4 06:46:18 UTC 2013


The idea should be to make sure that maintaining the feature supported 
matrix is not a nightmare. If we need to go and replicate entries in 
0000_config.sql file for each new version, then this is an issue. And I 
think we are all in agreement that this is not the way to go.

Either we go with
[1].  default value in ConfigValues, and only changed value in db script 
like in patch set 5 (http://gerrit.ovirt.org/#/c/12970/5)

If this mechanism is broken, do you know where/what is broken?

or  [2] . with the new approach where a Feature supported changes to 
true/false **from** a particular version.
(I think, for gluster features, the Feature From works for us as we do 
not see it changing from version to version once supported. )

But if there's a way to fix 1, let's do that to get this moving.
Mike, could you elaborate on what needs to be fixed in [1] ?



On 04/02/2013 04:30 PM, Shireesh Anjal wrote:
> On 04/02/2013 02:47 PM, Mike Kolesnik wrote:
>> ------------------------------------------------------------------------
>>
>>     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.
>
> The comment explicitly asks me to add entries for every version. What 
> you have looked at is my response to this comment, which suggests that 
> the current mechanism is not great. In fact, what I had done in 
> patch-set 5 is exactly what you are suggesting : true as default value 
> and explicit entries in config for the "false" values. But it was not 
> accepted.
>
>>
>>         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.
>
> Really? Does someone really think that there will be a feature that 
> will be supported in multiple different ranges of versions? I see zero 
> possibility for this. I would love to see some +1s on this concept 
> before I can accept this argument.
>
>> 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.
>
> I think the current design is wrong. A feature gets supported "from" a 
> particular version, and that's all that is required in most of the 
> cases. Expecting developers to add version-by-version mapping for 
> features is bad. The "to" part in my patch is just to handle rare 
> cases, if at all they come up. I'm willing to even remove that if such 
> a case doesn't exist today.
>
> Also, even though I have followed it for the sake of consistency, I 
> don't think these values need to be stored in the config (db) at all. 
> Only explanation I've got for it is that it was probably 'convenient' 
> for developers to use the config mechanism. I'm for having this check 
> purely in code in a central place, and not the config (db).
>
>> This way you can specify the feature is supported, and disable it for 
>> specific versions.
>
> So one has to look at both code (FeatureSupported) as well as db 
> (config) to get an idea of what versions the feature is supported in. 
> Not great.
>
>> 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.
>
> I disagree, and would like to use the "supported from" mechanism at 
> least for gluster features.
>
>>
>>                                 Thoughts?
>>
>>                                 Regards,
>>                                 Shireesh
>>
>>
>>
>
>
>
> _______________________________________________
> Engine-devel mailing list
> Engine-devel at ovirt.org
> http://lists.ovirt.org/mailman/listinfo/engine-devel

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.ovirt.org/pipermail/engine-devel/attachments/20130404/37db529c/attachment.html>


More information about the Engine-devel mailing list