Merge "Improve add/remove actions"

src/actions.yaml

@@ -74,8 +74,12 @@ remove-instance:
     - address
   description: |
      Remove an instance from the cluster. *Note* This action must be run on an
-     instance that is a functioning member of the cluster preferrably the the
+     instance that is a functioning member of the cluster preferably the
-     juju leader to guarantee charm related flags are cleared.
+     juju leader to guarantee charm related flags are cleared. Due to bug
+     LP#1954306, the force parameter should always be used. Additionally,
+     the instance being removed must be either ONLINE without an ERROR state,
+     or it must unreachable (mysql service not running on the node being
+     removed).
 add-instance:
   params:
     address:

src/lib/charm/openstack/mysql_innodb_cluster.py

@@ -715,8 +715,8 @@ class MySQLInnoDBClusterCharm(
         except subprocess.CalledProcessError as e:
             ch_core.hookenv.log(
                 "Failed configuring instance {}: {}"
-                .format(address, e.stderr.decode("UTF-8")), "ERROR")
+                .format(address, self._error_str(e)), "ERROR")
-            return
+            raise
 
         # After configuration of the remote instance, the remote instance
         # restarts mysql. We need to pause here for that to complete.
@@ -812,7 +812,7 @@ class MySQLInnoDBClusterCharm(
         except subprocess.CalledProcessError as e:
             ch_core.hookenv.log(
                 "Failed creating cluster: {}"
-                .format(e.stderr.decode("UTF-8")), "ERROR")
+                .format(self._error_str(e)), "ERROR")
             return
         ch_core.hookenv.log("Cluster Created: {}"
                             .format(output.decode("UTF-8")),
@@ -847,10 +847,9 @@ class MySQLInnoDBClusterCharm(
         except subprocess.CalledProcessError as e:
             ch_core.hookenv.log(
                 "Failed setting cluster option {}={}: {}"
-                .format(key, value, e.stderr.decode("UTF-8")),
+                .format(key, value, self._error_str(e)), "ERROR")
-                "ERROR")
             # Reraise for action handling
-            raise e
+            raise
 
     def get_ip_allowlist_str_from_db(self, m_helper=None):
         """Helper for retrieving ip allow list
@@ -958,7 +957,7 @@ class MySQLInnoDBClusterCharm(
             # Creating separate checks in order to get good logging on each
             # outcome.
             output = None
-            _stderr = e.stderr.decode("UTF-8")
+            _stderr = self._error_str(e)
             if "Clone process has finished" in _stderr:
                 output = e.stderr
                 ch_core.hookenv.log(
@@ -981,7 +980,7 @@ class MySQLInnoDBClusterCharm(
                 ch_core.hookenv.log(
                     "Failed adding instance {} to cluster: {}"
                     .format(address, _stderr), "ERROR")
-                return
+                raise
         ch_core.hookenv.log("Instance Clustered {}: {}"
                             .format(address, output.decode("UTF-8")),
                             level="DEBUG")
@@ -1011,11 +1010,12 @@ class MySQLInnoDBClusterCharm(
         except subprocess.CalledProcessError as e:
             # If the shell reports the server went away we expect this
             # when giving the RESTART command
-            if _server_gone_away_error not in e.stderr.decode("UTF-8"):
+            msg = self._error_str(e)
+            if _server_gone_away_error not in msg:
                 ch_core.hookenv.log(
                     "Failed restarting instance {}: {}"
-                    .format(address, e.stderr.decode("UTF-8")), "ERROR")
+                    .format(address, msg), "ERROR")
-                raise e
+                raise
 
         # After configuration of the remote instance, the remote instance
         # restarts mysql. We need to pause here for that to complete.
@@ -1054,10 +1054,10 @@ class MySQLInnoDBClusterCharm(
         except subprocess.CalledProcessError as e:
             ch_core.hookenv.log(
                 "Failed rebooting from complete outage: {}"
-                .format(e.stderr.decode("UTF-8")),
+                .format(self._error_str(e)),
                 "ERROR")
             # Reraise for action handling
-            raise e
+            raise
 
     def rejoin_instance(self, address):
         """Rejoin instance to the cluster
@@ -1089,10 +1089,10 @@ class MySQLInnoDBClusterCharm(
         except subprocess.CalledProcessError as e:
             ch_core.hookenv.log(
                 "Failed rejoining instance {}: {}"
-                .format(address, e.stderr.decode("UTF-8")),
+                .format(address, self._error_str(e)),
                 "ERROR")
             # Reraise for action handling
-            raise e
+            raise
 
     def remove_instance(self, address, force=False):
         """Remove instance from the cluster
@@ -1137,10 +1137,10 @@ class MySQLInnoDBClusterCharm(
         except subprocess.CalledProcessError as e:
             ch_core.hookenv.log(
                 "Failed removing instance {}: {}"
-                .format(address, e.stderr.decode("UTF-8")),
+                .format(address, self._error_str(e)),
                 "ERROR")
             # Reraise for action handling
-            raise e
+            raise
 
     def cluster_rescan(self):
         """Rescan the cluster
@@ -1169,10 +1169,10 @@ class MySQLInnoDBClusterCharm(
             return output
         except subprocess.CalledProcessError as e:
             ch_core.hookenv.log(
-                "Failed rescanning the cluster.",
+                "Failed rescanning the cluster: {}".format(
-                "ERROR")
+                    self._error_str(e)), "ERROR")
             # Reraise for action handling
-            raise e
+            raise
 
     def configure_and_add_instance(self, address):
         """Configure and add an instance to the cluster.
@@ -1202,8 +1202,20 @@ class MySQLInnoDBClusterCharm(
                     "all"):
                 raise Exception(
                     "Not all cluster users created.")
+
+        configured = False
+        try:
             self.configure_instance(address)
+            configured = True
             self.add_instance_to_cluster(address)
+        except subprocess.CalledProcessError as e:
+            # Addressing special case of bug LP#1983158
+            if (not configured and
+                    "Server in SUPER_READ_ONLY mode" in self._error_str(e)):
+                self.add_instance_to_cluster(address)
+                self.configure_instance(address)
+            else:
+                raise
 
     def get_cluster_status(self, nocache=False, extended=0):
         """Get cluster status
@@ -1280,7 +1292,7 @@ class MySQLInnoDBClusterCharm(
         :rtype: str
         """
         try:
-            return e.stderr.decode()
+            return e.stderr.decode("UTF-8")
         except Exception:
             pass
         return str(e)

unit_tests/test_lib_charm_openstack_mysql_innodb_cluster.py

@@ -15,6 +15,7 @@
 import copy
 import collections
 import re
+import subprocess
 from unittest import mock
 
 import charms_openstack.test_utils as test_utils
@@ -92,7 +93,7 @@ class TestMySQLInnoDBClusterCharm(test_utils.PatchHelper):
 
     def setUp(self):
         super().setUp()
-        self.patch_object(mysql_innodb_cluster, "subprocess")
+        self.subprocess = mock.MagicMock()
         self.patch_object(mysql_innodb_cluster.uuid, "uuid4")
         self.uuid_of_cluster = "uuid-of-cluster"
         self.uuid4.return_value = self.uuid_of_cluster
@@ -410,6 +411,7 @@ class TestMySQLInnoDBClusterCharm(test_utils.PatchHelper):
         midbc = mysql_innodb_cluster.MySQLInnoDBClusterCharm()
         midbc._get_password = mock.MagicMock()
         midbc._get_password.side_effect = self._fake_data
+        mysql_innodb_cluster.subprocess = self.subprocess
         midbc.configure_mysql_password()
         _calls = []
         for package in ["mysql-server", "mysql-server-8.0"]:
@@ -690,6 +692,30 @@ class TestMySQLInnoDBClusterCharm(test_utils.PatchHelper):
         midbc.configure_instance('10.10.30.30')
         midbc.run_mysqlsh_script.assert_not_called()
 
+    def test_configure_instance_error(self):
+        _pass = "clusterpass"
+        _addr = "10.10.30.30"
+        self.data = {"cluster-password": _pass}
+        self.leader_data = {
+            'cluster-instance-configured-10-10-30-30': 'False',
+        }
+        midbc = mysql_innodb_cluster.MySQLInnoDBClusterCharm()
+        midbc._get_password = mock.MagicMock()
+        midbc._get_password.side_effect = self._fake_data
+        midbc.wait_until_connectable = mock.MagicMock()
+        midbc.run_mysqlsh_script = mock.MagicMock(
+            side_effect=[subprocess.CalledProcessError(
+                2, 'foo', b'output', b'error')])
+        _script = (
+            "dba.configure_instance('{}:{}@{}')\n"
+            .format(midbc.cluster_user, midbc.cluster_password, _addr))
+
+        with self.assertRaises(subprocess.CalledProcessError):
+            midbc.configure_instance(_addr)
+        midbc.run_mysqlsh_script.assert_called_once_with(_script)
+        midbc.wait_until_connectable.assert_not_called()
+        self.leader_set.assert_not_called()
+
     @mock.patch(('charm.openstack.mysql_innodb_cluster.'
                  'MySQLInnoDBClusterCharm.cluster_peer_addresses'),
                 new_callable=mock.PropertyMock)
@@ -860,6 +886,47 @@ class TestMySQLInnoDBClusterCharm(test_utils.PatchHelper):
             {"cluster-instance-clustered-{}"
              .format(_remote_addr.replace(".", "-")): True})
 
+    def test_add_instance_to_cluster_error(self):
+        _pass = "clusterpass"
+        _local_addr = "10.10.50.50"
+        _remote_addr = "10.10.60.60"
+        _name = "theCluster"
+        _allowlist = '10.0.0.0/24'
+        self.get_relation_ip.return_value = _local_addr
+        self.get_relation_ip.return_value = _local_addr
+        self.data = {"cluster-password": _pass}
+        self.leader_data = {
+            'cluster-instance-configured-10-10-60-60': 'False',
+        }
+
+        midbc = mysql_innodb_cluster.MySQLInnoDBClusterCharm()
+        midbc.get_cluster_primary_address = mock.MagicMock(
+            return_value=_local_addr)
+        midbc._get_password = mock.MagicMock()
+        midbc._get_password.side_effect = self._fake_data
+        midbc.wait_until_connectable = mock.MagicMock()
+        midbc.run_mysqlsh_script = mock.MagicMock(
+            side_effect=[subprocess.CalledProcessError(
+                2, 'foo', b'output', b'error')])
+        midbc.options.cluster_name = _name
+        midbc.is_address_in_replication_ip_allowlist = lambda x: True
+        midbc.generate_ip_allowlist_str = lambda: _allowlist
+        _script = (
+            "shell.connect('{}:{}@{}')\n"
+            "cluster = dba.get_cluster('{}')\n"
+            "cluster.add_instance("
+            "{{'user': '{}', 'host': '{}', 'password': '{}', 'port': '3306'}},"
+            "{{'recoveryMethod': 'clone', 'waitRecovery': '2', "
+            "'interactive': False, 'ipAllowlist': '{}'}})"
+            .format(
+                midbc.cluster_user, midbc.cluster_password,
+                midbc.cluster_address, midbc.cluster_name,
+                midbc.cluster_user, _remote_addr, midbc.cluster_password,
+                _allowlist))
+        with self.assertRaises(subprocess.CalledProcessError):
+            midbc.add_instance_to_cluster(_remote_addr)
+        midbc.run_mysqlsh_script.assert_called_once_with(_script)
+
     def test_get_allowed_units(self):
         _allowed = ["unit/2", "unit/1", "unit/0"]
         _expected = "unit/0 unit/1 unit/2"
@@ -1623,6 +1690,7 @@ class TestMySQLInnoDBClusterCharm(test_utils.PatchHelper):
         self.subprocess.check_output.return_value = _byte_string
         _script = "print('Hello World!')"
         midbc = mysql_innodb_cluster.MySQLInnoDBClusterCharm()
+        mysql_innodb_cluster.subprocess = self.subprocess
         self.assertEqual(
             _byte_string,
             midbc.run_mysqlsh_script(_script))
@@ -1659,7 +1727,7 @@ class TestMySQLInnoDBClusterCharm(test_utils.PatchHelper):
                  "--set-gtid-purged=COMMENTED",
                  "--result-file", _filename, "--all-databases"]),
             mock.call(["/usr/bin/gzip", _filename])]
-
+        mysql_innodb_cluster.subprocess = self.subprocess
         self.assertEqual(midbc.mysqldump(_path), "{}.gz".format(_filename))
         midbc.write_root_my_cnf.assert_called_once()
         self.subprocess.check_call.assert_has_calls(_calls)
@@ -1707,7 +1775,7 @@ class TestMySQLInnoDBClusterCharm(test_utils.PatchHelper):
         _sql = mock.MagicMock()
         self._open.return_value = _sql
         self.subprocess.Popen.return_value = _restore
-
+        mysql_innodb_cluster.subprocess = self.subprocess
         midbc.restore_mysqldump(_dump_file)
         midbc.write_root_my_cnf.assert_called_once()
         self.subprocess.check_call.assert_called_once_with(
@@ -1935,6 +2003,88 @@ class TestMySQLInnoDBClusterCharm(test_utils.PatchHelper):
         with self.assertRaises(Exception):
             midbc.configure_and_add_instance(address=_remote_addr)
 
+    def test_configure_and_add_instance_bug1983158_success(self):
+        _pass = "clusterpass"
+        _name = "theCluster"
+        _string = "status output"
+        _local_addr = "10.10.50.50"
+        _remote_addr = "10.10.50.70"
+        _user = "user"
+        self.get_relation_ip.return_value = _local_addr
+        self.patch_object(
+            mysql_innodb_cluster.reactive, "endpoint_from_flag",
+            return_value=self.cluster)
+
+        midbc = mysql_innodb_cluster.MySQLInnoDBClusterCharm()
+        midbc.get_cluster_primary_address = mock.MagicMock(
+            return_value=_local_addr)
+        midbc.options.cluster_name = _name
+        midbc.run_mysqlsh_script = mock.MagicMock()
+        midbc.run_mysqlsh_script.return_value = _string.encode("UTF-8")
+        midbc._get_password = mock.MagicMock()
+        midbc._get_password.return_value = _pass
+        self.data = {
+            "cluster-address": _remote_addr,
+            "cluster-user": _user,
+            "cluster-password": _pass,
+        }
+        midbc.create_user = mock.MagicMock()
+        midbc.create_user.return_value = True
+        midbc.configure_instance = mock.MagicMock()
+        midbc.add_instance_to_cluster = mock.MagicMock()
+        midbc.configure_instance.side_effect = [
+            subprocess.CalledProcessError(
+                1, 'foo', b'output', b'Server in SUPER_READ_ONLY mode'),
+            None
+        ]
+
+        midbc.configure_and_add_instance(address=_remote_addr)
+        midbc.create_user.assert_called_once_with(
+            _remote_addr, _user, _pass, "all")
+        midbc.configure_instance.assert_has_calls([
+            mock.call(_remote_addr), mock.call(_remote_addr)
+        ])
+        midbc.add_instance_to_cluster.assert_called_once_with(_remote_addr)
+
+    def test_configure_and_add_instance_bug1983158_error(self):
+        _pass = "clusterpass"
+        _name = "theCluster"
+        _string = "status output"
+        _local_addr = "10.10.50.50"
+        _remote_addr = "10.10.50.70"
+        _user = "user"
+        self.get_relation_ip.return_value = _local_addr
+        self.patch_object(
+            mysql_innodb_cluster.reactive, "endpoint_from_flag",
+            return_value=self.cluster)
+
+        midbc = mysql_innodb_cluster.MySQLInnoDBClusterCharm()
+        midbc.get_cluster_primary_address = mock.MagicMock(
+            return_value=_local_addr)
+        midbc.options.cluster_name = _name
+        midbc.run_mysqlsh_script = mock.MagicMock()
+        midbc.run_mysqlsh_script.return_value = _string.encode("UTF-8")
+        midbc._get_password = mock.MagicMock()
+        midbc._get_password.return_value = _pass
+        self.data = {
+            "cluster-address": _remote_addr,
+            "cluster-user": _user,
+            "cluster-password": _pass,
+        }
+        midbc.create_user = mock.MagicMock()
+        midbc.create_user.return_value = True
+        midbc.configure_instance = mock.MagicMock()
+        midbc.add_instance_to_cluster = mock.MagicMock()
+        midbc.configure_instance.side_effect = [
+            subprocess.CalledProcessError(1, 'foo', b'output', b'error'),
+        ]
+        with self.assertRaises(subprocess.CalledProcessError):
+            midbc.configure_and_add_instance(address=_remote_addr)
+        midbc.create_user.assert_called_once_with(
+            _remote_addr, _user, _pass, "all")
+        midbc.configure_instance.assert_called_once_with(_remote_addr)
+        midbc.add_instance_to_cluster.assert_not_called()
+
     def test_clear_flags_for_removed_instance(self):
         _addr = "10.5.0.10"
         _expected = {