Merge lp:~jcannava/nova/lp741274 into lp:~hudson-openstack/nova/trunk

[Jason Cannavale]

MySQL database tables are currently using the MyISAM engine. Created migration script nova/db/sqlalchemy/migrate_repo/versions/021_set_engine_mysql_innodb.py to change all current tables to InnoDB.

[Vish Ishaya (vishvananda)]

nice. LGTM

review: Approve

[Vish Ishaya (vishvananda)]

Is there a way to make sure new tables that get added get set properly? Also FWIW if you set up mysql to default to INNOdb this isn't necessary.

[Jason Cannavale (jcannava)]

I was going to take a look at that next, it seems strange that the code in
there already isn't setting it.

Jason

On 5/16/11 6:33 PM, "Vish Ishaya" <email address hidden> wrote:

>Is there a way to make sure new tables that get added get set properly?
>Also FWIW if you set up mysql to default to INNOdb this isn't necessary.
>--
>https://code.launchpad.net/~jason-cannavale-com/nova/lp741274/+merge/61183
>You are the owner of lp:~jason-cannavale-com/nova/lp741274.

Confidentiality Notice: This e-mail message (including any attached or
embedded documents) is intended for the exclusive and confidential use of the
individual or entity to which this message is addressed, and unless otherwise
expressly indicated, is confidential and privileged information of Rackspace.
Any dissemination, distribution or copying of the enclosed material is prohibited.
If you receive this transmission in error, please notify us immediately by e-mail
at <email address hidden>, and delete the original message.
Your cooperation is appreciated.

[Dan Prince (dan-prince)]

Hi Jason,

Can you bump the migration number up to 019? We are up to 018 today with 018_rename_server_management_url.

review: Needs Fixing

[Jason Cannavale (jcannava)]

Updated version number.

[Dan Prince (dan-prince)]

review: Abstain

[Jay Pipes (jaypipes)]

lgtm

review: Approve

[Dan Prince (dan-prince)]

Hey Jason,

Would it be better to wait on this one until we figure out why the tables don't get set to InnoDB first via our existing migrations? There is a work around for this (setting the MySQL default).

Seems like if we do this now we'll have to keep adding MySQL specific code to our migrations (to ensure they are all using InnoDB) until we get the table creation issue fixed.

review: Needs Information

[Jason Cannavale (jcannava)]

> Hey Jason,
>
> Would it be better to wait on this one until we figure out why the tables
> don't get set to InnoDB first via our existing migrations? There is a work
> around for this (setting the MySQL default).
>
> Seems like if we do this now we'll have to keep adding MySQL specific code to
> our migrations (to ensure they are all using InnoDB) until we get the table
> creation issue fixed.

Hey Dan,

I disagree that the workaround is great from a real world scenario where Nova is not the only schema running on the database server. From an administrator perspective my other databases may be running in any number of table types based on what their transaction type is and having to default everything to InnoDB for Nova would make me think twice. There are still valid use cases for MyISAM..

From what I can tell, after about two weeks of looking at it, the call in the nova code is correct the issue is with sqlalchemy. When it creates the tables in the first place the directive to set the InnoDB flag is being completely ignored. In my opinion, it comes down to do we wait for an update to sqlalchemy to fix the problem which is out of our control, do we put this in now and deal with adding a single line for new tables until that is corrected, or do we ensure that every migration script that is written includes the transition to InnoDB?

[Soren Hansen (soren)]

Please set a commit message that sums up the changes made here (it's what will end up in our changelg). We can't merge this without a commit message.

[Mark Washenberger (markwash)]

Can you help me understand why this belongs in nova? It seems like it should be a deployer's decision what type of engine to run.

[Jason Cannavale (jcannava)]

> Can you help me understand why this belongs in nova? It seems like it should
> be a deployer's decision what type of engine to run.

So the easy thing to consider here is that every software project has set of configurations that are known to perform well or the code that specifically makes use of features in a database system. A good example here is if I was to download magento or some of the other CMS/E-Commerce platforms. When I go to install them the database tables are already configured to what the developers/QA/Test folks have determined performs well and no further changes are necessary. From there I or the deployer can make any modifications and tweak it however I see fit (i.e. changing the engine from InnoDB on specific tables), but at least at the distribution point I have a baseline known good configuration.

Specifically to nova, it seems this intent has been there for a while but sqlalchemy was not setting up the tables (basically ignoring the settings currently in nova core) hence the migration script. Thinking about it further, consider a situation like nova where the tables and entries are going to be accessed at large volumes it makes sense that we would set the nova tables to InnoDB to have the ability to recover from crashes (log replay), better caching, etc.. as a baseline deployment.

Just my .02, if there isn't a consensus here that works also.

[Jay Pipes (jaypipes)]

hehe, lgtm again :)

review: Approve

[Paul Voccio (pvo)]

lgtm

review: Approve

[Rick Harris (rconradharris)]

lgtm.

Dan- Were your questions answered above?

review: Approve

[Dan Prince (dan-prince)]

I would still prefer to spend more time and figure out why the code we already have in nova/db/sqlalchemy/models.py isn't setting this correctly?

