standard-test-roles

Clone
Source Code
GIT

#33 inventory: enable standard-inventory-docker to pass extra arguments to "docker run" from TEST_DOCKER_EXTRA_ARGS environment variable
Merged 6 years ago by stefw. Opened 6 years ago by merlinm.
Unknown source inventory-docker-extra-args  into  master

file modified
+14 -5 
@@ -19,14 +19,16 @@
 

      parser = argparse.ArgumentParser(description="Inventory for a container image in a registry")
 

      parser.add_argument("--list", action="store_true", help="Verbose output")
 

      parser.add_argument('--host', help="Get host variables")
 

+     parser.add_argument('--docker-extra-args', help="Extra docker arguments for launching container",
 

+                         default=os.environ.get("TEST_DOCKER_EXTRA_ARGS", ""))
 

      parser.add_argument("subjects", nargs="*", default=shlex.split(os.environ.get("TEST_SUBJECTS", "")))
 

      opts = parser.parse_args()
 

  

      try:
 

          if opts.host:
 

-             name, data = host(opts.host)
 

+             name, data = host(opts.host, opts.docker_extra_args)
 

          else:
 

-             data = list(opts.subjects)
 

+             data = list(opts.subjects, opts.docker_extra_args)
 

          sys.stdout.write(json.dumps(data, indent=4, separators=(',', ': ')))
 

      except RuntimeError, ex:
 

          sys.stderr.write("{0}: {1}\n".format(os.path.basename(sys.argv[0]), str(ex)))
 
@@ -34,19 +36,19 @@
 

  

      return 0
 

  

- def list(subjects):
 

+ def list(subjects, docker_extra_args):
 

      hosts = [ ]
 

      variables = { }
 

      for subject in subjects:
 

          if subject.startswith("docker:"):
 

              image = subject[7:]
 

-             name, vars = host(image)
 

+             name, vars = host(image, docker_extra_args)
 

              if vars:
 

                  hosts.append(name)
 

                  variables[name] = vars
 

      return { "localhost": { "hosts": hosts, "vars": { } }, "subjects": { "hosts": hosts, "vars": { } }, "_meta": { "hostvars": variables } }
 

  

- def host(image):
 

+ def host(image, docker_extra_args):
 

      null = open(os.devnull, 'w')
 

  

      try:
 
@@ -65,11 +67,18 @@
 

      except ValueError:
 

          diagnose = 0
 

  

+     # Check for any additional arguments to include when starting docker container
 

+     try:
 

+         extra_arg_list = shlex.split(docker_extra_args)
 

+     except ValueError:
 

+         raise RuntimeError("Could not parse DOCKER_EXTRA_ARGS")
 

+ 

      sys.stderr.write("Launching Docker container for {0}\n".format(image))
 

  

      # And launch the actual container
 

      cmd = [
 

          "/usr/bin/docker", "run", "--detach", "--cidfile={0}".format(cidfile),
 

+     ] + extra_arg_list + [
 

          "--entrypoint=/bin/sh", image, "-c", "sleep 1000000"
 

      ]

This is the first step needed to address issue #30. Actually, it's all that's needed to address the issue--the rest is left up to a custom inventory script to be written and included in the tests/inventory directory.

I'm interested, what is the benefit of using an environment variable over additional command line args here?

However, if you think the environment variable is better than a command line argument, I do think we should stick to the TEST_ namespace for environment variables.

I went with the environment variable approach because:

  1. You suggested it in issue #30 :smirk:
  2. It was simple to implement
  3. I couldn't find any clear definition of the command line options and arguments the scripts might receive; it already appears they are gathering extra arguments as the "subjects"
  4. Scripts will just ignore any environment variables they don't care about; unexpected arguments will cause them to fail

That said, I could make further updates to have the script accept a --docker-extra-args=... option (using the environment variable as a default) if you like.

I also have no issue changing the name of the environment variable. Is TEST_DOCKER_EXTRA_ARGS acceptable? Or should INVENTORY be embedded in the name, too?

rebased
6 years ago

PR has been updated to accept docker options from the --docker-extra-args=... command line option, using the TEST_DOCKER_EXTRA_ARGS environment variable as a default.

Your logic is rock solid. Especially point 4.

Do you have an example of this in use? A pull request with such an example would be perfect. Once we have that, this looks good to me.

The following commit serves as a proof of concept example:

https://upstreamfirst.fedorainfracloud.org/fork/merlinm/sed/c/7cbc3a7e5cbbe00a7d5880b4175f3a1db5a29ae6?branch=custom-inventory-wrappers

