Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

mysql_db: add use_shell parameter to prevent Broken pipe errors #151

Merged
merged 4 commits into from
Apr 20, 2020

Conversation

Andersson007
Copy link
Contributor

@Andersson007 Andersson007 commented Apr 11, 2020

SUMMARY

Fixes ansible/ansible#20196

mysql_db: prevent broken pipe errors when state=import by using an intermediate shell process
Added the use_shell parameter.

ISSUE TYPE
  • Bugfix Pull Request
  • Feature Pull Request
COMPONENT NAME

plugins/modules/database/mysql/mysql_db.py

@felixfontein
Copy link
Collaborator

You definitely need a changelog fragment :)

@Andersson007
Copy link
Contributor Author

@felixfontein , yep, thanks! but i'm not sure it'll help because i can't reproduce the situation to check the result.
If anybody confirm it solves the problem, i'll definitely add a fragment (and even gonna make it configurable by a new parameter, maybe it's important for somebody to run it not via a shell process).
But anyway i believe it must help.
Will wait for feedback a bit:)

@felixfontein
Copy link
Collaborator

@Andersson007 maybe mark the PR as [WIP] in that case :)

@Andersson007 Andersson007 changed the title mysql_db: prevent broken pipe errors by using an intermediate shell process [WIP] mysql_db: prevent broken pipe errors by using an intermediate shell process Apr 12, 2020
@Andersson007
Copy link
Contributor Author

@felixfontein yep, worth doing it not to confuse contributors:)

@Tronde
Copy link

Tronde commented Apr 13, 2020

Hi @Andersson007,
I would like to help and try your change. But I don't know how.

Until now I've only used Ansible installed from Repos. How could I test your PR? Could I just check it out via git and run it from git directory on my local machine?

@felixfontein
Copy link
Collaborator

@Tronde I've started writing some docs on how to use collections from Git repositories locally: https://github.com/ansible/ansible/pull/68899/files#diff-69c6b826eb7e42d76e3c24504fe1292bR578-R600

If you do this (with @Andersson007's fork of community.general and the right branch) and use the FQCN community.general.mysql_db for the module, you should be able to test this.

@Andersson007
Copy link
Contributor Author

Andersson007 commented Apr 13, 2020

@felixfontein thanks!
@Tronde , for me the simplest and fastest way is:

  1. find mysql_db.py on your ansible machine
  2. back up the file
  3. change it as i changed in this pr
  4. when you finish the test, return it as it was

Myabe it's not the best practice but it should work:)

@Andersson007
Copy link
Contributor Author

The fix is pretty simple

@Tronde
Copy link

Tronde commented Apr 13, 2020

Well, I found the mysql_db.py on my system, but it seems to be quite different from the on above. I'm using ansible 2.9.6 from epel7. I've inserted the shell=True snipped in my /usr/lib/python2.7/site-packages/ansible/modules/database/mysql/mysql_db.py at line 275:

    if comp_prog_path:
        p1 = subprocess.Popen([comp_prog_path, '-dc', target], stdout=subprocess.PIPE, stderr=subprocess.PIPE)
        p2 = subprocess.Popen(cmd, stdin=p1.stdout, stdout=subprocess.PIPE, stderr=subprocess.PIPE, shell=True)
        (stdout2, stderr2) = p2.communicate()
        p1.stdout.close()
        p1.wait()
        if p1.returncode != 0:
            stderr1 = p1.stderr.read()
            return p1.returncode, '', stderr1
        else:
            return p2.returncode, stdout2, stderr2
    else:
        cmd = ' '.join(cmd)
        cmd += " < %s" % shlex_quote(target)
        rc, stdout, stderr = module.run_command(cmd, use_unsafe_shell=True)
        return rc, stdout, stderr

But that did not fix the issue.

@Andersson007
Copy link
Contributor Author

@Tronde thank you for testing! i came up with a new possible solution:) coming soon

@Andersson007
Copy link
Contributor Author

@Tronde , did you convert cmd previously to a string (like in my changes)? Any errors during execution?

@Andersson007
Copy link
Contributor Author

anyway, could you try the last changes please. I hope we'll solve the problem together.
All what is needed is just remove all stuff between if comp_prog_path: and else and put there the following code

        cmd = " ".join(cmd)
        # The line below is for returned data only:
        executed_commands.append('%s -dc %s | %s' % (comp_prog_path, target, cmd))

        cmd = "%s -dc %s | %s" % (comp_prog_path, shlex_quote(target), cmd)
        rc, stdout, stderr = module.run_command(cmd, use_unsafe_shell=True)
        return rc, stdout, stderr

If it solves the problem, i'll do it configurable via a new parameter.
I'll be waiting for your feedback! Thanks in advance:)

@Tronde
Copy link

Tronde commented Apr 14, 2020

Thanks for your patience with me. So my code looks like the following, now:

    if comp_prog_path:
        cmd = " ".join(cmd)
        # The line below is for returned data only:
        executed_commands.append('%s -dc %s | %s' % (comp_prog_path, target, cmd))

        cmd = "%s -dc %s | %s" % (comp_prog_path, shlex_quote(target), cmd)
        rc, stdout, stderr = module.run_command(cmd, use_unsafe_shell=True)
        return rc, stdout, stderr
        else:
            return p2.returncode, stdout2, stderr2
    else:
        cmd = ' '.join(cmd)
        cmd += " < %s" % shlex_quote(target)
        rc, stdout, stderr = module.run_command(cmd, use_unsafe_shell=True)
        return rc, stdout, stderr

