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

ansible-postgresql_exec custom module error ValueError: I/O operation on closed file #555

Closed
gilesw opened this Issue Feb 28, 2019 · 7 comments

Comments

Projects
None yet
2 participants
@gilesw
Copy link

commented Feb 28, 2019

ubuntu, running off pips.

ansible 2.7.8
  config file = /home.local/gilesadm/work/ansible-config/ansible.cfg
  configured module search path = [u'/home.local/gilesadm/.ansible/plugins/modules', u'/usr/share/ansible/plugins/modules']
  ansible python module location = /opt/python-tools/local/lib/python2.7/site-packages/ansible
  executable location = /opt/python-tools/bin/ansible
  python version = 2.7.3 (default, Oct 26 2016, 21:01:49) [GCC 4.6.3]

I'm hitting an error running a custom module I use:-

gilesw/ansible-postgresql_exec#1

Hopefully a simple one to fix.

@gilesw gilesw changed the title Error message running a custom module ansible-postgresql_exec custom module error ValueError: I/O operation on closed file Feb 28, 2019

@dw

This comment has been minimized.

Copy link
Owner

commented Feb 28, 2019

This one looks interesting -- I bet it's something to do with a difference in stdin/stdout. Should have time to look at this over the weekend, thanks for reporting!

@dw

This comment has been minimized.

Copy link
Owner

commented Feb 28, 2019

It's almost certainly due to

# fixes https://github.com/ansible/ansible/issues/3518                                 
import sys                                                                             
reload(sys)                                                                            
sys.setdefaultencoding('utf8')                                                         
import pipes            

Due to ansible/ansible#3518

@dw

This comment has been minimized.

Copy link
Owner

commented Feb 28, 2019

@dw

This comment has been minimized.

Copy link
Owner

commented Feb 28, 2019

It's been gone from the Ansible tree completely since Aug 2015, so this is just about third party compatibility

$ git log -S "sys.setdefaultencoding"  HEAD

commit ef594f708c6eca9ad45d4926b942cf6ce0f15ee1
Author: Brian Coca <brian.coca+git@gmail.com>
Date:   Thu Aug 27 12:27:38 2015 -0400

    remove old dead code

commit 43c1a9744765eebfb9eaf9113336d552cfc9096b
Author: Toshio Kuratomi <toshio@fedoraproject.org>
Date:   Mon Mar 30 19:19:34 2015 -0700

    Various unicode and backslash escape cleanups
    
    * Do backslash escape parsing in parse_kv() [was being done in the copy
      module purely for newlines in the copy module's content param before]
    * Make parse_kv always return unicode
    * Add bandaid to transform args to unicode until we can fix things
      calling parse_kv to always send it unicode.
    * Make split_args deal with unicode internally.  Warning, no bandaid for
      things calling split_args without giving it unicode (shouldn't matter
      as dealt with str internally before)
    * Fix copy and unarchive action plugins to not use setdefaultencoding
    * Remove escaping from copy (it was broken and made content into latin-1
      sometimes). escaping is now in parse_kv.
    * Expect that content is now a unicode string so transform to bytes just
      before writing to the file.
    * Add initial unittests for split_args and parse_kv.  4 failing
      tests.because split_args is injecting extra newlines.

commit 065733ad93abcc0031bbd3578bfc34b4f31a1753
Author: James Cammarata <jimi@sngx.net>
Date:   Fri Jan 2 07:51:15 2015 -0600

    Moving more action plugins over and fixing some bugs with role loading

commit 62d79568be16084718bda2d890b2b4e1d10cc41d
Author: James Cammarata <jimi@sngx.net>
Date:   Fri Nov 14 16:14:08 2014 -0600

    Creating playbook executor and dependent classes

commit bf916fb58a351ee409ef5bbb3899079712226ab7
Author: root <root@TINY.(none)>
Date:   Mon Nov 24 18:03:32 2014 +0000

    Adding first pass at win_copy, win_file and win_template modules.

commit 5062f4962f8e7939c4656a059fc86e0fb4a3bf55
Author: Matt Martz <matt@sivel.net>
Date:   Fri Mar 7 21:51:12 2014 -0600

    Unit tests for ansible.utils

commit c0a7f516714bc394c06eacd5be5a12e86411248e
Author: Matthew Thode <mthode@mthode.org>
Date:   Mon Dec 9 12:49:03 2013 -0600

    tests depend on the default encoding being utf8
    
    So we set the utils default encoding to be utf8

commit 2c28e1daea2fee5edccd6860bf493a4d8888b4a7
Author: Dylan Martin <dmartin@sccd.ctc.edu>
Date:   Mon Oct 21 15:22:42 2013 -0500

    unarchive module & action_plugin added

commit 9991a530ab039a2407344a295177ccfeb0f3a2b6
Author: Serge van Ginderachter <serge@vanginderachter.be>
Date:   Thu Aug 8 22:09:45 2013 +0200

    fix an encoding bug in copy content = lookup plugin
    
    closes #3518

This might warrant a monkey-patch. The alternative is breaking compatibility with ancient existing modules. The question is what the patch should do!

Some obvious behaviours

  • reload() on sys should silently succeed with no effect, that's a crazy hack to begin with
  • setdefaultencoding can be used to reopen our redirected stdio as UTF-8, without tearing apart the whole sys module

The alternative is to stop relying on sys.stdout redirects, and instead rely on FD redirection. It's slower but a lot more robust.

@dw

This comment has been minimized.

Copy link
Owner

commented Feb 28, 2019

Hi @gilesw

I've been over this module a few times, and I'm a little perplexed what the setdefaultencoding() workaround is being used for. It's naturally been made obsolete in newer Ansible versions, but that doesn't explain why it is needed in older Ansibles.

For example on Python 2, the 'read_file()' function will yield bytes, while on Python 3 it will decode UTF-8 by default

The only remaining use is on line 279, where "getdefaultencoding()" is used instead of simply 'utf-8' or similar.

Mitogen takes compatibility very seriously, but in this case, it'd be nice to understand what we're trying to be compatible with first before fixing it :)

