commit 6cc500974d958982e6692d87a6a99354d9f2b50a Author: David Ames Date: Tue Oct 6 15:58:35 2020 -0700 Handle attempts to write to a RO node Though the charm took pains to avoid it there are states the charm can get into in which it attempts to write to a read only node. This change catches the OperationalError error code 1290: "The MySQL server is running with the --super-read-only option so it cannot execute this statement" gracefully. Change-Id: If9188f5e72701f4bd7575b09217f355fa3d505a2 Closes-Bug: #1882205 diff --git a/src/lib/charm/openstack/mysql_innodb_cluster.py b/src/lib/charm/openstack/mysql_innodb_cluster.py index c657999..25e61dc 100644 --- a/src/lib/charm/openstack/mysql_innodb_cluster.py +++ b/src/lib/charm/openstack/mysql_innodb_cluster.py @@ -194,6 +194,9 @@ class MySQLInnoDBClusterCharm(charms_openstack.charm.OpenStackCharm): # For caching cluster status information _cached_cluster_status = None + _read_only_error = 1290 + _user_create_failed = 1396 + @property def mysqlsh_bin(self): """Determine binary path for MySQL Shell. @@ -488,8 +491,8 @@ class MySQLInnoDBClusterCharm(charms_openstack.charm.OpenStackCharm): :param cluster_password: Cluster user's password :type cluster_password: str :side effect: Executes SQL to create DB user - :returns: This function is called for its side effect - :rtype: None + :returns: True if successful, False if there are failures + :rtype: Boolean """ SQL_CLUSTER_USER_CREATE = ( "CREATE USER '{user}'@'{host}' " @@ -517,22 +520,34 @@ class MySQLInnoDBClusterCharm(charms_openstack.charm.OpenStackCharm): host=address, password=cluster_password) ) - except mysql.MySQLdb._exceptions.OperationalError: - ch_core.hookenv.log("User {} already exists." - .format(cluster_user), "WARNING") - - m_helper.execute(SQL_CLUSTER_USER_GRANT.format( - permissions="ALL PRIVILEGES", - user=cluster_user, - host=address) - ) - m_helper.execute(SQL_CLUSTER_USER_GRANT.format( - permissions="GRANT OPTION", - user=cluster_user, - host=address) - ) - - m_helper.execute("flush privileges") + m_helper.execute(SQL_CLUSTER_USER_GRANT.format( + permissions="ALL PRIVILEGES", + user=cluster_user, + host=address) + ) + m_helper.execute(SQL_CLUSTER_USER_GRANT.format( + permissions="GRANT OPTION", + user=cluster_user, + host=address) + ) + + m_helper.execute("flush privileges") + except mysql.MySQLdb._exceptions.OperationalError as e: + if e.args[0] == self._read_only_error: + ch_core.hookenv.log( + "Attempted to write to the RO node: {} in " + "configure_db_for_hosts. Skipping." + .format(m_helper.connection.get_host_info()), + "WARNING") + return False + if e.args[0] == self._user_create_failed: + ch_core.hookenv.log( + "User {} exists." + .format(cluster_user), "WARNING") + continue + else: + raise + return True def configure_instance(self, address): """Configure MySQL instance for clustering. @@ -874,10 +889,12 @@ class MySQLInnoDBClusterCharm(charms_openstack.charm.OpenStackCharm): "create cluster user for {}.".format(address)) # Make sure we have the user in the DB for unit in cluster.all_joined_units: - self.create_cluster_user( - unit.received['cluster-address'], - unit.received['cluster-user'], - unit.received['cluster-password']) + if not self.create_cluster_user( + unit.received['cluster-address'], + unit.received['cluster-user'], + unit.received['cluster-password']): + raise Exception( + "Not all cluster users created.") self.configure_instance(address) self.add_instance_to_cluster(address) @@ -1119,17 +1136,20 @@ class MySQLInnoDBClusterCharm(charms_openstack.charm.OpenStackCharm): allowed_units = " ".join( [x.unit_name for x in unit.relation.joined_units]) - interface.set_db_connection_info( - unit.relation.relation_id, - db_host, - password, - allowed_units=allowed_units, - prefix=prefix, - wait_timeout=self.options.wait_timeout, - ssl_ca=self.ssl_ca) + # Only set relation data if db/user create was successful + if password: + interface.set_db_connection_info( + unit.relation.relation_id, + db_host, + password, + allowed_units=allowed_units, + prefix=prefix, + wait_timeout=self.options.wait_timeout, + ssl_ca=self.ssl_ca) # Validate that all attempts succeeded. - # i.e. We were not attempting writes during a topology change. + # i.e. We were not attempting writes during a topology change, + # we are not attempting to write to a read only node. if all(completed): return True return False @@ -1179,7 +1199,18 @@ class MySQLInnoDBClusterCharm(charms_openstack.charm.OpenStackCharm): return for host in hosts: - password = rw_helper.configure_db(host, database, username) + try: + password = rw_helper.configure_db(host, database, username) + except mysql.MySQLdb._exceptions.OperationalError as e: + if e.args[0] == self._read_only_error: + password = None + ch_core.hookenv.log( + "Attempted to write to the RO node: {} in " + "configure_db_for_hosts. Skipping." + .format(rw_helper.connection.get_host_info()), + "WARNING") + else: + raise return password @@ -1225,7 +1256,18 @@ class MySQLInnoDBClusterCharm(charms_openstack.charm.OpenStackCharm): return for host in hosts: - password = rw_helper.configure_router(host, username) + try: + password = rw_helper.configure_router(host, username) + except mysql.MySQLdb._exceptions.OperationalError as e: + if e.args[0] == self._read_only_error: + password = None + ch_core.hookenv.log( + "Attempted to write to the RO node: {} in " + "configure_db_router. Skipping." + .format(rw_helper.connection.get_host_info()), + "WARNING") + else: + raise return password diff --git a/src/reactive/mysql_innodb_cluster_handlers.py b/src/reactive/mysql_innodb_cluster_handlers.py index d1808b9..3a2c75d 100644 --- a/src/reactive/mysql_innodb_cluster_handlers.py +++ b/src/reactive/mysql_innodb_cluster_handlers.py @@ -55,10 +55,13 @@ def create_local_cluster_user(): """ ch_core.hookenv.log("Creating local cluster user.", "DEBUG") with charm.provide_charm_instance() as instance: - instance.create_cluster_user( - instance.cluster_address, - instance.cluster_user, - instance.cluster_password) + if not instance.create_cluster_user( + instance.cluster_address, + instance.cluster_user, + instance.cluster_password): + ch_core.hookenv.log("Local cluster user was not created.", + "WARNING") + return reactive.set_flag("local.cluster.user-created") instance.assess_status() @@ -100,10 +103,12 @@ def create_remote_cluster_user(): ch_core.hookenv.log("Creating remote users.", "DEBUG") with charm.provide_charm_instance() as instance: for unit in cluster.all_joined_units: - instance.create_cluster_user( - unit.received['cluster-address'], - unit.received['cluster-user'], - unit.received['cluster-password']) + if not instance.create_cluster_user( + unit.received['cluster-address'], + unit.received['cluster-user'], + unit.received['cluster-password']): + ch_core.hookenv.log("Not all remote users created.", "WARNING") + return # Optimize clustering by causing a cluster relation changed cluster.set_unit_configure_ready() diff --git a/unit_tests/test_lib_charm_openstack_mysql_innodb_cluster.py b/unit_tests/test_lib_charm_openstack_mysql_innodb_cluster.py index 6ea6bb8..c486e29 100644 --- a/unit_tests/test_lib_charm_openstack_mysql_innodb_cluster.py +++ b/unit_tests/test_lib_charm_openstack_mysql_innodb_cluster.py @@ -21,6 +21,13 @@ import charms_openstack.test_utils as test_utils import charm.openstack.mysql_innodb_cluster as mysql_innodb_cluster +class FakeException(Exception): + + def __init__(self, code, message): + self.code = code + self.message = message + + class TestMySQLInnoDBClusterProperties(test_utils.PatchHelper): def setUp(self): @@ -453,16 +460,9 @@ class TestMySQLInnoDBClusterCharm(test_utils.PatchHelper): _localhost = "localhost" _helper.reset_mock() self.get_relation_ip.return_value = _addr - midbc.create_cluster_user(_addr, _user, _pass) + midbc.create_cluster_user(_localhost, _user, _pass) _calls = [ mock.call("CREATE USER '{}'@'{}' IDENTIFIED BY '{}'" - .format(_user, _addr, _pass)), - mock.call("GRANT ALL PRIVILEGES ON *.* TO '{}'@'{}'" - .format(_user, _addr)), - mock.call("GRANT GRANT OPTION ON *.* TO '{}'@'{}'" - .format(_user, _addr)), - mock.call('flush privileges'), - mock.call("CREATE USER '{}'@'{}' IDENTIFIED BY '{}'" .format(_user, _localhost, _pass)), mock.call("GRANT ALL PRIVILEGES ON *.* TO '{}'@'{}'" .format(_user, _localhost)), @@ -472,6 +472,30 @@ class TestMySQLInnoDBClusterCharm(test_utils.PatchHelper): _helper.execute.assert_has_calls( _calls, any_order=True) + # Exception handling + self.patch_object( + mysql_innodb_cluster.mysql.MySQLdb, "_exceptions") + self._exceptions.OperationalError = FakeException + + # User Exists + _helper.reset_mock() + _helper.execute.side_effect = ( + self._exceptions.OperationalError(1396, "User exists")) + self.assertTrue(midbc.create_cluster_user(_localhost, _user, _pass)) + + # Read only node + _helper.reset_mock() + _helper.execute.side_effect = ( + self._exceptions.OperationalError(1290, "Super read only")) + self.assertFalse(midbc.create_cluster_user(_localhost, _user, _pass)) + + # Unhandled Exception + _helper.reset_mock() + _helper.execute.side_effect = ( + self._exceptions.OperationalError(99999, "BROKEN")) + with self.assertRaises(FakeException): + midbc.create_cluster_user(_localhost, _user, _pass) + def test_configure_instance(self): _pass = "clusterpass" _addr = "10.10.30.30" @@ -694,6 +718,18 @@ class TestMySQLInnoDBClusterCharm(test_utils.PatchHelper): self.interface.set_db_connection_info.assert_has_calls( _set_calls, any_order=True) + # DB/User create is unsuccessful + midbc.configure_db_for_hosts.reset_mock() + midbc.configure_db_for_hosts.side_effect = None + midbc.configure_db_for_hosts.return_value = None + midbc.configure_db_router.side_effect = None + midbc.configure_db_router.return_value = None + + # Execute the function under test expect incomplete + self.interface.set_db_connection_info.reset_mock() + self.assertFalse(midbc.create_databases_and_users(self.interface)) + self.interface.set_db_connection_info.assert_not_called() + def test_create_databases_and_users_db_router(self): # The test setup is a bit convoluted and requires mimicking reactive, # however, this is the heart of the charm and therefore deserves to @@ -820,6 +856,18 @@ class TestMySQLInnoDBClusterCharm(test_utils.PatchHelper): self.interface.set_db_connection_info.assert_has_calls( _set_calls, any_order=True) + # DB/User create is unsuccessful + midbc.configure_db_router.reset_mock() + midbc.configure_db_for_hosts.side_effect = None + midbc.configure_db_for_hosts.return_value = None + midbc.configure_db_router.side_effect = None + midbc.configure_db_router.return_value = None + + # Execute the function under test expect incomplete + self.interface.set_db_connection_info.reset_mock() + self.assertFalse(midbc.create_databases_and_users(self.interface)) + self.interface.set_db_connection_info.assert_not_called() + def test_configure_db_for_hosts(self): _db = "db" _user = "user" @@ -857,6 +905,26 @@ class TestMySQLInnoDBClusterCharm(test_utils.PatchHelper): _helper.configure_db.assert_has_calls( _calls, any_order=True) + # Exception handling + self.patch_object( + mysql_innodb_cluster.mysql.MySQLdb, "_exceptions") + self._exceptions.OperationalError = FakeException + + # Super read only + _helper.reset_mock() + _helper.configure_db.side_effect = ( + self._exceptions.OperationalError(1290, "Super REad only")) + self.assertEqual( + None, + midbc.configure_db_for_hosts(_json_addrs, _db, _user)) + + # Unhandled Exception + _helper.reset_mock() + _helper.configure_db.side_effect = ( + self._exceptions.OperationalError(999, "BROKEN")) + with self.assertRaises(FakeException): + midbc.configure_db_for_hosts(_json_addrs, _db, _user) + def test_configure_db_router(self): _user = "user" _addr = "10.10.90.90" @@ -893,6 +961,26 @@ class TestMySQLInnoDBClusterCharm(test_utils.PatchHelper): _helper.configure_router.assert_has_calls( _calls, any_order=True) + # Exception handling + self.patch_object( + mysql_innodb_cluster.mysql.MySQLdb, "_exceptions") + self._exceptions.OperationalError = FakeException + + # Super read only + _helper.reset_mock() + _helper.configure_router.side_effect = ( + self._exceptions.OperationalError(1290, "Super REad only")) + self.assertEqual( + None, + midbc.configure_db_router(_json_addrs, _user)) + + # Unhandled Exception + _helper.reset_mock() + _helper.configure_router.side_effect = ( + self._exceptions.OperationalError(999, "BROKEN")) + with self.assertRaises(FakeException): + midbc.configure_db_router(_json_addrs, _user) + def test_states_to_check(self): self.patch_object( mysql_innodb_cluster.charms_openstack.charm.OpenStackCharm, @@ -1445,6 +1533,7 @@ class TestMySQLInnoDBClusterCharm(test_utils.PatchHelper): "cluster-password": _pass, } _create_cluster_user = mock.MagicMock() + _create_cluster_user.return_value = True midbc.create_cluster_user = _create_cluster_user _configure_instance = mock.MagicMock() midbc.configure_instance = _configure_instance @@ -1456,3 +1545,8 @@ class TestMySQLInnoDBClusterCharm(test_utils.PatchHelper): _remote_addr, _user, _pass) _configure_instance.assert_called_once_with(_remote_addr) _add_instance_to_cluster.assert_called_once_with(_remote_addr) + + # Not all users created + _create_cluster_user.return_value = False + with self.assertRaises(Exception): + midbc.configure_and_add_instance(address=_remote_addr) diff --git a/unit_tests/test_mysql_innodb_cluster_handlers.py b/unit_tests/test_mysql_innodb_cluster_handlers.py index 642a05b..e051d8e 100644 --- a/unit_tests/test_mysql_innodb_cluster_handlers.py +++ b/unit_tests/test_mysql_innodb_cluster_handlers.py @@ -143,6 +143,7 @@ class TestMySQLInnoDBClusterHandlers(test_utils.PatchHelper): self.set_flag.assert_called_once_with("charm.installed") def test_create_local_cluster_user(self): + self.midbc.create_cluster_user.return_value = True handlers.create_local_cluster_user() self.midbc.create_cluster_user.assert_called_once_with( self.midbc.cluster_address, @@ -150,6 +151,12 @@ class TestMySQLInnoDBClusterHandlers(test_utils.PatchHelper): self.midbc.cluster_password) self.set_flag.assert_called_once_with("local.cluster.user-created") + # Not successful + self.midbc.create_cluster_user.return_value = False + self.set_flag.reset_mock() + handlers.create_local_cluster_user() + self.set_flag.assert_not_called() + def test_send_cluster_connection_info(self): self.endpoint_from_flag.return_value = self.cluster handlers.send_cluster_connection_info() @@ -165,6 +172,7 @@ class TestMySQLInnoDBClusterHandlers(test_utils.PatchHelper): self.data = {"cluster-address": _addr, "cluster-user": _user, "cluster-password": _pass} + self.midbc.create_cluster_user.return_value = True self.endpoint_from_flag.return_value = self.cluster handlers.create_remote_cluster_user() self.midbc.create_cluster_user.assert_called_once_with( @@ -173,6 +181,12 @@ class TestMySQLInnoDBClusterHandlers(test_utils.PatchHelper): self.set_flag.assert_called_once_with( "local.cluster.all-users-created") + # Not successful + self.midbc.create_cluster_user.return_value = False + self.cluster.set_unit_configure_ready.reset_mock() + handlers.create_remote_cluster_user() + self.cluster.set_unit_configure_ready.assert_not_called() + def test_initialize_cluster(self): handlers.initialize_cluster() self.midbc.configure_instance.assert_called_once_with(