Now, my playbook fails with:
FAILED! => {"msg": "Unable to import mysql_db due to invalid syntax"}

I'm not sure if that's related to the change in the code or the sql file. But I could import the file locally.

@Andersson007
Copy link
Contributor Author

Andersson007 commented Apr 14, 2020

@Tronde

       else:
            return p2.returncode, stdout2, stderr2

please remove these lines and run again

@Tronde
Copy link

Tronde commented Apr 14, 2020

I did and got a module error:

FAILED! => {"changed": false, "module_stderr": "Shared connection to 10.0.2.15 closed.\r\n", "module_stdout": "\r\nTraceback (most recent call last):\r\n  File \"/home/jkastning/.ansible/tmp/ansible-tmp-1586873944.41-230345502361639/AnsiballZ_mysql_db.py\", line 102, in <module>\r\n    _ansiballz_main()\r\n  File \"/home/jkastning/.ansible/tmp/ansible-tmp-1586873944.41-230345502361639/AnsiballZ_mysql_db.py\", line 94, in _ansiballz_main\r\n    invoke_module(zipped_mod, temp_path, ANSIBALLZ_PARAMS)\r\n  File \"/home/jkastning/.ansible/tmp/ansible-tmp-1586873944.41-230345502361639/AnsiballZ_mysql_db.py\", line 40, in invoke_module\r\n    runpy.run_module(mod_name='ansible.modules.database.mysql.mysql_db', init_globals=None, run_name='__main__', alter_sys=True)\r\n  File \"/usr/lib64/python2.7/runpy.py\", line 176, in run_module\r\n    fname, loader, pkg_name)\r\n  File \"/usr/lib64/python2.7/runpy.py\", line 82, in _run_module_code\r\n    mod_name, mod_fname, mod_loader, pkg_name)\r\n  File \"/usr/lib64/python2.7/runpy.py\", line 72, in _run_code\r\n    exec code in run_globals\r\n  File \"/tmp/ansible_mysql_db_payload_kqF2w7/ansible_mysql_db_payload.zip/ansible/modules/database/mysql/mysql_db.py\", line 453, in <module>\r\n  File \"/tmp/ansible_mysql_db_payload_kqF2w7/ansible_mysql_db_payload.zip/ansible/modules/database/mysql/mysql_db.py\", line 446, in main\r\n  File \"/tmp/ansible_mysql_db_payload_kqF2w7/ansible_mysql_db_payload.zip/ansible/modules/database/mysql/mysql_db.py\", line 276, in db_import\r\nNameError: global name 'executed_commands' is not defined\r\n", "msg": "MODULE FAILURE\nSee stdout/stderr for the exact error", "rc": 1}

@Andersson007
Copy link
Contributor Author

Andersson007 commented Apr 14, 2020

Oh, sorry

# The line below is for returned data only:
        executed_commands.append('%s -dc %s | %s' % (comp_prog_path, target, cmd))

it doesn't exist in 2.9, so we need to remove this too.
One more time, please
Waiting for your feedback

@Tronde
Copy link

Tronde commented Apr 15, 2020

Hi @Andersson007,
That looks pretty good now. With the change from your last comment it works fine. The import of my test case was successful.

That was fun. Thank you for guiding me.

@Andersson007
Copy link
Contributor Author

@Tronde
Nice!
About the guiding - no problem at all!:) Thank you much for helping, it would've been impossible to find a working solution without it!
Ok, i'll complete the PR and let you know

@Andersson007 Andersson007 changed the title [WIP] mysql_db: prevent broken pipe errors by using an intermediate shell process mysql_db: prevent broken pipe errors by using an intermediate shell process Apr 15, 2020
@Andersson007 Andersson007 added the feature This issue/PR relates to a feature request label Apr 15, 2020
@Andersson007
Copy link
Contributor Author

ready_for_review

@Andersson007
Copy link
Contributor Author

@bmalynovytch , needs your opinion

@Andersson007
Copy link
Contributor Author

@Tronde done, if this solution is ok for you, please put shipit here (in a separate row)

@Tronde
Copy link

Tronde commented Apr 15, 2020

shipit

@Andersson007 Andersson007 changed the title mysql_db: prevent broken pipe errors by using an intermediate shell process mysql_db: add use_shell parameter to prevent Broken pipe errors Apr 15, 2020
@ansibullbot
Copy link
Collaborator

@ansibullbot ansibullbot added affects_2.10 bug This issue/PR relates to a bug has_issue integration tests/integration module module tests tests labels Apr 17, 2020
@Andersson007 Andersson007 merged commit 3b5520e into ansible-collections:master Apr 20, 2020
@Andersson007
Copy link
Contributor Author

has CI + tested manually, so, merged into master

@Andersson007
Copy link
Contributor Author

@Tronde thanks much for reporting and helping!
@felixfontein thanks for reviewing!

@Tronde
Copy link

Tronde commented Apr 21, 2020

@Andersson007 it has been a pleasure to help!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This issue/PR relates to a bug feature This issue/PR relates to a feature request has_issue integration tests/integration module module tests tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug / Regression] mysql_db import fail to decompress dumps
4 participants