Merge lp:~hopem/charms/trusty/percona-cluster/fix-1389670 into lp:~openstack-charmers-archive/charms/trusty/percona-cluster/trunk

[Edward Hope-Morley]

[lp:~hopem/charms/trusty/percona-cluster/fix-1389670]

45. By Edward Hope-Morley on 2015-01-26

more log cleanup

46. By Edward Hope-Morley on 2015-01-27

fixed no hostname case and updated unit test

[lp:~hopem/charms/trusty/percona-cluster/fix-1389670]

47. By Edward Hope-Morley on 2015-01-28

cleanup

[Liam Young (gnuoy)]

Approve

review: Approve

=== modified file 'hooks/mysql.py'
--- hooks/mysql.py	2014-11-24 12:12:15 +0000
+++ hooks/mysql.py	2015-01-28 18:54:36 +0000
@@ -11,11 +11,11 @@
 from charmhelpers.core.host import pwgen, mkdir, write_file
 from charmhelpers.core.hookenv import (
     relation_get,
-    relation_ids,
     related_units,
     service_name,
     unit_get,
     log,
+    DEBUG,
     INFO,
 )
 from charmhelpers.core.hookenv import config as config_get
@@ -331,36 +331,38 @@
     return mysql_config
 
 
-def get_allowed_units(database, username):
+def get_allowed_units(database, username, relation_id=None):
     m_helper = MySQLHelper()
     m_helper.connect(password=get_mysql_root_password())
     allowed_units = set()
-    for relid in relation_ids('shared-db'):
-        for unit in related_units(relid):
-            hosts = relation_get(attribute="%s_%s" % (database, 'hostname'),
-                                 unit=unit, rid=relid)
-            if not hosts:
-                hosts = [relation_get(attribute='private-address', unit=unit,
-                                      rid=relid)]
-            else:
-                # hostname can be json-encoded list of hostnames
-                try:
-                    _hosts = json.loads(hosts)
-                except ValueError:
-                    pass
+    for unit in related_units(relation_id):
+        settings = relation_get(rid=relation_id, unit=unit)
+        # First check for setting with prefix, then without
+        for attr in ["%s_hostname" % (database), 'hostname']:
+            hosts = settings.get(attr, None)
+            if hosts:
+                break
+
+        if hosts:
+            # hostname can be json-encoded list of hostnames
+            try:
+                hosts = json.loads(hosts)
+            except ValueError:
+                hosts = [hosts]
+        else:
+            hosts = [settings['private-address']]
+
+        if hosts:
+            for host in hosts:
+                if m_helper.grant_exists(database, username, host):
+                    log("Grant exists for host '%s' on db '%s'" %
+                        (host, database), level=DEBUG)
+                    if unit not in allowed_units:
+                        allowed_units.add(unit)
                 else:
-                    hosts = _hosts
-
-            if not isinstance(hosts, list):
-                hosts = [hosts]
-
-            if hosts:
-                for host in hosts:
-                    log("Checking host '%s' grant" % (host), level=INFO)
-                    if m_helper.grant_exists(database, username, host):
-                        if unit not in allowed_units:
-                            allowed_units.add(unit)
-            else:
-                log("No hosts found for grant check", level=INFO)
+                    log("Grant does NOT exist for host '%s' on db '%s'" %
+                        (host, database), level=DEBUG)
+        else:
+            log("No hosts found for grant check", level=INFO)
 
     return allowed_units

=== modified file 'hooks/percona_hooks.py'
--- hooks/percona_hooks.py	2014-12-17 18:59:55 +0000
+++ hooks/percona_hooks.py	2015-01-28 18:54:36 +0000
@@ -18,12 +18,12 @@
     config,
     remote_unit,
     relation_type,
+    DEBUG,
     INFO,
 )
 from charmhelpers.core.host import (
     service_restart,
     file_hash,
-    write_file,
     lsb_release,
 )
 from charmhelpers.core.templating import render
@@ -231,6 +231,23 @@
     return unit_get('private-address')
 
 
