Merge lp:~nick-moffitt/charms/precise/keystone/migrate-sqlite-to-mysql into lp:~charmers/charms/precise/keystone/trunk

[Nick Moffitt]

This change uses sqlfairy (http://packages.ubuntu.com/precise/libsql-translator-perl) to migrate the schema and then all rows from the default sqlite database over to the new database when the mysql relation fires for the first time. The rationale is that users may not realize that they're using sqlite until it's too late, and adding the relation caused a loss of all authentication data.

The SQL generated from sqlfairy needed some manipulation in order to work.
In particular:

    * "UNIQUE `` (`name`)" commands needed a UID of some sort between the ``
      for mysql to accept them
    * As we're running these mysql 'LOAD DATA' commands from a remote
      client, 'LOAD DATA INFILE' needed to become 'LOAD DATA LOCAL INFILE',
      and the mysql client needed to be explicitly told to allow this
      syntax.

The install hook edits configuration entries, risking loss of idempotence. Instead of just re-running it in the upgrade hook, I've replicated the apt-get commands.

[Adam Gandelman (gandelman-a)]

Hey Nick-

Thanks for submitting. This is quite useful. Tested using a sqlite database /w a few thousand users and the migration worked great. I'm just a bit confused about the last bit WRT upgrade. The added 'apt-get' commands you added to the install hook, which currently isn't called on charm upgrade. Did you mean to add a new upgrade hook that re-runs apt-get update only?

review: Needs Information

[Nick Moffitt (nick-moffitt)]

Adam, thanks for helping to test this!

In my initial use case, we had a system that we realized with some shock had been running with sqlite, and we were about to see an increase in the volume of the database. Since this charm was already running on a production system, I needed to make sure that the new package dependencies (sqlfairy itself, for instance) were installed before the relation hook fired.

In my understanding, no configuration steps should be taken in install hooks, since a fresh install will get a config-changed after the install hook fires anyway. Unfortunately this charm seemed to have done some config steps in the install hook, so I was nervous about trusting its idempotence and calling it from my upgrade hook.

Since this was meant to roll out a change to our production service, I took the simplest step forward that would obviously work correctly. So that's why there's duplication of the apt-get steps in the upgrade hook function.

I would love to see that bit detangled somewhat, but it seemed out of scope for this merge.

[Nick Moffitt (nick-moffitt)]

Oh, I see now that I may have mispasted during the merge! I'll tidy that up.

[Adam Gandelman (gandelman-a)]

Nick-

Yes, you're right about the config options being set in the install hook being a bug. They were originally there to work on old deficiencies in packaging that are fixed now, and could probably be dropped from there / moved to config_changed.

That said, I'm not sure about duplicating the apt-get commands wholesale in upgrade-charm. As its proposed here, if there has been a newer package published to the archive for Keystone (or any other package that may have charm modified configs) since the last charm upgrade, apt-get will block waiting for user guidance on how to handle the config changes (merge, overwrite, shell, etc) and the upgrade hook will never complete. I see a couple of options:

- Call apt-get /w "--option Dpkg::Options::=--force-confnew -n" to ensure we keep our juju modified configs in place. This could be problematic, though, if the updated packages contain critical config file changes, too.

- Avoid calling apt-get in the upgrade hook and instead late install the packages required for migration as needed in migrate_db(), similar to how subprocess is being imported late there.

The latter seems safest to me, since we're only acting on the migration related packages, and ideally only gets run once upon relating to a new mysql server. What do you think?

review: Needs Fixing

[Nick Moffitt (nick-moffitt)]

So it sounds like even the apt commands in the install hook are not idempotent! If that's the case, how do we solve this more generally?

[Nick Moffitt (nick-moffitt)]

And I think you're right about the apt commands being best suited to the particular hook. I'll have a go at that next.

[Adam Gandelman (gandelman-a)]

Marking 'Rejected' to get it out of the charm review queue, feel free to mark as 'Needs Review' when ready.

[Nick Moffitt (nick-moffitt)]

I've updated my branch, but this MP seems to still have the old diff. I've marked it back to "needs review" in the hopes that it will catch up as a result.

[Juan L. Negron (negronjl)]

I'm deferring this to Adam.

-Juan

[Adam Gandelman (gandelman-a)]

Hey Nick-- Sorry for the delay on this. We're in the process of testing and merging back 6 mo. worth of charm work to the main cs:precise/keystone branch. I'm hesitant to merge this until thats done, but the changes you've committed in response to my comments look good. We will hopefully have the main branch updated before next week, and I'll merge this in as soon as we do.

[Adam Gandelman (gandelman-a)]

Hey Nick-

We've landed all of the pending keystone stuff. Any chance you can rebase this against current state of lp:charms/keystone? We'll be busy for a while migrating existing shell-based openstack stuff to python, so the keystone charm should be relatively quite in the meantime. Thanks

Unmerged revisions

42. By Nick Moffitt on 2013-04-26

Moved sqlfairy installation insurance down to the db_changed hook, so that it happens only when it's needed.
This is in response to https://code.launchpad.net/~nick-moffitt/charms/precise/keystone/migrate-sqlite-to-mysql/+merge/149334/comments/327313

41. By Nick Moffitt on 2013-02-26

Fix the half-finished upgrade-charm work.

40. By Nick Moffitt on 2013-02-19

Automatically migrate the database schema and contents when moving from sqlite to mysql.

This change uses sqlfairy (http://packages.ubuntu.com/precise/libsql-translator-perl) to migrate the schema and then all rows from the default sqlite database over to the new database when the mysql relation fires for the first time. The rationale is that users may not realize that they're using sqlite until it's too late, and adding the relation caused a loss of all authentication data.

The SQL generated from sqlfairy needed some manipulation in order to work.
In particular:

    * "UNIQUE `` (`name`)" commands needed a UID of some sort between the ``
      for mysql to accept them
    * As we're running these mysql 'LOAD DATA' commands from a remote
      client, 'LOAD DATA INFILE' needed to become 'LOAD DATA LOCAL INFILE',
      and the mysql client needed to be explicitly told to allow this
      syntax.

The install hook edits configuration entries, risking loss of idempotence. Instead of just re-running it in the upgrade hook, I've replicated the apt-get commands.

=== modified file 'hooks/keystone-hooks'
--- hooks/keystone-hooks	2012-12-12 03:52:01 +0000
+++ hooks/keystone-hooks	2013-02-26 18:13:22 +0000
@@ -8,7 +8,7 @@
 
 config = config_get()
 
-packages = "keystone python-mysqldb pwgen"
+packages = "keystone python-mysqldb pwgen sqlfairy libdbd-sqlite3-perl mysql-client-core-5.5"
 service = "keystone"
 
 # used to verify joined services are valid openstack components.
@@ -75,6 +75,12 @@
     time.sleep(5)
     ensure_initial_admin(config)
 
+def upgrade_charm():
+    # ensure we have all necessary packages for any relations or config
+    # changes.
+    execute("apt-get update", die=True)
+    execute("apt-get -y install %s" % packages, die=True, echo=True)
+
 def db_joined():
     relation_data = { "database": config["database"],
                       "username": config["database-user"],
@@ -82,31 +88,91 @@
     relation_set(relation_data)
 
 def db_changed():
+    def migrate_db(db_uri, relation_data):
+        import subprocess
+        schemacmd = ['sqlt',
+                '-f', 'DBI', '--dsn', 'dbi:SQLite:{}'.format(db_uri.path),
+                '-t', 'MySQL',
+                '--add-drop-table']
+        p = subprocess.Popen(schemacmd, stdout=subprocess.PIPE)
+        idx = 0
+        schema = ''
+        for line in p.stdout:
+            if 'UNIQUE ``' in line:
+                # Anonymous indexes collide, so we give a random name
+                line = line.replace('``', '`idx{}`'.format(idx))
+                idx += 1
+            schema += line
+        mysqlcmd = ['mysql', '--local-infile',
+                '--host={private-address}'.format(**relation_data),
+                '--user={database-user}'.format(**config),
+                '--password={password}'.format(**relation_data),
+                '--database={database}'.format(**config)]
+        p = subprocess.Popen(mysqlcmd, stdout=subprocess.PIPE,
+                stdin=subprocess.PIPE)
+        print schema
+        p.communicate(schema)
+        if p.returncode:
+            juju_log("Schema failed to update.")
+            raise subprocess.CalledProcessError(p.returncode, mysqlcmd)
+        # This next step can be confusing: sqlt generates a perl script we
+        # then run to do the actual data migration.
+        make_dumpercmd = ['sqlt',
+                '-f', 'DBI', '--dsn', 'dbi:SQLite:{}'.format(db_uri.path),
+                '-t', 'Dumper',
+                '--use-same-auth']
+        p = subprocess.check_call(make_dumpercmd,
+                stdout=open('db_migrate.pl', 'w'))
+        run_dumpercmd = ['perl', 'db_migrate.pl', '--mysql-loadfile']
+        out = subprocess.check_output(run_dumpercmd)
+        # We need to load data from this system, not the one running mysql
+        loadfiledata = out.replace('LOAD DATA INFILE',
+                'LOAD DATA LOCAL INFILE')
+        p = subprocess.Popen(mysqlcmd, stdout=subprocess.PIPE,
+                stdin=subprocess.PIPE)
+        p.communicate(loadfiledata)
+        if p.returncode:
+            juju_log("Data failed to update.")
+            raise subprocess.CalledProcessError(p.returncode, mysqlcmd)
+
+    from urlparse import urlparse
+    from ConfigParser import ConfigParser
+
     relation_data = relation_get_dict()
     if ('password' not in relation_data or
         'private-address' not in relation_data):
         juju_log("private-address or password not set. Peer not ready, exit 0")
         exit(0)
+
+    keyconf = ConfigParser()
+    keyconf.read(keystone_conf)
+    db_uri = urlparse(keyconf.get('sql','connection'))
+
     update_config_block('sql', connection="mysql://%s:%s@%s/%s" %
                             (config["database-user"],
                              relation_data["password"],
                              relation_data["private-address"],
                              config["database"]))
     execute("service keystone stop", echo=True)
-    execute("keystone-manage db_sync", echo=True)
-    execute("service keystone start")
-    time.sleep(5)
-    ensure_initial_admin(config)
 
-    # If the backend database has been switched to something new and there
-    # are existing identity-service relations,, service entries need to be
-    # recreated in the new database.  Re-executing identity-service-changed
-    # will do this.
-    for id in relation_ids(relation_name='identity-service'):
-        for unit in relation_list(relation_id=id):
-            juju_log("Re-exec'ing identity-service-changed for: %s - %s" %
-                     (id, unit))
-            identity_changed(relation_id=id, remote_unit=unit)
+    if db_uri.scheme == 'sqlite':
+        juju_log("Migrating schema and data from sqlite to mysql")
+        migrate_db(db_uri, relation_data)
+        execute("service keystone start")
+    else:
+        execute("keystone-manage db_sync", echo=True)
+        execute("service keystone start")
+        time.sleep(5)
+        ensure_initial_admin(config)
+        # If the backend database has been switched to something new and
+        # there are existing identity-service relations,, service entries
+        # need to be recreated in the new database.  Re-executing
+        # identity-service-changed will do this.
+        for id in relation_ids(relation_name='identity-service'):
+            for unit in relation_list(relation_id=id):
+                juju_log("Re-exec'ing identity-service-changed for: %s - %s" %
+                         (id, unit))
+                identity_changed(relation_id=id, remote_unit=unit)
 
 def identity_joined():
     """ Do nothing until we get information about requested service """
@@ -254,6 +320,7 @@
 
 hooks = {
     "install": install_hook,
+    "upgrade-charm": upgrade_charm,
     "shared-db-relation-joined": db_joined,
     "shared-db-relation-changed": db_changed,
     "identity-service-relation-joined": identity_joined,
 
=== added symlink 'hooks/upgrade-charm'
=== target is u'keystone-hooks'