----- Original Message ----- > 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" <[email protected]> To: "Mike Kolesnik" > > > > > <[email protected]> Cc: [email protected] 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. > > 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. 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. This way you can specify the feature is supported, and disable it for specific versions. 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. > > > > > > > > Thoughts? > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Regards, > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Shireesh > > > > > > > > > > > > > > > > > > > > > > > > > > > >
_______________________________________________ Engine-devel mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/engine-devel