+def configure_db_for_hosts(hosts, database, username):
+    """Hosts may be a json-encoded list of hosts or a single hostname."""
+    try:
+        hosts = json.loads(hosts)
+        log("Multiple hostnames provided by relation: %s" % (', '.join(hosts)),
+            level=DEBUG)
+    except ValueError:
+        log("Single hostname provided by relation: %s" % (hosts),
+            level=DEBUG)
+        hosts = [hosts]
+
+    for host in hosts:
+        password = configure_db(host, database, username)
+
+    return password
+
+
 # TODO: This could be a hook common between mysql and percona-cluster
 @hooks.hook('shared-db-relation-changed')
 def shared_db_changed(relation_id=None, unit=None):
@@ -270,25 +287,18 @@
         database = settings['database']
         username = settings['username']
 
-        # Hostname can be json-encoded list of hostnames
-        try:
-            hostname = json.loads(hostname)
-        except ValueError:
-            pass
-
-        if isinstance(hostname, list):
-            for host in hostname:
-                password = configure_db(host, database, username)
-        else:
-            password = configure_db(hostname, database, username)
-
-        allowed_units = unit_sorted(get_allowed_units(database, username))
+        # NOTE: do this before querying access grants
+        password = configure_db_for_hosts(hostname, database, username)
+
+        allowed_units = unit_sorted(get_allowed_units(database, username,
+                                                      relation_id=relation_id))
         allowed_units = ' '.join(allowed_units)
+        relation_set(relation_id=relation_id, allowed_units=allowed_units)
+
         db_host = get_db_host(hostname)
         peer_store_and_set(relation_id=relation_id,
                            db_host=db_host,
-                           password=password,
-                           allowed_units=allowed_units)
+                           password=password)
     else:
         # Process multiple database setup requests.
         # from incoming relation data:
@@ -316,34 +326,36 @@
                 databases[db] = {}
             databases[db][x] = v
 
+        allowed_units = {}
         return_data = {}
         for db in databases:
             if singleset.issubset(databases[db]):
                 database = databases[db]['database']
                 hostname = databases[db]['hostname']
                 username = databases[db]['username']
-                try:
-                    hostname = json.loads(hostname)
-                except ValueError:
-                    pass
-
-                if isinstance(hostname, list):
-                    for host in hostname:
-                        password = configure_db(host, database, username)
-                else:
-                    password = configure_db(hostname, database, username)
-
-                return_data['_'.join([db, 'password'])] = password
-                return_data['_'.join([db, 'allowed_units'])] = \
-                    " ".join(unit_sorted(get_allowed_units(database,
-                                                           username)))
+
+                # NOTE: do this before querying access grants
+                password = configure_db_for_hosts(hostname, database, username)
+
+                a_units = get_allowed_units(database, username,
+                                            relation_id=relation_id)
+                a_units = ' '.join(unit_sorted(a_units))
+                allowed_units['%s_allowed_units' % (db)] = a_units
+
+                return_data['%s_password' % (db)] = password
                 db_host = get_db_host(hostname)
 
-        if len(return_data) > 0:
-            peer_store_and_set(relation_id=relation_id,
+        if allowed_units:
+            relation_set(relation_id=relation_id, **allowed_units)
+        else:
+            log("No allowed_units - not setting relation settings",
+                level=DEBUG)
+
+        if return_data:
+            peer_store_and_set(relation_id=relation_id, db_host=db_host,
                                **return_data)
-            peer_store_and_set(relation_id=relation_id,
-                               db_host=db_host)
+        else:
+            log("No return data - not setting relation settings", level=DEBUG)
 
     peer_store_and_set(relation_id=relation_id,
                        relation_settings={'access-network': access_network})

=== modified file 'hooks/percona_utils.py'
--- hooks/percona_utils.py	2014-12-17 21:03:54 +0000
+++ hooks/percona_utils.py	2015-01-28 18:54:36 +0000
@@ -16,7 +16,7 @@
     local_unit,
     config,
     log,
-    INFO
+    DEBUG,
 )
 from charmhelpers.fetch import (
     apt_install,
@@ -104,11 +104,12 @@
                 if not hostname or hostname in hosts:
                     log("(unit=%s) Ignoring hostname '%s' provided by cluster "
                         "relation for addr %s" %
-                        (unit, hostname, private_address))
+                        (unit, hostname, private_address), level=DEBUG)
                     continue
                 else:
                     log("(unit=%s) hostname '%s' provided by cluster relation "
-                        "for addr %s" % (unit, hostname, private_address))
+                        "for addr %s" % (unit, hostname, private_address),
+                        level=DEBUG)
 
                 hosts_map[private_address] = hostname
                 hosts.append(hostname)