Based on my knowledge of ansible dynamic inventory scripts, it seems that all scripts from /usr/share/ansible/inventory need to have their own wrapper in the tests/inventory directory. This is less than ideal if additional inventory scripts get added in the future, since every package that makes use of custom inventory scripts would then need to have a corresponding wrapper added.

Your logic is rock solid. Especially point 4.

@stefw, So... should I revert the command line argument handling I added to this PR?

First of all, I don't think this problem blocks this PR:

Based on my knowledge of ansible dynamic inventory scripts, it seems that all scripts from /usr/share/ansible/inventory need to have their own wrapper in the tests/inventory directory. This is less than ideal if additional inventory scripts get added in the future, since every package that makes use of custom inventory scripts would then need to have a corresponding wrapper added.

Until we see this, we won't know if it's a real problem. It's possible, even likely, that new inventory scripts will launch a subject that is foreign to existing tests.

But yes, even if it's not a real problem, having a wrapper for each and every existing inventory script when a test would like to override one of them, is unsightly.

A solution of the top of my head: Have a standard script that wraps all the dynamic inventory scripts in /usr/share/ansible/inventory after setting up environment variables and merges their results. I've seen this elsewhere, but exactly where escapes me at the moment.

Let me know if you want me to try this out, or are willing to give it a go.

Note that tests/inventory can either be a file, an executable, or a directory, or nothing according to the spec. In the first three cases we substutite /usr/share/ansible/inventory. So an executable script as tests/inventory could implement or link the above logic.

Works well. Tested with the sed package.

Pull-Request has been merged by stefw
6 years ago

A solution of the top of my head: Have a standard script that wraps all the dynamic inventory scripts in /usr/share/ansible/inventory after setting up environment variables and merges their results. I've seen this elsewhere, but exactly where escapes me at the moment.

I was wondering if it was possible to somehow merge the dynamic inventory results. If you figure out where you saw it, please let me know--if not, I'll start digging and see what I can find.

Let me know if you want me to try this out, or are willing to give it a go.

I'd definitely like to try it out. Having a single somewhat future-proof script to maintain would certainly be preferable.

@stefw, I think I've worked out a way to handle wrapping and merging the dynamic inventory scripts! Please take a look at my commits (files inventory and merge-standard-inventory) in the following branch:

https://upstreamfirst.fedorainfracloud.org/fork/merlinm/sed/commits/custom-inventory-wrapper-2

If we have the standard-test-roles package install merge-standard-inventory somewhere, it will be extremely simply and maintainable for the test developers/maintainers to use.

@stefw, assuming the above wrapper/merge script looks reasonable, where would be an appropriate location to install it? Perhaps /usr/share/ansible/utils, /usr/share/ansible/inventory_{utils,library,support,misc,...}, or simply /usr/bin?

I believe the intent is that a small wrapper script executes this script? I would suggest that in the $PATH would be appropriate, and easiest to consume, and change later.

But that's not a strong opinion.

I've tested the new code:

$ sudo docker ps
CONTAINER ID        IMAGE                         COMMAND                  CREATED             STATUS              PORTS               NAMES
3ad61736662f        docker.io/library/fedora:26   "/bin/sh -c 'sleep..."   6 seconds ago       Up 4 seconds                            practical_jepsen
$ sudo docker exec -ti practical_jepsen /bin/bash
[root@3ad61736662f /]# ls /dev
autofs           mapper              stdin   tty36  tty9    uhid
bsg              mcelog              stdout  tty37  ttyS0   uinput
btrfs-control    mei0                tty     tty38  ttyS1   urandom

In addition the tests pass on classic, atomic and container test contexts with appropriate subjects.

I like the approach. The linger code was a surprise, but that's to be expected given our use of Ansible inventory scripts. Will do proper code review once this is a pull request in standard-test-roles.

I believe the intent is that a small wrapper script executes this script? I would suggest
that in the $PATH would be appropriate, and easiest to consume, and change later.
But that's not a strong opinion.

Yes, it would be executed by a very small inventory wrapper script such as I provided in my sed example:

1
2
3
#!/bin/bash
export TEST_DOCKER_EXTRA_ARGS="--privileged"
exec ./merge-standard-inventory "$@"

Installing it to /usr/bin would certainly be the most straight forward for the consumer.

See PR#36 for the inventory merge script.
Metadata
None
No Tags
Changes Summary 1
+14 -5
file changed
inventory/standard-inventory-docker
Powered by Pagure 5.13.3
Documentation • File an Issue • About • SSH Hostkey/Fingerprint
© Red Hat, Inc. and others.