DockerRunner: Enable background container to run commands against #51

AbcSxyZ · 2021-12-17T14:31:04Z

Following a comment where @patricklodder was speaking about having a mechanism to start/stop a container for the entire test.

Implemented it in the idea to add some tests who can need it, at least datadir test from #50.

It changes the way commands are proceeded : A single background container is started for the entire test of a TestRunner subclass, and all commands for the test are executed with docker exec (calling the entrypoint directly).

Run CI tests

Will need some documentation to facilitate usage, but for now it's possible to run it using:

python3 -m tests.integration_runner --platform amd64 --image [dogecoin-image] --version 1.14.5 [--verbose]

patricklodder · 2021-12-18T14:26:05Z

Is it possible to retain the single-shot run method so that we can separate framework updates from test updates?

AbcSxyZ · 2021-12-18T14:27:49Z

Sure

AbcSxyZ · 2021-12-18T19:40:42Z

I restored run commands. I used exec everywhere for now, where would you use run ? What kind of difference are you thinking of ?

patricklodder · 2021-12-18T23:05:11Z

What kind of difference are you thinking of ?

Currently, there is a difference between exec and run: run uses entry point which de-escalates the uid, exec does not have an entry point and we don't USER right now, so everything there is root. This yields different results so we need to retain both until we have a better security solution.

Additionally, you're changing the version test right now, that's not needed with this enhancement.

AbcSxyZ · 2021-12-18T23:14:26Z

Exec commands are also using entrypoint. Using verbose to show tests commands:

$ docker exec -e VERSION=1 c5a168d2a634dde84 entrypoint.py dogecoind
$ docker exec c5a168d2a634dde84 entrypoint.py dogecoin-cli -?
$ docker exec c5a168d2a634dde84 entrypoint.py dogecoin-tx -?

Actually, I also add the entrypoint manually when using docker run, it's done in _construct_command:

docker/tests/integration/framework/docker_runner.py

Line 75 in 84b4f91

def _construct_command(self, docker_cmd, envs, args):

EDIT: My runs commands sucks

With my fix, docker run launch the following commands:

$ docker run -e VERSION=1 dogecoin entrypoint.py dogecoind                                                                                                                      
$ docker run dogecoin entrypoint.py dogecoin-cli -?                                                                                                                             
$ docker run dogecoin entrypoint.py dogecoin-tx -?

patricklodder · 2021-12-18T23:25:48Z

Exec commands are also using entrypoint. Using verbose to show tests commands:

That's not what we'd recommend a user to do though, right? I'll later on comment on that when I review the code.

AbcSxyZ · 2021-12-18T23:30:14Z

Yes, it builds manually the command created by ENTRYPOINT + CMD.

patricklodder · 2021-12-18T23:41:30Z

tests/integration/framework/docker_runner.py

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


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?

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.

patricklodder · 2021-12-18T23:44:27Z

tests/integration/framework/docker_runner.py

+        stop_command = ["docker", "rm", "-f", self.container_id]
+        self._shell(stop_command, silent=True)
+
+    def _start(self):


Why protected?

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.

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.

tests/integration/framework/docker_runner.py

patricklodder · 2021-12-18T23:47:44Z

tests/integration/framework/docker_runner.py


-    def run_interactive_command(self, envs, args):


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

Related to #51 (comment)

patricklodder · 2021-12-18T23:51:32Z

tests/integration/framework/test_runner.py

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


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.

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.

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

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.

patricklodder · 2021-12-18T23:53:40Z

tests/integration/framework/test_runner.py

@@ -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):


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

patricklodder · 2021-12-18T23:54:41Z

tests/integration/version.py

@@ -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"], [])


Please revert all changes to this file.

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 ?

AbcSxyZ · 2021-12-19T18:23:47Z

I did some fixes to reproduce real docker run commands. For versions tests with each command launched with both run and exec, it generates the following:

version
----------------------
$ docker run -e VERSION=1 --platform amd64 dogecoin
$ docker exec -e VERSION=1 acc889cd4db4eba2 entrypoint.py dogecoind
$ docker run --platform amd64 dogecoin dogecoin-cli -?
$ docker exec acc889cd4db4eba2 entrypoint.py dogecoin-cli -?
$ docker run --platform amd64 dogecoin dogecoin-tx -?
$ docker exec acc889cd4db4eba2 entrypoint.py dogecoin-tx -?

AbcSxyZ closed this Dec 18, 2021

AbcSxyZ reopened this Dec 18, 2021

AbcSxyZ marked this pull request as draft December 18, 2021 14:27

patricklodder added the qa Such quality label Dec 18, 2021

AbcSxyZ marked this pull request as ready for review December 18, 2021 19:24

AbcSxyZ added 2 commits December 18, 2021 20:35

DockerRunner: Enable background container to run commands against

02a9074

Restore run commands to DockerRunner

84b4f91

AbcSxyZ marked this pull request as draft December 18, 2021 23:16

test: Fix docker run commands for DockerRunner

eba0eb9

AbcSxyZ marked this pull request as ready for review December 18, 2021 23:25

patricklodder requested changes Dec 18, 2021

View reviewed changes

AbcSxyZ added 6 commits December 19, 2021 17:34

test: update background container to be optional

b8d2171

update version tests to docker_run

9b3ac80

Use sleep infinity to start background container

eb89b6c

improve doc

62a40d5

fix platform for docker run + change run commands to match real behavior

775eec6

reduce hash size of container_id

407dbac

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DockerRunner: Enable background container to run commands against #51

DockerRunner: Enable background container to run commands against #51

AbcSxyZ commented Dec 17, 2021 •

edited

Loading

patricklodder commented Dec 18, 2021

AbcSxyZ commented Dec 18, 2021

AbcSxyZ commented Dec 18, 2021

patricklodder commented Dec 18, 2021

AbcSxyZ commented Dec 18, 2021 •

edited

Loading

patricklodder commented Dec 18, 2021

AbcSxyZ commented Dec 18, 2021 •

edited

Loading

patricklodder Dec 18, 2021

AbcSxyZ Dec 19, 2021 •

edited

Loading

patricklodder Dec 18, 2021

AbcSxyZ Dec 19, 2021 •

edited

Loading

patricklodder Dec 19, 2021

patricklodder Dec 18, 2021

AbcSxyZ Dec 19, 2021

patricklodder Dec 18, 2021

AbcSxyZ Dec 19, 2021

AbcSxyZ Dec 19, 2021 •

edited

Loading

patricklodder Dec 20, 2021

AbcSxyZ Dec 20, 2021 •

edited

Loading

patricklodder Dec 18, 2021

patricklodder Dec 18, 2021

AbcSxyZ Dec 19, 2021

AbcSxyZ commented Dec 19, 2021 •

edited

Loading

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

DockerRunner: Enable background container to run commands against #51

Are you sure you want to change the base?

DockerRunner: Enable background container to run commands against #51

Conversation

AbcSxyZ commented Dec 17, 2021 • edited Loading

Run CI tests

patricklodder commented Dec 18, 2021

AbcSxyZ commented Dec 18, 2021

AbcSxyZ commented Dec 18, 2021

patricklodder commented Dec 18, 2021

AbcSxyZ commented Dec 18, 2021 • edited Loading

patricklodder commented Dec 18, 2021

AbcSxyZ commented Dec 18, 2021 • edited Loading

Choose a reason for hiding this comment

AbcSxyZ Dec 19, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AbcSxyZ Dec 19, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AbcSxyZ Dec 19, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AbcSxyZ Dec 20, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AbcSxyZ commented Dec 19, 2021 • edited Loading

AbcSxyZ commented Dec 17, 2021 •

edited

Loading

AbcSxyZ commented Dec 18, 2021 •

edited

Loading

AbcSxyZ commented Dec 18, 2021 •

edited

Loading

AbcSxyZ Dec 19, 2021 •

edited

Loading

AbcSxyZ Dec 19, 2021 •

edited

Loading

AbcSxyZ Dec 19, 2021 •

edited

Loading

AbcSxyZ Dec 20, 2021 •

edited

Loading

AbcSxyZ commented Dec 19, 2021 •

edited

Loading