@@ -175,8 +176,8 @@
     with open(HOSTS_FILE, 'r') as hosts:
         lines = hosts.readlines()
 
-    log("Updating hosts file with: %s (current: %s)" % (map, lines),
-        level=INFO)
+    log("Updating %s with: %s (current: %s)" % (HOSTS_FILE, map, lines),
+        level=DEBUG)
 
     newlines = []
     for ip, hostname in map.items():
@@ -189,7 +190,8 @@
             if len(line) < 2 or not (_line[0] == ip or hostname in _line[1:]):
                 keepers.append(line)
             else:
-                log("Removing line '%s' from hosts file" % (line))
+                log("Marking line '%s' for update or removal" % (line.strip()),
+                    level=DEBUG)
 
         lines = keepers
         newlines.append("%s %s\n" % (ip, hostname))

=== modified file 'unit_tests/test_mysql.py'
--- unit_tests/test_mysql.py	2014-10-15 16:09:41 +0000
+++ unit_tests/test_mysql.py	2015-01-28 18:54:36 +0000
@@ -15,27 +15,39 @@
     @mock.patch('mysql.MySQLHelper', autospec=True)
     @mock.patch('mysql.relation_get')
     @mock.patch('mysql.related_units')
-    @mock.patch('mysql.relation_ids')
     @mock.patch('mysql.log')
-    def test_get_allowed_units(self, mock_log, mock_relation_ids,
-                               mock_related_units, 
+    def test_get_allowed_units(self, mock_log, mock_related_units, 
                                mock_relation_get, mock_helper,
                                mock_get_password):
-        mock_relation_ids.return_value = ['r1']
-        mock_related_units.return_value = ['ru1', 'ru2']
-
-        def mock_rel_get(attribute, unit, rid):
-            if unit == 'ru2':
-                d = {'private-address': '1.2.3.4',
-                     'dbA_hostname': json.dumps(['2.3.4.5', '6.7.8.9'])}
-            else:
-                d = {'private-address': '1.2.3.4'}
-
-            return d.get(attribute, None)
+
+        def mock_rel_get(unit, rid):
+            if unit == 'unit/0':
+                # Non-prefixed settings
+                d = {'private-address': '10.0.0.1',
+                     'hostname': 'hostA'}
+            elif unit == 'unit/1':
+                # Containing prefixed settings
+                d = {'private-address': '10.0.0.2',
+                     'dbA_hostname': json.dumps(['10.0.0.2', '2001:db8:1::2'])}
+            elif unit == 'unit/2':
+                # No hostname
+                d = {'private-address': '10.0.0.3'}
+
+            return d
 
         mock_relation_get.side_effect = mock_rel_get
+        mock_related_units.return_value = ['unit/0', 'unit/1', 'unit/2']
+
         units = mysql.get_allowed_units('dbA', 'userA')
-        mock_helper.return_value.grant_exists.assert_called_with('dbA',
-                                                                 'userA',
-                                                                 '6.7.8.9')
-        self.assertEqual(units, set(['ru1', 'ru2']))
+
+        calls = [mock.call('dbA', 'userA', 'hostA'),
+                 mock.call().__nonzero__(),
+                 mock.call('dbA', 'userA', '10.0.0.2'),
+                 mock.call().__nonzero__(),
+                 mock.call('dbA', 'userA', '2001:db8:1::2'),
+                 mock.call().__nonzero__(),
+                 mock.call('dbA', 'userA', '10.0.0.3'),
+                 mock.call().__nonzero__()]
+
+        mock_helper.return_value.grant_exists.assert_has_calls(calls)
+        self.assertEqual(units, set(['unit/0', 'unit/1', 'unit/2']))