First pass at allowing binary modules #13771

sivel · 2016-01-08T16:35:04Z

#6374 is a bit out of date with all of the changes for 2.0.

This PR adds some proof of concept changes that allow ansible to utilize binary modules that have no shebang and whatnot.

binary detection is done similarly to the file command using a concept found at http://stackoverflow.com/a/7392391

There are no doubt some things missing right now, but this is working for the standard action (~~not tested with async~~) and the ssh transport.

A few things that need to be looked at:

~~async~~ (Async seems to work fine per my tests)
~~windows~~ (Had to change some things around to allow .exe extensions for modules, but working now, assumes windows binaries are always .exe)
~~binaries other than go, and non ELF~~ (Have testing things from executables, to images, to PDFs, which all report properly)

bcoca · 2016-01-08T16:36:11Z

he, i was doing same last night, but ran out of gas.

bcoca · 2016-01-08T16:36:46Z

i was actually going to leverage the stat module to get the 'binary status'

sivel · 2016-01-08T16:36:48Z

Just as an example, this is the golang code for the module I was testing with:

package main

import (
    "fmt"
    "os"
)

func main() {
    fmt.Printf(`{"arg": "%v"}`, os.Args[1])
}

A more full featured example can be found at: https://gist.github.com/sivel/ccd81bdfb31ca0c0e05d

sivel · 2016-01-11T17:33:03Z

So far it looks like everything is working as expected. I've tested with ssh, paramiko, local, and winrm. I've also mixed non binary and binary modules within the same playbook.

See this gist for details about the test playbook and example module: https://gist.github.com/sivel/ccd81bdfb31ca0c0e05d

cygnetix · 2016-01-19T23:02:23Z

This is awesome @sivel. Thanks for taking the time to work on this. I can't wait to see it merged (fingers crossed).

tom025 · 2016-02-03T19:25:43Z

Thanks for effort @sivel can't wait to see this merged too.

sivel · 2016-02-12T18:01:14Z

I've added integration tests for binary modules.

Executed like:

INVENTORY=binary_hosts make test_binary_modules

My binary_hosts looks roughly like:

localhost ansible_connection=local
windows ansible_host=1.1.1.1 ansible_user=Administrator ansible_password=MY_PASSWORD ansible_port=5986 ansible_connection=winrm ansible_winrm_server_cert_validation=ignore ansible_winrm_transport=ssl
paramiko ansible_host=2.2.2.2 ansible_user=root ansible_password=MY_PASSWORD ansible_connection=paramiko
ssh ansible_host=3.3.3.3 ansible_user=root ansible_password=MY_PASSWORD ansible_connection=ssh

In my particular case localhost is a Mac (darwin).

sivel · 2016-02-12T18:04:00Z

The current build failure is due to travis not properly installing python2.4 and not related to this PR.

sivel · 2016-02-12T22:11:07Z

Rebased and merge conflicts resolved due to b4b24a0

sivel · 2016-04-12T23:09:50Z

