From ebe503823a3ffbfaee51acec67d20895d8af56c6 Mon Sep 17 00:00:00 2001 From: Andrew Klychkov Date: Fri, 18 Dec 2020 11:17:18 +0300 Subject: [PATCH] mysql_replication: fix crashes caused by deprecated terminology (#71) * mysql_replication: fix crashes caused by deprecated terminology * Fix unrelated sanity errors * Tests: mysql 8.0.21 -> 8.022 * Adjust integration tests * Add version check to the tests * Add debug statement * Adjust mysql version * Fix tests * Add unit tests * Add changelog fragment * Improve code and coverage * Get rid of extra blank line * Improve coverage * Change suggested --- .github/workflows/ansible-test-plugins.yml | 46 ++++++++++++- ...eplication_add_replica_keyword_support.yml | 2 + plugins/modules/mysql_replication.py | 66 +++++++++++++------ .../targets/setup_mysql/defaults/main.yml | 2 +- .../tasks/mysql_replication_channel.yml | 22 +++++-- .../tasks/mysql_replication_initial.yml | 38 ++++++++++- tests/unit/plugins/modules/__init__.py | 0 .../plugins/modules/test_mysql_replication.py | 47 +++++++++++++ 8 files changed, 194 insertions(+), 29 deletions(-) create mode 100644 changelogs/fragments/71-mysql_replication_add_replica_keyword_support.yml create mode 100644 tests/unit/plugins/modules/__init__.py create mode 100644 tests/unit/plugins/modules/test_mysql_replication.py diff --git a/.github/workflows/ansible-test-plugins.yml b/.github/workflows/ansible-test-plugins.yml index 3c3ea5e..cd27b10 100644 --- a/.github/workflows/ansible-test-plugins.yml +++ b/.github/workflows/ansible-test-plugins.yml @@ -55,7 +55,7 @@ jobs: matrix: mysql: - 5.7.31 - - 8.0.21 + - 8.0.22 ansible: - stable-2.9 - stable-2.10 @@ -67,7 +67,7 @@ jobs: - pymysql==0.9.3 - mysqlclient==2.0.1 exclude: - - mysql: 8.0.21 + - mysql: 8.0.22 connector: pymysql==0.7.10 steps: @@ -101,3 +101,45 @@ jobs: - uses: codecov/codecov-action@v1 with: fail_ci_if_error: false + + units: + runs-on: ubuntu-latest + name: Units (Ⓐ${{ matrix.ansible }}) + strategy: + # As soon as the first unit test fails, + # cancel the others to free up the CI queue + fail-fast: true + matrix: + ansible: + - stable-2.9 + - stable-2.10 + - devel + + steps: + - name: Check out code + uses: actions/checkout@v2 + with: + path: ./ansible_collections/community/mysql + + - name: Set up Python + uses: actions/setup-python@v2 + with: + python-version: 3.8 + + - name: Install ansible-base (${{matrix.ansible}}) + run: pip install https://github.com/ansible/ansible/archive/${{ matrix.ansible }}.tar.gz --disable-pip-version-check + + # Run the unit tests + - name: Run unit test + run: ansible-test units -v --color --docker --coverage + working-directory: ./ansible_collections/community/mysql + + # ansible-test support producing code coverage date + - name: Generate coverage report + run: ansible-test coverage xml -v --requirements --group-by command --group-by version + working-directory: ./ansible_collections/community/mysql + + # See the reports at https://codecov.io/gh/GITHUBORG/REPONAME + - uses: codecov/codecov-action@v1 + with: + fail_ci_if_error: false diff --git a/changelogs/fragments/71-mysql_replication_add_replica_keyword_support.yml b/changelogs/fragments/71-mysql_replication_add_replica_keyword_support.yml new file mode 100644 index 0000000..424077d --- /dev/null +++ b/changelogs/fragments/71-mysql_replication_add_replica_keyword_support.yml @@ -0,0 +1,2 @@ +bugfixes: +- mysql_replication - fix crashes of mariadb >= 10.5.1 and mysql >= 8.0.22 caused by using deprecated terminology (https://github.com/ansible-collections/community.mysql/issues/70). diff --git a/plugins/modules/mysql_replication.py b/plugins/modules/mysql_replication.py index a755246..322a6a8 100644 --- a/plugins/modules/mysql_replication.py +++ b/plugins/modules/mysql_replication.py @@ -242,21 +242,40 @@ import warnings from ansible.module_utils.basic import AnsibleModule from ansible_collections.community.mysql.plugins.module_utils.mysql import mysql_connect, mysql_driver, mysql_driver_fail_msg, mysql_common_argument_spec from ansible.module_utils._text import to_native +from distutils.version import LooseVersion executed_queries = [] +def uses_replica_terminology(cursor): + """Checks if REPLICA must be used instead of SLAVE""" + cursor.execute("SELECT VERSION() AS version") + result = cursor.fetchone() + + if isinstance(result, dict): + version_str = result['version'] + else: + version_str = result[0] + + version = LooseVersion(version_str) + + if 'mariadb' in version_str.lower(): + return version >= LooseVersion('10.5.1') + else: + return version >= LooseVersion('8.0.22') + + def get_master_status(cursor): cursor.execute("SHOW MASTER STATUS") masterstatus = cursor.fetchone() return masterstatus -def get_slave_status(cursor, connection_name='', channel=''): +def get_slave_status(cursor, connection_name='', channel='', term='REPLICA'): if connection_name: - query = "SHOW SLAVE '%s' STATUS" % connection_name + query = "SHOW %s '%s' STATUS" % (term, connection_name) else: - query = "SHOW SLAVE STATUS" + query = "SHOW %s STATUS" % term if channel: query += " FOR CHANNEL '%s'" % channel @@ -266,11 +285,11 @@ def get_slave_status(cursor, connection_name='', channel=''): return slavestatus -def stop_slave(module, cursor, connection_name='', channel='', fail_on_error=False): +def stop_slave(module, cursor, connection_name='', channel='', fail_on_error=False, term='REPLICA'): if connection_name: - query = "STOP SLAVE '%s'" % connection_name + query = "STOP %s '%s'" % (term, connection_name) else: - query = 'STOP SLAVE' + query = 'STOP %s' % term if channel: query += " FOR CHANNEL '%s'" % channel @@ -288,11 +307,11 @@ def stop_slave(module, cursor, connection_name='', channel='', fail_on_error=Fal return stopped -def reset_slave(module, cursor, connection_name='', channel='', fail_on_error=False): +def reset_slave(module, cursor, connection_name='', channel='', fail_on_error=False, term='REPLICA'): if connection_name: - query = "RESET SLAVE '%s'" % connection_name + query = "RESET %s '%s'" % (term, connection_name) else: - query = 'RESET SLAVE' + query = 'RESET %s' % term if channel: query += " FOR CHANNEL '%s'" % channel @@ -310,11 +329,11 @@ def reset_slave(module, cursor, connection_name='', channel='', fail_on_error=Fa return reset -def reset_slave_all(module, cursor, connection_name='', channel='', fail_on_error=False): +def reset_slave_all(module, cursor, connection_name='', channel='', fail_on_error=False, term='REPLICA'): if connection_name: - query = "RESET SLAVE '%s' ALL" % connection_name + query = "RESET %s '%s' ALL" % (term, connection_name) else: - query = 'RESET SLAVE ALL' + query = 'RESET %s ALL' % term if channel: query += " FOR CHANNEL '%s'" % channel @@ -347,11 +366,11 @@ def reset_master(module, cursor, fail_on_error=False): return reset -def start_slave(module, cursor, connection_name='', channel='', fail_on_error=False): +def start_slave(module, cursor, connection_name='', channel='', fail_on_error=False, term='REPLICA'): if connection_name: - query = "START SLAVE '%s'" % connection_name + query = "START %s '%s'" % (term, connection_name) else: - query = 'START SLAVE' + query = 'START %s' % term if channel: query += " FOR CHANNEL '%s'" % channel @@ -467,6 +486,13 @@ def main(): else: module.fail_json(msg="unable to find %s. Exception message: %s" % (config_file, to_native(e))) + # Since MySQL 8.0.22 and MariaDB 10.5.1, + # "REPLICA" must be used instead of "SLAVE" + if uses_replica_terminology(cursor): + replica_term = 'REPLICA' + else: + replica_term = 'SLAVE' + if mode in "getmaster": status = get_master_status(cursor) if not isinstance(status, dict): @@ -476,7 +502,7 @@ def main(): module.exit_json(queries=executed_queries, **status) elif mode in "getslave": - status = get_slave_status(cursor, connection_name, channel) + status = get_slave_status(cursor, connection_name, channel, replica_term) if not isinstance(status, dict): status = dict(Is_Slave=False, msg="Server is not configured as mysql slave") else: @@ -531,13 +557,13 @@ def main(): result['changed'] = True module.exit_json(queries=executed_queries, **result) elif mode in "startslave": - started = start_slave(module, cursor, connection_name, channel, fail_on_error) + started = start_slave(module, cursor, connection_name, channel, fail_on_error, replica_term) if started is True: module.exit_json(msg="Slave started ", changed=True, queries=executed_queries) else: module.exit_json(msg="Slave already started (Or cannot be started)", changed=False, queries=executed_queries) elif mode in "stopslave": - stopped = stop_slave(module, cursor, connection_name, channel, fail_on_error) + stopped = stop_slave(module, cursor, connection_name, channel, fail_on_error, replica_term) if stopped is True: module.exit_json(msg="Slave stopped", changed=True, queries=executed_queries) else: @@ -549,13 +575,13 @@ def main(): else: module.exit_json(msg="Master already reset", changed=False, queries=executed_queries) elif mode in "resetslave": - reset = reset_slave(module, cursor, connection_name, channel, fail_on_error) + reset = reset_slave(module, cursor, connection_name, channel, fail_on_error, replica_term) if reset is True: module.exit_json(msg="Slave reset", changed=True, queries=executed_queries) else: module.exit_json(msg="Slave already reset", changed=False, queries=executed_queries) elif mode in "resetslaveall": - reset = reset_slave_all(module, cursor, connection_name, channel, fail_on_error) + reset = reset_slave_all(module, cursor, connection_name, channel, fail_on_error, replica_term) if reset is True: module.exit_json(msg="Slave reset", changed=True, queries=executed_queries) else: diff --git a/tests/integration/targets/setup_mysql/defaults/main.yml b/tests/integration/targets/setup_mysql/defaults/main.yml index 7075a4a..7bcb2d2 100644 --- a/tests/integration/targets/setup_mysql/defaults/main.yml +++ b/tests/integration/targets/setup_mysql/defaults/main.yml @@ -7,7 +7,7 @@ percona_client_version: 5.7 mariadb_install: false -mysql_version: 8.0.21 +mysql_version: 8.0.22 mariadb_version: 10.5.4 mysql_base_port: 3306 diff --git a/tests/integration/targets/test_mysql_replication/tasks/mysql_replication_channel.yml b/tests/integration/targets/test_mysql_replication/tasks/mysql_replication_channel.yml index 3102675..cb6d23a 100644 --- a/tests/integration/targets/test_mysql_replication/tasks/mysql_replication_channel.yml +++ b/tests/integration/targets/test_mysql_replication/tasks/mysql_replication_channel.yml @@ -48,7 +48,7 @@ - assert: that: - result is changed - - result.queries == ["START SLAVE FOR CHANNEL '{{ test_channel }}'"] + - result.queries == ["START SLAVE FOR CHANNEL '{{ test_channel }}'"] or result.queries == ["START REPLICA FOR CHANNEL '{{ test_channel }}'"] # Test getslave mode: - name: Get standby status with channel @@ -69,6 +69,20 @@ - slave_status.Last_IO_Error == '' - slave_status.Channel_Name == '{{ test_channel }}' - slave_status is not changed + when: mysql8022_and_higher == false + + - assert: + that: + - slave_status.Is_Slave == true + - slave_status.Source_Host == '{{ mysql_host }}' + - slave_status.Exec_Source_Log_Pos == mysql_primary_status.Position + - slave_status.Source_Port == {{ mysql_primary_port }} + - slave_status.Last_IO_Errno == 0 + - slave_status.Last_IO_Error == '' + - slave_status.Channel_Name == '{{ test_channel }}' + - slave_status is not changed + when: mysql8022_and_higher == true + # Test stopslave mode: - name: Stop slave with channel @@ -82,7 +96,7 @@ - assert: that: - result is changed - - result.queries == ["STOP SLAVE FOR CHANNEL '{{ test_channel }}'"] + - result.queries == ["STOP SLAVE FOR CHANNEL '{{ test_channel }}'"] or result.queries == ["STOP REPLICA FOR CHANNEL '{{ test_channel }}'"] # Test reset - name: Reset slave with channel @@ -96,7 +110,7 @@ - assert: that: - result is changed - - result.queries == ["RESET SLAVE FOR CHANNEL '{{ test_channel }}'"] + - result.queries == ["RESET SLAVE FOR CHANNEL '{{ test_channel }}'"] or result.queries == ["RESET REPLICA FOR CHANNEL '{{ test_channel }}'"] # Test reset all - name: Reset slave all with channel @@ -110,4 +124,4 @@ - assert: that: - result is changed - - result.queries == ["RESET SLAVE ALL FOR CHANNEL '{{ test_channel }}'"] + - result.queries == ["RESET SLAVE ALL FOR CHANNEL '{{ test_channel }}'"] or result.queries == ["RESET REPLICA ALL FOR CHANNEL '{{ test_channel }}'"] diff --git a/tests/integration/targets/test_mysql_replication/tasks/mysql_replication_initial.yml b/tests/integration/targets/test_mysql_replication/tasks/mysql_replication_initial.yml index eb1b51a..e26dcd2 100644 --- a/tests/integration/targets/test_mysql_replication/tasks/mysql_replication_initial.yml +++ b/tests/integration/targets/test_mysql_replication/tasks/mysql_replication_initial.yml @@ -8,6 +8,22 @@ login_host: 127.0.0.1 block: + - name: find out the database version + mysql_info: + <<: *mysql_params + login_port: '{{ mysql_primary_port }}' + filter: version + register: db + + - name: Set mysql8022_and_higher + set_fact: + mysql8022_and_higher: false + + - name: Set mysql8022_and_higher + set_fact: + mysql8022_and_higher: true + when: + - db.version.major > 8 or (db.version.major == 8 and db.version.minor > 0) or (db.version.major == 8 and db.version.minor == 0 and db.version.release >= 22) - name: alias mysql command to include default options set_fact: @@ -120,7 +136,7 @@ - assert: that: - result is changed - - result.queries == ["START SLAVE"] + - result.queries == ["START SLAVE"] or result.queries == ["START REPLICA"] # Test getslave mode: - name: Get standby status @@ -139,6 +155,18 @@ - slave_status.Last_IO_Errno == 0 - slave_status.Last_IO_Error == '' - slave_status is not changed + when: mysql8022_and_higher == false + + - assert: + that: + - slave_status.Is_Slave == true + - slave_status.Source_Host == '{{ mysql_host }}' + - slave_status.Exec_Source_Log_Pos == mysql_primary_status.Position + - slave_status.Source_Port == {{ mysql_primary_port }} + - slave_status.Last_IO_Errno == 0 + - slave_status.Last_IO_Error == '' + - slave_status is not changed + when: mysql8022_and_higher == true # Create test table and add data to it: - name: Create test table @@ -164,6 +192,12 @@ - assert: that: - slave_status.Exec_Master_Log_Pos != mysql_primary_status.Position + when: mysql8022_and_higher == false + + - assert: + that: + - slave_status.Exec_Source_Log_Pos != mysql_primary_status.Position + when: mysql8022_and_higher == true - shell: pip show pymysql | awk '/Version/ {print $2}' register: pymysql_version @@ -192,7 +226,7 @@ - assert: that: - result is changed - - result.queries == ["STOP SLAVE"] + - result.queries == ["STOP SLAVE"] or result.queries == ["STOP REPLICA"] # Test stopslave mode: - name: Stop slave that is no longer running diff --git a/tests/unit/plugins/modules/__init__.py b/tests/unit/plugins/modules/__init__.py new file mode 100644 index 0000000..e69de29 diff --git a/tests/unit/plugins/modules/test_mysql_replication.py b/tests/unit/plugins/modules/test_mysql_replication.py new file mode 100644 index 0000000..fea04a2 --- /dev/null +++ b/tests/unit/plugins/modules/test_mysql_replication.py @@ -0,0 +1,47 @@ +# -*- coding: utf-8 -*- +# Copyright: (c) 2020, Andrew Klychkov (@Andersson007) + +from __future__ import (absolute_import, division, print_function) +__metaclass__ = type + +import pytest + +from ansible_collections.community.mysql.plugins.modules.mysql_replication import uses_replica_terminology + + +class dummy_cursor_class(): + def __init__(self, output, ret_val_type='dict'): + self.output = output + self.ret_val_type = ret_val_type + + def execute(self, query): + pass + + def fetchone(self): + if self.ret_val_type == 'dict': + return {'version': self.output} + + elif self.ret_val_type == 'list': + return [self.output] + + +@pytest.mark.parametrize( + 'f_output,c_output,c_ret_type', + [ + (False, '5.5.1-mysql', 'list'), + (False, '5.7.0-mysql', 'dict'), + (False, '8.0.0-mysql', 'list'), + (False, '8.0.11-mysql', 'dict'), + (False, '8.0.21-mysql', 'list'), + (False, '10.5.0-mariadb', 'dict'), + (True, '8.0.22-mysql', 'list'), + (True, '8.1.2-mysql', 'dict'), + (True, '9.0.0-mysql', 'list'), + (True, '10.5.1-mariadb', 'dict'), + (True, '10.6.0-mariadb', 'dict'), + (True, '11.5.1-mariadb', 'dict'), + ] +) +def test_uses_replica_terminology(f_output, c_output, c_ret_type): + cursor = dummy_cursor_class(c_output, c_ret_type) + assert uses_replica_terminology(cursor) == f_output