[Engine-devel] [RFC] ovirt-engine - vdc_config default options

[Alon Bar-Lev]

----- Original Message -----
> From: "Yair Zaslavsky" <yzaslavs at redhat.com>
> To: engine-devel at ovirt.org
> Sent: Monday, September 10, 2012 8:22:11 AM
> Subject: Re: [Engine-devel] [RFC] ovirt-engine - vdc_config default options
> 
> 
> 
> On 09/10/2012 12:58 AM, Alon Bar-Lev wrote:
> > Hello All,
> >
> > I would like to discuss the method we use to manage default
> > options. I believe it can be significantly simplified.
> >
> > Please read though and comment.
> >
> > Thank you,
> > Alon Bar-Lev
> >
> > ---
> >
> > CURRENT STATE
> >
> > Most options are located in three different locations.
> >
> > Let's explain by example... The
> > FenceQuietTimeBetweenOperationsInSec option.
> >
> > ---
> > ConfigValues.java
> > ---
> > public enum ConfigValues {
> > <snip>
> >      @Reloadable
> >      @TypeConverterAttribute(Integer.class)
> >      @DefaultValueAttribute("180")
> >      FenceQuietTimeBetweenOperationsInSec(30),
> > <snip>
> > }
> > ---
> >
> > ---
> > 0000_config.sql
> > ---
> > <snip>
> > select
> > fn_db_add_config_value('FenceQuietTimeBetweenOperationsInSec','180','general');
> > <snip>
> > ---
> >
> > ---
> > engine-config.properties
> > ---
> > <snip>
> > FenceQuietTimeBetweenOperationsInSec.type=Integer
> > FenceQuietTimeBetweenOperationsInSec.validValues=60..600
> > FenceQuietTimeBetweenOperationsInSec.description="Fence quiet time
> > between operations (in seconds)"
> > FenceQuietTimeBetweenOperationsInSec.alternateKey=Fence_Quiet_Time
> > <snip>
> > ---
> >
> > ConfigValues.java is the base, it defines all options and
> > appropriate metadata as annotations. It defines the option name,
> > type, default value, and if reloadable.
> >
> > 0000_config.sql adds all parameters into the database using their
> > default value, we actually duplicate the parameter name and
> > default value into the sql script. Please note that even if we do
> > not add the parameter to the database the engine infrastructure
> > will use the default value specify at ConfigValues.
> >
> > engine-config.properties is used as an interface to engine-config
> > utility, a command-line utility that can manipulate engine
> > options. engine-config manages only options that are specified in
> > this property files. Property files specifies user friendly
> > description, valid values, alias, but duplicate the option name
> > and type that already specified in ConfigValues. engine-config
> > will only manage options that exists in database.
> >
> > QUESTIONS
> >
> > 1. Why do we store default values in database?
> >
> >  From what I managed to gather, we store default values in database
> >  under the assumption that we need to keep defaults when we
> >  upgrade from one version to another.
> >
> > However, in most cases when upgrading an application the new
> > defaults should apply as long as they were not overridden by user.
> > Keeping old default as a new value is an exception that can be
> > taken care of during the upgrade process for selected options.
> >
> > 2. Why do we keep properties file?
> >
> > In practice we could specify the metadata within the property files
> > as annotations in ConfigValues.java. So why do we actually need
> > the properties file?
> >
> >  From what I managed to gather, we use the property file as an
> >  interface to support personal, to allow adding/removing options
> >  exposed to the engine-config utility. An addition of option to
> >  the property file exposes it.
> >
> > SUGGESTED STATE
> >
> > Establish a single place to manage options.
> >
> > Do not store default values in database.
> >
> > Provide alternate method of exposing internal options to
> > engine-config utility.
> >
> > ---
> > ConfigValues.java
> > ---
> > public enum ConfigValues {
> > <snip>
> >      @Reloadable
> >      @Public
> >      @TypeConverterAttribute(Integer.class)
> >      @Restriction.IntegerRange(60, 600)
> This definition will probably not work, should be
> @IntegerRestrictionRange(60,600)
> >      @DefaultValueAttribute("180")
> >      @Description("Fence quiet time between operations (in
> >      seconds)")
> >      FenceQuietTimeBetweenOperationsInSec(30),
> > <snip>
> > }
> > ---
> >
> > BENEFITS
> >
> > No duplication of option name, type, default value, etc... between
> > multiple files. One place to role them all.
> >
> > Simpler upgrade sequence, in most cases as we do want the new
> > defaults to apply.
> >
> > METHOD
> >
> > 1. Add @Public annotation for ConfigValues, to expose options to
> > engine-config instead of the properties file.
> 
> I would still recommend in this case to have an optional properties
> file
> that will allow you to override the behaviour of the @Public
> annotation
> (i.e - cancel it).

Why would you want to cancel it?
I understand why you want to expose more...
But cancel?
I don't mind adding a /etc/<something> to allow exposing more, but why adding --internal parameter to engine-config is not enough?

> In evolution of java frameworks from the stage where metadata was
> provided via external configuration file (XML, properties file
> ,etc...)
> to a stage where metadata was provided via annotations, in order to
> support changes at metadata without needing to recompile and to
> support
> backward compatibility, many frameworks simply made the configuration
> file to be optional for purpose of overriding the annotation
> deceleration

I don't mind to use XML for holding metadata, or any format, as long as we have single location to declare new option.

We can even use a table within database...

*BUT* the metadata should be in one place, and there should be no default values within the options tables so an update to the metadata will be in effect as long as the user had not overridden the default.

> 
> >
> > 2. Add @Restrict annotation for ConfigValues, instead of
> > validValues of properties file.
> 
> Bare in mind that annotations are limited in the values they can
> accept.
> Most likely it will work, but we need to check it.

Sure...

> 
> >
> > 3. Add @Description annotation for ConfigValues, instead of
> > description of properties file.
> >
> > 4. Add engine-config --internal parameter to allow get/set/list of
> > none @Public option, instead of the need to update the properties
> > file.
> >
> > 5. Modify enigne-config [set] to add option if does not exist in
> > database and does not match default value.
> 
> >
> > 6. Modify engine-config [set] to delete option if value matches
> > default value.
> >
> > 7. Modify engine-config [get] to retrieve default from ConfigValues
> > if value is missing from database.
> 
> I would recommend as a future feature to consider modifying [set] to
> provide an option to reset the value to the default value, as
> presented
> at ConfigValue

Good idea! so we can have --set "[default]" or similar to revert to default value.

> >
> > 8. Create upgrade script which deletes all options with value that
> > matches the default from the database.
> >
> > IMPLICATIONS
> >
> > The option alias is removed. Can be added at later time if required
> > using own @Alias annotation. Not sure it is actually required.
> >
> > _______________________________________________
> > Engine-devel mailing list
> > Engine-devel at ovirt.org
> > http://lists.ovirt.org/mailman/listinfo/engine-devel
> >
> _______________________________________________
> Engine-devel mailing list
> Engine-devel at ovirt.org
> http://lists.ovirt.org/mailman/listinfo/engine-devel
>