Refactor TrackedContainer run_detached/exec_cmd functions (#2256)

* Refactor TrackedContainer run_detached/exec_cmd functions

* Add get_logs() method

* Small fixes

* Make get_health() a method

* Remove kwargs, always print output

* Small fixes

docs/maintaining/tagging_examples/docker_runner.py

@@ -3,4 +3,4 @@
 from tagging.utils.docker_runner import DockerRunner
 
 with DockerRunner("ubuntu") as container:
-    DockerRunner.exec_cmd(container, cmd="env", print_output=True)
+    DockerRunner.exec_cmd(container, cmd="env")

tagging/taggers/versions.py

@@ -15,7 +15,6 @@ def _get_pip_package_version(container: Container, package: str) -> str:
     package_info = DockerRunner.exec_cmd(
         container,
         cmd=f"pip show {package}",
-        print_output=False,
     )
     version_line = package_info.split("\n")[1]
     assert version_line.startswith(PIP_VERSION_PREFIX)

tagging/utils/docker_runner.py

@@ -43,12 +43,11 @@ class DockerRunner:
         LOGGER.info(f"Container {self.container.name} removed")
 
     @staticmethod
-    def exec_cmd(container: Container, cmd: str, print_output: bool = True) -> str:
+    def exec_cmd(container: Container, cmd: str) -> str:
         LOGGER.info(f"Running cmd: `{cmd}` on container: {container.name}")
         exec_result = container.exec_run(cmd)
         output = exec_result.output.decode().rstrip()
         assert isinstance(output, str)
-        if print_output:
         LOGGER.info(f"Command output: {output}")
         assert exec_result.exit_code == 0, f"Command: `{cmd}` failed"
         return output

tagging/utils/quoted_output.py

@@ -8,7 +8,7 @@ from tagging.utils.docker_runner import DockerRunner
 
 
 def quoted_output(container: Container, cmd: str) -> str:
-    cmd_output = DockerRunner.exec_cmd(container, cmd, print_output=False)
+    cmd_output = DockerRunner.exec_cmd(container, cmd)
     # For example, `mamba info` adds redundant empty lines
     cmd_output = cmd_output.strip("\n")
     # For example, R packages list contains trailing backspaces

tests/by_image/base-notebook/test_container_options.py

@@ -15,13 +15,13 @@ LOGGER = logging.getLogger(__name__)
 def test_cli_args(container: TrackedContainer, http_client: requests.Session) -> None:
     """Image should respect command line args (e.g., disabling token security)"""
     host_port = find_free_port()
-    running_container = container.run_detached(
+    container.run_detached(
         command=["start-notebook.py", "--IdentityProvider.token=''"],
         ports={"8888/tcp": host_port},
     )
     resp = http_client.get(f"http://localhost:{host_port}")
     resp.raise_for_status()
-    logs = running_container.logs().decode()
+    logs = container.get_logs()
     LOGGER.debug(logs)
     assert "ERROR" not in logs
     warnings = TrackedContainer.get_warnings(logs)
@@ -32,7 +32,7 @@ def test_cli_args(container: TrackedContainer, http_client: requests.Session) ->
 def test_nb_user_change(container: TrackedContainer) -> None:
     """Container should change the username (`NB_USER`) of the default user."""
     nb_user = "nayvoj"
-    running_container = container.run_detached(
+    container.run_detached(
         tty=True,
         user="root",
         environment=[f"NB_USER={nb_user}", "CHOWN_HOME=yes"],
@@ -47,8 +47,7 @@ def test_nb_user_change(container: TrackedContainer) -> None:
     )
     command = f'stat -c "%F %U %G" /home/{nb_user}/.jupyter'
     expected_output = f"directory {nb_user} users"
-    exec_result = running_container.exec_run(command, workdir=f"/home/{nb_user}")
+    output = container.exec_cmd(command, workdir=f"/home/{nb_user}")
-    output = exec_result.output.decode().strip("\n")
     assert (
         output == expected_output
     ), f"Hidden folder .jupyter was not copied properly to {nb_user} home folder. stat: {output}, expected {expected_output}"
@@ -62,7 +61,7 @@ def test_unsigned_ssl(
     and Jupyter Server should use it to enable HTTPS.
     """
     host_port = find_free_port()
-    running_container = container.run_detached(
+    container.run_detached(
         environment=["GEN_CERT=yes"],
         ports={"8888/tcp": host_port},
     )
@@ -74,7 +73,7 @@ def test_unsigned_ssl(
     resp = http_client.get(f"https://localhost:{host_port}", verify=False)
     resp.raise_for_status()
     assert "login_submit" in resp.text
-    logs = running_container.logs().decode()
+    logs = container.get_logs()
     assert "ERROR" not in logs
     warnings = TrackedContainer.get_warnings(logs)
     assert not warnings
@@ -102,14 +101,14 @@ def test_custom_internal_port(
     when using custom internal port"""
     host_port = find_free_port()
     internal_port = env.get("JUPYTER_PORT", 8888)
-    running_container = container.run_detached(
+    container.run_detached(
         command=["start-notebook.py", "--IdentityProvider.token=''"],
         environment=env,
         ports={internal_port: host_port},
     )
     resp = http_client.get(f"http://localhost:{host_port}")
     resp.raise_for_status()
-    logs = running_container.logs().decode()
+    logs = container.get_logs()
     LOGGER.debug(logs)
     assert "ERROR" not in logs
     warnings = TrackedContainer.get_warnings(logs)

tests/by_image/base-notebook/test_healthcheck.py

@@ -3,10 +3,8 @@
 import logging
 import time
 
-import docker
 import pytest  # type: ignore
 
-from tests.utils.get_container_health import get_health
 from tests.utils.tracked_container import TrackedContainer
 
 LOGGER = logging.getLogger(__name__)
@@ -14,12 +12,11 @@ LOGGER = logging.getLogger(__name__)
 
 def get_healthy_status(
     container: TrackedContainer,
-    docker_client: docker.DockerClient,
     env: list[str] | None,
     cmd: list[str] | None,
     user: str | None,
 ) -> str:
-    running_container = container.run_detached(
+    container.run_detached(
         tty=True,
         environment=env,
         command=cmd,
@@ -32,11 +29,11 @@ def get_healthy_status(
     while time.time() < finish_time:
         time.sleep(sleep_time)
 
-        status = get_health(running_container, docker_client)
+        status = container.get_health()
         if status == "healthy":
             return status
 
-    return get_health(running_container, docker_client)
+    return status
 
 
 @pytest.mark.parametrize(
@@ -84,12 +81,11 @@ def get_healthy_status(
 )
 def test_healthy(
     container: TrackedContainer,
-    docker_client: docker.DockerClient,
     env: list[str] | None,
     cmd: list[str] | None,
     user: str | None,
 ) -> None:
-    assert get_healthy_status(container, docker_client, env, cmd, user) == "healthy"
+    assert get_healthy_status(container, env, cmd, user) == "healthy"
 
 
 @pytest.mark.parametrize(
@@ -118,12 +114,11 @@ def test_healthy(
 )
 def test_healthy_with_proxy(
     container: TrackedContainer,
-    docker_client: docker.DockerClient,
     env: list[str] | None,
     cmd: list[str] | None,
     user: str | None,
 ) -> None:
-    assert get_healthy_status(container, docker_client, env, cmd, user) == "healthy"
+    assert get_healthy_status(container, env, cmd, user) == "healthy"
 
 
 @pytest.mark.parametrize(
@@ -142,10 +137,9 @@ def test_healthy_with_proxy(
 )
 def test_not_healthy(
     container: TrackedContainer,
-    docker_client: docker.DockerClient,
     env: list[str] | None,
     cmd: list[str] | None,
 ) -> None:
     assert (
-        get_healthy_status(container, docker_client, env, cmd, user=None) != "healthy"
+        get_healthy_status(container, env, cmd, user=None) != "healthy"
     ), "Container should not be healthy for this testcase"

tests/by_image/base-notebook/test_ips.py

@@ -38,13 +38,9 @@ def test_ipv46(container: TrackedContainer, ipv6_network: str) -> None:
     host_data_dir = THIS_DIR / "data"
     cont_data_dir = "/home/jovyan/data"
     LOGGER.info("Testing that server is listening on IPv4 and IPv6 ...")
-    running_container = container.run_detached(
+    container.run_detached(
         network=ipv6_network,
         volumes={str(host_data_dir): {"bind": cont_data_dir, "mode": "ro,z"}},
         tty=True,
     )
-
+    container.exec_cmd(f"python {cont_data_dir}/check_listening.py")
-    command = ["python", f"{cont_data_dir}/check_listening.py"]
-    exec_result = running_container.exec_run(command)
-    LOGGER.info(exec_result.output.decode())
-    assert exec_result.exit_code == 0

tests/by_image/base-notebook/test_start_container.py

@@ -42,14 +42,14 @@ def test_start_notebook(
         f"Test that the start-notebook.py launches the {expected_command} server from the env {env} ..."
     )
     host_port = find_free_port()
-    running_container = container.run_detached(
+    container.run_detached(
         tty=True,
         environment=env,
         ports={"8888/tcp": host_port},
     )
     # sleeping some time to let the server start
     time.sleep(2)
-    logs = running_container.logs().decode()
+    logs = container.get_logs()
     LOGGER.debug(logs)
     # checking that the expected command is launched
     assert (
@@ -76,10 +76,9 @@ def test_tini_entrypoint(
     https://superuser.com/questions/632979/if-i-know-the-pid-number-of-a-process-how-can-i-get-its-name
     """
     LOGGER.info(f"Test that {command} is launched as PID {pid} ...")
-    running_container = container.run_detached(tty=True)
+    container.run_detached(tty=True)
     # Select the PID 1 and get the corresponding command
-    exec_result = running_container.exec_run(f"ps -p {pid} -o comm=")
+    output = container.exec_cmd(f"ps -p {pid} -o comm=")
-    output = exec_result.output.decode().strip("\n")
     assert "ERROR" not in output
     assert "WARNING" not in output
     assert output == command, f"{command} shall be launched as pid {pid}, got {output}"

tests/by_image/docker-stacks-foundation/test_packages.py

@@ -109,25 +109,16 @@ def get_package_import_name(package: str) -> str:
     return PACKAGE_MAPPING.get(package, package)
 
 
-def _check_import_package(
-    package_helper: CondaPackageHelper, command: list[str]
-) -> None:
-    """Generic function executing a command"""
-    LOGGER.debug(f"Trying to import a package with [{command}] ...")
-    exec_result = package_helper.running_container.exec_run(command)
-    assert exec_result.exit_code == 0, exec_result.output.decode()
-
-
 def check_import_python_package(
     package_helper: CondaPackageHelper, package: str
 ) -> None:
     """Try to import a Python package from the command line"""
-    _check_import_package(package_helper, ["python", "-c", f"import {package}"])
+    package_helper.container.exec_cmd(f'python -c "import {package}"')
 
 
 def check_import_r_package(package_helper: CondaPackageHelper, package: str) -> None:
     """Try to import an R package from the command line"""
-    _check_import_package(package_helper, ["R", "--slave", "-e", f"library({package})"])
+    package_helper.container.exec_cmd(f"R --slave -e library({package})")
 
 
 def _check_import_packages(

tests/by_image/docker-stacks-foundation/test_user_options.py

@@ -39,7 +39,7 @@ def test_gid_change(container: TrackedContainer) -> None:
 def test_nb_user_change(container: TrackedContainer) -> None:
     """Container should change the username (`NB_USER`) of the default user."""
     nb_user = "nayvoj"
-    running_container = container.run_detached(
+    container.run_detached(
         tty=True,
         user="root",
         environment=[f"NB_USER={nb_user}", "CHOWN_HOME=yes"],
@@ -50,7 +50,7 @@ def test_nb_user_change(container: TrackedContainer) -> None:
     # Use sleep, not wait, because the container sleeps forever.
     time.sleep(1)
     LOGGER.info(f"Checking if the user is changed to {nb_user} by the start script ...")
-    output = running_container.logs().decode()
+    output = container.get_logs()
     assert "ERROR" not in output
     assert "WARNING" not in output
     assert (
@@ -60,17 +60,13 @@ def test_nb_user_change(container: TrackedContainer) -> None:
     LOGGER.info(f"Checking {nb_user} id ...")
     command = "id"
     expected_output = f"uid=1000({nb_user}) gid=100(users) groups=100(users)"
-    exec_result = running_container.exec_run(
+    output = container.exec_cmd(command, user=nb_user, workdir=f"/home/{nb_user}")
-        command, user=nb_user, workdir=f"/home/{nb_user}"
-    )
-    output = exec_result.output.decode().strip("\n")
     assert output == expected_output, f"Bad user {output}, expected {expected_output}"
 
     LOGGER.info(f"Checking if {nb_user} owns his home folder ...")
     command = f'stat -c "%U %G" /home/{nb_user}/'
     expected_output = f"{nb_user} users"
-    exec_result = running_container.exec_run(command, workdir=f"/home/{nb_user}")
+    output = container.exec_cmd(command, workdir=f"/home/{nb_user}")
-    output = exec_result.output.decode().strip("\n")
     assert (
         output == expected_output
     ), f"Bad owner for the {nb_user} home folder {output}, expected {expected_output}"
@@ -80,8 +76,7 @@ def test_nb_user_change(container: TrackedContainer) -> None:
     )
     command = f'stat -c "%F %U %G" /home/{nb_user}/work'
     expected_output = f"directory {nb_user} users"
-    exec_result = running_container.exec_run(command, workdir=f"/home/{nb_user}")
+    output = container.exec_cmd(command, workdir=f"/home/{nb_user}")
-    output = exec_result.output.decode().strip("\n")
     assert (
         output == expected_output
     ), f"Folder work was not copied properly to {nb_user} home folder. stat: {output}, expected {expected_output}"

tests/by_image/scipy-notebook/test_matplotlib.py

@@ -38,18 +38,16 @@ def test_matplotlib(
     cont_data_dir = "/home/jovyan/data"
     output_dir = "/tmp"
     LOGGER.info(description)
-    running_container = container.run_detached(
+    container.run_detached(
         volumes={str(host_data_dir): {"bind": cont_data_dir, "mode": "ro"}},
         tty=True,
         command=["bash", "-c", "sleep infinity"],
     )
+
     command = f"python {cont_data_dir}/{test_file}"
-    exec_result = running_container.exec_run(command)
+    container.exec_cmd(command)
-    LOGGER.debug(exec_result.output.decode())
+
-    assert exec_result.exit_code == 0, f"Command {command} failed"
     # Checking if the file is generated
     # https://stackoverflow.com/a/15895594/4413446
     command = f"test -s {output_dir}/{expected_file}"
-    exec_result = running_container.exec_run(command)
+    container.exec_cmd(command)
-    LOGGER.debug(exec_result.output.decode())
-    assert exec_result.exit_code == 0, f"Command {command} failed"

tests/conftest.py

@@ -51,7 +51,6 @@ def container(
     container = TrackedContainer(
         docker_client,
         image_name,
-        detach=True,
     )
     yield container
     container.remove()

tests/utils/conda_package_helper.py

@@ -29,7 +29,6 @@ from collections import defaultdict
 from itertools import chain
 from typing import Any
 
-from docker.models.containers import Container
 from tabulate import tabulate
 
 from tests.utils.tracked_container import TrackedContainer
@@ -41,58 +40,37 @@ class CondaPackageHelper:
     """Conda package helper permitting to get information about packages"""
 
     def __init__(self, container: TrackedContainer):
-        self.running_container: Container = CondaPackageHelper.start_container(
+        self.container = container
-            container
+
+        LOGGER.info(f"Starting container {self.container.image_name} ...")
+        self.container.run_detached(
+            tty=True,
+            command=["bash", "-c", "sleep infinity"],
         )
+
         self.requested: dict[str, set[str]] | None = None
         self.installed: dict[str, set[str]] | None = None
         self.available: dict[str, set[str]] | None = None
         self.comparison: list[dict[str, str]] = []
 
-    @staticmethod
-    def start_container(container: TrackedContainer) -> Container:
-        """Start the TrackedContainer and return an instance of a running container"""
-        LOGGER.info(f"Starting container {container.image_name} ...")
-        return container.run_detached(
-            tty=True,
-            command=["bash", "-c", "sleep infinity"],
-        )
-
-    @staticmethod
-    def _conda_export_command(from_history: bool) -> list[str]:
-        """Return the mamba export command with or without history"""
-        cmd = ["mamba", "env", "export", "--no-build", "--json"]
-        if from_history:
-            cmd.append("--from-history")
-        return cmd
-
     def installed_packages(self) -> dict[str, set[str]]:
         """Return the installed packages"""
         if self.installed is None:
             LOGGER.info("Grabbing the list of installed packages ...")
-            self.installed = CondaPackageHelper._parse_package_versions(
+            env_export = self.container.exec_cmd("mamba env export --no-build --json")
-                self._execute_command(
+            self.installed = CondaPackageHelper._parse_package_versions(env_export)
-                    CondaPackageHelper._conda_export_command(from_history=False)
-                )
-            )
         return self.installed
 
     def requested_packages(self) -> dict[str, set[str]]:
         """Return the requested package (i.e. `mamba install <package>`)"""
         if self.requested is None:
             LOGGER.info("Grabbing the list of manually requested packages ...")
-            self.requested = CondaPackageHelper._parse_package_versions(
+            env_export = self.container.exec_cmd(
-                self._execute_command(
+                "mamba env export --no-build --json --from-history"
-                    CondaPackageHelper._conda_export_command(from_history=True)
-                )
             )
+            self.requested = CondaPackageHelper._parse_package_versions(env_export)
         return self.requested
 
-    def _execute_command(self, command: list[str]) -> str:
-        """Execute a command on a running container"""
-        exec_result = self.running_container.exec_run(command, stderr=False)
-        return exec_result.output.decode()  # type: ignore
-
     @staticmethod
     def _parse_package_versions(env_export: str) -> dict[str, set[str]]:
         """Extract packages and versions from the lines returned by the list of specifications"""
@@ -126,7 +104,7 @@ class CondaPackageHelper:
             )
             # Keeping command line output since `mamba search --outdated --json` is way too long ...
             self.available = CondaPackageHelper._extract_available(
-                self._execute_command(["mamba", "search", "--outdated", "--quiet"])
+                self.container.exec_cmd("mamba search --outdated --quiet")
             )
         return self.available
 

tests/utils/get_container_health.py

@@ -1,9 +0,0 @@
-# Copyright (c) Jupyter Development Team.
-# Distributed under the terms of the Modified BSD License.
-import docker
-from docker.models.containers import Container
-
-
-def get_health(container: Container, client: docker.DockerClient) -> str:
-    inspect_results = client.api.inspect_container(container.name)
-    return inspect_results["State"]["Health"]["Status"]  # type: ignore

tests/utils/tracked_container.py

@@ -19,22 +19,18 @@ class TrackedContainer:
         Docker client instance
     image_name: str
         Name of the docker image to launch
-    **kwargs: dict, optional
-        Default keyword arguments to pass to docker.DockerClient.containers.run
     """
 
     def __init__(
         self,
         docker_client: docker.DockerClient,
         image_name: str,
-        **kwargs: Any,
     ):
         self.container: Container | None = None
         self.docker_client: docker.DockerClient = docker_client
         self.image_name: str = image_name
-        self.kwargs: Any = kwargs
 
-    def run_detached(self, **kwargs: Any) -> Container:
+    def run_detached(self, **kwargs: Any) -> None:
         """Runs a docker container using the pre-configured image name
         and a mix of the pre-configured container options and those passed
         to this method.
@@ -47,18 +43,33 @@ class TrackedContainer:
         **kwargs: dict, optional
             Keyword arguments to pass to docker.DockerClient.containers.run
             extending and/or overriding key/value pairs passed to the constructor
-
-        Returns
-        -------
-        docker.Container
         """
-        all_kwargs = self.kwargs | kwargs
+        LOGGER.info(f"Running {self.image_name} with args {kwargs} ...")
-        LOGGER.info(f"Running {self.image_name} with args {all_kwargs} ...")
         self.container = self.docker_client.containers.run(
-            self.image_name,
+            self.image_name, **kwargs, detach=True
-            **all_kwargs,
         )
-        return self.container
+
+    def get_logs(self) -> str:
+        assert self.container is not None
+        logs = self.container.logs().decode()
+        assert isinstance(logs, str)
+        return logs
+
+    def get_health(self) -> str:
+        assert self.container is not None
+        inspect_results = self.docker_client.api.inspect_container(self.container.name)
+        return inspect_results["State"]["Health"]["Status"]  # type: ignore
+
+    def exec_cmd(self, cmd: str, **kwargs: Any) -> str:
+        assert self.container is not None
+        container = self.container
+        LOGGER.info(f"Running cmd: `{cmd}` on container: {container.name}")
+        exec_result = container.exec_run(cmd, **kwargs)
+        output = exec_result.output.decode().rstrip()
+        assert isinstance(output, str)
+        LOGGER.info(f"Command output: {output}")
+        assert exec_result.exit_code == 0, f"Command: `{cmd}` failed"
+        return output
 
     def run_and_wait(
         self,
@@ -68,10 +79,10 @@ class TrackedContainer:
         no_failure: bool = True,
         **kwargs: Any,
     ) -> str:
-        running_container = self.run_detached(**kwargs)
+        self.run_detached(**kwargs)
-        rv = running_container.wait(timeout=timeout)
+        assert self.container is not None
-        logs = running_container.logs().decode()
+        rv = self.container.wait(timeout=timeout)
-        assert isinstance(logs, str)
+        logs = self.get_logs()
         LOGGER.debug(logs)
         assert no_warnings == (not self.get_warnings(logs))
         assert no_errors == (not self.get_errors(logs))