Repository: ReproNim/reproman

Issue:
retrace docker container 
currently we can retrace only an image, but not the container
We should be able to retrace container, and point to the docker image which was used to start it

That could provide a solution to be able to trace execution of docker images.

Discussion:

Patch:
diff --git a/reproman/interface/docker.shim b/reproman/interface/docker.shim
new file mode 100755
index 0000000..3c46075
--- /dev/null
+++ b/reproman/interface/docker.shim
@@ -0,0 +1,136 @@
+#!/usr/bin/perl
+#emacs: -*- mode: shell-script; c-basic-offset: 4; tab-width: 4; indent-tabs-mode: t -*-
+#ex: set sts=4 ts=4 sw=4 noet:
+#
+#
+# This script is a shim for the docker executable. The shim intercepts
+# calls to the docker executable, runs an image id discovery process, and
+# then proceeds with the original call to the original docker executable.
+#
+require 5.002;
+use strict;
+use warnings;
+use List::Util qw(first);
+
+# Parse docker switches and run command parameters.
+my ($run_token_index) = first { $ARGV[$_] eq 'run' } (0..$#ARGV);
+die("No 'run' command found in docker arguments") if (!defined $run_token_index);
+my ($DEBUG) = first { $ARGV[$_] eq '--debug' or $ARGV[$_] eq '-D' } (0..$run_token_index-1);
+my $orig_run_args = join(" ", @ARGV[($run_token_index+1)..$#ARGV]);
+
+# Print debug output
+# 
+# Parameters
+# ----------
+# $output
+#     string : Text to print to the screen.
+# 
+sub debug {
+    return if (!defined $DEBUG);
+    foreach my $line (@_) {
+        print "[DEBUG DOCKER SHIM] ${line}";
+        print "\n" if (!($line =~ /\n$/));
+    }
+}
+
+# Run commands in shell
+# 
+# Parameters
+# ----------
+# $command
+#     string : Command to execute in the shell
+# $default_message
+#     string : Message to send if nothing is found in command stderr
+#
+# Returns
+# -------
+# string : stdout of the executed command
+# 
+sub exec_handler {
+
+	my $command = shift;
+	my $default_message = shift;
+
+	my $output = qx/$command 2>&1/;
+	if ($? != 0) {
+		die $output ? $output : $default_message;
+	}
+	return $output
+}
+
+# Gather ENV info
+my $TRACER_DIR = $ENV{"REPROMAN_TRACER_DIR"};
+if (!$TRACER_DIR) {
+	die "ENV variable REPROMAN_TRACER_DIR not found";
+}
+debug("TRACER_DIR: ${TRACER_DIR}");
+my $EXTRA_TRACE_FILE = $ENV{"REPROMAN_EXTRA_TRACE_FILE"};
+if (!$EXTRA_TRACE_FILE) {
+	die "ENV variable REPROMAN_EXTRA_TRACE_FILE not found";
+}
+debug("EXTRA_TRACE_FILE: ${EXTRA_TRACE_FILE}");
+my $WAIT_SCRIPT = "wait.sh";
+
+# Locate original Docker executable (Most likely in /usr/bin)
+# The path of the original executable will appear second to our shim
+# as we add the shim to the front of the path in interface/execute.py
+my $stdout = exec_handler("which -a docker", "Unable to execute system 'which' command");
+my @docker_paths = split(/\s+/, $stdout);
+my $original_docker_exec = $docker_paths[1];
+if (!$original_docker_exec) {
+  die "Original docker executable not found";
+}
+debug("Found docker executable: ${original_docker_exec}");
+
+# Create the script that will be the entrypoint of the container needed to
+# figure out the image RepoDigest value. The container will be removed when no
+# longer needed or, if the shim fails, will remove itself after the sleep
+# command completes.
+open(my $fh, '>', "${TRACER_DIR}/${WAIT_SCRIPT}")
+  or die "Could not open file '${TRACER_DIR}/${WAIT_SCRIPT}' $!";
+print $fh <<'EOF';
+#!/bin/sh
+sleep 30
+EOF
+close($fh);
+chmod(0755, "${TRACER_DIR}/${WAIT_SCRIPT}");
+
+# Pre-run the docker container in detached mode to get its id
+my $cmd = "${original_docker_exec} run -d --rm ";
+$cmd .= "-v ${TRACER_DIR}/${WAIT_SCRIPT}:/opt/${WAIT_SCRIPT} ";
+$cmd .= "--entrypoint /opt/${WAIT_SCRIPT} ${orig_run_args}";
+debug("PRE-RUN COMMAND: ${cmd}");
+$stdout = exec_handler($cmd, "Failed running command: ${cmd}");
+my @output = split("\n", $stdout);
+my $container_id = $output[@output - 1];
+debug("Found container id: ${container_id}");
+
+# Look up the docker image id by inspecting the container
+$cmd = "${original_docker_exec} container inspect ${container_id}";
+$stdout = exec_handler($cmd, "Failed to inspect container ${container_id}");
+$stdout =~ /"Image": "(\S+)"/;
+my $image_id = $1;
+debug("Found image id: ${image_id}");
+
+# Look up the image RepoDigest by inspecting the image
+$cmd = "${original_docker_exec} image inspect ${image_id}";
+$stdout = exec_handler($cmd, "Failed to inspect image ${image_id}");
+$stdout =~ /"RepoDigests": \[\s+"(\S+)"\s+\]/;
+my $digest_id = $1;
+debug("Found digest ID: ${digest_id}");
+open($fh, '>>', "${EXTRA_TRACE_FILE}")
+  or die "Could not open file '${EXTRA_TRACE_FILE}' $!";
+if ($digest_id) {
+  print $fh "- ${digest_id}\n";
+}
+close($fh);
+
+# Clean up the temp directory and the pre-run docker container
+$cmd = "${original_docker_exec} stop ${container_id}";
+exec_handler($cmd, "Failed to stop container ${container_id}");
+unlink("${TRACER_DIR}/${WAIT_SCRIPT}");
+
+# Run the original Docker command
+$cmd = "${original_docker_exec} " . join(" ", @ARGV);
+$stdout = exec_handler($cmd, "Failed to execute docker command: ${cmd}");
+print($stdout);
diff --git a/reproman/interface/execute.py b/reproman/interface/execute.py
index f8c8f20..8adfd53 100644
--- a/reproman/interface/execute.py
+++ b/reproman/interface/execute.py
@@ -16,6 +16,7 @@ import os.path as op
 import sys
 import time
 import uuid
+import yaml
 
 from .base import Interface
 from ..support.exceptions import CommandError
@@ -132,6 +133,12 @@ class TracedCommand(CommandAdapter):
         self.local_trace_dir = op.join(local_cache_dir, 'traces', self.exec_id)
         self.local_tracer_gz = op.join(self.local_tracer_dir,
                                        "reproman_trace.gz")
+        self.local_docker_shim = op.join(op.dirname(op.realpath(__file__)),
+                                        "docker.shim")
+        # extra_trace_file contains trace output generated by additional trace
+        # operations performed in addition to the main reprozip trace.
+        # e.g. the remote Docker container trace
+        self.extra_trace_file = "trace.extra.yml"
 
         # Remote session variables
         self.remote_dir = remote_dir
@@ -190,11 +197,19 @@ class TracedCommand(CommandAdapter):
         # TODO: might want to add also a "marker" so within the trace
         #       we could avoid retracing session establishing bits themselves
 
+        # Upload Docker shim
+        remote_shims_dir = op.join(remote_reproman_dir, 'shims')
+        self.remote_docker_shim = op.join(remote_shims_dir, "docker")
+        self.session.put(self.local_docker_shim, self.remote_docker_shim)
+
     def pre_command(self):
         self._prepare_local()
         self._prepare_remote()
 
     def execute(self):
+        remote_tracer_dir = op.dirname(self.remote_docker_shim)
+        remote_extra_trace_path = op.join(self.remote_trace_dir,
+            self.extra_trace_file)
         cmd_prefix = [
             self.remote_tracer,
             "--logfile", op.join(self.remote_trace_dir, "tracer.log"),
@@ -202,13 +217,24 @@ class TracedCommand(CommandAdapter):
             "--"
         ]
         return self.session.execute_command(
-            cmd_prefix + [self.command] + self.cmd_args)
+            cmd_prefix + [self.command] + self.cmd_args,
+            env=dict(
+                PATH="{}:$PATH".format(remote_tracer_dir),
+                REPROMAN_TRACER_DIR=remote_tracer_dir,
+                REPROMAN_EXTRA_TRACE_FILE=remote_extra_trace_path
+            ),
+            with_shell=True
+        )
 
     def post_command(self):
         # Copy all the tracing artifacts here if not present already (e.g.
         # if session was a local shell)
         if not op.exists(self.local_trace_dir):
-            for fname in ["tracer.log", "trace.sqlite3"]:
+            remote_files = ["tracer.log", "trace.sqlite3"]
+            if self.session.exists(op.join(self.remote_trace_dir,
+                    self.extra_trace_file)):
+                remote_files.append(self.extra_trace_file)
+            for fname in remote_files:
                 self.session.get(op.join(self.remote_trace_dir, fname),
                                  op.join(self.local_trace_dir, fname))
             lgr.info(
@@ -228,12 +254,21 @@ class TracedCommand(CommandAdapter):
             sort_packages=False,
             find_inputs_outputs=True)
 
+        local_extra_trace_file = op.join(self.local_trace_dir,
+            self.extra_trace_file)
+        if op.exists(local_extra_trace_file):
+            with open(local_extra_trace_file, 'r') as fp:
+                extra_files = yaml.safe_load(fp)
+        else:
+            extra_files = None
+
         from reproman.api import retrace
         reproman_spec_path = op.join(self.local_trace_dir, "reproman.yml")
         retrace(
             spec=op.join(self.local_trace_dir, "config.yml"),
             output_file=reproman_spec_path,
-            resref=self.session
+            resref=self.session,
+            path=extra_files
         )
         lgr.info("ReproMan trace %s", reproman_spec_path)
 
diff --git a/reproman/resource/docker_container.py b/reproman/resource/docker_container.py
index 751744d..7faad1d 100644
--- a/reproman/resource/docker_container.py
+++ b/reproman/resource/docker_container.py
@@ -223,20 +223,10 @@ class DockerSession(POSIXSession):
     container = attrib(default=attr.NOTHING)
 
     @borrowdoc(Session)
-    def _execute_command(self, command, env=None, cwd=None):
-        #command_env = self.get_updated_env(env)
-        if env:
-            raise NotImplementedError("passing env variables to docker session execution")
-
-        if cwd:
-            # TODO: implement
-            # raise NotImplementedError("handle cwd for docker")
-            lgr.warning("cwd is not handled in docker yet")
-            pass
-        # if command_env:
-            # TODO: might not work - not tested it
-            # command = ['export %s=%s' % k for k in command_env.items()] + command
-
+    def _execute_command(self, command, env=None, cwd=None, with_shell=True):
+        command = self._prefix_command(utils.command_as_string(command),
+                                        env=env, cwd=cwd,
+                                        with_shell=with_shell)
         # The following call may throw the following exception:
         #    docker.errors.APIError - If the server returns an error.
         lgr.debug('Running command %r', command)
diff --git a/reproman/resource/session.py b/reproman/resource/session.py
index e211e78..a4d17d5 100644
--- a/reproman/resource/session.py
+++ b/reproman/resource/session.py
@@ -22,8 +22,7 @@ from reproman.support.exceptions import SessionRuntimeError
 from reproman.cmd import Runner
 from reproman.dochelpers import exc_str, borrowdoc
 from reproman.support.exceptions import CommandError
-from reproman.utils import updated
-from reproman.utils import to_unicode
+from reproman.utils import updated, to_unicode
 
 import logging
 lgr = logging.getLogger('reproman.session')
@@ -35,7 +34,6 @@ class Session(object):
 
     INTERNAL_COMMANDS = ['mkdir', 'isdir', 'put', 'get', 'chown', 'chmod']
 
-
     def __attrs_post_init__(self):
         """
         Maintain both current and future session environments.
@@ -44,7 +42,7 @@ class Session(object):
         to make it available for sessions beyond the current one.
         """
         self._env = {}           # environment which would be in-effect only for this session
-        self._env_permanent = {} # environment variables which would be in-effect in future sessions if resource is persistent
+        self._env_permanent = {}  # environment variables which would be in-effect in future sessions if resource is persistent
 
     def __enter__(self):
         self.open()
@@ -167,7 +165,7 @@ class Session(object):
     # TODO: move logic/handling of batched commands defined in
     # Resource  and probably env vars handling
 
-    def execute_command(self, command, env=None, cwd=None):
+    def execute_command(self, command, env=None, cwd=None, with_shell=False):
         """
         Execute the given command in the environment.
 
@@ -182,6 +180,8 @@ class Session(object):
             removed
         cwd : string, optional
             Path of directory in which to run the command
+        with_shell: boolean, optional
+            Wrap the command with "/bin/sh -c" if the command is a string
 
         Returns
         -------
@@ -196,10 +196,10 @@ class Session(object):
         return self._execute_command(
             command,
             cwd=cwd,
-            **run_kw
-        )  # , shell=True)
+            with_shell=with_shell,
+            **run_kw)
 
-    def _execute_command(self, command, env=None, cwd=None):
+    def _execute_command(self, command, env=None, cwd=None, with_shell=False):
         """
         Execute the given command in the environment.
 
@@ -214,6 +214,8 @@ class Session(object):
             removed
         cwd : string, optional
             Path of directory in which to run the command
+        with_shell: boolean, optional
+            Wrap the command with "/bin/sh -c" if the command is a string
 
         Returns
         -------
@@ -523,6 +525,49 @@ class POSIXSession(Session):
             output[split[0]] = split[1]
         return output
 
+    def _prefix_command(self, command, env=None, cwd=None, with_shell=False):
+        """Wrap the command in a shell call with ENV vars and CWD prefixed
+        statment to command. Will pass through the command unchanged if env
+        and cwd are None
+
+        Parameters
+        ----------
+        command : string or list
+            Shell command to be run. No shell is used if command is a list
+        env : dict
+            ENV vars to prefix to command
+        cwd : string
+            Working directory to cd into before the command is executed
+        with_shell : boolean
+            if True and command is a string, a "/bin/sh -c" call will wrap the
+            cwd, env and command
+
+        Returns
+        -------
+        string or list
+            command with env, cwd, and/or with_shell settings prefixed
+        """
+        prefix = []
+        if env:
+            prefix = ["export"]
+            prefix += ["{}={}".format(shlex_quote(k[0]), shlex_quote(k[1]))
+                        for k in env.items()]
+            prefix += ["&&"]
+        if cwd:
+            prefix += ["cd", cwd, "&&"]
+        if with_shell and isinstance(command, str):
+            if prefix:
+                cmd = "{} {}".format(" ".join(prefix), command)
+            else:
+                cmd = "{}".format(command)
+            command = "/bin/sh -c '{}'".format(cmd)
+        elif prefix:
+            if isinstance(command, str):
+                command = " ".join(prefix) + " " + command
+            else:
+                command = prefix + command
+        return command
+
     @borrowdoc(Session)
     def source_script(self, command, permanent=False, diff=True, shell=None):
         orig_env = self.query_envvars()
@@ -588,7 +633,8 @@ class POSIXSession(Session):
     def exists(self, path):
         """Return if file exists"""
         try:
-            out, err = self.execute_command(self.exists_command(path))
+            out, err = self.execute_command(self.exists_command(path),
+                                            with_shell=False)
         except Exception as exc:  # TODO: More specific exception?
             lgr.debug("Check for file presence failed: %s", exc_str(exc))
             return False
@@ -707,11 +753,21 @@ def get_local_session(env={'LC_ALL': 'C'}, pty=False, shared=None):
         session.set_envvar(env)
     return session
 
+
 def get_updated_env(env, update):
     """Given an environment and set of updates, return updated one
 
     Special handling -- keys with None for the value, will be removed
     """
+    # If an update env var references itself, merge the original env value
+    # into the update value.
+    # (This is to somewhat generalize the "PATH=/foo:$PATH" special case.)
+    for k in update:
+        if k not in env or not isinstance(update[k], str) or \
+                not isinstance(env[k], str):
+            continue
+        update[k] = re.sub(r"\${?" + k + r"}?(:|\s|$)", env[k] + r"\1",
+                            update[k])
     env_ = updated(env, update)
     # pop those explicitly set to None
     for e in list(env_):
diff --git a/reproman/resource/shell.py b/reproman/resource/shell.py
index 763ed74..316ed8c 100644
--- a/reproman/resource/shell.py
+++ b/reproman/resource/shell.py
@@ -44,7 +44,7 @@ class ShellSession(POSIXSession):
         self._runner = None
 
     @borrowdoc(Session)
-    def _execute_command(self, command, env=None, cwd=None):
+    def _execute_command(self, command, env=None, cwd=None, with_shell=False):
         # XXX should it be a generic behavior to auto-start?
         if self._runner is None:
             self.open()
@@ -93,6 +93,7 @@ class ShellSession(POSIXSession):
         # put is the same as get for the shell resource
         self.get(src_path, dest_path, uid, gid)
 
+
 @attr.s
 class Shell(Resource):
 
diff --git a/reproman/resource/singularity.py b/reproman/resource/singularity.py
index 0d9057c..bd87c42 100644
--- a/reproman/resource/singularity.py
+++ b/reproman/resource/singularity.py
@@ -176,15 +176,12 @@ class SingularitySession(POSIXSession):
     _runner = Runner()
 
     @borrowdoc(Session)
-    def _execute_command(self, command, env=None, cwd=None):
-        if env:
-            raise NotImplementedError("passing env variables to session execution")
-        if cwd:
-            raise NotImplementedError("handle cwd for singularity")
+    def _execute_command(self, command, env=None, cwd=None, with_shell=True):
+        command = self._prefix_command(command_as_string(command), env=env,
+                                        cwd=cwd, with_shell=with_shell)
         lgr.debug('Running command %r', command)
         stdout, stderr = self._runner.run(
-            "singularity exec instance://{} {}".format(
-                self.name, command_as_string(command)),
+            "singularity exec instance://{} {}".format(self.name, command),
             expect_fail=True)
 
         return (stdout, stderr)
diff --git a/reproman/resource/ssh.py b/reproman/resource/ssh.py
index 718576a..cff875e 100644
--- a/reproman/resource/ssh.py
+++ b/reproman/resource/ssh.py
@@ -176,18 +176,13 @@ class SSHSession(POSIXSession):
     connection = attrib(default=attr.NOTHING)
 
     @borrowdoc(Session)
-    def _execute_command(self, command, env=None, cwd=None, handle_permission_denied=True):
+    def _execute_command(self, command, env=None, cwd=None, with_shell=False,
+                        handle_permission_denied=True):
         # TODO -- command_env is not used etc...
         # command_env = self.get_updated_env(env)
         from invoke.exceptions import UnexpectedExit
-        command = command_as_string(command)
-        if env:
-            command = ' '.join(['%s=%s' % k for k in env.items()]) \
-                      + ' ' + command
-
-        if cwd:
-            raise NotImplementedError("implement cwd support")
-
+        command = self._prefix_command(command_as_string(command), env=env,
+                                        cwd=cwd, with_shell=with_shell)
         try:
             result = self.connection.run(command, hide=True)
         except UnexpectedExit as e:
diff --git a/setup.py b/setup.py
index eab94f3..88e5416 100755
--- a/setup.py
+++ b/setup.py
@@ -157,6 +157,7 @@ setup(
             findsome(opj("distributions", "tests", "files"), {"yml", "yaml"}) +
             findsome("examples", {"trig", "yml", "yaml"}) +
             findsome(opj("formats", "tests", "files"), {"yml", "yaml"}) +
+            findsome(opj("interface"), {"shim"}) +
             findsome(opj("interface", "tests"), {"yml", "yaml"}) +
             findsome(opj("interface", "tests", "files"), {"yml", "yaml"}) +
             findsome(opj("support", "jobs", "job_templates", "runscript"),


Test patch:
diff --git a/reproman/distributions/tests/test_redhat.py b/reproman/distributions/tests/test_redhat.py
index 72a41c9..fbe4bf4 100644
--- a/reproman/distributions/tests/test_redhat.py
+++ b/reproman/distributions/tests/test_redhat.py
@@ -208,13 +208,13 @@ def test_distribution(docker_container, centos_spec):
 
     with swallow_logs(new_level=logging.DEBUG) as log:
         dist.initiate(session)
-        assert "Running command ['yum', 'repolist']" in log.lines
-        assert "Running command ['yum', 'install', '-y', 'epel-release']" in \
+        assert "Running command 'yum repolist'" in log.lines
+        assert "Running command 'yum install -y epel-release'" in \
             log.lines
 
         dist.install_packages(session)
         assert "Installing fido-1.1.2-1.el7.x86_64" in log.lines
-        assert "Running command ['yum', 'install', '-y', 'fido-1.1.2-1.el7.x86_64']" \
+        assert "Running command 'yum install -y fido-1.1.2-1.el7.x86_64'" \
             in log.lines
 
 
diff --git a/reproman/interface/tests/test_execute.py b/reproman/interface/tests/test_execute.py
index 86ab287..a1bd69a 100644
--- a/reproman/interface/tests/test_execute.py
+++ b/reproman/interface/tests/test_execute.py
@@ -14,6 +14,9 @@ from unittest.mock import patch
 import os
 import os.path as op
 import pytest
+import logging
+from shutil import copyfile
+import tempfile
 
 import attr
 
@@ -22,10 +25,13 @@ from reproman.formats.reproman import RepromanProvenance
 from reproman.utils import swallow_outputs
 from reproman.interface.execute import TracedCommand
 from ...resource.base import ResourceManager
-from ...tests.utils import assert_is_subset_recur
+from ...support.exceptions import CommandError
+from ...tests.utils import assert_is_subset_recur, assert_in, assert_in_in
 from ...tests.skip import mark
 from ...tests.fixtures import get_docker_fixture
 from ...consts import TEST_SSH_DOCKER_DIGEST
+from ...cmd import Runner
+from ...utils import swallow_logs
 
 
 docker_container = get_docker_fixture(
@@ -143,3 +149,42 @@ def test_trace_local(trace_info):
 
     expect = {"packages": [{"files": ["/bin/ls"], "name": "coreutils"}]}
     assert_is_subset_recur(expect, attr.asdict(deb_dists[0]), [dict, list])
+
+
+@mark.skipif_no_network
+@mark.skipif_no_docker_engine
+def test_docker_shim(tmpdir):
+    tmpdir = str(tmpdir)
+    shim = op.join(tmpdir, "docker")
+    copyfile(op.join(op.dirname(op.realpath(__file__)), "../docker.shim"),
+            shim)
+    os.chmod(shim, 0o755)
+    image_digest = 'debian@sha256:a94839ed73dff831b2382b087b1008877d796946cc716afd4fc72150e082d1b8'
+    command = shim + " --debug run --rm {} cat /etc/debian_version".format(
+        image_digest)
+
+    env = {
+        "PATH": "{}:{}".format(tmpdir, os.environ['PATH']),
+        "REPROMAN_TRACER_DIR": tmpdir,
+        "REPROMAN_EXTRA_TRACE_FILE": op.join(tmpdir, "trace.extra.out")
+    }
+
+    # Test with theoretically successful docker run
+    with swallow_logs(new_level=logging.DEBUG) as log:
+        Runner().run(command, env=env)
+        assert 'buster/sid' in log.out
+        assert '[DEBUG DOCKER SHIM] Found docker executable: /usr/bin/docker' in log.out
+        assert '[DEBUG DOCKER SHIM] Found digest ID: {}'.format(image_digest) in log.out
+        assert op.exists(op.join(tmpdir, env['REPROMAN_EXTRA_TRACE_FILE']))
+
+    # Test with missing ENV variables
+    with pytest.raises(CommandError):
+        Runner().run(command, env={})
+
+    # Test with bad docker command
+    command = shim + " --debug run --rm {} bad-command".format(
+        image_digest)
+    with swallow_logs(new_level=logging.DEBUG) as log:
+        with pytest.raises(CommandError):
+            Runner().run(command, env=env)
+        assert "executable file not found" in log.out
diff --git a/reproman/interface/tests/test_install.py b/reproman/interface/tests/test_install.py
index 90192a8..0a7be01 100644
--- a/reproman/interface/tests/test_install.py
+++ b/reproman/interface/tests/test_install.py
@@ -24,7 +24,7 @@ def test_install_interface(demo1_spec):
         patch('reproman.distributions.debian.DebianDistribution.install_packages'), \
         patch('reproman.resource.ResourceManager._get_inventory') as get_inventory, \
         patch('requests.get') as requests, \
-        swallow_logs(new_level=logging.DEBUG) as log:
+            swallow_logs(new_level=logging.DEBUG) as log:
 
         client.return_value = MagicMock(
             containers=lambda all: [
@@ -67,24 +67,24 @@ def test_install_interface(demo1_spec):
 
         calls = [
             call(base_url='tcp://127.0.0.1:2375'),
-            call().exec_create(cmd=['bash', '-c', 'test -e /etc/apt/sources.list.d/reproman.sources.list && echo Found'], container={'Id': '326b0fdfbf838', 'State': 'running', 'Names': ['/my-resource']}),
+            call().exec_create(cmd="bash -c 'test -e /etc/apt/sources.list.d/reproman.sources.list && echo Found'", container={'Id': '326b0fdfbf838', 'State': 'running', 'Names': ['/my-resource']}),
             call().exec_create(cmd='sh -c \'echo "# ReproMan repo sources" > /etc/apt/sources.list.d/reproman.sources.list\'', container={'Id': '326b0fdfbf838', 'State': 'running', 'Names': ['/my-resource']}),
             call().exec_create(cmd="grep -q 'deb http://snapshot.debian.org/archive/debian/20170531T084046Z/ sid main contrib non-free' /etc/apt/sources.list.d/reproman.sources.list", container={'Id': '326b0fdfbf838', 'State': 'running', 'Names': ['/my-resource']}),
             call().exec_create(cmd="grep -q 'deb http://snapshot.debian.org/archive/debian/20171208T032012Z/ sid main contrib non-free' /etc/apt/sources.list.d/reproman.sources.list", container={'Id': '326b0fdfbf838', 'State': 'running', 'Names': ['/my-resource']}),
             call().exec_create(cmd="grep -q 'deb http://snapshot-neuro.debian.net:5002/archive/neurodebian/20171208T032012Z/ xenial main contrib non-free' /etc/apt/sources.list.d/reproman.sources.list", container={'Id': '326b0fdfbf838', 'State': 'running', 'Names': ['/my-resource']}),
             call().exec_create(cmd="grep -q 'deb http://snapshot-neuro.debian.net:5002/archive/neurodebian/20171208T032012Z/ xenial main contrib non-free' /etc/apt/sources.list.d/reproman.sources.list", container={'Id': '326b0fdfbf838', 'State': 'running', 'Names': ['/my-resource']}),
-            call().exec_create(cmd=['apt-key', 'adv', '--recv-keys', '--keyserver', 'hkp://pool.sks-keyservers.net:80', '0xA5D32F012649A5A9'], container={'Id': '326b0fdfbf838', 'State': 'running', 'Names': ['/my-resource']}),
+            call().exec_create(cmd='apt-key adv --recv-keys --keyserver hkp://pool.sks-keyservers.net:80 0xA5D32F012649A5A9', container={'Id': '326b0fdfbf838', 'State': 'running', 'Names': ['/my-resource']}),
             call().exec_create(cmd="grep -q 'deb http://snapshot-neuro.debian.net:5002/archive/neurodebian/20171208T032012Z/ xenial main contrib non-free' /etc/apt/sources.list.d/reproman.sources.list", container={'Id': '326b0fdfbf838', 'State': 'running', 'Names': ['/my-resource']}),
             call().exec_create(cmd="grep -q 'deb http://snapshot-neuro.debian.net:5002/archive/neurodebian/20171208T032012Z/ xenial main contrib non-free' /etc/apt/sources.list.d/reproman.sources.list", container={'Id': '326b0fdfbf838', 'State': 'running', 'Names': ['/my-resource']}),
-            call().exec_create(cmd=['apt-key', 'adv', '--recv-keys', '--keyserver', 'hkp://pool.sks-keyservers.net:80', '0xA5D32F012649A5A9'], container={'Id': '326b0fdfbf838', 'State': 'running', 'Names': ['/my-resource']}),
-            call().exec_create(cmd=['apt-get', '-o', 'Acquire::Check-Valid-Until=false', 'update'], container={'Id': '326b0fdfbf838', 'State': 'running', 'Names': ['/my-resource']})
+            call().exec_create(cmd='apt-key adv --recv-keys --keyserver hkp://pool.sks-keyservers.net:80 0xA5D32F012649A5A9', container={'Id': '326b0fdfbf838', 'State': 'running', 'Names': ['/my-resource']}),
+            call().exec_create(cmd='apt-get -o Acquire::Check-Valid-Until=false update', container={'Id': '326b0fdfbf838', 'State': 'running', 'Names': ['/my-resource']})
         ]
         client.assert_has_calls(calls, any_order=True)
 
         assert_in('Adding Debian update to environment command list.', log.lines)
-        assert_in("Running command ['bash', '-c', 'test -e /etc/apt/sources.list.d/reproman.sources.list && echo Found']", log.lines)
+        assert_in('Running command "bash -c \'test -e /etc/apt/sources.list.d/reproman.sources.list && echo Found\'"', log.lines)
         assert_in('Running command "grep -q \'deb http://snapshot.debian.org/archive/debian/20170531T084046Z/ sid main contrib non-free\' /etc/apt/sources.list.d/reproman.sources.list"', log.lines)
         assert_in('Running command "grep -q \'deb http://snapshot.debian.org/archive/debian/20171208T032012Z/ sid main contrib non-free\' /etc/apt/sources.list.d/reproman.sources.list"', log.lines)
         assert_in('Running command "grep -q \'deb http://snapshot-neuro.debian.net:5002/archive/neurodebian/20171208T032012Z/ xenial main contrib non-free\' /etc/apt/sources.list.d/reproman.sources.list"', log.lines)
-        assert_in("Running command ['apt-key', 'adv', '--recv-keys', '--keyserver', 'hkp://pool.sks-keyservers.net:80', '0xA5D32F012649A5A9']", log.lines)
-        assert_in("Running command ['apt-get', '-o', 'Acquire::Check-Valid-Until=false', 'update']", log.lines)
+        assert_in("Running command 'apt-key adv --recv-keys --keyserver hkp://pool.sks-keyservers.net:80 0xA5D32F012649A5A9'", log.lines)
+        assert_in("Running command 'apt-get -o Acquire::Check-Valid-Until=false update'", log.lines)
diff --git a/reproman/resource/tests/test_docker_container.py b/reproman/resource/tests/test_docker_container.py
index 540d186..98df863 100644
--- a/reproman/resource/tests/test_docker_container.py
+++ b/reproman/resource/tests/test_docker_container.py
@@ -34,7 +34,7 @@ def test_dockercontainer_class():
 
     with patch('docker.Client') as client, \
         patch('dockerpty.start') as dockerpty, \
-        swallow_logs(new_level=logging.DEBUG) as log:
+            swallow_logs(new_level=logging.DEBUG) as log:
 
         client.return_value = MagicMock(
             containers=lambda all: [
@@ -61,7 +61,7 @@ def test_dockercontainer_class():
             create_container=lambda name, image, stdin_open, tty, command: {
                 'Id': '18b31b30e3a5'
             },
-            exec_inspect=lambda id: { 'ExitCode': 0 },
+            exec_inspect=lambda id: {'ExitCode': 0},
             exec_start=lambda exec_id, stream: [
                 b'stdout line 1',
                 b'stdout line 2',
@@ -131,8 +131,8 @@ def test_dockercontainer_class():
         command = ['apt-get', 'install', 'xeyes']
         resource.add_command(command)
         resource.execute_command_buffer()
-        assert_in("Running command '['apt-get', 'install', 'bc']'", log.lines)
-        assert_in("Running command '['apt-get', 'install', 'xeyes']'", log.lines)
+        assert_in("Running command 'apt-get install bc'", log.lines)
+        assert_in("Running command 'apt-get install xeyes'", log.lines)
 
         # Test starting resource.
         resource.start()
@@ -143,7 +143,7 @@ def test_dockercontainer_class():
 
         # Test logging into the container.
         with resource.get_session(pty=True):
-            pass # we do nothing really
+            pass  # we do nothing really
         assert dockerpty.called
 
         # Test stopping resource.
diff --git a/reproman/resource/tests/test_session.py b/reproman/resource/tests/test_session.py
index 1a747bf..784d1ab 100644
--- a/reproman/resource/tests/test_session.py
+++ b/reproman/resource/tests/test_session.py
@@ -94,6 +94,12 @@ def test_get_updated_env():
     assert get_updated_env({'a': 1}, {'a': None}) == {}
     assert get_updated_env({'a': 1, 'b': 2}, {'a': None}) == {'b': 2}
     assert get_updated_env({'a': 1, 'b': 2}, {'a': None, 'b': 3}) == {'b': 3}
+    assert get_updated_env({'a': '/foo', 'b': 2}, {'a': '/bar:$a', 'b': 3}) \
+        == {'a': '/bar:/foo', 'b': 3}
+    assert get_updated_env({'a': '/foo', 'b': 2}, {'a': '/bar:$ab', 'b': 3}) \
+        == {'a': '/bar:$ab', 'b': 3}
+    assert get_updated_env({'a': '/foo', 'b': 2}, {'a': '/bar:${a}:/blee', 'b': 3}) \
+        == {'a': '/bar:/foo:/blee', 'b': 3}
 
 
 def test_get_local_session():
@@ -226,22 +232,14 @@ def check_methods(resource_test_dir):
             session._execute_command(['cat', '/no/such/file'])
 
         # Check _execute_command with env set
-        # TODO: Implement env parameter for _execute_command()
-        if cls_name not in ["ShellSession", "SSHSession", "PTYSSHSession"]:
-            with pytest.raises(NotImplementedError):
-                out, err = session._execute_command(['cat', '/etc/hosts'],
-                    env={'NEW_VAR': 'NEW_VAR_VALUE'})
+        out, err = session._execute_command(['env'],
+            env={'NEW_VAR': 'NEW_VAR_VALUE'})
+        assert 'NEW_VAR=NEW_VAR_VALUE' in out
 
         # Check _execute_command with cwd set
-        # TODO: Implement cwd parameter for _execute_command()
-        if "Docker"in cls_name:
-            with swallow_logs(new_level=logging.WARN) as log:
-                session._execute_command(['cat', '/etc/hosts'], cwd='/tmp')
-                assert "cwd is not handled in docker yet" in log.out
-        elif cls_name != 'ShellSession':
-            with pytest.raises(NotImplementedError):
-                out, err = session._execute_command(['cat', '/etc/hosts'],
-                    cwd='/tmp')
+        out, err = session._execute_command(['pwd'],
+            cwd='/var')
+        assert '/var' == out.rstrip("\n")
 
         # Check exists() method
         result = session.exists('/etc')
diff --git a/reproman/resource/tests/test_ssh.py b/reproman/resource/tests/test_ssh.py
index 79b2b22..007c442 100644
--- a/reproman/resource/tests/test_ssh.py
+++ b/reproman/resource/tests/test_ssh.py
@@ -15,6 +15,7 @@ import re
 import uuid
 from pytest import raises
 
+from ...support.exceptions import CommandError
 from ...utils import merge_dicts
 from ...utils import swallow_logs
 from ...tests.utils import assert_in
@@ -111,7 +112,7 @@ def test_ssh_class(setup_ssh, resource_test_dir):
         assert not session.isdir('not-a-dir')
         assert not session.isdir('/etc/hosts')
 
-        with raises(NotImplementedError):
+        with raises(CommandError):
             session._execute_command('non-existent-command', cwd='/path')