Per the core meeting today, this will be targeted for implementation in v2.2, to land in devel after the 2.1 release (https://meetbot.fedoraproject.org/ansible-meeting/2016-04-12/ansible_core_meeting.2016-04-12-19.02.html)

I've rebased against devel, and handled merge conflicts.

I have fixed failing tests, but have not added any additional unit tests around this as of yet. Integration tests are there, but are not currently running as part of make.

abadger · 2016-04-25T18:34:02Z

lib/ansible/plugins/action/__init__.py

@@ -549,7 +563,7 @@ def _execute_module(self, module_name=None, module_args=None, tmp=None, task_var

        # Fix permissions of the tmp path and tmp files.  This should be
        # called after all files have been transferred.
-        self._fixup_perms(tmp, remote_user, recursive=True)
+        self._fixup_perms(tmp, remote_user, recursive=True, execute=is_binary)


I've been thinking perhaps we should always set execute=True? In the python module case, it is still a script intended to be executed so it seems reasonable.

We execute the interpreter directly by substituting ansible__interpreter so not sure that this is needed (unless we want to change to always execute the file?

The two things I was thinking about were: When someone sets ANSIBLE_KEEP_REMOTE_FILES=1 and then wants to execute the wrapper and async. async currently sets execute=True so that async_wrapper can execute the module that was copied over. Started me wondering if there's any harm to just setting execute=True by default.

abadger · 2016-04-25T18:43:23Z

I like this for 2.2. I reviewed everything except the shell changes. @nitzmahone you probably shold look at those since it looks like powershell has the most complexity to it.

About the integration tests -- it would be nice to turn it on by default. I wonder if we could build the go module and have it in the git repo in addition to the source code? Would that work? What do others think about that?

abadger · 2016-04-25T18:44:47Z

docsite/rst/developing_modules.rst

+would resemble something similar to::
+
+    {
+        "name": "Ansible",


Maybe use something different than name and Ansible here to be clear that this the module parameters rather than more special variables. Maybe something like "data" like the ping module uses?

nitzmahone · 2016-04-25T20:44:11Z

@abadger agreed on all counts- if the integration tests aren't running on this all the time, it will get broken. I need to try with a .NET module and see, but in that case, we definitely don't want any interpreter (just direct exec the module)

abadger · 2016-05-12T17:26:04Z

lib/ansible/executor/module_common.py

+    textchars = bytearray(set([7, 8, 9, 10, 12, 13, 27]) | set(range(0x20, 0x100)) - set([0x7f]))
+
+    with open(module_path, 'rb') as f:
+        start = f.read(1024)


Do we need to re-read this file or can we adapt this code to use module_data?

I'll do some testing, and look at how we are opening the file elsewhere.

abadger · 2016-05-12T17:36:42Z

A few minor comments. Everything else looks good. If this is passing tests and you think it's ready to merge to devel then I'm okay with it. Tests -- I'm not sure how long compiling the module takes but I expect it's minimal. I think you're right, we probably should put golang into the docker images.

…o the top of the stack

…e already has access to it

robinro · 2016-05-20T09:26:09Z

I find it suboptimal to download go and and build stuff during the tests, while on the other hand we try preinstalling packages where possible to make travis faster. Maybe you can instead use precompiled binaries?

robinro · 2016-05-20T09:33:38Z

This change broke my tests for the new images on opensuseleap and ubuntu16.04 due to missing with curl/wget (why use both?).

There's something broken with the tests in general, because even the travis run that gave you a green light for the merge was done without curl installed:
https://travis-ci.org/ansible/ansible/jobs/129803359#L9053
No idea why it didn't fail.

robinro · 2016-05-20T09:43:22Z

Ah, now I understand the idea, it uses either curl or wget. Still...

sivel · 2016-05-20T12:08:57Z

We will be moving adding golang to the docker containers. To get things in faster we didn't block this to wait on updating the images. This is on the to do list. We however will still do compilation at build, and will not be storing the binary files in the repo.

robinro · 2016-05-20T12:23:06Z

Is there already a PR for golang?
There is an update to the dockerfiles by @gundalow queued up in #15692 which I depend on for #15936.

sivel · 2016-05-20T12:29:38Z

It is on the todo list, meaning it still needs to be worked on. There is no PR currently.

bcoca added the feature_pull_request label Jan 8, 2016

sivel force-pushed the binary-modules branch from 793db18 to bff6e00 Compare January 8, 2016 17:47

sivel force-pushed the binary-modules branch from e2ee671 to 3bb2785 Compare January 12, 2016 18:15

sivel mentioned this pull request Jan 13, 2016

Add the ability to run binary modules #6374

Closed

sivel added this to the next milestone Jan 13, 2016

sivel force-pushed the binary-modules branch from 3bb2785 to dfbf085 Compare January 15, 2016 17:11

sivel force-pushed the binary-modules branch from dfbf085 to fc55bd1 Compare February 12, 2016 17:56

sivel force-pushed the binary-modules branch from 22f4e0f to 6b17eb8 Compare February 12, 2016 22:10

sivel removed this from the 2.1.0 milestone Apr 12, 2016

sivel force-pushed the binary-modules branch 2 times, most recently from 6c1ce7b to 069424c Compare April 12, 2016 20:53

sivel self-assigned this Apr 12, 2016

sivel added this to the 2.2.0 milestone Apr 12, 2016

sivel force-pushed the binary-modules branch 2 times, most recently from 224bf01 to ab03ca5 Compare April 13, 2016 18:58

abadger reviewed Apr 25, 2016
View reviewed changes

sivel added 9 commits May 12, 2016 12:25

Don't register new vars that aren't needed

35246ab

Add integration tests for binary modules

6ad8ec0

Add GPL3 header to helloworld.go

2d18607

Re-implement/move some code lost due to merge conflicts

ddf3c38

Add note about reading input for binary modules

c22c1b4

Bumping binary modules functionality to 2.2

a4d2238

Resolve test failures

3466e73

Move binary module detection into executor/module_common.py

0faddfa

Improve documentation about the JSON args file

2e8146c

sivel force-pushed the binary-modules branch from 3552879 to 2e00b83 Compare May 12, 2016 17:25

abadger reviewed May 12, 2016
View reviewed changes

sivel added 3 commits May 12, 2016 12:53

Run test_binary_modules

651b83d

Make _is_binary use already read module_data, move _is_binary check t…

34adb54

…o the top of the stack

modify_module does not need to return module_path, as the calling cod…

ca22783

…e already has access to it

sivel force-pushed the binary-modules branch from 2e00b83 to ca22783 Compare May 12, 2016 17:53

sivel merged commit 196453b into ansible:devel May 12, 2016

ansibot added feature This issue/PR relates to a feature request. and removed feature_pull_request labels Mar 4, 2018

ansible locked and limited conversation to collaborators Apr 26, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

First pass at allowing binary modules #13771

First pass at allowing binary modules #13771

sivel commented Jan 8, 2016

bcoca commented Jan 8, 2016

bcoca commented Jan 8, 2016

sivel commented Jan 8, 2016

sivel commented Jan 11, 2016

cygnetix commented Jan 19, 2016

tom025 commented Feb 3, 2016

sivel commented Feb 12, 2016

sivel commented Feb 12, 2016

sivel commented Feb 12, 2016

sivel commented Apr 12, 2016

abadger Apr 25, 2016

bcoca Apr 25, 2016

abadger May 11, 2016

abadger commented Apr 25, 2016

abadger Apr 25, 2016

nitzmahone commented Apr 25, 2016

abadger May 12, 2016

sivel May 12, 2016

abadger commented May 12, 2016

robinro commented May 20, 2016

robinro commented May 20, 2016

robinro commented May 20, 2016 •

edited

Loading

sivel commented May 20, 2016

robinro commented May 20, 2016

sivel commented May 20, 2016

First pass at allowing binary modules #13771

First pass at allowing binary modules #13771

Conversation

sivel commented Jan 8, 2016

bcoca commented Jan 8, 2016

bcoca commented Jan 8, 2016

sivel commented Jan 8, 2016

sivel commented Jan 11, 2016

cygnetix commented Jan 19, 2016

tom025 commented Feb 3, 2016

sivel commented Feb 12, 2016

sivel commented Feb 12, 2016

sivel commented Feb 12, 2016

sivel commented Apr 12, 2016

abadger Apr 25, 2016

Choose a reason for hiding this comment

bcoca Apr 25, 2016

Choose a reason for hiding this comment

abadger May 11, 2016

Choose a reason for hiding this comment

abadger commented Apr 25, 2016

abadger Apr 25, 2016

Choose a reason for hiding this comment

nitzmahone commented Apr 25, 2016

abadger May 12, 2016

Choose a reason for hiding this comment

sivel May 12, 2016

Choose a reason for hiding this comment

abadger commented May 12, 2016

robinro commented May 20, 2016

robinro commented May 20, 2016

robinro commented May 20, 2016 • edited Loading

sivel commented May 20, 2016

robinro commented May 20, 2016

sivel commented May 20, 2016

robinro commented May 20, 2016 •

edited

Loading