3:47 p.m.
This is a multi-part message in MIME format.
--------------020009000007090902020606
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
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(a)redhat.com>
>>> To: "Mike Kolesnik" <mkolesni(a)redhat.com>
>>> Cc: engine-devel(a)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...
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
--------------020009000007090902020606
Content-Type: text/html; charset=UTF-8
Content-Transfer-Encoding: 7bit
<html>
<head>
<meta content="text/html; charset=UTF-8"
http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
<div class="moz-cite-prefix">On 03/27/2013 05:48 PM, Mike Kolesnik
wrote:<br>
</div>
<blockquote
cite="mid:36623975.7044697.1364386718971.JavaMail.root@redhat.com"
type="cite">
<pre wrap="">----- Original Message -----
</pre>
<blockquote type="cite">
<pre wrap="">On 03/20/2013 08:20 PM, Yair Zaslavsky wrote:
</pre>
<blockquote type="cite">
<pre wrap="">
----- Original Message -----
</pre>
<blockquote type="cite">
<pre wrap="">From: "Shireesh Anjal" <a
class="moz-txt-link-rfc2396E"
href="mailto:sanjal@redhat.com"><sanjal@redhat.com></a>
To: "Mike Kolesnik" <a class="moz-txt-link-rfc2396E"
href="mailto:mkolesni@redhat.com"><mkolesni@redhat.com></a>
Cc: <a class="moz-txt-link-abbreviated"
href="mailto:engine-devel@ovirt.org">engine-devel@ovirt.org</a>
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:
</pre>
<blockquote type="cite">
<pre wrap="">On 03/18/2013 12:59 PM, Mike Kolesnik wrote:
</pre>
<blockquote type="cite">
<pre wrap="">----- Original Message -----
</pre>
<blockquote type="cite">
<pre wrap="">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:
</pre>
</blockquote>
<pre wrap="">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.
</pre>
</blockquote>
</blockquote>
<pre wrap="">Why are we even storing this information in
config? Is this
something
that can be "configured" at customer site?
</pre>
</blockquote>
<pre wrap="">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).
</pre>
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<pre wrap="">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)
</pre>
</blockquote>
<pre wrap="">I'm not sure why we would want to
complicate this simple
mechanism?
Is there much to gain?
</pre>
</blockquote>
<pre wrap="">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.
</pre>
</blockquote>
</blockquote>
</blockquote>
<pre wrap="">
I've sent a patch (<a class="moz-txt-link-freetext"
href="http://gerrit.ovirt.org/12970">http://gerrit.ovirt.org...>)
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 :)
</pre>
</blockquote>
<pre wrap="">
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.</pre>
</blockquote>
<br>
Review comments here are on the contrary:<br>
<meta http-equiv="content-type" content="text/html;
charset=UTF-8">
<a
href="http://gerrit.ovirt.org/#/c/12970/5/backend/manager/dbscripts/...
<br>
<blockquote
cite="mid:36623975.7044697.1364386718971.JavaMail.root@redhat.com"
type="cite">
<pre wrap="">
I don't think "From, To, etc" is a good design, it's not a standard way
and is very restrictive.</pre>
</blockquote>
<br>
Can you please explain in what way is it restrictive?<br>
<br>
Also, what is the "etc" you are referring to?<br>
<br>
<blockquote
cite="mid:36623975.7044697.1364386718971.JavaMail.root@redhat.com"
type="cite">
<pre wrap="">
</pre>
<blockquote type="cite">
<pre wrap="">
</pre>
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<pre wrap="">Thoughts?
Regards,
Shireesh
</pre>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
<br>
</body>
</html>
--------------020009000007090902020606--