Fix mysqldump ignoring errors #403

laurent-indermuehle · 2022-06-21T14:49:13Z

SUMMARY

The mysql_db module does not display mysqldump errors when compression is used.

Fixes #256

ISSUE TYPE

Bugfix Pull Request

COMPONENT NAME

mysql_db

ADDITIONAL INFORMATION

The issue #256 initially was met the first time because of a too big field and a max_allowed_packet to small. As this is difficult to test, the tests were written with a MySQL view pointing to a non-existent table.

# Before
changed: [testhost]

# After
fatal: [testhost]: FAILED! => {"changed": false, "msg": "mysqldump: [Warning] Using a password on the command line interface can be insecure.\nmysqldump: Got error: 1356: View 'db2.v1' references invalid table(s) or column(s) or function(s) or definer/invoker of view lack rights to use them when using LOCK TABLES\n"}

sh is missing the pipefail flag. We must use bash for this.

codecov · 2022-06-22T05:19:13Z

Codecov Report

Merging #403 (85696fe) into main (0df46e0) will increase coverage by 0.13%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #403      +/-   ##
==========================================
+ Coverage   77.71%   77.84%   +0.13%     
==========================================
  Files          27       27              
  Lines        2315     2320       +5     
  Branches      558      560       +2     
==========================================
+ Hits         1799     1806       +7     
+ Misses        356      355       -1     
+ Partials      160      159       -1