@gilesw

This comment has been minimized.

Copy link
Author

commented Mar 1, 2019

@dw thanks for looking into this. I'm afraid I'm not the original author, or any kind of python programmer. I just looked at the postgresql modules that are part of Ansible as a sort of template to make recent modifications and added output from the query that's run (which seems vital to me.)

@dw dw closed this in 2bd0bbd Mar 6, 2019

dw added a commit that referenced this issue Mar 6, 2019

Merge remote-tracking branch 'origin/dmw'
* origin/dmw:
  issue #555: ansible: workaround ancient reload(sys) hack.
@dw

This comment has been minimized.

Copy link
Owner

commented Mar 6, 2019

This is now on the master branch and will make it into the next release. To be updated when a new release is made, subscribe to https://www.freelists.org/list/mitogen-announce

Thanks for reporting this!

dw added a commit that referenced this issue Mar 6, 2019

Merge remote-tracking branch 'origin/026' into stable
* origin/026:
  docs: update Changelog for release.
  Bump version for release.
  issue #555: ansible: workaround ancient reload(sys) hack.
  issue #554: mitogen_action_script fix
  issue #554: fix Ansible 2.4 compatibility
  issue #554: don't rely on tmp_path autoremoval in test.
  issue #554: track and remove multiple make_tmp_path() calls.
  docs: update Changelog.
  docs: drastically simplify install/changelog.
  issue #552: include process identity in log messages.
  issue #550: update Changelog.
  issue #550: parent: add explanatory comment.
  issue #550: fix up TTY ioctls on WSL 2016 Anniversary Update
  docs: update Changelog.
  service: make service list optional.
  docs: update Changelog; closes #548.
  issue #548: always treat transport=smart as 'ssh' for mitogen_via=.
  docs: better intro paragraph.
  .ci: copy private key file to tempdir.
  os_fork: more doc tweaks
  os_fork: more doc tweaks
  os_fork: yet more doc tidyup
  os_fork: more doc tweaks
  os_fork: clean up docs
  .ci: import soak scripts.
  .ci: allow containers for different jobs to run simultaneously
  os_fork: python 3 fixes and tests.
  issue #535: activate Corker on 2.4 in master too.
  issue #535: update Changelog.
  issue #535: wire mitogen.os_fork into Broker and Pool.
  issue #535: parent: add create_socketpair(size=..) parameter.
  issue #535: introduce mitogen.os_fork module and Corker class.
  issue #535: docs: update Changelog
  issue #535: service: support Pool.defer() like Broker.defer()
  issue #535: core: unicode.encode() may take importer lock on 2.x
  issue #535: docs: fix up Select doc
  issue #535: docs: update Changelog.
  issue #535: core/select: support selecting from Latches.
  core: increase cookie field lengths to 64-bit; closes #545.
  tests: ensure serialization restrictions are in effect
  tests/bench: set process affinity in throughput.py.
  docs: update copyright year.
  docs: update Changelog.
  core: Make Latch.put(obj=) optional.
  docs: change 'unreleased' Changelog format and add a hint.
  docs: update Changelog; closes #542.
  issue #542: return of select poller, new selection logic
  issue #542: .ci: move some tests to Azure and enable Mac job.
  ansible: create stub __init__.py for sdist.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.