Line 43 for example:

    __table_args__ = {'mysql_engine': 'InnoDB'}

Do we need to put the equivalent Table metadata directly into our migrations as well?

---

In any case I don't see the work around for this (setting MySQL to use InnoDB by default). MySQL 5.5 changes this to be the default anyway right?

I hate to see us put in DB specific migrations when it seems like the root cause of this issue is still present (our __table_args__ options aren't doing what they are supposed to).

That said this doesn't hurt anything and I won't hold it up any longer.

review: Abstain

=== modified file 'Authors'
--- Authors	2011-06-02 17:16:06 +0000
+++ Authors	2011-06-03 21:43:32 +0000
@@ -31,6 +31,7 @@
 Hisaki Ohara <[e-mail]>
 Ilya Alekseyev <[e-mail]>
 Isaku Yamahata <[e-mail]>
+Jason Cannavale <[e-mail]>
 Jason Koelker <[e-mail]>
 Jay Pipes <[e-mail]>
 Jesse Andrews <[e-mail]>

=== added directory 'build'
=== added directory 'dist'
=== added directory 'nova.egg-info'
=== added file 'nova/db/sqlalchemy/migrate_repo/versions/022_set_engine_mysql_innodb.py'
--- nova/db/sqlalchemy/migrate_repo/versions/022_set_engine_mysql_innodb.py	1970-01-01 00:00:00 +0000
+++ nova/db/sqlalchemy/migrate_repo/versions/022_set_engine_mysql_innodb.py	2011-06-03 21:43:32 +0000
@@ -0,0 +1,65 @@
+# vim: tabstop=4 shiftwidth=4 softtabstop=4
+
+# Copyright 2011 OpenStack LLC.
+#
+#    Licensed under the Apache License, Version 2.0 (the "License"); you may
+#    not use this file except in compliance with the License. You may obtain
+#    a copy of the License at
+#
+#         http://www.apache.org/licenses/LICENSE-2.0
+#
+#    Unless required by applicable law or agreed to in writing, software
+#    distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+#    WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+#    License for the specific language governing permissions and limitations
+#    under the License.
+
+from sqlalchemy import MetaData, Table
+
+meta = MetaData()
+
+
+def upgrade(migrate_engine):
+    # Upgrade operations go here. Don't create your own engine;
+    # bind migrate_engine to your metadata
+    meta.bind = migrate_engine
+    if migrate_engine.name == "mysql":
+        migrate_engine.execute("ALTER TABLE auth_tokens Engine=InnoDB")
+        migrate_engine.execute("ALTER TABLE certificates Engine=InnoDB")
+        migrate_engine.execute("ALTER TABLE compute_nodes Engine=InnoDB")
+        migrate_engine.execute("ALTER TABLE console_pools Engine=InnoDB")
+        migrate_engine.execute("ALTER TABLE consoles Engine=InnoDB")
+        migrate_engine.execute("ALTER TABLE export_devices Engine=InnoDB")
+        migrate_engine.execute("ALTER TABLE fixed_ips Engine=InnoDB")
+        migrate_engine.execute("ALTER TABLE floating_ips Engine=InnoDB")
+        migrate_engine.execute("ALTER TABLE instance_actions Engine=InnoDB")
+        migrate_engine.execute("ALTER TABLE instance_metadata Engine=InnoDB")
+        migrate_engine.execute("ALTER TABLE instance_types Engine=InnoDB")
+        migrate_engine.execute("ALTER TABLE instances Engine=InnoDB")
+        migrate_engine.execute("ALTER TABLE iscsi_targets Engine=InnoDB")
+        migrate_engine.execute("ALTER TABLE key_pairs Engine=InnoDB")
+        migrate_engine.execute("ALTER TABLE migrate_version Engine=InnoDB")
+        migrate_engine.execute("ALTER TABLE migrations Engine=InnoDB")
+        migrate_engine.execute("ALTER TABLE networks Engine=InnoDB")
+        migrate_engine.execute("ALTER TABLE projects Engine=InnoDB")
+        migrate_engine.execute("ALTER TABLE quotas Engine=InnoDB")
+        migrate_engine.execute(
+         "ALTER TABLE security_group_instance_association Engine=InnoDB")
+        migrate_engine.execute(
+         "ALTER TABLE security_group_rules Engine=InnoDB")
+        migrate_engine.execute("ALTER TABLE security_groups Engine=InnoDB")
+        migrate_engine.execute("ALTER TABLE services Engine=InnoDB")
+        migrate_engine.execute(
+         "ALTER TABLE user_project_association Engine=InnoDB")
+        migrate_engine.execute(
+         "ALTER TABLE user_project_role_association Engine=InnoDB")
+        migrate_engine.execute(
+         "ALTER TABLE user_role_association Engine=InnoDB")
+        migrate_engine.execute("ALTER TABLE users Engine=InnoDB")
+        migrate_engine.execute("ALTER TABLE volumes Engine=InnoDB")
+        migrate_engine.execute("ALTER TABLE zones Engine=InnoDB")
+        migrate_engine.execute("ALTER TABLE snapshots Engine=InnoDB")
+
+
+def downgrade(migrate_engine):
+    meta.bind = migrate_engine