Open Stack

+Dan Smith

Good morning Mike :) Comments inline...

On 07/23/2017 08:05 PM, Michael Bayer wrote:
> On Sun, Jul 23, 2017 at 6:10 PM, Jay Pipes <jaypipes at gmail.com> wrote:
>> Glad you brought this up, Mike. I was going to start a thread about this.
>> Comments inline.
>>
>> On 07/23/2017 05:02 PM, Michael Bayer wrote:
>> Well, besides that point (which I agree with), that is attempting to change
>> an existing database schema migration, which is a no-no in my book ;)
> 
> OK this point has come up before and I always think there's a bit of
> an over-broad kind of purism being applied (kind of like when someone
> says, "never use global variables ever!" and I say, "OK so sys.modules
> is out right ?" :)  ).

I'm not being a purist. I'm being a realist :) See below...

> I agree with "never change a migration" to the extent that you should
> never change the *SQL steps emitted for a database migration*.  That
> is, if your database migration emitted "ALTER TABLE foobar foobar
> foobar" on a certain target databse, that should never change.   No
> problem there.
> 
> However, what we're doing here is adding new datatype logic for the
> NDB backend which are necessarily different; not to mention that NDB
> requires more manipulation of constraints to make certain changes
> happen.  To make all that work, the *Python code that emits the SQL
> for the migration* needs to have changes made, mostly (I would say
> completely) in the form of new conditionals for NDB-specific concepts.
>     In the case of the datatypes, the migrations will need to refer to
> a SQLAlchemy type object that's been injected with the ndb-specific
> logic when the NDB backend is present; I've made sure that when the
> NDB backend is *not* present, the datatypes behave exactly the same as
> before.

No disagreement here.

> So basically, *SQL steps do not change*, but *Python code that emits
> the SQL steps* necessarily has to change to accomodate for when the
> "ndb" flag is present - this because these migrations have to run on
> brand new ndb installations in order to create the database.   If Nova
> and others did the initial "create database" without using the
> migration files and instead used a create_all(), things would be
> different, but that's not how things work (and also it is fine that
> the migrations are used to build up the DB).

So, I see your point here, but my concern here is that if we *modify* an 
existing schema migration that has already been tested to properly apply 
a schema change for MySQL/InnoDB and PostgreSQL with code that is 
specific to NDB, we introduce the potential for bugs where users report 
that the same migration works sometimes but fails other times.

I would much prefer to *add* a brand new schema migration that handles 
conversion of the entire InnoDB schema at a certain point to an 
NDB-compatible one *after* that point. That way, we isolate the NDB 
changes to one specific schema migration -- and can point users to that 
one specific migration in case bugs arise. This is the reason that every 
release we add a number of "placeholder" schema migration numbered files 
to handle situations such as these.

I understand that Oracle wants to support older versions of OpenStack in 
their distribution and that's totally cool with me. But, the proper way 
IMHO to do this kind of thing is to take one of the placeholder 
migrations and use that as the NDB-conversion migration. I would posit 
that since Oracle will need to keep some not-insignificant amount of 
Python code in their distribution fork of Nova in order to bring in the 
oslo.db and Nova NDB support, that it will actually be *easier* for them 
to maintain a *separate* placeholder schema migration for all NDB 
conversion work instead of changing an existing schema migration with a 
new patch.

All the best,
-jay