Impacted Files	Coverage Δ
plugins/modules/mysql_db.py	`75.50% <100.00%> (+1.10%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0df46e0...85696fe. Read the comment docs.

Andersson007 · 2022-06-22T05:47:55Z

@laurent-indermuehle thanks for the fix!

The CI tests have failed. Please see the logs.

Anyway I'm afraid in this implementation it'll be a breaking change.. at least for FreeBSD users as bash is not installed there by default (I read it in the official doc)
Any ideas on how to overcome it?
Maybe a new boolean option that will allow to do what you added with no by default for now to preserve backwards compatibility in systems with no bash?
Then we could properly change the default later in the next major release.

laurent-indermuehle · 2022-06-22T06:34:41Z

@Andersson007 I didn't thought that bash could be missing on some systems.
I like your idea of a quick fix using the bool with no by default. I'll try to implement that.

For the CI, it's my first time working with Github Actions. I see that some tests are returning a none 0 code but have no clue why. On my machine, units, integrations and sanity tests passes.

Andersson007 · 2022-06-22T07:12:37Z

@Andersson007 I didn't thought that bash could be missing on some systems. I like your idea of a quick fix using the bool with no by default. I'll try to implement that.

Thanks!

For the CI, it's my first time working with Github Actions. I see that some tests are returning a none 0 code but have no clue why. On my machine, units, integrations and sanity tests passes.

It's because your local fork contains default versions of things we test against (listed in the setup_mysql role that prepares the test environment and is run before every target as a dependency).
There's a file https://github.com/ansible-collections/community.mysql/blob/main/.github/workflows/ansible-test-plugins.yml.
It changes the defaults in the role.

When you're debugging, the easiest (imo) algorithm is to:

click Details near a failing test
click the gear in the upper-right corener
choose View raw logs
use search in browser with FAILED, ignore things marked with ignoring

Following these steps, i can see that the error is

2022-06-22T05:52:42.2627776Z TASK [test_mysql_db : Check dumps errors | Setup test | Create 2 schemas] ******
2022-06-22T05:52:42.6390493Z �[0;31mfatal: [testhost]: FAILED! => {"changed": false, "msg": "unable to connect to database, check login_user and login_password are correct or /root/.my.cnf has the credentials. Exception message: (2003, \"Can't connect to MySQL server on 'localhost' ([Errno 99] Cannot assign requested address)\")"}�[0m
2022-06-22T05:52:42.7416213Z

Andersson007 · 2022-06-22T07:14:13Z

It looks like only tests against Python 3.6 failed

laurent-indermuehle · 2022-06-22T08:43:01Z

@Andersson007 Thanks for the workflow to read GH Actions logs! It helps :)

I'm not sure what the issue is. I see so many things that looks weird to me:

True instead of true or yes: https://github.com/ansible-collections/community.mysql/blob/main/tests/integration/targets/test_mysql_db/tasks/state_present_absent.yml#L22
No such file or directory on Python 3.6:

TASK [test_mysql_db : remove database if it exists] 
[fatal: [testhost]: FAILED! => {
  "cmd": "'mysql -uroot -pmsandbox -P3307 --protocol=tcp -sse '\"'\"'drop database data'\"'\"''",
  "msg": "[Errno 2] No such file or directory: b\"mysql -uroot -pmsandbox -P3307 --protocol=tcp -sse 'drop database data'\": b\"mysql -uroot -pmsandbox -P3307 --protocol=tcp -sse 'drop database data'\"",
  "rc": 2}

Unable to drop 'data' schema:

TASK [test_mysql_db : make sure the test database is not there] 
[changed: [testhost] => {
  "changed": true, 
  "cmd": ["mysql", "-uroot", "-pmsandbox", "-P3307", "--protocol=tcp", "data"],
  "msg": "non-zero return code", "rc": 1,
  "stderr": "mysql: ERROR 1049 (42000): Unknown database 'data'", 
}

I'm running ansible-test locally for Python 3.6. I'll see if the problem is the same on my machine.

laurent-indermuehle · 2022-06-22T08:55:39Z

Ok, so MySQL8 causes issue with its new authentication plugin:

TASK [test_mysql_db : state dump/import - create database]
unable to connect to database, check login_user and login_password are correct or /root/.my.cnf has the credentials. 
Exception message: cryptography is required for sha256_password or caching_sha2_password

I'm starting to wounder if using module_defaults was a good idea? https://github.com/laurent-indermuehle/community.mysql/blob/fix_mysqldump_ignore_errors/tests/integration/targets/test_mysql_db/tasks/issue_256_mysqldump_errors.yml#L6-L12

laurent-indermuehle · 2022-06-22T09:08:00Z

ok, I missed the blue-green 'ignoring' ;)
Many errors are the actuals tests... Searching in the row log removes the colors :P

laurent-indermuehle · 2022-06-22T13:49:28Z

I pushed a version with the new option "pipefail".
The tests are enhanced and continue to pass. I hope the CI will be happy thought.

Tests continues to fails using mysql 8 due to a missing package cryptography.

I added a changelog fragment and a documentation for the option. I put the "version_added" at 3.3.1 but not sure what number to put.

Do I have forgot anything?

Andersson007

@laurent-indermuehle thanks!

I think it would be nice to add an example to the EXAMPLES block.

I took only a quick look today. I'll take a deeper one tomorrow or on Friday. Thanks!

changelogs/fragments/fix-256-mysql_dump-errors.yml

plugins/modules/mysql_db.py

Co-authored-by: Andrew Klychkov <[email protected]>

laurent-indermuehle · 2022-06-22T15:31:43Z

@Andersson007 I'm done for today. I'm sorry for the number of commits. I'm not used to work on Ansible plugins... yet ;)

Thank you if you can review the tests and fixes.
I took the liberty to fix some stuffs related to others tests (IF EXISTS, python3-cryptography), I hope that's ok.

Andersson007 · 2022-06-22T18:10:25Z

@Andersson007 I'm done for today. I'm sorry for the number of commits. I'm not used to work on Ansible plugins... yet ;)

@laurent-indermuehle no problem at all! Please do any number of commits/ask any questions needed! Thanks for working on this:)

Andersson007

@laurent-indermuehle one small thing from me besides adding the example as mentioned in my yesterday's comment.

plugins/modules/mysql_db.py

Co-authored-by: Andrew Klychkov <[email protected]>

laurent-indermuehle · 2022-06-23T11:18:56Z

Great, I commited your changes. I think we are done with this PR, unless the CI tells otherwise.

I'm not 100% satisfied because, the default is highly insecure or prevent us from using compression. If a way exists to pipe commands together with Python subcommand.Popen() while preserving the return code of each command, it would be ways better. But I failed to find it. Especially in our case, where this method is wrapped by Ansible module.run_command().

Without help from an expert I can't offer a better solution.

I was happy to help, I learned alot and your guidances was very appreciated.

Andersson007 · 2022-06-23T12:58:52Z

I'm not 100% satisfied because, the default is highly insecure or prevent us from using compression.

Agreed. We can change the default later in a major release

I was happy to help, I learned alot and your guidances was very appreciated.

I'm happy to help:)

laurent-indermuehle · 2022-06-23T20:20:47Z

After having read the documentation for the subprocess module, it seems we can use 2 Popen object and check the returned code of each.

While experimenting with that, I found the following code from mysql_db.py into the db_import() function: https://github.com/ansible-collections/community.mysql/blob/main/plugins/modules/mysql_db.py#L492-L512

I'm a bit worried about the "'Broken pipe' errors that occasionally occur" message, but aside from that, it seems to be the solution I was looking for? Right?

Andersson007 · 2022-06-24T05:14:06Z

After having read the documentation for the subprocess module, it seems we can use 2 Popen object and check the returned code of each.

While experimenting with that, I found the following code from mysql_db.py into the db_import() function: https://github.com/ansible-collections/community.mysql/blob/main/plugins/modules/mysql_db.py#L492-L512

ah, forgot that the module uses the subprocess module directly

I'm a bit worried about the "'Broken pipe' errors that occasionally occur" message

Yep, sounds scary

but aside from that, it seems to be the solution I was looking for? Right?

Would you like to continue experimenting or we should merge the PR?
Of course it would be great if everything goes seamlessly without any additional options but the current solution feels (not ideal but) safer (especially considering the "Broken pipe" comment - yeah, i recalled there was the issue, then we added the use_shell to workaround it)

I'm not sure, let's ask the other maintainers, @bmalynovytch ideas?

laurent-indermuehle · 2022-06-24T13:16:37Z

I created a playbook and a container to test locally with a 100MB database to see if the changes affect performances.

Then I created yet another option to mysql_db called 'popen' that uses the following code to compress the dump:

import gzip
from shlex import split as shlex_split
if popen:
        p1 = subprocess.Popen(shlex_split(cmd), stdout=subprocess.PIPE, stderr=subprocess.PIPE)

        with gzip.open(shlex_quote(target), 'wb', compresslevel=5) as f:
            stdout, stderr = p1.communicate()
            f.write(stdout)
            p1.wait()

        if p1.returncode != 0:
            return p1.returncode, '', to_native(stderr)
        else:
            return 0, 'Dump done', ''

This works, the error is captured popen the same way pipefail did. But:

I don't know if we can add Python gzip module as a dependency
How to handle others compressions. But aside for xz, they all seems to be available in the standard library: https://docs.python.org/3/library/archiving.html

The good news is, that the performances are almost equivalent :

dump sql : 2.1s
dump gzip : 2.47s
dump gzip pipefail : 2.53s
dump gzip popen + gzip module : 2.9s

Without the , compresslevel=5, Python defaults to 9. Which resulted in 6+ seconds per dump. I found that 5 produce the same file size as the shell version.

I'm running out of time. I'll get back in 2 weeks. What is left to do:

Try to replace subprocess.Popen by module.run_command as it is recommended by Ansible best practices

Try to replace gzip module by chaining the output of p1 on a second subprocess, E.G.:

compress_cmd = shlex_split(path + ' > ' + shlex_quote(target))
p2 = module.run_command(stdin=p1.stdout, ...)

Try to debug broken pipe in db_import
Align compression code between db_dump and db_import
Find out how to use shlex.split from Ansible

laurent-indermuehle · 2022-06-24T14:35:21Z

Here is a version without the gzip module:

from shlex import split as shlex_split
[...]
    if popen:
        compress_cmd = [module.get_bin_path('gzip', True), ' > ', shlex_quote(target)]
        p1 = subprocess.Popen(shlex_split(cmd), stdout=subprocess.PIPE, stderr=subprocess.PIPE)
        p2 = subprocess.Popen(compress_cmd, stdin=p1.stdout, stdout=subprocess.PIPE, stderr=subprocess.PIPE, shell=True)
        stdout1, stderr1 = p1.communicate()
        stdout2, stderr2 = p2.communicate()

        if p1.returncode != 0:
            return p1.returncode, '', to_native(stderr1)
        else:
            return p2.returncode, stdout2, to_native(stderr2)

Should I go this route?

I tried to use run_command(), but because it uses the .close() method, it will be impossible to pipe two run_command() together. I tried pass_fds, data, close_fds, with no luck. I got a broken pipe every time.

Andersson007 · 2022-06-27T07:11:39Z

@laurent-indermuehle thanks for the experimenting!

About the dependencies introduction - I would avoid it but it's only my view.
About using Popen - IIRC that bug with the crashing pipe was floating, so the solution we already have in this PR feels like the safest.

@bmalynovytch @rsicart any ideas?

bmalynovytch

The approach is the one I would have had.
Good job !

bmalynovytch · 2022-06-28T09:46:27Z

tests/integration/targets/test_mysql_db/tasks/state_present_absent.yml

-    "{{ mysql_command }} -sse 'drop database {{ db_name }}'"
-  ignore_errors: True
+    "{{ mysql_command }} -sse 'DROP DATABASE IF EXISTS {{ db_name }}'"
+  ignore_errors: true



Is this related to the issue ? 🤨

No sorry. I did that when the tests were failing on python 3.6.

Should I revert that?

@laurent-indermuehle

No sorry. I did that when the tests were failing on python 3.6.

I'm curious why they fail if they are not related to the changes. If it's safe, we can leave it as is.

Andersson007 · 2022-06-30T04:55:25Z

@laurent-indermuehle thanks for the contribution!
@bmalynovytch thanks for reviewing!
I'll create an issue to change the default in the next major release.

laurent-indermuehle added 5 commits June 17, 2022 17:54

Add schema and tables for the tests

f720be4

Add tests for full dump with and without compression

5ed8661

Add test for distinct dump with and without compression

4f85e37

Fix sh not seeing errors for command before the pipe

9716a4d

sh is missing the pipefail flag. We must use bash for this.

Add cleanup to prevent the following tests from failing

bb11029

laurent-indermuehle added 5 commits June 22, 2022 11:11

Fix fqcn in module_defaults

1489f2f

Add changelog fragment

db02d8e

Add check to the error message to ensure we captured the right one

36bbc64

Add option to activate the fix on systems with bash

3fe6f9b

Fix errors when data schema is already absent

c1c44f2

Andersson007 reviewed Jun 22, 2022

View reviewed changes

changelogs/fragments/fix-256-mysql_dump-errors.yml Outdated Show resolved Hide resolved

plugins/modules/mysql_db.py Outdated Show resolved Hide resolved

plugins/modules/mysql_db.py Outdated Show resolved Hide resolved

plugins/modules/mysql_db.py Outdated Show resolved Hide resolved

laurent-indermuehle and others added 7 commits June 22, 2022 16:37

Update changelogs/fragments/fix-256-mysql_dump-errors.yml

220d683

Co-authored-by: Andrew Klychkov <[email protected]>

Add markup for commands in the documentation string

ebf56f0

Co-authored-by: Andrew Klychkov <[email protected]>

Add markup and next release version in the documentation string

f170da4

Co-authored-by: Andrew Klychkov <[email protected]>

Fix missing dependency for MySQL 8

9699b92

Add pipefail to tests of uncompressed dumps to enure it still works

b5e8cdc

Fix "bash command not found" if pipefail is used for uncompressed dump

eae5df2

Fix sanity pep8

2bc2e59

Andersson007 reviewed Jun 23, 2022

View reviewed changes

plugins/modules/mysql_db.py Outdated Show resolved Hide resolved

laurent-indermuehle and others added 2 commits June 23, 2022 09:27

Document example of dump with pipefail

0b3d793

Add dedpulication to command construct

85696fe

Co-authored-by: Andrew Klychkov <[email protected]>

Andersson007 requested a review from rsicart June 27, 2022 07:12

bmalynovytch approved these changes Jun 28, 2022

View reviewed changes

Andersson007 approved these changes Jun 30, 2022

View reviewed changes

Andersson007 merged commit 5108ca5 into ansible-collections:main Jun 30, 2022

This was referenced Jun 30, 2022

mysql_db module does not detect mysqldump failure #256

Closed

[4.0.0] mysql_db: change a default of the pipefail option to true #407

Open

Announce pipefail default change in community.mysql 4.0.0 #408

Merged

laurent-indermuehle deleted the fix_mysqldump_ignore_errors branch August 13, 2022 19:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix mysqldump ignoring errors #403

Fix mysqldump ignoring errors #403

laurent-indermuehle commented Jun 21, 2022

codecov bot commented Jun 22, 2022 •

edited

Loading

Andersson007 commented Jun 22, 2022 •

edited

Loading

laurent-indermuehle commented Jun 22, 2022

Andersson007 commented Jun 22, 2022 •

edited

Loading

Andersson007 commented Jun 22, 2022

laurent-indermuehle commented Jun 22, 2022 •

edited

Loading

laurent-indermuehle commented Jun 22, 2022

laurent-indermuehle commented Jun 22, 2022

laurent-indermuehle commented Jun 22, 2022

Andersson007 left a comment

laurent-indermuehle commented Jun 22, 2022

Andersson007 commented Jun 22, 2022

Andersson007 left a comment

laurent-indermuehle commented Jun 23, 2022

Andersson007 commented Jun 23, 2022

laurent-indermuehle commented Jun 23, 2022

Andersson007 commented Jun 24, 2022

laurent-indermuehle commented Jun 24, 2022

laurent-indermuehle commented Jun 24, 2022

Andersson007 commented Jun 27, 2022

bmalynovytch left a comment

bmalynovytch Jun 28, 2022

laurent-indermuehle Jun 28, 2022

Andersson007 Jun 29, 2022

Andersson007 commented Jun 30, 2022

Fix mysqldump ignoring errors #403

Fix mysqldump ignoring errors #403

Conversation

laurent-indermuehle commented Jun 21, 2022

SUMMARY

ISSUE TYPE

COMPONENT NAME

ADDITIONAL INFORMATION

codecov bot commented Jun 22, 2022 • edited Loading

Codecov Report

Andersson007 commented Jun 22, 2022 • edited Loading

laurent-indermuehle commented Jun 22, 2022

Andersson007 commented Jun 22, 2022 • edited Loading

Andersson007 commented Jun 22, 2022

laurent-indermuehle commented Jun 22, 2022 • edited Loading

laurent-indermuehle commented Jun 22, 2022

laurent-indermuehle commented Jun 22, 2022

laurent-indermuehle commented Jun 22, 2022

Andersson007 left a comment

Choose a reason for hiding this comment

laurent-indermuehle commented Jun 22, 2022

Andersson007 commented Jun 22, 2022

Andersson007 left a comment

Choose a reason for hiding this comment

laurent-indermuehle commented Jun 23, 2022

Andersson007 commented Jun 23, 2022

laurent-indermuehle commented Jun 23, 2022

Andersson007 commented Jun 24, 2022

laurent-indermuehle commented Jun 24, 2022

laurent-indermuehle commented Jun 24, 2022

Andersson007 commented Jun 27, 2022

bmalynovytch left a comment

Choose a reason for hiding this comment

bmalynovytch Jun 28, 2022

Choose a reason for hiding this comment

laurent-indermuehle Jun 28, 2022

Choose a reason for hiding this comment

Andersson007 Jun 29, 2022

Choose a reason for hiding this comment

Andersson007 commented Jun 30, 2022

codecov bot commented Jun 22, 2022 •

edited

Loading

Andersson007 commented Jun 22, 2022 •

edited

Loading

Andersson007 commented Jun 22, 2022 •

edited

Loading

laurent-indermuehle commented Jun 22, 2022 •

edited

Loading