Bug 1288185

+++ This bug was initially created as a clone of Bug #1285904 +++

Description of problem:
During upgrades the vmdb configurations (stored in the database) are not updated with new defaults added to vmdb.tmpl.yml.
This results generic failures with the code that assumes the new keys being available.

How reproducible:
100%

Steps to Reproduce:
1. upgrade to a newer version

Actual results:
The new defaults in vmdb.tmpl.yml are not added in the configuration in the database.

Expected results:
The new defaults in vmdb.tmpl.yml should be added in the configuration in the database.

--- Additional comment from Joe Rafaniello on 2015-11-30 10:00:55 EST ---

Hi Federico, 

Thanks for the bug report.
This, unfortunately, is working as designed.

We currently don't have a good separation of user settings vs "suggested" defaults vs "required" defaults in the configuration.  

If a user made a change to a setting, in most cases, we want to allow that setting to remain after upgrading.  In some case, we need to overwrite the user settings regardless of what they configured.  These two requirements contradict each other.  This is why we don't automatically migrate their configuration to the new vmdb.tmpl.yml.  The developer who adds sections or keys to the configuration needs to explicitly migrate via "merge_from_template" (see below) or provide intelligent defaults in the code.

We do have plans and some code for a general rewrite of the configuration that would allow a better story for the problems above but it's not yet done.

For now, I can recommend you take a look at how we do it elsewhere.

The key methods are:
VMDB::Config#merge_from_template_if_missing
and 
VMDB::Config#merge_from_template

The first one will take a VMDB::Config object and merge from the template specific sections that you want ONLY if they're missing entirely from the user's settings.

The second one will a VMDB::Config object and merge from the template specific sections that you want and OVERWRITE what may already be there.

The general flow is something like:

cfg = VMDB::Config.new("vmdb")
cfg.merge_from_template_if_missing(*database_metrics_collection_schedule)
# more code
cfg.save

I don't know if Jason Frey has a bug open for the general "rewrite" of the configuration that would make this problem easier.  If not, we can re-open this bug and use it.

If I missed something, please don't hesitate or ask as we an easily reopen if need be.

Thanks!
Joe

--- Additional comment from Joe Rafaniello on 2015-11-30 10:10:10 EST ---

Oh and if you need help migrating anything using merge_from_template_if_missing or merge_from_template, please ping me on gitter/irc/github.

Thanks,
Joe

--- Additional comment from Federico Simoncelli on 2015-12-01 12:04:01 EST ---

(In reply to Joe Rafaniello from comment #1)
> Hi Federico, 
> 
> Thanks for the bug report.
> This, unfortunately, is working as designed.
> 
> We currently don't have a good separation of user settings vs "suggested"
> defaults vs "required" defaults in the configuration.  
> 
> If a user made a change to a setting, in most cases, we want to allow that
> setting to remain after upgrading.  In some case, we need to overwrite the
> user settings regardless of what they configured.  These two requirements
> contradict each other.  This is why we don't automatically migrate their
> configuration to the new vmdb.tmpl.yml.  The developer who adds sections or
> keys to the configuration needs to explicitly migrate via
> "merge_from_template" (see below) or provide intelligent defaults in the
> code.
> 
> We do have plans and some code for a general rewrite of the configuration
> that would allow a better story for the problems above but it's not yet done.
> 
> For now, I can recommend you take a look at how we do it elsewhere.
> 
> The key methods are:
> VMDB::Config#merge_from_template_if_missing
> and 
> VMDB::Config#merge_from_template
> 
> The first one will take a VMDB::Config object and merge from the template
> specific sections that you want ONLY if they're missing entirely from the
> user's settings.
> 
> The second one will a VMDB::Config object and merge from the template
> specific sections that you want and OVERWRITE what may already be there.
> 
> The general flow is something like:
> 
> cfg = VMDB::Config.new("vmdb")
> cfg.merge_from_template_if_missing(*database_metrics_collection_schedule)
> # more code
> cfg.save
> 
> I don't know if Jason Frey has a bug open for the general "rewrite" of the
> configuration that would make this problem easier.  If not, we can re-open
> this bug and use it.

The gist of this BZ was to plan for something better than what exists today.

It is clear that somehow you dealt with this in the past, but the issues of the current solution are:

1. it generates inconsistent configs between an upgraded system and a fresh installed one (beside user's changes). In fact if you don't use merge_from_template_if_missing by default the upgraded system will have missing configs.

2. code needs a special handling for missing configs that is never used/tested except for upgraded systems (which means that you require the developer to write extra logic and it's not tested)

3. it's not clear when/where you should run merge_from_template_if_missing (for example it could be enough when you update the appliance and not every time you run a specific method as today)

4. current use of merge_from_template_if_missing is scattered all over the code and it's not clear when it should run (it's obviously specific to the case but too much is left to the reader, while as mentioned above it could be done just once at the end of the db migration)


As first step wouldn't it be enough to chain all the merge_from_template_if_missing after db:migrate?

I am reopening this because I strongly believe that we can do better than what we have today.

--- Additional comment from Joe Rafaniello on 2015-12-01 16:21:23 EST ---

Federico, can you review Jason's bug below.

The general problem of having ways for settings to come from region, zone, server, development, template, user with some form of inheritance is the gist of Jason's bug https://bugzilla.redhat.com/show_bug.cgi?id=1082155

If there is anything in this bug that's missing in Jason's design document or his bug, we should discuss it there.

If I don't hear from you or bug 1082155 addresses the issues you are seeing, I'll close this bug as a duplicate.

Thanks!