[Engine-devel] FeatureSupported and compatibility versions
Shireesh Anjal
sanjal at redhat.com
Tue Apr 2 11:00:51 UTC 2013
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
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.ovirt.org/pipermail/devel/attachments/20130402/6c58a19c/attachment-0001.html>
More information about the Devel
mailing list