[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