Re: [Engine-devel] FeatureSupported and compatibility versions

2 Apr 2013

      This is a multi-part message in MIME format.
--------------060100050808070407030307
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit

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@redhat.com>
                    To: "Mike Kolesnik"<mkolesni@redhat.com>
                    Cc:engine-devel@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_up...
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
--------------060100050808070407030307
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 04/02/2013 02:47 PM, Mike Kolesnik
      wrote:<br>
    </div>
    <blockquote
      cite="mid:1842623267.574910.1364894251176.JavaMail.root@redhat.com"
      type="cite">
      <div style="font-family: times new roman, new york, times, serif;
        font-size: 12pt; color: #000000">
        <hr id="zwchr">
        <blockquote style="border-left:2px solid
#1010FF;margin-left:5px;padding-left:5px;color:#000;font-weight:normal;font-style:normal;text-decoration:none;font-family:Helvetica,Arial,sans-serif;font-size:12pt;"
          data-mce-style="border-left: 2px solid #1010FF; margin-left:
          5px; padding-left: 5px; color: #000; font-weight: normal;
          font-style: normal; text-decoration: none; font-family:
          Helvetica,Arial,sans-serif; font-size: 12pt;">
          <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">
            <pre>----- Original Message -----
</pre>
            <blockquote>
              <pre>On 03/20/2013 08:20 PM, Yair Zaslavsky wrote:
</pre>
              <blockquote>
                <pre>----- Original Message -----
</pre>
                <blockquote>
                  <pre>From: "Shireesh Anjal" <a moz-do-not-send="true" class="moz-txt-link-rfc2396E" href="mailto:sanjal@redhat.com" target="_blank" data-mce-href="mailto:sanjal@redhat.com"><sanjal@redhat.com></a>
To: "Mike Kolesnik" <a moz-do-not-send="true" class="moz-txt-link-rfc2396E" href="mailto:mkolesni@redhat.com" target="_blank" data-mce-href="mailto:mkolesni@redhat.com"><mkolesni@redhat.com></a>
Cc: <a moz-do-not-send="true" class="moz-txt-link-abbreviated" href="mailto:engine-devel@ovirt.org" target="_blank" data-mce-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>
                    <pre>On 03/18/2013 12:59 PM, Mike Kolesnik wrote:
</pre>
                    <blockquote>
                      <pre>----- Original Message -----
</pre>
                      <blockquote>
                        <pre>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>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>Why are we even storing this information in config? Is this
something
that can be "configured" at customer site?
</pre>
                </blockquote>
                <pre>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>
                  <blockquote>
                    <blockquote>
                      <blockquote>
                        <pre>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>I'm not sure why we would want to complicate this simple
mechanism?

Is there much to gain?
</pre>
                    </blockquote>
                    <pre>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>I've sent a patch (<a moz-do-not-send="true" class="moz-txt-link-freetext" href="http://gerrit.ovirt.org/12970" target="_blank" data-mce-href="http://gerrit.ovirt.org/12970">http://gerrit.ovirt.org/12970</a>) 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>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>
          <a moz-do-not-send="true"
href="http://gerrit.ovirt.org/#/c/12970/5/backend/manager/dbscripts/upgrade/pre_up..."
            target="_blank"
data-mce-href="http://gerrit.ovirt.org/#/c/12970/5/backend/manager/dbscripts/upgrade/pre_upgrade/0000_config.sql">http://gerrit.ovirt.org/#/c/12970/5/backend/manager/dbscripts/upgrade/pre_upgrade/0000_config.sql</a><br
            data-mce-bogus="1">
        </blockquote>
        <div>The comment in the review simply states that the mechanism
          is probably broken, not that that's the way it has to be done.<br>
        </div>
      </div>
    </blockquote>
    <br>
    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. <br>
    <br>
    <blockquote
      cite="mid:1842623267.574910.1364894251176.JavaMail.root@redhat.com"
      type="cite">
      <div style="font-family: times new roman, new york, times, serif;
        font-size: 12pt; color: #000000">
        <blockquote style="border-left:2px solid
