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

Ziploader #15246

Merged
merged 13 commits into from Apr 5, 2016
Merged

Ziploader #15246

merged 13 commits into from Apr 5, 2016

Conversation

abadger
Copy link
Contributor

@abadger abadger commented Apr 1, 2016

ISSUE TYPE
  • Feature Pull Request
SUMMARY

Ziploader is a new way to construct modules for shipping over the wire from the controller to the managed machine. It works for new-style python modules (all non-windows modules that are shipped with ansible). All other module types continue to be processed by module replacer.

This pull request should be feature complete with regards to replacing module replacer. It additionally has some slightly better error reporting than module replacer as module_utils tracebacks will display proper line number and file name with this code. If ZIP_DEFLATED is selected as the compression type (the default) then the over-the-wire size of modules is also reduced at the cost of spending CPU compressing the modules.

ziploader is mostly backwards compatible with module replacer code. These are the things that I've identified that would be backwards incompatible:

Technically backwards incompatible changes

ziploader changes

  • module_utils code no longer has access to globals from another file. This is
    due to not substituting the code into one big python module. Modules
    themselves still have access to module_utils globals until explicitly changed
    because they use "import *"
    • signed off at meeting in early March. jimi-c, bcoca.

Changes made at the same time as ziploader as general module_common cleanup

  • No longer substitute all REPLACER strings
    • Removed remnants of REPLACER_ARGS (not substituted in v2 code so not backwards incompatible)
    • In old-style and non_native_want_json modules, do no REPLACER substitution.
      • jimi-c says this is how it was supposed to be but the code didn't reflect
        it before, March 29
    • In new-style JSONARGS modules do not substitute
      from ansible.module_utils.FOO import *. The new code decides that if
      we are using from ansible.module_utils.FOO then there's no reason to
      use JSONARGS (Note: JSONARGS modules never substituted REPLACER in the
      past and continues not to substitute it now).
    • Removed REPLACER_WINARGS (use REPLACER_JSONARGS instead)
      [x] signed off by nitzmahone, Mar 30
    • In new-style windows modules, only substitute REPLACER_WINDOWS
      (include powershell.ps1) and REPLACER_JSONARGS (arguments as a json
      string). All others deemed to be python or *nix centric and therefore not
      needed for windows powershell modules.
    • In new-style python modules (determined by presence of REPLACER or
      "[..]import[..]ansible.module_utils[...]" only substitute REPLACER (with
      import * from basic). Some are deemed not needed and others were, added
      as constants:
      • REPLACER_COMPLEX: added to os.environ['ANSIBLE_MODULE_ARGS']
      • REPLACER_WINDOWS: windows only (substitution is powershell)
      • REPLACER_JSONARGS: not needed for new-style python scripts as they get
        args via ANSIBLE_MODULE_ARGS
      • REPLACER_VERSION: from ansible import __version__ is the ziploader way to
        get this.
      • REPLACER_SELINUX: Added to os.environ['ANSIBLE_MODULE_CONSTANTS'] as
        SELINUX_SPECIAL_FS. changed from a string into a list. See the
        ziploader section for more info.
      • syslog.LOG_USER: Added to os.environ['ANSIBLE_MODULE_CONSTANTS'] as
        SYSLOG_FACILITY. Changed from the syslog variable into a string. See
        the ziploader section for more details.

The ziploader section mentioned above is here: https://gist.github.com/abadger/d3d1917cb9cb7b1cf31454f0c726e41b#file-gistfile1-txt-L278

@abadger
Copy link
Contributor Author

abadger commented Apr 1, 2016

Also -- I'll be working on adding more features on top of this. This PR is to get us a checkpoint at basic functionality so that we can revert to it if the new features don't pan out in time for 2.1 for whatever reasons.

@abadger abadger force-pushed the ziploader branch 3 times, most recently from c6b3ed0 to 18a7198 Compare April 2, 2016 03:08
if not os.path.exists(directory):
os.makedirs(directory)
f = open(dest_filename, 'w')
f.write(z.read(filename))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

need to close the fd here (will using a 'with' on the fd break in 2.4 even if we don't execute this function?)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now closed on the next line.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And yeah, with is a keyword being used in a way that couldn't be mistaken for a variable name so it will cause a syntaxerror on python-2.4.

@nitzmahone
Copy link
Member

If the couple things I mentioned get fixed, module debuggability on KEEP_REMOTE_FILES is still decent. I'll try out debugging modules natively w/ module_utils next week.

@abadger abadger force-pushed the ziploader branch 3 times, most recently from 87efcfe to 8c97fe9 Compare April 2, 2016 04:38
z = zipfile.ZipFile(zipped_mod)
for filename in z.namelist():
if filename.startswith('/'):
raise Exception('Something wrong with this module zip file: should not contain absolute paths')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we be raising an Exception here, or returning some JSON/sys.exit(1) as a nice exit response?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should be raising an Exception because this won't be sent back to ansible.

The debug() functionality is something I worked on with @nitzmahone last night. We realized that it would be harder to debug modules via ANSIBLE_KEEP_REMOTE_FILES using ziploader because what you would end up keeping is this wrapper with the zipped payload in ZIPDATA. So the debug() function contains two things to ameliorate that. First: If you run the wrapper file with explode as a command line arg it will take the payload and extract it into the ansible temporary directory (where the module file was uploaded). Second: If you run the wrapper file with execute as a command line arg it will run the module files that were exploded to disk rather than the zipped payload that's contained in the wrapper.

These two commands enable you to debug a module using the following steps:

$ ANSIBLE_KEEP_REMOTE_FILES=1 ansible localhost -m ping -a data=crash -vvv
Using /etc/ansible/ansible.cfg as config file
<127.0.0.1> ESTABLISH LOCAL CONNECTION FOR USER: badger
<127.0.0.1> EXEC /bin/sh -c '( umask 77 && mkdir -p "` echo $HOME/.ansible/tmp/ansible-tmp-1459605113.37-203813258173
480 `" && echo "` echo $HOME/.ansible/tmp/ansible-tmp-1459605113.37-203813258173480 `" )'
<127.0.0.1> PUT /var/tmp/tmpvdVWjY TO /home/badger/.ansible/tmp/ansible-tmp-1459605113.37-203813258173480/ping
<127.0.0.1> EXEC /bin/sh -c 'LANG=en_US.UTF-8 LC_ALL=en_US.UTF-8 LC_MESSAGES=en_US.UTF-8 /usr/bin/python /home/badger
/.ansible/tmp/ansible-tmp-1459605113.37-203813258173480/ping'
An exception occurred during task execution. The full traceback is:
[...]
# Log into remote machine
$ /usr/bin/python /home/badger
/.ansible/tmp/ansible-tmp-1459605113.37-203813258173480/ping explode
Module expanded into: /home/badger/.ansible/tmp/ansible-tmp-1459605113.37-203813258173480/ansible
# Edit the files in /home/badger/.ansible/tmp/ansible-tmp-1459605113.37-203813258173480/ansible to introspect what is happening as the module runs.   Change the code to see if it will still run, etc.
$ /usr/bin/python /home/badger/.ansible/tmp/ansible-tmp-1459605113.37-203813258173480/ping execute
# The execute command runs the exploded files instead of the zipped payload.  So it runs the module with all of the changes you made in the step above.

@abadger
Copy link
Contributor Author

abadger commented Apr 2, 2016

Talked to @jimi-c today and added more comments to the wrapper and implemented comment stripping of it. This lets us make the wrapper more readable in source code but still maintain a small size for over the wire transfer.

jimi-c and others added 11 commits April 4, 2016 13:26
* python3 compatible base64 encoding
* zipfile compression (still need to enable toggling this off for
  systems without zlib support in python)
* Allow non-wildcard imports (still need to make this recusrsive so that
  we can have module_utils code that imports other module_utils code.)
* Better tracebacks: module filename is kept and module_utils directory
  is kept so that tracebacks show the real filenames that the errors
  appear in.
This may be necessary on systems where python has been compiled without
zlib compression.
* module replacer only replaces values that make sense for that type of
  file (example: don't attempt to replace python imports if we're in
  a powershell module).
* Implement configurable shebang support for ziploader wrapper
* Implement client-side constants (for SELINUX_SPECIAL_FS and SYSLOG)
  via environment variable.
* Remove strip_comments param as we're never going to use it (ruins line
  numbering)
@abadger abadger merged commit 4b0aa12 into ansible:devel Apr 5, 2016
@abadger abadger deleted the ziploader branch April 5, 2016 18:06
@abadger
Copy link
Contributor Author

abadger commented Apr 5, 2016

Okay, All found problems addressed. Merging. New abadger/ziploader branch will have recursive imports.

@ansible ansible locked and limited conversation to collaborators Apr 26, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants