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

DockerRunner: Enable background container to run commands against #51

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
98 changes: 71 additions & 27 deletions tests/integration/framework/docker_runner.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,49 +8,93 @@
import sys

class DockerRunner:
"""Run docker containers for testing"""
"""Run a docker container in background to execute testing commands"""

def __init__(self, platform, image, verbose):
"""Sets platform and image for all tests ran with this instance"""
self.platform = platform
self.image = image
self.verbose = verbose
self.container_id = None

def construct_docker_command(self, envs, args):
# Launch the container
self._start()

def execute(self, envs, args):
"""Launch `docker exec` commands inside the background container"""
command = self._construct_command("exec", envs, args)
return self._shell(command)

def run(self, envs, args):
"""
Launch `docker run` commands to create a new container for each command
"""
command = self._construct_command("run", envs, args)
return self._shell(command)

def __del__(self):
"""Remove test container"""
stop_command = ["docker", "rm", "-f", self.container_id]
Copy link
Member

Choose a reason for hiding this comment

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

If we want to retain the container (to investigate because an error happened) it's useful to have a 2-step process: docker stop the container, and then docker rm the container. We can then choose to not rm on test failure, allowing a docker start and perhaps exec into a shell to investigate.

Can we split this up into 2 commands that are called independent from the destructor?

Copy link
Contributor Author

@AbcSxyZ AbcSxyZ Dec 19, 2021

Choose a reason for hiding this comment

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

With our 2 type of command, exec & run:

  • run commands outputs (stdout + stderr) are direclty handled. Stdout is returned, stderr displayed when error occurs (see DockerRunner._shell). Logs are already caught.
  • exec commands do not add any logs, they seem to handle only main process outputs.

We can do this, but logs are already displayed, not sure where we would use docker log.

self._shell(stop_command, silent=True)

def _start(self):
Copy link
Member

Choose a reason for hiding this comment

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

Why protected?

Copy link
Contributor Author

@AbcSxyZ AbcSxyZ Dec 19, 2021

Choose a reason for hiding this comment

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

Just a habit because it wasn't used by outside. It was designed with the idea that DockerRunner will always start a container for a test. If the design change and we do not use it necessary as you suggest, it needs to change.

Copy link
Member

Choose a reason for hiding this comment

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

I'm okay with it when it's designed to not be ran from outside ever because <reason>, but I do not really agree with the reasoning.

It was designed with the idea that DockerRunner will always start a container for a test.

That's a design change of DockerRunner introduced with this PR, my comment is based on original design that is more permissive. The reason why I didn't design it like that with the initial build of the framework is because I can think of tests where we want to just docker run a single command and check the output, to check the exact, automated behavior of that (as was done with the version test), or, to run multiple docker containers to test an end-to-end use case, e.g. doing development on regtest, where there are scenarios where you need at least 2 nodes that connect to each other, for example when you want to work with getblocktemplate or auxpow RPC interfaces.

"""
Construct a docker command with env and args
Launch a docker container. Done in 2 step using create + start.

Keep the container running with a shell open thanks to `--interactive`,
having stdin open keep the process running.
AbcSxyZ marked this conversation as resolved.
Show resolved Hide resolved
"""
command = ["docker", "run", "--platform", self.platform]
create_command = [
"docker", "create",
"--platform", self.platform,
"--interactive",
self.image,
"/bin/bash",
]
self.container_id = self._shell(create_command, silent=True)

start_command = [
"docker", "start", self.container_id
]
self._shell(start_command, silent=True)

def _shell(self, command, silent=False):
"""Run an arbitrary shell command and return its output"""
if self.verbose and not silent:
print(f"$ { ' '.join(command) }")

try:
result = subprocess.run(command, capture_output=True, check=True)
except subprocess.CalledProcessError as command_err:
print(command_err.stdout.decode("utf-8"), file=sys.stdout)
print(command_err.stderr.decode("utf-8"), file=sys.stderr)
raise command_err

return result.stdout.decode("utf-8").strip()

def _construct_command(self, docker_cmd, envs, args):
"""Construct a docker command with env and args"""
command = ["docker", docker_cmd]

for env in envs:
command.append("-e")
command.append(env)

command.append(self.image)

for arg in args:
command.append(arg)

return command
# Use container or image depending on command type
if docker_cmd == "exec":
tag = self.container_id
elif docker_cmd == "run":
tag = self.image

def run_interactive_command(self, envs, args):
Copy link
Member

Choose a reason for hiding this comment

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

could you please bring this method back so that the tests don't have to change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Related to #51 (comment)

"""
Run our target docker image with a list of
environment variables and a list of arguments
"""
command = self.construct_docker_command(envs, args)
command.append(tag)

if self.verbose:
print(f"Running command: { ' '.join(command) }")
# Launch all shell commands using entrypoint.py only
command.append("entrypoint.py")

try:
output = subprocess.run(command, capture_output=True, check=True)
except subprocess.CalledProcessError as docker_err:
print(f"Error while running command: { ' '.join(command) }", file=sys.stderr)
print(docker_err, file=sys.stderr)
print(docker_err.stderr.decode("utf-8"), file=sys.stderr)
print(docker_err.stdout.decode("utf-8"), file=sys.stdout)
# Need to add a default executables added by CMD normally
if len(args) == 0 or args[0].startswith("-"):
command.append("dogecoind")

raise docker_err
command.extend(args)

return output
return command
22 changes: 16 additions & 6 deletions tests/integration/framework/test_runner.py
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,8 @@ class TestConfigurationError(Exception):
class TestRunner:
"""Base class to define and run Dogecoin Core Docker tests with"""
def __init__(self):
"""Make sure there is an options object"""
self.options = {}
self.container = None

def add_options(self, parser):
"""Allow adding options in tests"""
Expand All @@ -25,15 +25,22 @@ def run_test(self):
"""Actual test, must be implemented by the final class"""
raise NotImplementedError

def run_command(self, envs, args):
Copy link
Member

Choose a reason for hiding this comment

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

Please restore this so that version test doesn't have to change

"""Run a docker command with env and args"""
def docker_exec(self, envs, args):
"""
Launch `docker exec` command, run command inside a background container.
Let execute mutliple instructions in the same container.
"""
assert self.options.platform is not None
assert self.options.image is not None

runner = DockerRunner(self.options.platform,
self.options.image, self.options.verbose)
return self.container.execute(envs, args)

return runner.run_interactive_command(envs, args)
def docker_run(self, envs, args):
"""Launch `docker run` command, create a new container for each run"""
assert self.options.platform is not None
assert self.options.image is not None

return self.container.run(envs, args)

def main(self):
"""main loop"""
Expand All @@ -48,6 +55,9 @@ def main(self):
self.add_options(parser)
self.options = parser.parse_args()

self.container = DockerRunner(self.options.platform,
self.options.image, self.options.verbose)

Copy link
Member

Choose a reason for hiding this comment

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

this forces all tests to run/exec a container. should leave it up to the individual tests in their run_test method, by putting this in a separate method that can be overridden by child classes and then call that method here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fine

Copy link
Contributor Author

@AbcSxyZ AbcSxyZ Dec 19, 2021

Choose a reason for hiding this comment

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

I changed it, the line is still (almost) the same, but it does not start automatically a background container.

It will start automatically on the first docker exec command, making background containers optional without the need to start the container manually when writing tests.

Copy link
Member

Choose a reason for hiding this comment

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

what if I want to change the way the container starts? i.e. test for permission escalation?

Copy link
Contributor Author

@AbcSxyZ AbcSxyZ Dec 20, 2021

Choose a reason for hiding this comment

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

Any example ? No idea how to perform this kind of test.

But you have the docker run interface as normally, and even if using entrypoint through docker exec any commands will run as root (outside of Dogecoin executables), as entrypoint is designed to accept arbitrary bash command.

self.run_test()
print("Tests successful")
sys.exit(0)
16 changes: 8 additions & 8 deletions tests/integration/version.py
Original file line number Diff line number Diff line change
Expand Up @@ -27,21 +27,21 @@ def run_test(self):
self.version_expr = re.compile(f".*{ self.options.version }.*")

# check dogecoind with only env
dogecoind = self.run_command(["VERSION=1"], [])
Copy link
Member

Choose a reason for hiding this comment

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

Please revert all changes to this file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Using self.docker_run would be fine for you ?
Also, methods TestRunner.docker_run and TestRunner.docker_exec return directly the output to avoid the use of .stdout access & encoding/decoding stuff. Error messages are directly handled on command execution (DockerRunner._shell) and create test failure, not used in tests (at least for now). You would like to restore this also ?

self.ensure_version_on_first_line(dogecoind.stdout)
dogecoind = self.docker_exec(["VERSION=1"], [])
self.ensure_version_on_first_line(dogecoind)

# check dogecoin-cli
dogecoincli = self.run_command([], ["dogecoin-cli", "-?"])
self.ensure_version_on_first_line(dogecoincli.stdout)
dogecoincli = self.docker_exec([], ["dogecoin-cli", "-?"])
self.ensure_version_on_first_line(dogecoincli)

# check dogecoin-tx
dogecointx = self.run_command([], ["dogecoin-tx", "-?"])
self.ensure_version_on_first_line(dogecointx.stdout)
dogecointx = self.docker_exec([], ["dogecoin-tx", "-?"])
self.ensure_version_on_first_line(dogecointx)

# make sure that we find version errors
caught_error = False
try:
self.ensure_version_on_first_line("no version here".encode('utf-8'))
self.ensure_version_on_first_line("no version here")
except AssertionError:
caught_error = True

Expand All @@ -50,7 +50,7 @@ def run_test(self):

def ensure_version_on_first_line(self, cmd_output):
"""Assert that the version is contained in the first line of output string"""
first_line = cmd_output.decode("utf-8").split("\n")[0]
first_line = cmd_output.split("\n")[0]

if re.match(self.version_expr, first_line) is None:
text = f"Could not find version { self.options.version } in { first_line }"
Expand Down