#1010FF;margin-left:5px;padding-left:5px;color:#000;font-weight:normal;font-style:normal;text-decoration:none;font-family:Helvetica,Arial,sans-serif;font-size:12pt;"
          data-mce-style="border-left: 2px solid #1010FF; margin-left:
          5px; padding-left: 5px; color: #000; font-weight: normal;
          font-style: normal; text-decoration: none; font-family:
          Helvetica,Arial,sans-serif; font-size: 12pt;"><br>
          <blockquote
            cite="mid:36623975.7044697.1364386718971.JavaMail.root@redhat.com">
            <pre>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?</blockquote>
        <div>What if for certain version it is not supported, you add
          "except"? Or do you specify 2 ranges?<br>
        </div>
        <div><br>
        </div>
        <div>Starting to add from/to creates a limited design of one
          range, which would be difficult to tune if necessary.</div>
      </div>
    </blockquote>
    <br>
    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.<br>
    <br>
    <blockquote
      cite="mid:1842623267.574910.1364894251176.JavaMail.root@redhat.com"
      type="cite">
      <div style="font-family: times new roman, new york, times, serif;
        font-size: 12pt; color: #000000">
        <div>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.<br>
        </div>
      </div>
    </blockquote>
    <br>
    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.<br>
    <br>
    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).<br>
    <br>
    <blockquote
      cite="mid:1842623267.574910.1364894251176.JavaMail.root@redhat.com"
      type="cite">
      <div style="font-family: times new roman, new york, times, serif;
        font-size: 12pt; color: #000000">
        <div>This way you can specify the feature is supported, and
          disable it for specific versions.<br>
        </div>
      </div>
    </blockquote>
    <br>
    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. <br>
    <br>
    <blockquote
      cite="mid:1842623267.574910.1364894251176.JavaMail.root@redhat.com"
      type="cite">
      <div style="font-family: times new roman, new york, times, serif;
        font-size: 12pt; color: #000000">
        <div>I think this direction gives us the flexibility that we
          would like to have.<br>
        </div>
        <div><br>
        </div>
        <div>Currently it doesn't work that way, but I think it's not
          impossible to change, and more worthwhile than introducing a
          new mechanism.<br>
        </div>
      </div>
    </blockquote>
    <br>
    I disagree, and would like to use the "supported from" mechanism at
    least for gluster features.<br>
    <br>
    <blockquote
      cite="mid:1842623267.574910.1364894251176.JavaMail.root@redhat.com"
      type="cite">
      <div style="font-family: times new roman, new york, times, serif;
        font-size: 12pt; color: #000000">
        <blockquote style="border-left:2px solid
#1010FF;margin-left:5px;padding-left:5px;color:#000;font-weight:normal;font-style:normal;text-decoration:none;font-family:Helvetica,Arial,sans-serif;font-size:12pt;"
          data-mce-style="border-left: 2px solid #1010FF; margin-left:
          5px; padding-left: 5px; color: #000; font-weight: normal;
          font-style: normal; text-decoration: none; font-family:
          Helvetica,Arial,sans-serif; font-size: 12pt;"><br>
          <blockquote
            cite="mid:36623975.7044697.1364386718971.JavaMail.root@redhat.com">
            <blockquote>
              <blockquote>
                <blockquote>
                  <blockquote>
                    <blockquote>
                      <blockquote>
                        <pre>Thoughts?

Regards,
Shireesh
</pre>
                      </blockquote>
                    </blockquote>
                  </blockquote>
                </blockquote>
              </blockquote>
            </blockquote>
          </blockquote>
          <br>
        </blockquote>
        <div><br>
        </div>
      </div>
    </blockquote>
    <br>
  </body>
</html>

--------------060100050808070407030307--