From 33286f5041044d08f4580b4b868b6c5c854e2361 Mon Sep 17 00:00:00 2001 From: Atul Nair Date: Fri, 29 May 2026 13:59:57 -0400 Subject: [PATCH 01/20] orchestrators/factory: collapse container enabled+launch into container.lifetime Add _resolve_container_lifetime() and invoke it from OrchestratorConfig.__init__, the single chokepoint that from_configs routes through. container.enabled is removed (hard ValueError if present); container.launch becomes a deprecated alias (true->per_run, false->external) emitting a DeprecationWarning. Default lifetime is per_run. Docstrings rewritten to the new schema. --- cvs/core/orchestrators/factory.py | 74 ++++++++++++++++++++++++++++--- 1 file changed, 69 insertions(+), 5 deletions(-) diff --git a/cvs/core/orchestrators/factory.py b/cvs/core/orchestrators/factory.py index 35203a2f..b906c633 100644 --- a/cvs/core/orchestrators/factory.py +++ b/cvs/core/orchestrators/factory.py @@ -5,10 +5,65 @@ All code contained here is Property of Advanced Micro Devices, Inc. ''' +import warnings + from cvs.core.orchestrators.baremetal import BaremetalOrchestrator from cvs.core.orchestrators.container import ContainerOrchestrator +VALID_CONTAINER_LIFETIMES = ("external", "per_run", "persistent") + + +def _resolve_container_lifetime(container): + """Normalize a container config block to a single resolved ``lifetime`` key. + + Collapses the legacy two-axis schema (``enabled`` + ``launch``) into one + tri-valued ``container.lifetime``. Mutates and returns the passed dict. + + Resolution rules (first match wins): + - ``enabled`` present (any value) -> ``ValueError``. The field is removed + from the schema; the user must delete it and set ``lifetime``. + - ``lifetime`` present -> validated, kept as-is. + - ``launch`` present (no enabled) -> ``DeprecationWarning``; ``True`` maps + to ``per_run``, ``False`` maps to ``external``. The legacy key is dropped. + - none of the above -> default ``per_run``. + + An empty/absent container block is returned untouched (baremetal path). + """ + if not container: + return container + + if 'enabled' in container: + raise ValueError( + "container.enabled is removed; delete the field and set " + "container.lifetime to one of 'external', 'per_run', 'persistent'" + ) + + if 'lifetime' in container: + lifetime = container['lifetime'] + if lifetime not in VALID_CONTAINER_LIFETIMES: + raise ValueError( + f"container.lifetime must be one of {VALID_CONTAINER_LIFETIMES}, " + f"got {lifetime!r}" + ) + return container + + if 'launch' in container: + launch = container.pop('launch') + lifetime = 'per_run' if launch else 'external' + warnings.warn( + f"container.launch is deprecated, use container.lifetime " + f"(mapped launch={launch!r} -> lifetime={lifetime!r})", + DeprecationWarning, + stacklevel=2, + ) + container['lifetime'] = lifetime + return container + + container['lifetime'] = 'per_run' + return container + + class OrchestratorConfig: """ Configuration for orchestrator creation. @@ -26,8 +81,7 @@ class OrchestratorConfig: Example container configuration: ```json "container": { - "enabled": true, - "launch": false, + "lifetime": "per_run", "runtime": { "name": "docker", "args": { @@ -47,7 +101,13 @@ class OrchestratorConfig: "name": "myuser_rocm_cvs_latest" } ``` - launch: Containers are already running, test suite should not start/stop them [default: false] + lifetime: Container lifecycle policy [default: 'per_run'] + - 'external' : containers managed outside CVS. Setup verifies they + are running; teardown is a no-op. + - 'per_run' : start at setup, remove at teardown (the default). + - 'persistent' : start if absent / attach if present; never torn down + by the run. Pin container.name explicitly under this + mode (the default _ name shifts on tag bumps). """ def __init__(self, **kwargs): @@ -72,7 +132,11 @@ def __init__(self, **kwargs): self.priv_key_file = kwargs['priv_key_file'] self.password = kwargs.get('password') self.head_node_dict = kwargs.get('head_node_dict', {}) - self.container = kwargs.get('container', {}) + # Normalize the container block to a resolved 'lifetime'. This is the + # single chokepoint: from_configs constructs via cls(**required_config), + # so both file-driven and direct programmatic construction hit the same + # normalization (and the same enabled-removed / launch-deprecated errors). + self.container = _resolve_container_lifetime(kwargs.get('container', {})) def get(self, key, default=None): """Get configuration value with default.""" @@ -97,7 +161,7 @@ def from_configs(cls, cluster_config, testsuite_config=None): Required keys: orchestrator, node_dict, username, priv_key_file Optional keys: container, head_node_dict, password (defaults provided for missing optional keys) - Container structure: {enabled: bool, launch: bool, runtime: {name: str, args: dict}, image: str, name: str, ...} + Container structure: {lifetime: 'external'|'per_run'|'persistent', runtime: {name: str, args: dict}, image: str, name: str, ...} testsuite_config: Test suite specific configuration (dict or path to _config.json) Can override any keys from cluster_config From b31b343598470ae6af9b10e5ae4a3b6115052f30 Mon Sep 17 00:00:00 2001 From: Atul Nair Date: Fri, 29 May 2026 13:59:57 -0400 Subject: [PATCH 02/20] runtimes: drop launch short-circuit, add image_sha_status DockerRuntime no longer interprets container.launch now that lifetime resolution lives in OrchestratorConfig. Add image_sha_status() to compare a running container's image SHA against the local image tag per host (used by the persistent attach path), and mirror it in the ContainerRuntime protocol and the Enroot stub. --- cvs/core/runtimes/base.py | 10 ++++++++++ cvs/core/runtimes/docker.py | 40 ++++++++++++++++++++++++++++++------- cvs/core/runtimes/enroot.py | 5 +++++ 3 files changed, 48 insertions(+), 7 deletions(-) diff --git a/cvs/core/runtimes/base.py b/cvs/core/runtimes/base.py index 9df3f99c..322cbb4c 100644 --- a/cvs/core/runtimes/base.py +++ b/cvs/core/runtimes/base.py @@ -19,6 +19,16 @@ def teardown_containers(self, container_name): """Tear down containers on all nodes.""" ... + def image_sha_status(self, container_name, image_name): + """Compare a running container's image SHA to a local image tag, per host. + + Returns: + dict: per-host result with keys 'container_sha' (image ID the running + container was created from), 'image_sha' (local image's current ID), + and 'exit_code' (the underlying probe command's exit code). + """ + ... + def is_running(self, container_name): """Check whether a container with the given name is running on each host. diff --git a/cvs/core/runtimes/docker.py b/cvs/core/runtimes/docker.py index 49cfef14..4309a3b4 100644 --- a/cvs/core/runtimes/docker.py +++ b/cvs/core/runtimes/docker.py @@ -44,8 +44,8 @@ def setup_containers( Args: container_config: Container configuration dictionary. Recognized - top-level keys include ``image``, ``launch``, ``runtime`` (with - nested ``args``), and ``env``. + top-level keys include ``image``, ``runtime`` (with nested + ``args``), and ``env``. container_name: Name to assign to the containers volumes: Optional list of volume mounts (overrides config) devices: Optional list of device passthroughs (overrides config) @@ -60,11 +60,6 @@ def setup_containers( self.log.warning("No container config or image specified, skipping container start") return False - launch = container_config.get('launch', False) - if not launch: - self.log.info("Container launch disabled, assuming containers are already running") - return True - image = container_config['image'] # Use provided parameters or fall back to config @@ -174,6 +169,37 @@ def is_running(self, container_name): } return out + def image_sha_status(self, container_name, image_name): + """Compare a running container's image SHA to the local image tag, per host. + + Used by the ``lifetime: persistent`` attach path to detect overlay + staleness and cross-host image skew. Runs both ``docker inspect`` probes + in one fan-out and emits ``|`` per host. + + Args: + container_name: Name of the running container to inspect. + image_name: Local image tag to compare against. + + Returns: + dict: per-host result with keys 'container_sha' (image ID the running + container was created from, ``.Image``), 'image_sha' (local image's + current ID, ``.Id``), and 'exit_code' (the probe's exit code). + """ + cmd = ( + f"echo \"$(sudo docker inspect --format '{{{{.Image}}}}' {container_name} 2>/dev/null)" + f"|$(sudo docker inspect --format '{{{{.Id}}}}' {image_name} 2>/dev/null)\"" + ) + raw = self.orchestrator.all.exec(cmd, timeout=30, detailed=True) + out = {} + for host, res in raw.items(): + parts = (res.get('output') or '').strip().split('|') + out[host] = { + 'exit_code': res.get('exit_code'), + 'container_sha': parts[0] if len(parts) > 0 else '', + 'image_sha': parts[1] if len(parts) > 1 else '', + } + return out + def teardown_containers(self, container_name): """Stop and remove Docker containers on all nodes.""" if not container_name: diff --git a/cvs/core/runtimes/enroot.py b/cvs/core/runtimes/enroot.py index 9d62bf4a..cc3dc6b7 100644 --- a/cvs/core/runtimes/enroot.py +++ b/cvs/core/runtimes/enroot.py @@ -23,6 +23,11 @@ def teardown_containers(self, container_name): self.log.warning("Enroot runtime not yet implemented") return True + def image_sha_status(self, container_name, image_name): + """Compare image SHAs for Enroot containers - not yet implemented.""" + self.log.error("Enroot runtime not yet implemented") + return {} + def is_running(self, container_name): """Check Enroot container status - not yet implemented.""" self.log.error("Enroot runtime not yet implemented") From fa6ec07fcf1fd6932c5e2d6e532e9ca5a5473599 Mon Sep 17 00:00:00 2001 From: Atul Nair Date: Fri, 29 May 2026 13:59:57 -0400 Subject: [PATCH 03/20] orchestrators/container: branch setup/teardown on container.lifetime setup_containers verifies-only for external, launches for per_run, and attaches-or-launches for persistent (with a per-host image-SHA check; cross-host SHA skew is a hard error). teardown is a no-op for external/persistent and force-removes for per_run. Extract the launch path into _launch_containers(); add a pgrep precheck so setup_sshd is idempotent on persistent re-runs. Drop the dead container_enabled attribute. --- cvs/core/orchestrators/container.py | 219 +++++++++++++++++++--------- 1 file changed, 148 insertions(+), 71 deletions(-) diff --git a/cvs/core/orchestrators/container.py b/cvs/core/orchestrators/container.py index 61666fe4..df22130f 100644 --- a/cvs/core/orchestrators/container.py +++ b/cvs/core/orchestrators/container.py @@ -80,7 +80,6 @@ def __init__(self, log, config, stop_on_errors=False): if not self.container_config: raise ValueError("ContainerOrchestrator requires 'container' config in OrchestratorConfig") - self.container_enabled = True self.container_id = None # Track running container ID # Initialize container runtime @@ -263,12 +262,15 @@ def setup_containers( ulimits=None, ): """ - Set up containers based on configuration. + Set up containers according to the configured container.lifetime policy. This method should be called explicitly by tests when they need containers. - It respects the 'enabled' flag and handles container lifecycle appropriately. - If 'launch' is true, checks that containers are already running and sets container_id. - If containers are not running when expected, fails and informs the user. + Behavior branches on container.lifetime: + - 'external' : verify the (externally managed) container is running and + set container_id; never starts anything. + - 'per_run' : start fresh containers on all hosts. + - 'persistent' : attach to a container already running on all hosts (with + an image-SHA check), otherwise start fresh. Idempotent. Args: volumes: Optional list of volume mounts (uses standards if not provided) @@ -282,67 +284,131 @@ def setup_containers( Returns: bool: True if containers were set up successfully or no setup needed """ - if not self.container_config or not self.container_config.get('enabled', False): - self.log.debug("Container mode not enabled, skipping setup") - return True - - if self.container_config.get('launch', False): - # Launch containers - self.log.info("Launching containers...") - container_name = self.get_container_name(self.container_config, self.container_config['image']) - self.container_id = container_name - - # Use provided parameters or get standards - volumes = volumes if volumes is not None else self.get_volumes() - devices = devices if devices is not None else self.get_devices() - capabilities = capabilities if capabilities is not None else self.get_capabilities() - security_opts = security_opts if security_opts is not None else self.get_security_opts() - environment = environment if environment is not None else self.get_environment() - groups = groups if groups is not None else self.get_groups() - ulimits = ulimits if ulimits is not None else self.get_ulimits() - - # Create a modified container config with standard settings - modified_config = dict(self.container_config) - - # Ensure runtime config exists - if 'runtime' not in modified_config: - modified_config['runtime'] = {} - if 'args' not in modified_config['runtime']: - modified_config['runtime']['args'] = {} - - # Set standard runtime args if not already set - runtime_args = modified_config['runtime']['args'] - if 'network' not in runtime_args: - runtime_args['network'] = self.get_network_mode() - if 'ipc' not in runtime_args: - runtime_args['ipc'] = self.get_ipc_mode() - if 'privileged' not in runtime_args: - runtime_args['privileged'] = self.is_privileged() - - # Add InfiniBand device discovery via shell expansion (per-host) - ib_device_expansion = '$(for dev in /dev/infiniband/*; do echo -n "--device $dev:$dev "; done)' - - return self.runtime.setup_containers( - modified_config, - container_name, - volumes=volumes, - devices=devices, - capabilities=capabilities, - security_opts=security_opts, - environment=environment, - groups=groups, - ulimits=ulimits, - device_expansion=ib_device_expansion, - ) + lifetime = self.container_config.get('lifetime', 'per_run') - # Assume containers are running image = self.container_config.get('image') if not image: self.log.error("Container image not specified in config") return False - container_name = self.get_container_name(self.container_config, image) - return self.verify_containers_running(container_name) + + if lifetime == 'external': + # Externally managed: verify only, never start. + return self.verify_containers_running(container_name) + + if lifetime == 'persistent': + # Attach if already running on every host, otherwise (re)launch. + status = self.runtime.is_running(container_name) + if status and all(info.get('running') for info in status.values()): + self.container_id = container_name + self.log.info(f"Attaching to running container '{container_name}'") + return self._verify_persistent_image(container_name, image) + self.log.info("Persistent container not running on all hosts, launching...") + return self._launch_containers( + volumes, devices, capabilities, security_opts, environment, groups, ulimits + ) + + # 'per_run' (default): always start fresh. + return self._launch_containers( + volumes, devices, capabilities, security_opts, environment, groups, ulimits + ) + + def _launch_containers( + self, + volumes=None, + devices=None, + capabilities=None, + security_opts=None, + environment=None, + groups=None, + ulimits=None, + ): + """Start fresh containers on all hosts via the runtime. + + Shared by the 'per_run' path and the 'persistent' path when no container + is already running. The runtime force-removes any stale same-named + container before starting. + """ + self.log.info("Launching containers...") + container_name = self.get_container_name(self.container_config, self.container_config['image']) + self.container_id = container_name + + # Use provided parameters or get standards + volumes = volumes if volumes is not None else self.get_volumes() + devices = devices if devices is not None else self.get_devices() + capabilities = capabilities if capabilities is not None else self.get_capabilities() + security_opts = security_opts if security_opts is not None else self.get_security_opts() + environment = environment if environment is not None else self.get_environment() + groups = groups if groups is not None else self.get_groups() + ulimits = ulimits if ulimits is not None else self.get_ulimits() + + # Create a modified container config with standard settings + modified_config = dict(self.container_config) + + # Ensure runtime config exists + if 'runtime' not in modified_config: + modified_config['runtime'] = {} + if 'args' not in modified_config['runtime']: + modified_config['runtime']['args'] = {} + + # Set standard runtime args if not already set + runtime_args = modified_config['runtime']['args'] + if 'network' not in runtime_args: + runtime_args['network'] = self.get_network_mode() + if 'ipc' not in runtime_args: + runtime_args['ipc'] = self.get_ipc_mode() + if 'privileged' not in runtime_args: + runtime_args['privileged'] = self.is_privileged() + + # Add InfiniBand device discovery via shell expansion (per-host) + ib_device_expansion = '$(for dev in /dev/infiniband/*; do echo -n "--device $dev:$dev "; done)' + + return self.runtime.setup_containers( + modified_config, + container_name, + volumes=volumes, + devices=devices, + capabilities=capabilities, + security_opts=security_opts, + environment=environment, + groups=groups, + ulimits=ulimits, + device_expansion=ib_device_expansion, + ) + + def _verify_persistent_image(self, container_name, image): + """Compare the running container's image SHA to the local image tag on + each host (persistent attach only). + + - Per-host mismatch (container created from an image older than the local + ```` tag): WARN and continue -- the overlay may be stale. + - Cross-host SHA skew (hosts running different image SHAs): ERROR and + return False -- a correctness problem, not mere staleness. + + Returns: + bool: False on cross-host skew, True otherwise. + """ + status = self.runtime.image_sha_status(container_name, image) + container_shas = set() + for host, info in status.items(): + container_sha = info.get('container_sha', '') + image_sha = info.get('image_sha', '') + if container_sha: + container_shas.add(container_sha) + if container_sha and image_sha and container_sha != image_sha: + self.log.warning( + f"Container '{container_name}' on {host} runs image " + f"{container_sha[:19]} but local '{image}' is {image_sha[:19]}; " + f"overlay may be stale" + ) + + if len(container_shas) > 1: + self.log.error( + f"Cross-host image SHA skew for container '{container_name}': " + f"hosts are running different images {sorted(container_shas)}" + ) + return False + return True def setup_sshd(self): """ @@ -365,6 +431,16 @@ def setup_sshd(self): self.log.info(f"Setting up SSH daemon in containers: {self.container_id}") + # Idempotency precheck (required by lifetime: persistent): on a second + # `cvs run` the container's sshd is already bound to 2224, so re-running + # `/usr/sbin/sshd -p2224` would fail with "address already in use". Skip + # the setup commands on any host that already has sshd on 2224. + precheck = self.exec("pgrep -f 'sshd.*2224' > /dev/null 2>&1", timeout=10, detailed=True) + hosts_needing_sshd = [host for host, output in precheck.items() if output['exit_code'] != 0] + if not hosts_needing_sshd: + self.log.info("SSH daemon already running on all hosts, skipping setup") + return True + # Execute SSH setup commands # Note: Commands with shell operators must be wrapped in bash -c for proper execution inside container ssh_setup_commands = [ @@ -377,8 +453,8 @@ def setup_sshd(self): ] for cmd in ssh_setup_commands: - result = self.exec(cmd, timeout=10, detailed=True) - # Check if command succeeded on all hosts + result = self.exec(cmd, hosts=hosts_needing_sshd, timeout=10, detailed=True) + # Check if command succeeded on all targeted hosts for hostname, output in result.items(): if output['exit_code'] != 0: self.log.error(f"SSH setup command failed on {hostname}: {cmd}") @@ -437,21 +513,22 @@ def verify_containers_running(self, container_name): def teardown_containers(self): """ - Tear down containers if they are running. + Tear down containers according to the configured container.lifetime policy. - This method should be called explicitly by tests for cleanup. - It respects the 'enabled' flag and handles container lifecycle appropriately. - If 'launch' is False, assumes containers should not be stopped (externally managed). + This method should be called explicitly by tests for cleanup. Behavior + branches on container.lifetime: + - 'external' : no-op (CVS does not own externally managed containers). + - 'persistent' : no-op (left running for the next run; user removes it + explicitly). + - 'per_run' : force-remove the container CVS started. Returns: bool: True if containers were torn down successfully or no teardown needed """ - if not self.container_config or not self.container_config.get('enabled', False): - self.log.debug("Container mode not enabled, skipping teardown") - return True + lifetime = self.container_config.get('lifetime', 'per_run') - if not self.container_config.get('launch', False): - self.log.debug("launch is False, not stopping externally managed containers") + if lifetime in ('external', 'persistent'): + self.log.debug(f"lifetime={lifetime}, leaving containers running") return True if not self.container_id: From c14b914324cc277b15cf96c45d1d4791e3f08cf7 Mon Sep 17 00:00:00 2001 From: Atul Nair Date: Fri, 29 May 2026 14:00:08 -0400 Subject: [PATCH 04/20] input/cluster_file: use container.lifetime in cluster_container.json sample Replace the enabled/launch keys (and their comments) with a single lifetime: per_run plus a comment describing the three policies. --- cvs/input/cluster_file/cluster_container.json | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/cvs/input/cluster_file/cluster_container.json b/cvs/input/cluster_file/cluster_container.json index 2a95f115..bd06a722 100644 --- a/cvs/input/cluster_file/cluster_container.json +++ b/cvs/input/cluster_file/cluster_container.json @@ -29,13 +29,10 @@ } }, - "_container_block_comment": "Container backend configuration. Consumed by ContainerOrchestrator in cvs/core/orchestrators/container.py. See README.md for the full schema and launch:true/launch:false semantics.", + "_container_block_comment": "Container backend configuration. Consumed by ContainerOrchestrator in cvs/core/orchestrators/container.py. See README.md for the full schema and lifetime semantics.", "container": { - "_enabled_comment": "Master switch. Must be true for the container backend to do anything; if false, setup_containers/teardown_containers short-circuit to no-ops.", - "enabled": true, - - "_launch_comment": "true = CVS owns container lifecycle (starts on setup_containers, stops on teardown_containers). false = containers are externally managed; CVS only verifies they are running and never stops them.", - "launch": true, + "_lifetime_comment": "Container lifecycle policy. 'external' = containers managed outside CVS (verify-only, never stopped). 'per_run' = CVS starts on setup and removes on teardown. 'persistent' = CVS starts if absent / attaches if present and leaves running on exit (unblocks install-then-run; pin 'name' explicitly).", + "lifetime": "per_run", "_image_comment": "Image with test dependencies (rvs, etc.) pre-installed and an sshd you can start on port 2224. Must be present locally on each node OR pullable from a reachable registry.", "image": "rocm/cvs:latest", From ab7430908ec2d4788ceea89c1eac9c203f8c744a Mon Sep 17 00:00:00 2001 From: Atul Nair Date: Fri, 29 May 2026 14:00:08 -0400 Subject: [PATCH 05/20] core/unittests: cover container.lifetime resolution and per-lifetime behavior Migrate the container/factory/docker unit tests off enabled+launch. Add _resolve_container_lifetime table cases (enabled->ValueError, launch-> DeprecationWarning, invalid->ValueError, defaults), persistent attach/launch/ idempotency, cross-host SHA skew, stale-overlay warn, and setup_sshd idempotency. --- .../orchestrators/unittests/test_container.py | 233 +++++++++++++++--- .../orchestrators/unittests/test_factory.py | 7 +- cvs/core/runtimes/unittests/test_docker.py | 25 +- 3 files changed, 223 insertions(+), 42 deletions(-) diff --git a/cvs/core/orchestrators/unittests/test_container.py b/cvs/core/orchestrators/unittests/test_container.py index 0d3e35ba..08c4d6eb 100644 --- a/cvs/core/orchestrators/unittests/test_container.py +++ b/cvs/core/orchestrators/unittests/test_container.py @@ -7,20 +7,21 @@ # Unit tests for cvs/core/orchestrators/container.py: ContainerOrchestrator construction # (incl. SSH port override and runtime wiring) and the setup_containers / teardown_containers -# short-circuit semantics inherited by the rvs_cvs.py orch fixture. Mocks Pssh and -# RuntimeFactory so tests run with no SSH or container runtime. +# lifetime branching inherited by the rvs_cvs.py orch fixture. Mocks Pssh and RuntimeFactory +# so tests run with no SSH or container runtime. # -# The teardown_containers short-circuit at cvs/core/orchestrators/container.py:439 is -# pinned here so any future change has a loud canary. +# The per-lifetime setup/teardown behavior is pinned here so any future change to the +# external / per_run / persistent contract has a loud canary. import unittest +import warnings from unittest.mock import MagicMock, patch -from cvs.core.orchestrators.factory import OrchestratorConfig +from cvs.core.orchestrators.factory import OrchestratorConfig, _resolve_container_lifetime from cvs.core.orchestrators.container import ContainerOrchestrator -def _make_orch_config(launch=False, enabled=True): +def _make_orch_config(lifetime="per_run"): """Minimal OrchestratorConfig that satisfies ContainerOrchestrator.__init__ without touching disk or SSH.""" return OrchestratorConfig( @@ -31,8 +32,7 @@ def _make_orch_config(launch=False, enabled=True): password=None, head_node_dict={"mgmt_ip": "10.0.0.1"}, container={ - "enabled": enabled, - "launch": launch, + "lifetime": lifetime, "image": "rocm/cvs:test", "name": "cvs_iter_test", "runtime": {"name": "docker", "args": {}}, @@ -41,8 +41,8 @@ def _make_orch_config(launch=False, enabled=True): class TestContainerOrchestrator(unittest.TestCase): - def _make(self, _mock_pssh, _mock_rf, launch=False, enabled=True): - cfg = _make_orch_config(launch=launch, enabled=enabled) + def _make(self, _mock_pssh, _mock_rf, lifetime="per_run"): + cfg = _make_orch_config(lifetime=lifetime) runtime = MagicMock(name="docker_runtime") _mock_rf.create.return_value = runtime orch = ContainerOrchestrator(MagicMock(), cfg) @@ -69,10 +69,14 @@ def test_init_overrides_ssh_port_to_container_sshd(self, _mock_pssh, mock_rf): orch, _ = self._make(_mock_pssh, mock_rf) self.assertEqual(orch.ssh_port, 2224) + # ------------------------------------------------------------------ + # setup_containers lifetime branching + # ------------------------------------------------------------------ + @patch("cvs.core.orchestrators.container.RuntimeFactory") @patch("cvs.core.orchestrators.baremetal.Pssh") - def test_setup_containers_launch_true_delegates_to_runtime(self, _mock_pssh, mock_rf): - orch, runtime = self._make(_mock_pssh, mock_rf, launch=True) + def test_setup_containers_per_run_delegates_to_runtime(self, _mock_pssh, mock_rf): + orch, runtime = self._make(_mock_pssh, mock_rf, lifetime="per_run") runtime.setup_containers.return_value = True result = orch.setup_containers() self.assertTrue(result) @@ -80,56 +84,159 @@ def test_setup_containers_launch_true_delegates_to_runtime(self, _mock_pssh, moc @patch("cvs.core.orchestrators.container.RuntimeFactory") @patch("cvs.core.orchestrators.baremetal.Pssh") - def test_setup_containers_short_circuits_when_disabled(self, _mock_pssh, mock_rf): - orch, runtime = self._make(_mock_pssh, mock_rf, enabled=False) - result = orch.setup_containers() - self.assertTrue(result) # Disabled is treated as success / no-op. + def test_setup_containers_external_verifies_only(self, _mock_pssh, mock_rf): + # external never starts a container; it verifies and sets container_id. + orch, runtime = self._make(_mock_pssh, mock_rf, lifetime="external") + runtime.is_running.return_value = { + "10.0.0.1": {"running": True, "exit_code": 0, "name": "cvs_iter_test"}, + "10.0.0.2": {"running": True, "exit_code": 0, "name": "cvs_iter_test"}, + } + self.assertTrue(orch.setup_containers()) runtime.setup_containers.assert_not_called() + self.assertEqual(orch.container_id, "cvs_iter_test") @patch("cvs.core.orchestrators.container.RuntimeFactory") @patch("cvs.core.orchestrators.baremetal.Pssh") - def test_teardown_containers_short_circuits_when_launch_false(self, _mock_pssh, mock_rf): - # launch:false means containers are externally managed; teardown is a no-op. - orch, runtime = self._make(_mock_pssh, mock_rf, launch=False) + def test_setup_containers_persistent_attaches_when_running(self, _mock_pssh, mock_rf): + orch, runtime = self._make(_mock_pssh, mock_rf, lifetime="persistent") + runtime.is_running.return_value = { + "10.0.0.1": {"running": True, "exit_code": 0, "name": "cvs_iter_test"}, + "10.0.0.2": {"running": True, "exit_code": 0, "name": "cvs_iter_test"}, + } + runtime.image_sha_status.return_value = { + "10.0.0.1": {"container_sha": "sha:abc", "image_sha": "sha:abc", "exit_code": 0}, + "10.0.0.2": {"container_sha": "sha:abc", "image_sha": "sha:abc", "exit_code": 0}, + } + self.assertTrue(orch.setup_containers()) + # Attach path: no new container started. + runtime.setup_containers.assert_not_called() + self.assertEqual(orch.container_id, "cvs_iter_test") + + @patch("cvs.core.orchestrators.container.RuntimeFactory") + @patch("cvs.core.orchestrators.baremetal.Pssh") + def test_setup_containers_persistent_starts_when_not_running(self, _mock_pssh, mock_rf): + orch, runtime = self._make(_mock_pssh, mock_rf, lifetime="persistent") + runtime.is_running.return_value = { + "10.0.0.1": {"running": False, "exit_code": 0, "name": ""}, + "10.0.0.2": {"running": False, "exit_code": 0, "name": ""}, + } + runtime.setup_containers.return_value = True + self.assertTrue(orch.setup_containers()) + runtime.setup_containers.assert_called_once() + # No SHA check when launching fresh. + runtime.image_sha_status.assert_not_called() + + @patch("cvs.core.orchestrators.container.RuntimeFactory") + @patch("cvs.core.orchestrators.baremetal.Pssh") + def test_setup_containers_persistent_idempotent_on_resetup(self, _mock_pssh, mock_rf): + # Re-running setup against an already-running persistent container is a + # no-op attach both times -- never starts a new container. + orch, runtime = self._make(_mock_pssh, mock_rf, lifetime="persistent") + runtime.is_running.return_value = { + "10.0.0.1": {"running": True, "exit_code": 0, "name": "cvs_iter_test"}, + } + runtime.image_sha_status.return_value = { + "10.0.0.1": {"container_sha": "sha:abc", "image_sha": "sha:abc", "exit_code": 0}, + } + self.assertTrue(orch.setup_containers()) + self.assertTrue(orch.setup_containers()) + runtime.setup_containers.assert_not_called() + + @patch("cvs.core.orchestrators.container.RuntimeFactory") + @patch("cvs.core.orchestrators.baremetal.Pssh") + def test_setup_containers_persistent_cross_host_sha_skew_errors(self, _mock_pssh, mock_rf): + # Nodes running different image SHAs is a correctness error, not staleness. + orch, runtime = self._make(_mock_pssh, mock_rf, lifetime="persistent") + runtime.is_running.return_value = { + "10.0.0.1": {"running": True, "exit_code": 0, "name": "cvs_iter_test"}, + "10.0.0.2": {"running": True, "exit_code": 0, "name": "cvs_iter_test"}, + } + runtime.image_sha_status.return_value = { + "10.0.0.1": {"container_sha": "sha:abc", "image_sha": "sha:abc", "exit_code": 0}, + "10.0.0.2": {"container_sha": "sha:def", "image_sha": "sha:def", "exit_code": 0}, + } + self.assertFalse(orch.setup_containers()) + + @patch("cvs.core.orchestrators.container.RuntimeFactory") + @patch("cvs.core.orchestrators.baremetal.Pssh") + def test_setup_containers_persistent_stale_overlay_warns_but_passes(self, _mock_pssh, mock_rf): + # Per-host staleness (container older than local image tag) warns, does not fail. + orch, runtime = self._make(_mock_pssh, mock_rf, lifetime="persistent") + runtime.is_running.return_value = { + "10.0.0.1": {"running": True, "exit_code": 0, "name": "cvs_iter_test"}, + } + runtime.image_sha_status.return_value = { + "10.0.0.1": {"container_sha": "sha:old", "image_sha": "sha:new", "exit_code": 0}, + } + self.assertTrue(orch.setup_containers()) + + # ------------------------------------------------------------------ + # teardown_containers lifetime branching + # ------------------------------------------------------------------ + + @patch("cvs.core.orchestrators.container.RuntimeFactory") + @patch("cvs.core.orchestrators.baremetal.Pssh") + def test_teardown_containers_short_circuits_when_lifetime_external(self, _mock_pssh, mock_rf): + # external means containers are externally managed; teardown is a no-op. + orch, runtime = self._make(_mock_pssh, mock_rf, lifetime="external") orch.container_id = "test_container" self.assertTrue(orch.teardown_containers()) runtime.teardown_containers.assert_not_called() @patch("cvs.core.orchestrators.container.RuntimeFactory") @patch("cvs.core.orchestrators.baremetal.Pssh") - def test_teardown_containers_calls_runtime_when_launch_true(self, _mock_pssh, mock_rf): - # launch:true means CVS owns the container lifecycle; teardown delegates to runtime. - orch, runtime = self._make(_mock_pssh, mock_rf, launch=True) + def test_teardown_containers_persistent_is_noop(self, _mock_pssh, mock_rf): + # persistent leaves the container running for the next run. + orch, runtime = self._make(_mock_pssh, mock_rf, lifetime="persistent") orch.container_id = "test_container" - runtime.teardown_containers.return_value = True self.assertTrue(orch.teardown_containers()) - runtime.teardown_containers.assert_called_once_with("test_container") - # container_id cleared on successful teardown. - self.assertIsNone(orch.container_id) + runtime.teardown_containers.assert_not_called() @patch("cvs.core.orchestrators.container.RuntimeFactory") @patch("cvs.core.orchestrators.baremetal.Pssh") - def test_teardown_containers_short_circuits_when_disabled(self, _mock_pssh, mock_rf): - orch, runtime = self._make(_mock_pssh, mock_rf, enabled=False) + def test_teardown_containers_calls_runtime_when_lifetime_per_run(self, _mock_pssh, mock_rf): + # per_run means CVS owns the container lifecycle; teardown delegates to runtime. + orch, runtime = self._make(_mock_pssh, mock_rf, lifetime="per_run") orch.container_id = "test_container" + runtime.teardown_containers.return_value = True self.assertTrue(orch.teardown_containers()) - runtime.teardown_containers.assert_not_called() + runtime.teardown_containers.assert_called_once_with("test_container") + # container_id cleared on successful teardown. + self.assertIsNone(orch.container_id) @patch("cvs.core.orchestrators.container.RuntimeFactory") @patch("cvs.core.orchestrators.baremetal.Pssh") def test_teardown_containers_short_circuits_when_no_container_id(self, _mock_pssh, mock_rf): - # container_id stays None when prepare/setup never ran; teardown is a no-op - # even with launch=True (CVS-owned), since there is nothing to tear down. - orch, runtime = self._make(_mock_pssh, mock_rf, launch=True) + # container_id stays None when setup never ran; per_run teardown is a no-op + # since there is nothing to tear down. + orch, runtime = self._make(_mock_pssh, mock_rf, lifetime="per_run") self.assertIsNone(orch.container_id) self.assertTrue(orch.teardown_containers()) runtime.teardown_containers.assert_not_called() + # ------------------------------------------------------------------ + # setup_sshd idempotency (required by lifetime: persistent) + # ------------------------------------------------------------------ + + @patch("cvs.core.orchestrators.container.RuntimeFactory") + @patch("cvs.core.orchestrators.baremetal.Pssh") + def test_setup_sshd_idempotent_when_already_running(self, _mock_pssh, mock_rf): + # When the pgrep precheck reports sshd already on 2224 for every host, the + # start commands must NOT run (would re-bind the port and fail). + orch, runtime = self._make(_mock_pssh, mock_rf, lifetime="persistent") + orch.container_id = "cvs_iter_test" + runtime.exec.return_value = { + "10.0.0.1": {"exit_code": 0, "output": ""}, + "10.0.0.2": {"exit_code": 0, "output": ""}, + } + self.assertTrue(orch.setup_sshd()) + # Only the precheck exec happened; no setup commands were issued. + runtime.exec.assert_called_once() + @patch("cvs.core.orchestrators.container.RuntimeFactory") @patch("cvs.core.orchestrators.baremetal.Pssh") def test_init_requires_container_config(self, _mock_pssh, _mock_rf): - # ContainerOrchestrator raises if 'container' config is empty. Construct - # the config directly to bypass the helper's auto-population. + # ContainerOrchestrator raises if 'container' config is empty. cfg = OrchestratorConfig( orchestrator="container", node_dict={"10.0.0.1": {}}, @@ -141,5 +248,63 @@ def test_init_requires_container_config(self, _mock_pssh, _mock_rf): ContainerOrchestrator(MagicMock(), cfg) +class TestResolveContainerLifetime(unittest.TestCase): + """One assertion per row of the lifetime resolution table.""" + + def test_enabled_present_raises(self): + with self.assertRaises(ValueError) as ctx: + _resolve_container_lifetime({"enabled": True, "image": "x"}) + self.assertIn("enabled", str(ctx.exception)) + + def test_enabled_false_also_raises(self): + # Any value of the removed field is a hard error, including False. + with self.assertRaises(ValueError): + _resolve_container_lifetime({"enabled": False, "image": "x"}) + + def test_explicit_lifetime_kept_no_warning(self): + with warnings.catch_warnings(): + warnings.simplefilter("error") # any warning would fail the test + out = _resolve_container_lifetime({"lifetime": "persistent", "image": "x"}) + self.assertEqual(out["lifetime"], "persistent") + + def test_invalid_lifetime_raises(self): + with self.assertRaises(ValueError): + _resolve_container_lifetime({"lifetime": "forever", "image": "x"}) + + def test_launch_true_maps_to_per_run_with_deprecation(self): + with self.assertWarns(DeprecationWarning): + out = _resolve_container_lifetime({"launch": True, "image": "x"}) + self.assertEqual(out["lifetime"], "per_run") + self.assertNotIn("launch", out) + + def test_launch_false_maps_to_external_with_deprecation(self): + with self.assertWarns(DeprecationWarning): + out = _resolve_container_lifetime({"launch": False, "image": "x"}) + self.assertEqual(out["lifetime"], "external") + self.assertNotIn("launch", out) + + def test_no_policy_keys_defaults_to_per_run(self): + out = _resolve_container_lifetime({"image": "x"}) + self.assertEqual(out["lifetime"], "per_run") + + def test_empty_block_untouched(self): + # Baremetal path: empty container block stays empty (no lifetime injected). + self.assertEqual(_resolve_container_lifetime({}), {}) + + @patch("cvs.core.orchestrators.baremetal.Pssh") + def test_orchestratorconfig_init_resolves_launch_alias(self, _mock_pssh): + # Direct construction routes through __init__ -> the same helper, so a + # legacy launch flag is resolved identically to from_configs. + with self.assertWarns(DeprecationWarning): + cfg = OrchestratorConfig( + orchestrator="container", + node_dict={"1.1.1.1": {}}, + username="u", + priv_key_file="/dev/null", + container={"launch": True, "image": "x"}, + ) + self.assertEqual(cfg.container["lifetime"], "per_run") + + if __name__ == "__main__": unittest.main() diff --git a/cvs/core/orchestrators/unittests/test_factory.py b/cvs/core/orchestrators/unittests/test_factory.py index 10d01382..bfa082eb 100644 --- a/cvs/core/orchestrators/unittests/test_factory.py +++ b/cvs/core/orchestrators/unittests/test_factory.py @@ -20,7 +20,7 @@ from cvs.core.orchestrators.container import ContainerOrchestrator -def _make_orch_config(orchestrator="baremetal", with_container=False, launch=False, enabled=True): +def _make_orch_config(orchestrator="baremetal", with_container=False, lifetime="per_run"): """Minimal OrchestratorConfig that satisfies BaremetalOrchestrator and (optionally) ContainerOrchestrator __init__ without touching disk or SSH.""" kwargs = dict( @@ -34,8 +34,7 @@ def _make_orch_config(orchestrator="baremetal", with_container=False, launch=Fal ) if with_container or orchestrator == "container": kwargs["container"] = { - "enabled": enabled, - "launch": launch, + "lifetime": lifetime, "image": "rocm/cvs:test", "name": "cvs_iter_test", "runtime": {"name": "docker", "args": {}}, @@ -181,7 +180,7 @@ def test_from_configs_resolves_user_id_throughout_container_block(self): "username": "{user-id}", "priv_key_file": "/home/{user-id}/.ssh/id_rsa", "container": { - "enabled": True, + "lifetime": "per_run", "image": "rocm/{user-id}:test", "name": "{user-id}_cvs", "runtime": { diff --git a/cvs/core/runtimes/unittests/test_docker.py b/cvs/core/runtimes/unittests/test_docker.py index bd1f84fc..98d8bd0a 100644 --- a/cvs/core/runtimes/unittests/test_docker.py +++ b/cvs/core/runtimes/unittests/test_docker.py @@ -43,11 +43,13 @@ def _fake_exec(cmd, timeout=None, detailed=False, print_console=True): return DockerRuntime(log, orchestrator) -def _container_config(launch=True, image="img:test", extra_runtime_args=None, **extra): - """Minimal container config dict for setup_containers.""" +def _container_config(image="img:test", extra_runtime_args=None, **extra): + """Minimal container config dict for setup_containers. + + The runtime no longer reads enabled/launch -- lifetime resolution lives in + OrchestratorConfig -- so neither key is present here. + """ cfg = { - "enabled": True, - "launch": launch, "image": image, "name": "cvs_iter_test", "runtime": {"name": "docker", "args": dict(extra_runtime_args or {})}, @@ -57,6 +59,21 @@ def _container_config(launch=True, image="img:test", extra_runtime_args=None, ** class TestDockerRuntimeSetupContainers(unittest.TestCase): + def test_setup_containers_always_proceeds(self): + # The legacy `if not launch: return True` short-circuit was removed. The + # orchestrator only calls runtime.setup_containers on start paths, so the + # runtime must always render a `docker run` when invoked. + captured = [] + rt = _make_runtime(captured) + result = rt.setup_containers( + container_config=_container_config(), + container_name="cvs_iter_test", + volumes=["/home/u:/workspace"], + ) + self.assertTrue(result) + self.assertEqual(len(captured), 1) + self.assertIn("docker run", captured[0]) + def test_user_volume_listed_once(self): # With runtime.args.volumes=['/foo:/bar'] AND positional volumes that # already include '/foo:/bar' (mirroring what From 7c8be5c8ea8c7676a726c3446c5bb103b7e6094f Mon Sep 17 00:00:00 2001 From: Atul Nair Date: Fri, 29 May 2026 14:00:08 -0400 Subject: [PATCH 06/20] docs: document container.lifetime schema (replace enabled/launch) Rewrite the cluster-file README/RST and the run-with-containers how-to around the lifetime truth table (external/per_run/persistent), add a persistent attach sequence diagram, drop the removed enabled/stale-name pitfalls, and add the pin-container.name guidance for persistent. --- cvs/input/cluster_file/README.md | 45 ++++++++++----- docs/how-to/run-with-containers.rst | 23 ++++---- .../configuration-files/cluster-file.rst | 56 ++++++++----------- 3 files changed, 68 insertions(+), 56 deletions(-) diff --git a/cvs/input/cluster_file/README.md b/cvs/input/cluster_file/README.md index bf4b1a55..45e3aed9 100644 --- a/cvs/input/cluster_file/README.md +++ b/cvs/input/cluster_file/README.md @@ -43,8 +43,7 @@ Consumed by `ContainerOrchestrator` in [`cvs/core/orchestrators/container.py`](. | Key | Type | Default | Purpose | | --- | --- | --- | --- | -| `enabled` | bool | `false` | Master switch. Must be `true` for the container backend to do anything; if `false`, `setup_containers` and `teardown_containers` short-circuit to no-ops. | -| `launch` | bool | `false` | `true` = CVS owns lifecycle (starts on `setup_containers`, stops on `teardown_containers`). `false` = containers are externally managed; CVS only verifies they are running and never stops them. | +| `lifetime` | str | `"per_run"` | Container lifecycle policy: `external`, `per_run`, or `persistent`. See the truth table below. | | `image` | str | (required) | Image with the test dependencies (rvs, etc.) pre-installed and an sshd you can start on port 2224. Must be present locally on each node OR pullable from a reachable registry. | | `name` | str | (required) | Container name on each host. For parallel runs make this per-iteration unique (e.g. `cvs_iter_`). | | `runtime.name` | str | `"docker"` | Container runtime. Supported: `docker` (concrete), `enroot` (stub). | @@ -70,16 +69,15 @@ RDMA-ready container; the keys below are only needed to extend or override. | `group_add` | list | `["video"]` (appended) | Supplementary groups inside the container. | | `ulimit` | list | `["memlock=-1"]` (appended) | Per-process resource limits. `memlock=-1` is required for RDMA. | -## `launch` truth table (teardown semantics) +## `lifetime` truth table (setup + teardown semantics) -`teardown_containers()` short-circuits in three cases per [`cvs/core/orchestrators/container.py`](../../core/orchestrators/container.py) (around line 439). This is pinned by the test `test_teardown_containers_short_circuits_when_launch_true` in [`cvs/core/orchestrators/unittests/test_container.py`](../../core/orchestrators/unittests/test_container.py). +`setup_containers()` and `teardown_containers()` in [`cvs/core/orchestrators/container.py`](../../core/orchestrators/container.py) branch on `container.lifetime`. This is pinned by the per-lifetime tests in [`cvs/core/orchestrators/unittests/test_container.py`](../../core/orchestrators/unittests/test_container.py). -| `enabled` | `launch` | `container_id` | Behavior | -| --- | --- | --- | --- | -| `false` | * | * | Short-circuit (no-op, returns `True`). | -| `true` | `true` | * | Short-circuit. The container is externally managed; CVS does not stop it. | -| `true` | `false` | unset | Short-circuit. Nothing was started. | -| `true` | `false` | set | Calls `runtime.teardown_containers(container_id)`. | +| `lifetime` | `setup_containers` | `teardown_containers` | +| --- | --- | --- | +| `external` | Verify the container is already running on every host; set `container_id`. Never starts anything. | No-op. CVS does not own externally managed containers. | +| `per_run` (default) | Start a fresh container on every host (force-removing any stale same-named container first). | Force-remove the container CVS started. | +| `persistent` | Attach if the container is already running on every host (with a per-host image-SHA check; cross-host SHA skew is a hard error). Otherwise start fresh. Idempotent across runs. | No-op. The container is left running for the next run; remove it yourself when done. | ## Prerequisites on each cluster node @@ -90,6 +88,8 @@ RDMA-ready container; the keys below are only needed to extend or override. ## What happens when you run a backend-blind suite +Default (`lifetime: per_run`) - start fresh, run, remove: + ```mermaid sequenceDiagram participant TestFix as orch fixture @@ -108,7 +108,27 @@ sequenceDiagram ContainerOrch->>Sshd: ssh root@host:2224 "rvs -c ..." Sshd-->>TestFix: stdout TestFix->>ContainerOrch: teardown_containers() - ContainerOrch->>Docker: docker stop and rm + ContainerOrch->>Docker: docker rm -f +``` + +Second `cvs run` with `lifetime: persistent` - attach to the surviving container, skip sshd, leave it running: + +```mermaid +sequenceDiagram + participant TestFix as orch fixture + participant ContainerOrch as ContainerOrchestrator + participant Docker as docker daemon on host + participant Sshd as container sshd:2224 + + TestFix->>ContainerOrch: setup_containers() + ContainerOrch->>Docker: is_running(name)? yes -> attach, image-SHA check + TestFix->>ContainerOrch: setup_sshd() + ContainerOrch->>Sshd: pgrep sshd:2224 already up -> skip start + TestFix->>ContainerOrch: orch.exec("rvs -c ...") + ContainerOrch->>Sshd: ssh root@host:2224 "rvs -c ..." + Sshd-->>TestFix: stdout + TestFix->>ContainerOrch: teardown_containers() + ContainerOrch-->>TestFix: no-op (container left running) ``` The same `cvs run` command works against either backend; only the cluster @@ -116,10 +136,9 @@ file changes. The `orch` fixture in [`cvs/tests/health/rvs_cvs.py`](../../tests/ ## Common pitfalls -- **Forgot `enabled: true`**. The container block exists but the orchestrator silently no-ops every lifecycle call. Symptom: `orch.exec` errors with "no container running". - **Image without `openssh-server`**. `setup_sshd` cannot start sshd on port 2224; `orch.exec` then fails to connect. Make sure your image installs `openssh-server` and exposes the binary at `/usr/sbin/sshd`. - **Image without the workload binary**. `cvs run rvs_cvs` will execute `rvs` inside the container; if the image lacks `/opt/rocm/bin/rvs` the test fails with a `command not found` flavor error. -- **`launch: true` plus a stale container with the same `name`**. `docker run` refuses to start because the name is taken. Either pick a per-iteration unique name or stop the stale container first. +- **`lifetime: persistent` without a pinned `name`**. The default container name is `_`, which shifts when you bump the image tag. A tag bump silently abandons the previous container's overlay (installs, clones) and starts fresh. Pin `container.name` explicitly when using `persistent`. - **Port 2224 collision on the host**. With `network: host` the in-container sshd binds to 2224 in the host's network namespace. If something else on the host already listens on 2224 the bind fails. Stop the conflicting service or change the in-image sshd port (and update the orchestrator's port to match). ## See also diff --git a/docs/how-to/run-with-containers.rst b/docs/how-to/run-with-containers.rst index 8b8f5007..44d5b765 100644 --- a/docs/how-to/run-with-containers.rst +++ b/docs/how-to/run-with-containers.rst @@ -49,8 +49,8 @@ Open the copied file and edit: - ``priv_key_file``: absolute path to your SSH private key. - ``head_node_dict.mgmt_ip`` and the keys of ``node_dict``: real IPs or hostnames of your cluster nodes. ``mgmt_ip`` must equal one of the keys in ``node_dict``. - ``container.image``: an image present on every node or pullable from a reachable registry. The image must include ``openssh-server`` and the workload binary (for example ``rvs``). -- ``container.name``: container name on each host. For parallel runs make this per-iteration unique (for example ``cvs_iter_``). -- ``container.launch``: see the lifecycle note below. +- ``container.name``: container name on each host. For parallel runs make this per-iteration unique (for example ``cvs_iter_``). Pin it explicitly when using ``lifetime: persistent``. +- ``container.lifetime``: ``external``, ``per_run``, or ``persistent``. See the lifecycle note below. For the full schema and runtime argument reference, see :doc:`/reference/configuration-files/cluster-file`. @@ -73,8 +73,9 @@ Run ``rvs_cvs`` the same way you run any CVS test suite, but with the container What happens during the run: - The ``orch`` fixture in ``rvs_cvs`` reads ``orchestrator: container`` from the cluster file and constructs a ``ContainerOrchestrator``. -- If ``container.launch`` is ``true``, CVS removes any container with the same name on each host, runs the configured image with the merged ``runtime.args``, and starts an in-container ``sshd`` on port ``2224``. -- If ``container.launch`` is ``false``, CVS verifies that a container with the configured name is already running on every host and reuses it. +- With ``lifetime: per_run``, CVS removes any container with the same name on each host, runs the configured image with the merged ``runtime.args``, and starts an in-container ``sshd`` on port ``2224``. +- With ``lifetime: persistent``, CVS attaches to a container already running on every host (skipping ``sshd`` setup if it is already up), or starts one fresh if none is running. +- With ``lifetime: external``, CVS verifies that a container with the configured name is already running on every host and reuses it. - All ``rvs`` invocations are routed through the container via ``docker exec`` and the in-container ``sshd``. Step 4: Verify the run actually used the container @@ -100,14 +101,15 @@ You should see one running container per node with the configured name and image Lifecycle and teardown ====================== -The ``container.launch`` flag controls who owns the container lifecycle: +The ``container.lifetime`` policy controls who owns the container lifecycle: -- ``launch: true`` - CVS starts the long-running container as part of test setup. Teardown is a no-op; CVS does not stop the container so that you can inspect it after the run. Stop and remove it manually when you are done. -- ``launch: false`` - CVS does not start anything. It verifies that a container with the configured name is already running on every host and reuses it. Teardown is a no-op. +- ``per_run`` (default) - CVS starts a fresh container at setup and force-removes it at teardown. Anything written to the container overlay is lost when the run ends. +- ``persistent`` - CVS starts the container if it is not already running, otherwise attaches to it. Teardown is a no-op, so the container (and its overlay) survives across runs. This unblocks install-then-run workflows: ``cvs run install_rvs`` followed by ``cvs run rvs_cvs`` in separate invocations. Pin ``container.name`` so a tag bump does not silently abandon the overlay. +- ``external`` - CVS does not start anything. It verifies that a container with the configured name is already running on every host and reuses it. Teardown is a no-op. -When ``container.enabled`` is ``false``, both setup and teardown short-circuit to no-ops. See the launch truth table in :doc:`/reference/configuration-files/cluster-file` for the full state matrix. +See the ``lifetime`` truth table in :doc:`/reference/configuration-files/cluster-file` for the full state matrix. -To stop and remove a container started with ``launch: true``, run on every node: +To stop and remove a container that CVS left running (``persistent`` or ``external``), run on every node: .. code:: bash @@ -119,10 +121,9 @@ Replace ``cvs_container`` with the actual ``container.name`` from your cluster f Common pitfalls =============== -- **Forgot ``enabled: true``.** The container block is present but the orchestrator silently no-ops every lifecycle call. The first ``orch.exec`` call then errors with "no container running". - **Image without ``openssh-server``.** ``setup_sshd`` cannot start ``sshd`` on port ``2224`` and ``orch.exec`` fails to connect. Make sure the image installs ``openssh-server`` and exposes the binary at ``/usr/sbin/sshd``. - **Image without the workload binary.** ``cvs run rvs_cvs`` invokes ``rvs`` inside the container; if the image lacks ``/opt/rocm/bin/rvs`` the test fails with a "command not found" style error. -- **``launch: true`` plus a stale container with the same name.** On rerun, ``docker run`` would refuse to start because the name is taken. CVS's launch path issues ``docker rm -f`` before ``docker run`` to handle this; if you start a container outside CVS with the same name, stop it first. +- **``lifetime: persistent`` without a pinned ``name``.** The default container name is ``_``, which shifts when you bump the image tag. A tag bump silently abandons the previous container's overlay (installs, clones) and starts fresh. Pin ``container.name`` when using ``persistent``. - **Port ``2224`` already bound on the host.** With ``network: host`` the in-container ``sshd`` binds to ``2224`` in the host's network namespace. If something else on the host already listens on ``2224`` the bind fails. Stop the conflicting service. - **Picked the wrong cluster template.** Use ``cluster_container.json`` (with ``orchestrator: container``) for container mode. Using ``cluster.json`` runs the suite on the host even when the image you wanted to test is sitting on every node. diff --git a/docs/reference/configuration-files/cluster-file.rst b/docs/reference/configuration-files/cluster-file.rst index 4d48c59c..3b2b1ca3 100644 --- a/docs/reference/configuration-files/cluster-file.rst +++ b/docs/reference/configuration-files/cluster-file.rst @@ -77,8 +77,7 @@ Both templates share the same top-level shape. The ``container`` block and the ` }, "container": { - "enabled": true, - "launch": true, + "lifetime": "per_run", "image": "rocm/cvs:latest", "name": "cvs_container", "runtime": { @@ -136,12 +135,9 @@ The ``container`` block configures the container backend. It is consumed by the * - Configuration parameter - Default value - Description - * - ``enabled`` - - ``false`` - - Master switch. Must be ``true`` for the container backend to do anything. When ``false``, ``setup_containers`` and ``teardown_containers`` short-circuit to no-ops. - * - ``launch`` - - ``false`` - - When ``true``, CVS starts the long-running containers on every host as part of test setup. When ``false``, CVS only verifies that containers with the configured name are already running on every host and never stops them. + * - ``lifetime`` + - ``per_run`` + - Container lifecycle policy: ``external``, ``per_run``, or ``persistent``. See the truth table below. * - ``image`` - (required) - Image with the test dependencies (for example ``rvs``) pre-installed and an ``sshd`` you can start on port ``2224``. Must be present locally on each node or pullable from a reachable registry. @@ -199,35 +195,31 @@ When ``runtime.name`` is ``docker``, the keys below configure the underlying ``d - ``["memlock=-1"]`` (appended) - Per-process resource limits. ``memlock=-1`` is required for RDMA. -``launch`` truth table -====================== +``lifetime`` truth table +======================== -The ``launch`` flag interacts with ``enabled`` and the runtime-tracked ``container_id`` to decide whether ``teardown_containers`` actually stops anything. The behavior below is pinned by the unit test ``test_teardown_containers_short_circuits_when_launch_true`` in `cvs/core/orchestrators/unittests/test_container.py `_. +``setup_containers`` and ``teardown_containers`` branch on ``container.lifetime``. The behavior below is pinned by the per-lifetime unit tests in `cvs/core/orchestrators/unittests/test_container.py `_. .. list-table:: - :widths: 2 2 2 5 + :widths: 2 4 4 :header-rows: 1 - * - ``enabled`` - - ``launch`` - - ``container_id`` - - Behavior of ``teardown_containers`` - * - ``false`` - - any - - any - - Short-circuit (no-op, returns ``True``). - * - ``true`` - - ``true`` - - any - - Short-circuit. Containers are treated as externally managed; CVS does not stop them. - * - ``true`` - - ``false`` - - unset - - Short-circuit. Nothing was started. - * - ``true`` - - ``false`` - - set - - Calls the runtime's ``teardown_containers`` to stop and remove the container. + * - ``lifetime`` + - ``setup_containers`` + - ``teardown_containers`` + * - ``external`` + - Verify a container with the configured name is already running on every host; set ``container_id``. Never starts anything. + - No-op. CVS does not own externally managed containers. + * - ``per_run`` (default) + - Start a fresh container on every host (force-removing any stale same-named container first). + - Force-remove the container CVS started. + * - ``persistent`` + - Attach if the container is already running on every host (with a per-host image-SHA check; cross-host SHA skew is a hard error). Otherwise start fresh. Idempotent across runs. + - No-op. The container is left running for the next run; remove it yourself when done. + +.. note:: + + With ``lifetime: persistent``, pin ``container.name`` explicitly. The default ``_`` name shifts when you bump the image tag, silently abandoning the previous container's overlay. Prerequisites on each cluster node ================================== From 0ddd3365c451c812d8aac6dae05ad0d2a2e60ecf Mon Sep 17 00:00:00 2001 From: Atul Nair Date: Fri, 29 May 2026 20:30:26 -0400 Subject: [PATCH 07/20] orchestrators/scripts: add default container provisioning script Minimal apt script run inside a freshly-launched container to install openssh-server, so CVS's in-container sshd can start on base images that do not pre-ship it. Short-circuits if sshd is already present. --- .../scripts/default_container_setup.sh | 20 +++++++++++++++++++ 1 file changed, 20 insertions(+) create mode 100644 cvs/core/orchestrators/scripts/default_container_setup.sh diff --git a/cvs/core/orchestrators/scripts/default_container_setup.sh b/cvs/core/orchestrators/scripts/default_container_setup.sh new file mode 100644 index 00000000..5cb32cb8 --- /dev/null +++ b/cvs/core/orchestrators/scripts/default_container_setup.sh @@ -0,0 +1,20 @@ +#!/usr/bin/env bash +# Default CVS container provisioning script. +# +# Run inside each freshly-launched container (via `docker exec`) before +# setup_sshd. CVS's container exec model needs an in-container sshd on port +# 2224; many base images do not ship one. This installs only the sshd binary +# (openssh-server) so the existing setup_sshd can start `/usr/sbin/sshd -p2224`. +# +# Override with container.setup_script in the cluster file to install other +# packages (or to support non-apt base images -- this default is apt-only). +set -euo pipefail + +if command -v sshd >/dev/null 2>&1 || [ -x /usr/sbin/sshd ]; then + echo "default_container_setup: sshd already present, nothing to install" + exit 0 +fi + +export DEBIAN_FRONTEND=noninteractive +apt-get update +apt-get install -y --no-install-recommends openssh-server From 778b39b711183f554ef25e9d8d379d911edda9ab Mon Sep 17 00:00:00 2001 From: Atul Nair Date: Fri, 29 May 2026 20:30:31 -0400 Subject: [PATCH 08/20] orchestrators/factory: resolve and validate container.setup_script Add _resolve_container_setup_script alongside the lifetime resolver: a user-supplied path is made absolute and must exist (ValueError at config load otherwise); when absent/null/empty it falls back to the packaged default, which is itself existence-checked so a broken install fails fast rather than as an OSError mid-run. --- cvs/core/orchestrators/factory.py | 74 ++++++++++++++++++++++++++++--- 1 file changed, 67 insertions(+), 7 deletions(-) diff --git a/cvs/core/orchestrators/factory.py b/cvs/core/orchestrators/factory.py index b906c633..6bad78f0 100644 --- a/cvs/core/orchestrators/factory.py +++ b/cvs/core/orchestrators/factory.py @@ -5,6 +5,7 @@ All code contained here is Property of Advanced Micro Devices, Inc. ''' +import os import warnings from cvs.core.orchestrators.baremetal import BaremetalOrchestrator @@ -13,6 +14,14 @@ VALID_CONTAINER_LIFETIMES = ("external", "per_run", "persistent") +# Packaged default provisioning script, run inside each freshly-launched +# container when container.setup_script is not set. Installs openssh-server so +# the in-container sshd can start on port 2224. Resolved __file__-relative so it +# works in both editable and wheel installs. +DEFAULT_CONTAINER_SETUP_SCRIPT = os.path.join( + os.path.dirname(os.path.abspath(__file__)), "scripts", "default_container_setup.sh" +) + def _resolve_container_lifetime(container): """Normalize a container config block to a single resolved ``lifetime`` key. @@ -64,6 +73,49 @@ def _resolve_container_lifetime(container): return container +def _resolve_container_setup_script(container): + """Resolve ``container.setup_script`` to a concrete, existing file path. + + The script is run inside each freshly-launched container (see + ``ContainerOrchestrator._provision_container``) to install packages on top + of the base image. Resolution rules: + + - empty/absent container block (baremetal path) -> returned untouched. + - ``setup_script`` set -> validated to exist on + the control host; ``ValueError`` (fail fast at config load) if missing. + - ``setup_script`` absent -> defaults to the + packaged ``default_container_setup.sh`` (installs openssh-server). + + Relative paths are resolved against the current working directory of the + process running ``cvs``. Mutates and returns the passed dict. + """ + if not container: + return container + + setup_script = container.get('setup_script') + if setup_script: + resolved = os.path.abspath(os.path.expanduser(setup_script)) + if not os.path.isfile(resolved): + raise ValueError( + f"container.setup_script not found: {setup_script!r} " + f"(resolved to {resolved!r})" + ) + container['setup_script'] = resolved + return container + + # No user-supplied script: fall back to the packaged default. Validate it + # exists here (same fail-fast as a user path) so a broken/incomplete install + # surfaces at config load rather than as an OSError mid-run inside + # _provision_container. + if not os.path.isfile(DEFAULT_CONTAINER_SETUP_SCRIPT): + raise ValueError( + f"packaged default container setup script is missing: " + f"{DEFAULT_CONTAINER_SETUP_SCRIPT!r} (broken CVS install?)" + ) + container['setup_script'] = DEFAULT_CONTAINER_SETUP_SCRIPT + return container + + class OrchestratorConfig: """ Configuration for orchestrator creation. @@ -98,7 +150,8 @@ class OrchestratorConfig: } }, "image": "rocm/cvs:latest", - "name": "myuser_rocm_cvs_latest" + "name": "myuser_rocm_cvs_latest", + "setup_script": "/path/to/setup.sh" } ``` lifetime: Container lifecycle policy [default: 'per_run'] @@ -108,6 +161,11 @@ class OrchestratorConfig: - 'persistent' : start if absent / attach if present; never torn down by the run. Pin container.name explicitly under this mode (the default _ name shifts on tag bumps). + setup_script: Optional path to a shell script run inside each freshly + launched container (per_run, and persistent when cold-started) before + sshd setup, to install packages on top of the base image. Omit to use + the packaged default that installs openssh-server. A non-existent path + fails at config load. """ def __init__(self, **kwargs): @@ -132,11 +190,13 @@ def __init__(self, **kwargs): self.priv_key_file = kwargs['priv_key_file'] self.password = kwargs.get('password') self.head_node_dict = kwargs.get('head_node_dict', {}) - # Normalize the container block to a resolved 'lifetime'. This is the - # single chokepoint: from_configs constructs via cls(**required_config), - # so both file-driven and direct programmatic construction hit the same - # normalization (and the same enabled-removed / launch-deprecated errors). - self.container = _resolve_container_lifetime(kwargs.get('container', {})) + # Normalize the container block. This is the single chokepoint: + # from_configs constructs via cls(**required_config), so both file-driven + # and direct programmatic construction hit the same normalization (and the + # same enabled-removed / launch-deprecated errors, and the same + # setup_script validation / default injection). + container = _resolve_container_lifetime(kwargs.get('container', {})) + self.container = _resolve_container_setup_script(container) def get(self, key, default=None): """Get configuration value with default.""" @@ -161,7 +221,7 @@ def from_configs(cls, cluster_config, testsuite_config=None): Required keys: orchestrator, node_dict, username, priv_key_file Optional keys: container, head_node_dict, password (defaults provided for missing optional keys) - Container structure: {lifetime: 'external'|'per_run'|'persistent', runtime: {name: str, args: dict}, image: str, name: str, ...} + Container structure: {lifetime: 'external'|'per_run'|'persistent', runtime: {name: str, args: dict}, image: str, name: str, setup_script: str, ...} testsuite_config: Test suite specific configuration (dict or path to _config.json) Can override any keys from cluster_config From f3d2b665d36574656d90930750705d4538a5cdc1 Mon Sep 17 00:00:00 2001 From: Atul Nair Date: Fri, 29 May 2026 20:31:10 -0400 Subject: [PATCH 09/20] orchestrators/container: provision launched containers via setup_script After a fresh launch (per_run, or persistent cold-start), run the resolved setup_script inside each container via docker exec before setup_sshd, so base images lacking sshd/packages are brought up to spec. external and persistent-attach skip provisioning (idempotent across runs). The script is shipped base64-encoded inline; oversized scripts and read errors fail with a clear message, and every failing host's output is logged. --- cvs/core/orchestrators/container.py | 80 ++++++++++++++++++++++++++++- 1 file changed, 79 insertions(+), 1 deletion(-) diff --git a/cvs/core/orchestrators/container.py b/cvs/core/orchestrators/container.py index df22130f..aab01f45 100644 --- a/cvs/core/orchestrators/container.py +++ b/cvs/core/orchestrators/container.py @@ -6,6 +6,7 @@ ''' from cvs.core.orchestrators.baremetal import BaremetalOrchestrator +import base64 import getpass import re from cvs.core.runtimes import RuntimeFactory @@ -42,6 +43,14 @@ "privileged": True, } +# Upper bound on a setup_script that can be delivered inline. _provision_container +# ships the script base64-encoded inside the `docker exec ... bash -c` command +# string, which rides the SSH exec channel (bounded to ~30 KB by libssh2 in +# cvs/lib/parallel/pssh.py). base64 inflates by ~4/3, so cap the raw script well +# under that so a too-large script fails with a clear message instead of an +# opaque SSH/exec truncation error. +MAX_INLINE_SETUP_SCRIPT_BYTES = 16384 + class ContainerOrchestrator(BaremetalOrchestrator): """ @@ -363,7 +372,7 @@ def _launch_containers( # Add InfiniBand device discovery via shell expansion (per-host) ib_device_expansion = '$(for dev in /dev/infiniband/*; do echo -n "--device $dev:$dev "; done)' - return self.runtime.setup_containers( + launched = self.runtime.setup_containers( modified_config, container_name, volumes=volumes, @@ -375,6 +384,75 @@ def _launch_containers( ulimits=ulimits, device_expansion=ib_device_expansion, ) + if not launched: + return False + + # Provision the freshly-launched container (install packages on top of + # the base image, e.g. openssh-server). Runs only on this fresh-start + # path, so 'external' and 'persistent'-attach skip it automatically. + return self._provision_container() + + def _provision_container(self): + """Run the configured setup_script inside the freshly-launched container. + + Reads the resolved ``container.setup_script`` on the control host, + base64-encodes it, and executes it inside the container on every host via + ``docker exec`` (``self.exec``), which works before sshd exists -- the same + mechanism ``setup_sshd`` uses. The default script installs + ``openssh-server`` so the subsequent ``setup_sshd`` can start sshd; a + user-supplied script can install anything else the base image lacks. + + Returns: + bool: True if provisioning succeeded on all hosts (or no script is + configured), False otherwise. + """ + setup_script = self.container_config.get('setup_script') + if not setup_script: + # Defensive: the factory always resolves a default for container + # configs, so this only triggers for a hand-built config dict. + return True + + try: + with open(setup_script, 'rb') as f: + script_bytes = f.read() + except OSError as exc: + self.log.error(f"Cannot read container setup_script {setup_script!r}: {exc}") + return False + + if len(script_bytes) > MAX_INLINE_SETUP_SCRIPT_BYTES: + # Delivered inline over the SSH exec channel; a too-large script would + # otherwise fail with an opaque truncation error mid-run. + self.log.error( + f"container.setup_script {setup_script!r} is too large for inline " + f"delivery ({len(script_bytes)} bytes > {MAX_INLINE_SETUP_SCRIPT_BYTES}). " + f"Slim the script, or bake the packages into the image." + ) + return False + + self.log.info(f"Provisioning containers via setup_script: {setup_script}") + encoded = base64.b64encode(script_bytes).decode('ascii') + # docker exec already wraps the command in `bash -c`, so decode the + # script and pipe it straight into bash. base64 sidesteps all quoting and + # newline issues with arbitrary script content over pssh. + cmd = f"echo {encoded} | base64 -d | bash" + result = self.exec(cmd, timeout=600, detailed=True) + + ok = True + for hostname, output in result.items(): + if output.get('exit_code') != 0: + # Surface the in-container stderr/stdout so a failed apt/bash is + # diagnosable from the log without re-running by hand. + detail = (output.get('output') or '').strip() + self.log.error( + f"Container provisioning failed on {hostname} " + f"(setup_script: {setup_script}, exit_code: {output.get('exit_code')})" + + (f": {detail}" if detail else "") + ) + ok = False + if not ok: + return False + self.log.info("Container provisioning succeeded on all hosts") + return True def _verify_persistent_image(self, container_name, image): """Compare the running container's image SHA to the local image tag on From 11218e3776ee506f8fb83160098954510ab82d2c Mon Sep 17 00:00:00 2001 From: Atul Nair Date: Fri, 29 May 2026 20:31:33 -0400 Subject: [PATCH 10/20] orchestrators/container: fix setup_sshd pgrep precheck matching its parent shell pgrep -f 'sshd.*2224' matched its own parent shell, whose argv contains the pattern, so the precheck always reported sshd already running and skipped starting it (and the post-start validation always passed). Use the [s]shd character-class trick at both sites so they match the real daemon only. --- cvs/core/orchestrators/container.py | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/cvs/core/orchestrators/container.py b/cvs/core/orchestrators/container.py index aab01f45..bd8e34f5 100644 --- a/cvs/core/orchestrators/container.py +++ b/cvs/core/orchestrators/container.py @@ -513,7 +513,10 @@ def setup_sshd(self): # `cvs run` the container's sshd is already bound to 2224, so re-running # `/usr/sbin/sshd -p2224` would fail with "address already in use". Skip # the setup commands on any host that already has sshd on 2224. - precheck = self.exec("pgrep -f 'sshd.*2224' > /dev/null 2>&1", timeout=10, detailed=True) + # Pattern uses the `[s]shd` trick so pgrep -f does not match its own + # parent shell (whose argv contains the pattern); a literal 'sshd.*2224' + # self-matches and makes this precheck always report "already running". + precheck = self.exec("pgrep -f '[s]shd.*2224' > /dev/null 2>&1", timeout=10, detailed=True) hosts_needing_sshd = [host for host, output in precheck.items() if output['exit_code'] != 0] if not hosts_needing_sshd: self.log.info("SSH daemon already running on all hosts, skipping setup") @@ -543,8 +546,9 @@ def setup_sshd(self): time.sleep(2) - # Validate sshd is running by checking process - check_cmd = "pgrep -f 'sshd.*2224' > /dev/null 2>&1" + # Validate sshd is running by checking process. Same `[s]shd` trick as the + # precheck so this validates the real sshd, not pgrep's own parent shell. + check_cmd = "pgrep -f '[s]shd.*2224' > /dev/null 2>&1" result = self.exec(check_cmd, timeout=10, detailed=True) # Check if all hosts have sshd running From dadc82b21456ce9ce7d53d62e00f71432fb15a49 Mon Sep 17 00:00:00 2001 From: Atul Nair Date: Fri, 29 May 2026 20:31:44 -0400 Subject: [PATCH 11/20] input/cluster_file: document and sample container.setup_script Add the optional setup_script key to cluster_container.json and the schema README, and drop the requirement that the image pre-ship openssh-server (now installed at launch by the default setup_script). Note the inline delivery constraints (bash/base64 in the image, ~16 KB raw cap) and that null/omit both mean "use the packaged default". --- cvs/input/cluster_file/README.md | 7 ++++--- cvs/input/cluster_file/cluster_container.json | 5 ++++- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/cvs/input/cluster_file/README.md b/cvs/input/cluster_file/README.md index 45e3aed9..fa4f558e 100644 --- a/cvs/input/cluster_file/README.md +++ b/cvs/input/cluster_file/README.md @@ -44,8 +44,9 @@ Consumed by `ContainerOrchestrator` in [`cvs/core/orchestrators/container.py`](. | Key | Type | Default | Purpose | | --- | --- | --- | --- | | `lifetime` | str | `"per_run"` | Container lifecycle policy: `external`, `per_run`, or `persistent`. See the truth table below. | -| `image` | str | (required) | Image with the test dependencies (rvs, etc.) pre-installed and an sshd you can start on port 2224. Must be present locally on each node OR pullable from a reachable registry. | +| `image` | str | (required) | Image with the test dependencies (rvs, etc.) the suite invokes. Must be present locally on each node OR pullable from a reachable registry. An in-container sshd is **not** required up front -- `setup_script` installs it if missing. | | `name` | str | (required) | Container name on each host. For parallel runs make this per-iteration unique (e.g. `cvs_iter_`). | +| `setup_script` | str | (packaged default) | Optional path to a shell script run inside each freshly-launched container (before sshd setup) to install packages on top of the base image. `null` or omitting the key both use the packaged default that installs `openssh-server` only (there is no value that disables provisioning). A non-existent path fails at config load. Delivered inline via `docker exec` (so the base image needs `bash` + `base64`, and the script must stay under ~16 KB); the default is apt-based and runs as the container's exec user (root) -- non-apt images need a custom script. | | `runtime.name` | str | `"docker"` | Container runtime. Supported: `docker` (concrete), `enroot` (stub). | | `runtime.args` | dict | `{}` | Backend-specific runtime arguments (see below). Defaults from `DEFAULT_CONTAINER_ARGS` in [`cvs/core/orchestrators/container.py`](../../core/orchestrators/container.py) apply for any key omitted here. | @@ -84,7 +85,7 @@ RDMA-ready container; the keys below are only needed to extend or override. - **Docker** installed and the SSH user has passwordless `sudo docker`. - **Host driver** loaded so `/dev/kfd` and `/dev/dri/*` (and `/dev/infiniband/*` if RDMA is in scope) are present for passthrough. - **`~/.ssh/`** of the SSH user is reachable; the orchestrator mounts it as `/host_ssh` and copies keys into `/root/.ssh` inside the container so `setup_sshd` can start an in-container sshd on port 2224. -- **The image** is either pre-loaded on every node (`docker load`) or pullable from a reachable registry. Inside the image you need: `openssh-server` (for the in-container sshd), the workload binaries the suite invokes (e.g. `/opt/rocm/bin/rvs`), and any ROCm runtime libs the workload needs. +- **The image** is either pre-loaded on every node (`docker load`) or pullable from a reachable registry. Inside the image you need: the workload binaries the suite invokes (e.g. `/opt/rocm/bin/rvs`) and any ROCm runtime libs the workload needs. `openssh-server` is **not** required in the image -- CVS provisions it at launch via `container.setup_script` (the packaged default installs it). On a non-apt base image, supply a custom `setup_script`. ## What happens when you run a backend-blind suite @@ -136,7 +137,7 @@ file changes. The `orch` fixture in [`cvs/tests/health/rvs_cvs.py`](../../tests/ ## Common pitfalls -- **Image without `openssh-server`**. `setup_sshd` cannot start sshd on port 2224; `orch.exec` then fails to connect. Make sure your image installs `openssh-server` and exposes the binary at `/usr/sbin/sshd`. +- **Base image cannot install `openssh-server`**. The default `setup_script` runs `apt-get install openssh-server` inside the container so `setup_sshd` can start sshd on port 2224. If the base image has no apt / no network to a package mirror, that install fails and `orch.exec` cannot connect. Fixes: bake `openssh-server` into the image, or point `container.setup_script` at a script that installs sshd the way that image expects (e.g. `dnf`/`microdnf`). - **Image without the workload binary**. `cvs run rvs_cvs` will execute `rvs` inside the container; if the image lacks `/opt/rocm/bin/rvs` the test fails with a `command not found` flavor error. - **`lifetime: persistent` without a pinned `name`**. The default container name is `_`, which shifts when you bump the image tag. A tag bump silently abandons the previous container's overlay (installs, clones) and starts fresh. Pin `container.name` explicitly when using `persistent`. - **Port 2224 collision on the host**. With `network: host` the in-container sshd binds to 2224 in the host's network namespace. If something else on the host already listens on 2224 the bind fails. Stop the conflicting service or change the in-image sshd port (and update the orchestrator's port to match). diff --git a/cvs/input/cluster_file/cluster_container.json b/cvs/input/cluster_file/cluster_container.json index bd06a722..a7c65a7f 100644 --- a/cvs/input/cluster_file/cluster_container.json +++ b/cvs/input/cluster_file/cluster_container.json @@ -34,12 +34,15 @@ "_lifetime_comment": "Container lifecycle policy. 'external' = containers managed outside CVS (verify-only, never stopped). 'per_run' = CVS starts on setup and removes on teardown. 'persistent' = CVS starts if absent / attaches if present and leaves running on exit (unblocks install-then-run; pin 'name' explicitly).", "lifetime": "per_run", - "_image_comment": "Image with test dependencies (rvs, etc.) pre-installed and an sshd you can start on port 2224. Must be present locally on each node OR pullable from a reachable registry.", + "_image_comment": "Image with the test dependencies (rvs, etc.) the suite invokes. Must be present locally on each node OR pullable from a reachable registry. An in-container sshd is NOT required up front -- it is installed by setup_script (see below) if missing.", "image": "rocm/cvs:latest", "_name_comment": "Container name on each host. For parallel runs make this per-iteration unique (e.g. cvs_iter_).", "name": "cvs_container", + "_setup_script_comment": "Optional path to a shell script run inside each freshly-launched container (before sshd setup) to install packages on top of the base image. null or omitting this key both use the packaged default that installs openssh-server (there is no 'disable provisioning' value). A non-existent path fails at config load. Delivered inline via docker exec, so the base image needs bash and base64; the default script is apt-based and runs as the container's exec user (root).", + "setup_script": null, + "runtime": { "_name_comment": "Supported: 'docker' (concrete), 'enroot' (stub).", "name": "docker", From 8fcfec9acba4be7ef62369e21eb146fa551d4b85 Mon Sep 17 00:00:00 2001 From: Atul Nair Date: Fri, 29 May 2026 20:31:49 -0400 Subject: [PATCH 12/20] docs: document container.setup_script in cluster-file references Update the how-to and cluster-file reference: add the setup_script field, describe the launch-time provisioning step, and soften the "image must contain openssh-server" prerequisite/pitfall. --- docs/how-to/run-with-containers.rst | 11 ++++++----- docs/reference/configuration-files/cluster-file.rst | 8 ++++++-- 2 files changed, 12 insertions(+), 7 deletions(-) diff --git a/docs/how-to/run-with-containers.rst b/docs/how-to/run-with-containers.rst index 44d5b765..b978a0d8 100644 --- a/docs/how-to/run-with-containers.rst +++ b/docs/how-to/run-with-containers.rst @@ -18,7 +18,7 @@ On every cluster node: - **Docker** installed, with passwordless ``sudo docker`` for the SSH user. - **Host driver** loaded so ``/dev/kfd``, ``/dev/dri/*``, and ``/dev/infiniband/*`` (when RDMA is in scope) are present for passthrough. - **SSH user home directory** reachable. The orchestrator mounts ``~/.ssh`` as ``/host_ssh`` inside the container so that the in-container ``sshd`` on port ``2224`` can authenticate. -- **Container image** either pre-loaded on every node (``docker load``) or pullable from a reachable registry. The image must contain ``openssh-server`` and the workload binaries the suite invokes (for example ``/opt/rocm/bin/rvs``). +- **Container image** either pre-loaded on every node (``docker load``) or pullable from a reachable registry. The image must contain the workload binaries the suite invokes (for example ``/opt/rocm/bin/rvs``). It does **not** need ``openssh-server`` baked in -- CVS provisions it at launch via ``container.setup_script`` (the packaged default installs it; a non-apt base image needs a custom script). On the head node where you launch ``cvs run``: @@ -48,8 +48,9 @@ Open the copied file and edit: - ``{user-id}``: your SSH user (or leave it for runtime resolution). - ``priv_key_file``: absolute path to your SSH private key. - ``head_node_dict.mgmt_ip`` and the keys of ``node_dict``: real IPs or hostnames of your cluster nodes. ``mgmt_ip`` must equal one of the keys in ``node_dict``. -- ``container.image``: an image present on every node or pullable from a reachable registry. The image must include ``openssh-server`` and the workload binary (for example ``rvs``). +- ``container.image``: an image present on every node or pullable from a reachable registry. The image must include the workload binary (for example ``rvs``); ``openssh-server`` is installed at launch by ``setup_script``, not required in the image. - ``container.name``: container name on each host. For parallel runs make this per-iteration unique (for example ``cvs_iter_``). Pin it explicitly when using ``lifetime: persistent``. +- ``container.setup_script`` (optional): path to a shell script run inside each freshly-launched container before sshd setup, to install packages on top of the base image. Omit to use the packaged default that installs ``openssh-server``. - ``container.lifetime``: ``external``, ``per_run``, or ``persistent``. See the lifecycle note below. For the full schema and runtime argument reference, see :doc:`/reference/configuration-files/cluster-file`. @@ -73,8 +74,8 @@ Run ``rvs_cvs`` the same way you run any CVS test suite, but with the container What happens during the run: - The ``orch`` fixture in ``rvs_cvs`` reads ``orchestrator: container`` from the cluster file and constructs a ``ContainerOrchestrator``. -- With ``lifetime: per_run``, CVS removes any container with the same name on each host, runs the configured image with the merged ``runtime.args``, and starts an in-container ``sshd`` on port ``2224``. -- With ``lifetime: persistent``, CVS attaches to a container already running on every host (skipping ``sshd`` setup if it is already up), or starts one fresh if none is running. +- With ``lifetime: per_run``, CVS removes any container with the same name on each host, runs the configured image with the merged ``runtime.args``, runs ``container.setup_script`` inside the new container (default: install ``openssh-server``), and starts an in-container ``sshd`` on port ``2224``. +- With ``lifetime: persistent``, CVS attaches to a container already running on every host (skipping provisioning and ``sshd`` setup if it is already up), or starts one fresh -- provisioning it -- if none is running. - With ``lifetime: external``, CVS verifies that a container with the configured name is already running on every host and reuses it. - All ``rvs`` invocations are routed through the container via ``docker exec`` and the in-container ``sshd``. @@ -121,7 +122,7 @@ Replace ``cvs_container`` with the actual ``container.name`` from your cluster f Common pitfalls =============== -- **Image without ``openssh-server``.** ``setup_sshd`` cannot start ``sshd`` on port ``2224`` and ``orch.exec`` fails to connect. Make sure the image installs ``openssh-server`` and exposes the binary at ``/usr/sbin/sshd``. +- **Base image cannot install ``openssh-server``.** The default ``setup_script`` runs ``apt-get install openssh-server`` inside the container so ``setup_sshd`` can start ``sshd`` on port ``2224``. If the base image has no apt or no network to a package mirror, that install fails and ``orch.exec`` cannot connect. Bake ``openssh-server`` into the image, or point ``container.setup_script`` at a script that installs it the way that image expects. - **Image without the workload binary.** ``cvs run rvs_cvs`` invokes ``rvs`` inside the container; if the image lacks ``/opt/rocm/bin/rvs`` the test fails with a "command not found" style error. - **``lifetime: persistent`` without a pinned ``name``.** The default container name is ``_``, which shifts when you bump the image tag. A tag bump silently abandons the previous container's overlay (installs, clones) and starts fresh. Pin ``container.name`` when using ``persistent``. - **Port ``2224`` already bound on the host.** With ``network: host`` the in-container ``sshd`` binds to ``2224`` in the host's network namespace. If something else on the host already listens on ``2224`` the bind fails. Stop the conflicting service. diff --git a/docs/reference/configuration-files/cluster-file.rst b/docs/reference/configuration-files/cluster-file.rst index 3b2b1ca3..bfc4f9b5 100644 --- a/docs/reference/configuration-files/cluster-file.rst +++ b/docs/reference/configuration-files/cluster-file.rst @@ -80,6 +80,7 @@ Both templates share the same top-level shape. The ``container`` block and the ` "lifetime": "per_run", "image": "rocm/cvs:latest", "name": "cvs_container", + "setup_script": null, "runtime": { "name": "docker", "args": { @@ -140,10 +141,13 @@ The ``container`` block configures the container backend. It is consumed by the - Container lifecycle policy: ``external``, ``per_run``, or ``persistent``. See the truth table below. * - ``image`` - (required) - - Image with the test dependencies (for example ``rvs``) pre-installed and an ``sshd`` you can start on port ``2224``. Must be present locally on each node or pullable from a reachable registry. + - Image with the test dependencies (for example ``rvs``) the suite invokes. Must be present locally on each node or pullable from a reachable registry. An in-container ``sshd`` is not required up front; it is installed at launch by ``setup_script``. * - ``name`` - (required) - Container name on each host. For parallel runs, make this per-iteration unique (for example ``cvs_iter_``). + * - ``setup_script`` + - (packaged default) + - Optional path to a shell script run inside each freshly-launched container (before sshd setup) to install packages on top of the base image. Omit to use the packaged default that installs ``openssh-server`` only. A non-existent path fails at config load. apt-based; non-apt images need a custom script. * - ``runtime.name`` - ``docker`` - Container runtime. Today only ``docker`` is implemented. ``enroot`` is registered as a stub and is not yet functional. @@ -229,7 +233,7 @@ To use the container backend, every cluster node must have: - **Docker installed** with passwordless ``sudo docker`` for the SSH user. - **Host driver loaded** so ``/dev/kfd``, ``/dev/dri/*``, and ``/dev/infiniband/*`` (when RDMA is in scope) are present for passthrough. - **SSH user home directory accessible**. The orchestrator mounts ``~/.ssh`` as ``/host_ssh`` and copies keys into ``/root/.ssh`` inside the container so that the in-container ``sshd`` on port ``2224`` can authenticate. -- **Container image** either pre-loaded on every node (``docker load``) or pullable from a reachable registry. The image must contain ``openssh-server`` (for the in-container ``sshd``) and the workload binaries the suite invokes (for example ``/opt/rocm/bin/rvs``). +- **Container image** either pre-loaded on every node (``docker load``) or pullable from a reachable registry. The image must contain the workload binaries the suite invokes (for example ``/opt/rocm/bin/rvs``). ``openssh-server`` (for the in-container ``sshd``) is not required in the image -- it is installed at launch by ``container.setup_script``. See also ======== From 87b9065235669bd02cdd520401fb35a90cd7dd14 Mon Sep 17 00:00:00 2001 From: Atul Nair Date: Fri, 29 May 2026 20:31:54 -0400 Subject: [PATCH 13/20] orchestrators/unittests: cover container.setup_script resolution Test _resolve_container_setup_script: falsy (absent/null/empty) -> packaged default, missing user path raises, relative/tilde paths resolve to abspath, the packaged default is present, and a missing default raises. --- .../orchestrators/unittests/test_factory.py | 121 +++++++++++++++++- 1 file changed, 120 insertions(+), 1 deletion(-) diff --git a/cvs/core/orchestrators/unittests/test_factory.py b/cvs/core/orchestrators/unittests/test_factory.py index bfa082eb..75ccb72b 100644 --- a/cvs/core/orchestrators/unittests/test_factory.py +++ b/cvs/core/orchestrators/unittests/test_factory.py @@ -15,7 +15,12 @@ import unittest from unittest.mock import MagicMock, patch -from cvs.core.orchestrators.factory import OrchestratorConfig, OrchestratorFactory +from cvs.core.orchestrators.factory import ( + DEFAULT_CONTAINER_SETUP_SCRIPT, + OrchestratorConfig, + OrchestratorFactory, + _resolve_container_setup_script, +) from cvs.core.orchestrators.baremetal import BaremetalOrchestrator from cvs.core.orchestrators.container import ContainerOrchestrator @@ -250,5 +255,119 @@ def test_from_configs_does_not_trip_changeme_in_testsuite(self): self.assertEqual(cfg.username, "literal_user") +class TestResolveContainerSetupScript(unittest.TestCase): + """container.setup_script default injection + path validation.""" + + def test_empty_block_untouched(self): + # Baremetal path: empty container block stays empty (no script injected). + self.assertEqual(_resolve_container_setup_script({}), {}) + + def test_falsy_setup_script_defaults(self): + # absent / None / "" are all falsy -> packaged default (no disable value). + for name, container in [ + ("absent", {"image": "x"}), + ("none", {"image": "x", "setup_script": None}), + ("empty", {"image": "x", "setup_script": ""}), + ]: + with self.subTest(name): + out = _resolve_container_setup_script(dict(container)) + self.assertEqual(out["setup_script"], DEFAULT_CONTAINER_SETUP_SCRIPT) + + def test_packaged_default_script_exists_on_disk(self): + # The default must actually be present (packaged + __file__-relative), + # otherwise every container run with no setup_script would fail to read it. + self.assertTrue(os.path.isfile(DEFAULT_CONTAINER_SETUP_SCRIPT)) + + def test_existing_user_path_kept_as_absolute(self): + with tempfile.NamedTemporaryFile("w", suffix=".sh", delete=False) as f: + f.write("#!/bin/bash\ntrue\n") + path = f.name + try: + out = _resolve_container_setup_script({"image": "x", "setup_script": path}) + self.assertEqual(out["setup_script"], os.path.abspath(path)) + finally: + os.unlink(path) + + def test_missing_user_path_raises(self): + with self.assertRaises(ValueError) as ctx: + _resolve_container_setup_script( + {"image": "x", "setup_script": "/no/such/setup_script.sh"} + ) + self.assertIn("setup_script", str(ctx.exception)) + + def test_missing_packaged_default_raises(self): + # The F2 fix branch: a broken install whose packaged default is missing + # must fail fast at resolution (ValueError), not as an OSError mid-run. + with patch( + "cvs.core.orchestrators.factory.DEFAULT_CONTAINER_SETUP_SCRIPT", + "/nonexistent/default_container_setup.sh", + ): + with self.assertRaises(ValueError) as ctx: + _resolve_container_setup_script({"image": "x"}) + self.assertIn("default", str(ctx.exception).lower()) + + def test_relative_user_path_resolved_to_abspath(self): + # A relative setup_script is resolved against the cwd and stored absolute. + with tempfile.NamedTemporaryFile("w", suffix=".sh", delete=False) as f: + f.write("#!/bin/bash\ntrue\n") + path = f.name + try: + rel = os.path.relpath(path, os.getcwd()) + out = _resolve_container_setup_script({"image": "x", "setup_script": rel}) + self.assertTrue(os.path.isabs(out["setup_script"])) + self.assertEqual(out["setup_script"], os.path.abspath(path)) + finally: + os.unlink(path) + + def test_tilde_user_path_is_expanded(self): + # A ~ path is expanded before the existence check (proven via the resolved + # path in the error message since the file does not exist). + with self.assertRaises(ValueError) as ctx: + _resolve_container_setup_script( + {"image": "x", "setup_script": "~/__cvs_missing_setup__.sh"} + ) + msg = str(ctx.exception) + self.assertIn(os.path.expanduser("~"), msg) + self.assertNotIn("~", msg.split("resolved to", 1)[-1]) + + @patch("cvs.core.orchestrators.baremetal.Pssh") + def test_orchestratorconfig_init_injects_default_setup_script(self, _mock_pssh): + # Direct construction routes through __init__ -> the same resolver, so a + # container config with no setup_script gets the packaged default. + cfg = OrchestratorConfig( + orchestrator="container", + node_dict={"1.1.1.1": {}}, + username="u", + priv_key_file="/dev/null", + container={"lifetime": "per_run", "image": "x"}, + ) + self.assertEqual(cfg.container["setup_script"], DEFAULT_CONTAINER_SETUP_SCRIPT) + + @patch("cvs.core.orchestrators.baremetal.Pssh") + def test_from_configs_null_setup_script_defaults(self, _mock_pssh): + # JSON null (Python None) for setup_script is treated like "absent" and + # resolves to the packaged default -- not "disable provisioning". + cluster = { + "orchestrator": "container", + "node_dict": {"1.1.1.1": {}}, + "username": "u", + "priv_key_file": "/dev/null", + "container": {"lifetime": "per_run", "image": "x", "setup_script": None}, + } + cfg = OrchestratorConfig.from_configs(cluster) + self.assertEqual(cfg.container["setup_script"], DEFAULT_CONTAINER_SETUP_SCRIPT) + + @patch("cvs.core.orchestrators.baremetal.Pssh") + def test_baremetal_init_does_not_inject_setup_script(self, _mock_pssh): + # Empty container block (baremetal) must not gain a setup_script key. + cfg = OrchestratorConfig( + orchestrator="baremetal", + node_dict={"1.1.1.1": {}}, + username="u", + priv_key_file="/dev/null", + ) + self.assertEqual(cfg.container, {}) + + if __name__ == "__main__": unittest.main() From aed4df145d8bb467673f50332ae6d7428877053f Mon Sep 17 00:00:00 2001 From: Atul Nair Date: Fri, 29 May 2026 20:32:00 -0400 Subject: [PATCH 14/20] orchestrators/unittests: cover container provisioning and sshd pgrep guard Add provisioning coverage (fresh-launch dispatch matrix, size guard, payload bytes, read/oversize failures, all-host failure logging) and pin the [s]shd precheck/validation pattern. Restructure with setUp-based patching and subTest tables in place of the per-method @patch boilerplate. --- .../orchestrators/unittests/test_container.py | 357 ++++++++++++------ 1 file changed, 252 insertions(+), 105 deletions(-) diff --git a/cvs/core/orchestrators/unittests/test_container.py b/cvs/core/orchestrators/unittests/test_container.py index 08c4d6eb..bd81e083 100644 --- a/cvs/core/orchestrators/unittests/test_container.py +++ b/cvs/core/orchestrators/unittests/test_container.py @@ -11,14 +11,35 @@ # so tests run with no SSH or container runtime. # # The per-lifetime setup/teardown behavior is pinned here so any future change to the -# external / per_run / persistent contract has a loud canary. +# external / per_run / persistent contract has a loud canary. Pssh + RuntimeFactory are +# patched once in setUp (not per method); _make() returns a fresh orch + runtime mock. +import base64 import unittest import warnings -from unittest.mock import MagicMock, patch +from unittest.mock import MagicMock, mock_open, patch from cvs.core.orchestrators.factory import OrchestratorConfig, _resolve_container_lifetime -from cvs.core.orchestrators.container import ContainerOrchestrator +from cvs.core.orchestrators.container import ContainerOrchestrator, MAX_INLINE_SETUP_SCRIPT_BYTES + + +# Reusable runtime.exec / is_running / image_sha_status fixtures (two-host cluster). +_OK = { + "10.0.0.1": {"exit_code": 0, "output": ""}, + "10.0.0.2": {"exit_code": 0, "output": ""}, +} +_RUNNING = { + "10.0.0.1": {"running": True, "exit_code": 0, "name": "cvs_iter_test"}, + "10.0.0.2": {"running": True, "exit_code": 0, "name": "cvs_iter_test"}, +} +_NOT_RUNNING = { + "10.0.0.1": {"running": False, "exit_code": 0, "name": ""}, + "10.0.0.2": {"running": False, "exit_code": 0, "name": ""}, +} +_SHA_MATCH = { + "10.0.0.1": {"container_sha": "sha:abc", "image_sha": "sha:abc", "exit_code": 0}, + "10.0.0.2": {"container_sha": "sha:abc", "image_sha": "sha:abc", "exit_code": 0}, +} def _make_orch_config(lifetime="per_run"): @@ -41,97 +62,108 @@ def _make_orch_config(lifetime="per_run"): class TestContainerOrchestrator(unittest.TestCase): - def _make(self, _mock_pssh, _mock_rf, lifetime="per_run"): + def setUp(self): + # Patch SSH transport + runtime factory for every test (replaces the + # per-method @patch decorators). Mocks are torn down via addCleanup. + p_pssh = patch("cvs.core.orchestrators.baremetal.Pssh") + p_rf = patch("cvs.core.orchestrators.container.RuntimeFactory") + self.mock_pssh = p_pssh.start() + self.mock_rf = p_rf.start() + self.addCleanup(p_pssh.stop) + self.addCleanup(p_rf.stop) + + def _make(self, lifetime="per_run"): cfg = _make_orch_config(lifetime=lifetime) runtime = MagicMock(name="docker_runtime") - _mock_rf.create.return_value = runtime + self.mock_rf.create.return_value = runtime orch = ContainerOrchestrator(MagicMock(), cfg) return orch, runtime - @patch("cvs.core.orchestrators.container.RuntimeFactory") - @patch("cvs.core.orchestrators.baremetal.Pssh") - def test_init_creates_runtime_via_factory(self, _mock_pssh, mock_rf): - orch, runtime = self._make(_mock_pssh, mock_rf) + # ------------------------------------------------------------------ + # construction + # ------------------------------------------------------------------ + + def test_init_creates_runtime_via_factory(self): + orch, runtime = self._make() self.assertIs(orch.runtime, runtime) # docker is the default runtime when container.runtime.name == "docker". - mock_rf.create.assert_called_once() - self.assertEqual(mock_rf.create.call_args[0][0], "docker") + self.mock_rf.create.assert_called_once() + self.assertEqual(self.mock_rf.create.call_args[0][0], "docker") - @patch("cvs.core.orchestrators.container.RuntimeFactory") - @patch("cvs.core.orchestrators.baremetal.Pssh") - def test_init_sets_orchestrator_type(self, _mock_pssh, mock_rf): - orch, _ = self._make(_mock_pssh, mock_rf) + def test_init_sets_orchestrator_type(self): + orch, _ = self._make() self.assertEqual(orch.orchestrator_type, "container") - @patch("cvs.core.orchestrators.container.RuntimeFactory") - @patch("cvs.core.orchestrators.baremetal.Pssh") - def test_init_overrides_ssh_port_to_container_sshd(self, _mock_pssh, mock_rf): - orch, _ = self._make(_mock_pssh, mock_rf) + def test_init_overrides_ssh_port_to_container_sshd(self): + orch, _ = self._make() self.assertEqual(orch.ssh_port, 2224) + def test_init_requires_container_config(self): + # ContainerOrchestrator raises if 'container' config is empty. + cfg = OrchestratorConfig( + orchestrator="container", + node_dict={"10.0.0.1": {}}, + username="testuser", + priv_key_file="/dev/null", + container={}, + ) + with self.assertRaises(ValueError): + ContainerOrchestrator(MagicMock(), cfg) + # ------------------------------------------------------------------ # setup_containers lifetime branching # ------------------------------------------------------------------ - @patch("cvs.core.orchestrators.container.RuntimeFactory") - @patch("cvs.core.orchestrators.baremetal.Pssh") - def test_setup_containers_per_run_delegates_to_runtime(self, _mock_pssh, mock_rf): - orch, runtime = self._make(_mock_pssh, mock_rf, lifetime="per_run") + def test_setup_containers_per_run_delegates_to_runtime(self): + orch, runtime = self._make(lifetime="per_run") runtime.setup_containers.return_value = True - result = orch.setup_containers() - self.assertTrue(result) + # The fresh-launch path also provisions via self.exec (-> runtime.exec). + runtime.exec.return_value = _OK + self.assertTrue(orch.setup_containers()) runtime.setup_containers.assert_called_once() - @patch("cvs.core.orchestrators.container.RuntimeFactory") - @patch("cvs.core.orchestrators.baremetal.Pssh") - def test_setup_containers_external_verifies_only(self, _mock_pssh, mock_rf): + def test_setup_containers_external_verifies_only(self): # external never starts a container; it verifies and sets container_id. - orch, runtime = self._make(_mock_pssh, mock_rf, lifetime="external") - runtime.is_running.return_value = { - "10.0.0.1": {"running": True, "exit_code": 0, "name": "cvs_iter_test"}, - "10.0.0.2": {"running": True, "exit_code": 0, "name": "cvs_iter_test"}, - } + orch, runtime = self._make(lifetime="external") + runtime.is_running.return_value = _RUNNING self.assertTrue(orch.setup_containers()) runtime.setup_containers.assert_not_called() self.assertEqual(orch.container_id, "cvs_iter_test") - @patch("cvs.core.orchestrators.container.RuntimeFactory") - @patch("cvs.core.orchestrators.baremetal.Pssh") - def test_setup_containers_persistent_attaches_when_running(self, _mock_pssh, mock_rf): - orch, runtime = self._make(_mock_pssh, mock_rf, lifetime="persistent") + def test_setup_containers_external_not_running_fails(self): + # external + container not actually running -> verification fails; nothing + # is launched and nothing is provisioned. + orch, runtime = self._make(lifetime="external") runtime.is_running.return_value = { - "10.0.0.1": {"running": True, "exit_code": 0, "name": "cvs_iter_test"}, - "10.0.0.2": {"running": True, "exit_code": 0, "name": "cvs_iter_test"}, - } - runtime.image_sha_status.return_value = { - "10.0.0.1": {"container_sha": "sha:abc", "image_sha": "sha:abc", "exit_code": 0}, - "10.0.0.2": {"container_sha": "sha:abc", "image_sha": "sha:abc", "exit_code": 0}, + "10.0.0.1": {"running": False, "exit_code": 0, "name": ""}, } + self.assertFalse(orch.setup_containers()) + runtime.setup_containers.assert_not_called() + runtime.exec.assert_not_called() + + def test_setup_containers_persistent_attaches_when_running(self): + orch, runtime = self._make(lifetime="persistent") + runtime.is_running.return_value = _RUNNING + runtime.image_sha_status.return_value = _SHA_MATCH self.assertTrue(orch.setup_containers()) # Attach path: no new container started. runtime.setup_containers.assert_not_called() self.assertEqual(orch.container_id, "cvs_iter_test") - @patch("cvs.core.orchestrators.container.RuntimeFactory") - @patch("cvs.core.orchestrators.baremetal.Pssh") - def test_setup_containers_persistent_starts_when_not_running(self, _mock_pssh, mock_rf): - orch, runtime = self._make(_mock_pssh, mock_rf, lifetime="persistent") - runtime.is_running.return_value = { - "10.0.0.1": {"running": False, "exit_code": 0, "name": ""}, - "10.0.0.2": {"running": False, "exit_code": 0, "name": ""}, - } + def test_setup_containers_persistent_starts_when_not_running(self): + orch, runtime = self._make(lifetime="persistent") + runtime.is_running.return_value = _NOT_RUNNING runtime.setup_containers.return_value = True + runtime.exec.return_value = _OK self.assertTrue(orch.setup_containers()) runtime.setup_containers.assert_called_once() # No SHA check when launching fresh. runtime.image_sha_status.assert_not_called() - @patch("cvs.core.orchestrators.container.RuntimeFactory") - @patch("cvs.core.orchestrators.baremetal.Pssh") - def test_setup_containers_persistent_idempotent_on_resetup(self, _mock_pssh, mock_rf): + def test_setup_containers_persistent_idempotent_on_resetup(self): # Re-running setup against an already-running persistent container is a # no-op attach both times -- never starts a new container. - orch, runtime = self._make(_mock_pssh, mock_rf, lifetime="persistent") + orch, runtime = self._make(lifetime="persistent") runtime.is_running.return_value = { "10.0.0.1": {"running": True, "exit_code": 0, "name": "cvs_iter_test"}, } @@ -142,26 +174,19 @@ def test_setup_containers_persistent_idempotent_on_resetup(self, _mock_pssh, moc self.assertTrue(orch.setup_containers()) runtime.setup_containers.assert_not_called() - @patch("cvs.core.orchestrators.container.RuntimeFactory") - @patch("cvs.core.orchestrators.baremetal.Pssh") - def test_setup_containers_persistent_cross_host_sha_skew_errors(self, _mock_pssh, mock_rf): + def test_setup_containers_persistent_cross_host_sha_skew_errors(self): # Nodes running different image SHAs is a correctness error, not staleness. - orch, runtime = self._make(_mock_pssh, mock_rf, lifetime="persistent") - runtime.is_running.return_value = { - "10.0.0.1": {"running": True, "exit_code": 0, "name": "cvs_iter_test"}, - "10.0.0.2": {"running": True, "exit_code": 0, "name": "cvs_iter_test"}, - } + orch, runtime = self._make(lifetime="persistent") + runtime.is_running.return_value = _RUNNING runtime.image_sha_status.return_value = { "10.0.0.1": {"container_sha": "sha:abc", "image_sha": "sha:abc", "exit_code": 0}, "10.0.0.2": {"container_sha": "sha:def", "image_sha": "sha:def", "exit_code": 0}, } self.assertFalse(orch.setup_containers()) - @patch("cvs.core.orchestrators.container.RuntimeFactory") - @patch("cvs.core.orchestrators.baremetal.Pssh") - def test_setup_containers_persistent_stale_overlay_warns_but_passes(self, _mock_pssh, mock_rf): + def test_setup_containers_persistent_stale_overlay_warns_but_passes(self): # Per-host staleness (container older than local image tag) warns, does not fail. - orch, runtime = self._make(_mock_pssh, mock_rf, lifetime="persistent") + orch, runtime = self._make(lifetime="persistent") runtime.is_running.return_value = { "10.0.0.1": {"running": True, "exit_code": 0, "name": "cvs_iter_test"}, } @@ -174,29 +199,23 @@ def test_setup_containers_persistent_stale_overlay_warns_but_passes(self, _mock_ # teardown_containers lifetime branching # ------------------------------------------------------------------ - @patch("cvs.core.orchestrators.container.RuntimeFactory") - @patch("cvs.core.orchestrators.baremetal.Pssh") - def test_teardown_containers_short_circuits_when_lifetime_external(self, _mock_pssh, mock_rf): + def test_teardown_containers_short_circuits_when_lifetime_external(self): # external means containers are externally managed; teardown is a no-op. - orch, runtime = self._make(_mock_pssh, mock_rf, lifetime="external") + orch, runtime = self._make(lifetime="external") orch.container_id = "test_container" self.assertTrue(orch.teardown_containers()) runtime.teardown_containers.assert_not_called() - @patch("cvs.core.orchestrators.container.RuntimeFactory") - @patch("cvs.core.orchestrators.baremetal.Pssh") - def test_teardown_containers_persistent_is_noop(self, _mock_pssh, mock_rf): + def test_teardown_containers_persistent_is_noop(self): # persistent leaves the container running for the next run. - orch, runtime = self._make(_mock_pssh, mock_rf, lifetime="persistent") + orch, runtime = self._make(lifetime="persistent") orch.container_id = "test_container" self.assertTrue(orch.teardown_containers()) runtime.teardown_containers.assert_not_called() - @patch("cvs.core.orchestrators.container.RuntimeFactory") - @patch("cvs.core.orchestrators.baremetal.Pssh") - def test_teardown_containers_calls_runtime_when_lifetime_per_run(self, _mock_pssh, mock_rf): + def test_teardown_containers_calls_runtime_when_lifetime_per_run(self): # per_run means CVS owns the container lifecycle; teardown delegates to runtime. - orch, runtime = self._make(_mock_pssh, mock_rf, lifetime="per_run") + orch, runtime = self._make(lifetime="per_run") orch.container_id = "test_container" runtime.teardown_containers.return_value = True self.assertTrue(orch.teardown_containers()) @@ -204,48 +223,176 @@ def test_teardown_containers_calls_runtime_when_lifetime_per_run(self, _mock_pss # container_id cleared on successful teardown. self.assertIsNone(orch.container_id) - @patch("cvs.core.orchestrators.container.RuntimeFactory") - @patch("cvs.core.orchestrators.baremetal.Pssh") - def test_teardown_containers_short_circuits_when_no_container_id(self, _mock_pssh, mock_rf): + def test_teardown_containers_short_circuits_when_no_container_id(self): # container_id stays None when setup never ran; per_run teardown is a no-op # since there is nothing to tear down. - orch, runtime = self._make(_mock_pssh, mock_rf, lifetime="per_run") + orch, runtime = self._make(lifetime="per_run") self.assertIsNone(orch.container_id) self.assertTrue(orch.teardown_containers()) runtime.teardown_containers.assert_not_called() # ------------------------------------------------------------------ - # setup_sshd idempotency (required by lifetime: persistent) + # setup_sshd (required by lifetime: persistent) # ------------------------------------------------------------------ - @patch("cvs.core.orchestrators.container.RuntimeFactory") - @patch("cvs.core.orchestrators.baremetal.Pssh") - def test_setup_sshd_idempotent_when_already_running(self, _mock_pssh, mock_rf): + def test_setup_sshd_idempotent_when_already_running(self): # When the pgrep precheck reports sshd already on 2224 for every host, the # start commands must NOT run (would re-bind the port and fail). - orch, runtime = self._make(_mock_pssh, mock_rf, lifetime="persistent") + orch, runtime = self._make(lifetime="persistent") orch.container_id = "cvs_iter_test" - runtime.exec.return_value = { - "10.0.0.1": {"exit_code": 0, "output": ""}, - "10.0.0.2": {"exit_code": 0, "output": ""}, - } + runtime.exec.return_value = _OK self.assertTrue(orch.setup_sshd()) # Only the precheck exec happened; no setup commands were issued. runtime.exec.assert_called_once() + # Guard the subtle self-match fix: the precheck must use the `[s]shd` + # trick, not a literal 'sshd.*2224' (which matches pgrep's own parent + # shell and makes this precheck always report "already running"). + precheck_cmd = runtime.exec.call_args[0][1] + self.assertIn("[s]shd.*2224", precheck_cmd) + self.assertNotIn("'sshd.*2224'", precheck_cmd) + + @patch("time.sleep") + def test_setup_sshd_installs_and_validates_when_not_running(self, _mock_sleep): + # The other setup_sshd branch: precheck reports NO sshd -> the start + # commands run (incl. /usr/sbin/sshd -p2224) and the post-start validation + # runs. Both pgrep sites must use the non-self-matching [s]shd pattern. + orch, runtime = self._make(lifetime="per_run") + orch.container_id = "cvs_iter_test" + need = { + "10.0.0.1": {"exit_code": 1, "output": ""}, + "10.0.0.2": {"exit_code": 1, "output": ""}, + } + # precheck (needs sshd) -> 6 ssh_setup_commands -> post-start validation. + runtime.exec.side_effect = [need] + [_OK] * 6 + [_OK] + self.assertTrue(orch.setup_sshd()) + issued = [c[0][1] for c in runtime.exec.call_args_list] + self.assertIn("/usr/sbin/sshd -p2224", issued) + # precheck (first) and post-start validation (last) both use the trick. + self.assertIn("[s]shd.*2224", issued[0]) + self.assertIn("[s]shd.*2224", issued[-1]) - @patch("cvs.core.orchestrators.container.RuntimeFactory") - @patch("cvs.core.orchestrators.baremetal.Pssh") - def test_init_requires_container_config(self, _mock_pssh, _mock_rf): - # ContainerOrchestrator raises if 'container' config is empty. - cfg = OrchestratorConfig( - orchestrator="container", - node_dict={"10.0.0.1": {}}, - username="testuser", - priv_key_file="/dev/null", - container={}, - ) - with self.assertRaises(ValueError): - ContainerOrchestrator(MagicMock(), cfg) + # ------------------------------------------------------------------ + # Container provisioning (setup_script) -- runs only on fresh launch. + # ------------------------------------------------------------------ + + def test_provisioning_runs_only_on_fresh_launch(self): + # Dispatch matrix: provisioning (one exec) happens only on a fresh launch + # (per_run, persistent cold-start); external and persistent-attach skip it. + # (name, lifetime, is_running, image_sha, expect_provision) + cases = [ + ("per_run", "per_run", None, None, True), + ("persistent_cold", "persistent", _NOT_RUNNING, None, True), + ("external", "external", _RUNNING, None, False), + ("persistent_attach", "persistent", _RUNNING, _SHA_MATCH, False), + ] + for name, lifetime, is_running, image_sha, expect in cases: + with self.subTest(name): + orch, runtime = self._make(lifetime=lifetime) + runtime.setup_containers.return_value = True + runtime.exec.return_value = _OK + if is_running is not None: + runtime.is_running.return_value = is_running + if image_sha is not None: + runtime.image_sha_status.return_value = image_sha + self.assertTrue(orch.setup_containers()) + if expect: + runtime.exec.assert_called_once() + provision_cmd = runtime.exec.call_args[0][1] + self.assertIn("base64 -d", provision_cmd) + self.assertIn("| bash", provision_cmd) + else: + runtime.exec.assert_not_called() + + def test_provisioning_size_guard(self): + # Strict `>`: exactly MAX bytes proceeds; MAX+1 is rejected before any exec. + cases = [ + ("at_limit_ok", b"x" * MAX_INLINE_SETUP_SCRIPT_BYTES, True), + ("over_limit_rejected", b"x" * (MAX_INLINE_SETUP_SCRIPT_BYTES + 1), False), + ] + for name, payload, expect_ok in cases: + with self.subTest(name): + orch, runtime = self._make(lifetime="per_run") + orch.container_id = "cvs_iter_test" + runtime.exec.return_value = {"10.0.0.1": {"exit_code": 0, "output": ""}} + with patch("builtins.open", mock_open(read_data=payload)): + self.assertEqual(orch._provision_container(), expect_ok) + if expect_ok: + runtime.exec.assert_called_once() + else: + runtime.exec.assert_not_called() + + def test_provisioning_failure_fails_launch(self): + # A non-zero provisioning exit on any host fails setup_containers. + orch, runtime = self._make(lifetime="per_run") + runtime.setup_containers.return_value = True + runtime.exec.return_value = { + "10.0.0.1": {"exit_code": 0, "output": ""}, + "10.0.0.2": {"exit_code": 100, "output": "apt failed"}, + } + self.assertFalse(orch.setup_containers()) + + def test_provisioning_not_attempted_when_runtime_launch_fails(self): + # If the container failed to start, provisioning must not be attempted. + orch, runtime = self._make(lifetime="per_run") + runtime.setup_containers.return_value = False + self.assertFalse(orch.setup_containers()) + runtime.exec.assert_not_called() + + def test_provisioning_noop_when_no_setup_script(self): + # Defensive skip: a hand-built config with no setup_script provisions nothing. + orch, runtime = self._make(lifetime="per_run") + orch.container_id = "cvs_iter_test" + orch.container_config["setup_script"] = None + self.assertTrue(orch._provision_container()) + runtime.exec.assert_not_called() + + def test_provisioning_failure_logs_every_host_with_detail(self): + # F1: all failing hosts are logged (not just the first), each with its + # captured stderr/stdout so the failure is diagnosable from the log. + orch, runtime = self._make(lifetime="per_run") + orch.container_id = "cvs_iter_test" + runtime.exec.return_value = { + "10.0.0.1": {"exit_code": 100, "output": "apt boom one"}, + "10.0.0.2": {"exit_code": 5, "output": "apt boom two"}, + } + self.assertFalse(orch._provision_container()) + logged = " ".join(str(c) for c in orch.log.error.call_args_list) + self.assertIn("10.0.0.1", logged) + self.assertIn("apt boom one", logged) + self.assertIn("10.0.0.2", logged) + self.assertIn("apt boom two", logged) + + def test_provisioning_ships_the_user_supplied_script(self): + # A user-supplied setup_script (not just the default) is the payload shipped. + orch, runtime = self._make(lifetime="per_run") + orch.container_id = "cvs_iter_test" + custom = b"#!/bin/bash\necho custom-provision\napt-get install -y foo\n" + runtime.exec.return_value = {"10.0.0.1": {"exit_code": 0, "output": ""}} + with patch("builtins.open", mock_open(read_data=custom)): + self.assertTrue(orch._provision_container()) + cmd = runtime.exec.call_args[0][1] + encoded = cmd.split("echo ", 1)[1].split(" | base64", 1)[0] + self.assertEqual(base64.b64decode(encoded), custom) + + def test_provisioning_payload_is_the_resolved_script(self): + # Pin that the base64 actually carries the resolved setup_script bytes, + # not just that the command contains "base64 -d" (guards a broken encoder). + orch, runtime = self._make(lifetime="per_run") + runtime.setup_containers.return_value = True + runtime.exec.return_value = _OK + self.assertTrue(orch.setup_containers()) + cmd = runtime.exec.call_args[0][1] + encoded = cmd.split("echo ", 1)[1].split(" | base64", 1)[0] + with open(orch.container_config["setup_script"], "rb") as f: + self.assertEqual(base64.b64decode(encoded), f.read()) + + def test_provisioning_read_failure_returns_false(self): + # An unreadable setup_script fails the launch and never reaches docker exec. + orch, runtime = self._make(lifetime="per_run") + orch.container_id = "cvs_iter_test" + with patch("builtins.open", side_effect=OSError("boom")): + self.assertFalse(orch._provision_container()) + runtime.exec.assert_not_called() class TestResolveContainerLifetime(unittest.TestCase): From 6d182a3d50eb9fa3380c6af23270159a74b1ae66 Mon Sep 17 00:00:00 2001 From: Atul Nair Date: Sun, 31 May 2026 19:16:57 -0400 Subject: [PATCH 15/20] orchestrators/runtimes: harden container persistent lifetime Post-review hardening of the container persistent-lifetime feature: - container: branch persistent setup_containers on per-host running state. Attach when the container runs on every host, cold-start when it runs on no host, and hard-fail (no relaunch) when it runs on some hosts but not all -- relaunching force-removes the still-running hosts and destroys their overlay, the opposite of what persistent promises. - container: add `set -o pipefail` to the inline setup_script delivery so a missing or failing base64 in the image fails loudly instead of silently no-opping and later surfacing as an opaque sshd startup failure. - container/runtimes: fail the persistent image-SHA check when a host's SHA is unreadable (previously a silent pass), and drop the always-zero exit_code from image_sha_status and the ContainerRuntime protocol since the wrapping echo makes it meaningless. - factory: remove the container.launch deprecation mapping. launch now hard-errors like the already-removed enabled, so a stale launch flag can never silently override an explicit lifetime. - runtimes/docker: raise the container-start timeout from 60s to 900s (CONTAINER_START_TIMEOUT_S) so the launch `docker run` can pull a multi-GB image on a cold node instead of timing out at one minute. - unittests + cluster-file docs: updated for the new contracts. --- cvs/core/orchestrators/container.py | 60 +++++++++++++--- cvs/core/orchestrators/factory.py | 32 ++++----- .../orchestrators/unittests/test_container.py | 70 +++++++++++++------ cvs/core/runtimes/base.py | 4 +- cvs/core/runtimes/docker.py | 21 ++++-- cvs/input/cluster_file/README.md | 2 +- docs/how-to/run-with-containers.rst | 2 +- .../configuration-files/cluster-file.rst | 2 +- 8 files changed, 135 insertions(+), 58 deletions(-) diff --git a/cvs/core/orchestrators/container.py b/cvs/core/orchestrators/container.py index bd8e34f5..b2aebc51 100644 --- a/cvs/core/orchestrators/container.py +++ b/cvs/core/orchestrators/container.py @@ -306,13 +306,34 @@ def setup_containers( return self.verify_containers_running(container_name) if lifetime == 'persistent': - # Attach if already running on every host, otherwise (re)launch. status = self.runtime.is_running(container_name) - if status and all(info.get('running') for info in status.values()): + running_hosts = [h for h, info in status.items() if info.get('running')] + missing_hosts = [h for h, info in status.items() if not info.get('running')] + + if running_hosts and not missing_hosts: + # Running on every host: attach (with image-SHA check). self.container_id = container_name self.log.info(f"Attaching to running container '{container_name}'") return self._verify_persistent_image(container_name, image) - self.log.info("Persistent container not running on all hosts, launching...") + + if running_hosts and missing_hosts: + # Partial: refuse to auto-relaunch. _launch_containers force-removes + # the same-named container on ALL hosts before recreating, which would + # destroy the overlay (installs, clones) on the still-running hosts -- + # the opposite of what 'persistent' promises. Fail loudly and let the + # user choose: remove on all hosts and rerun (clean rebuild), or + # restart the container on the missing hosts to reattach. + self.log.error( + f"Persistent container '{container_name}' is running on {running_hosts} " + f"but missing on {missing_hosts}. Refusing to auto-relaunch: that would " + f"force-remove and rebuild the containers on the still-running hosts, " + f"destroying their overlay. Either remove '{container_name}' on all hosts " + f"and rerun, or restart it on {missing_hosts} to reattach." + ) + return False + + # Not running on any host: legitimate cold start, launch fresh on all. + self.log.info("Persistent container not running on any host, launching...") return self._launch_containers( volumes, devices, capabilities, security_opts, environment, groups, ulimits ) @@ -433,8 +454,12 @@ def _provision_container(self): encoded = base64.b64encode(script_bytes).decode('ascii') # docker exec already wraps the command in `bash -c`, so decode the # script and pipe it straight into bash. base64 sidesteps all quoting and - # newline issues with arbitrary script content over pssh. - cmd = f"echo {encoded} | base64 -d | bash" + # newline issues with arbitrary script content over pssh. `set -o pipefail` + # is required: without it the pipeline's exit code is bash's (the last + # stage), so a missing/failing `base64` in the image would exit 0 and the + # provisioning would silently no-op, surfacing later as an opaque sshd + # failure. pipefail makes that fail here with a diagnosable error instead. + cmd = f"set -o pipefail; echo {encoded} | base64 -d | bash" result = self.exec(cmd, timeout=600, detailed=True) ok = True @@ -458,28 +483,45 @@ def _verify_persistent_image(self, container_name, image): """Compare the running container's image SHA to the local image tag on each host (persistent attach only). + This runs only after ``is_running`` confirmed the container is up on every + host, so every host MUST yield a readable container SHA. + + - Unreadable SHA on any host (probe failed / container vanished): ERROR + and return False -- we cannot vouch for consistency, so do not silently + pass. - Per-host mismatch (container created from an image older than the local ```` tag): WARN and continue -- the overlay may be stale. - Cross-host SHA skew (hosts running different image SHAs): ERROR and return False -- a correctness problem, not mere staleness. Returns: - bool: False on cross-host skew, True otherwise. + bool: False on unreadable SHA or cross-host skew, True otherwise. """ status = self.runtime.image_sha_status(container_name, image) container_shas = set() + unreadable_hosts = [] for host, info in status.items(): container_sha = info.get('container_sha', '') image_sha = info.get('image_sha', '') - if container_sha: - container_shas.add(container_sha) - if container_sha and image_sha and container_sha != image_sha: + if not container_sha: + unreadable_hosts.append(host) + continue + container_shas.add(container_sha) + if image_sha and container_sha != image_sha: self.log.warning( f"Container '{container_name}' on {host} runs image " f"{container_sha[:19]} but local '{image}' is {image_sha[:19]}; " f"overlay may be stale" ) + if unreadable_hosts: + self.log.error( + f"Could not read the running image SHA for container " + f"'{container_name}' on {unreadable_hosts}; cannot verify image " + f"consistency across hosts" + ) + return False + if len(container_shas) > 1: self.log.error( f"Cross-host image SHA skew for container '{container_name}': " diff --git a/cvs/core/orchestrators/factory.py b/cvs/core/orchestrators/factory.py index 6bad78f0..7da4c8f8 100644 --- a/cvs/core/orchestrators/factory.py +++ b/cvs/core/orchestrators/factory.py @@ -6,7 +6,6 @@ ''' import os -import warnings from cvs.core.orchestrators.baremetal import BaremetalOrchestrator from cvs.core.orchestrators.container import ContainerOrchestrator @@ -26,15 +25,15 @@ def _resolve_container_lifetime(container): """Normalize a container config block to a single resolved ``lifetime`` key. - Collapses the legacy two-axis schema (``enabled`` + ``launch``) into one - tri-valued ``container.lifetime``. Mutates and returns the passed dict. + The legacy two-axis schema (``enabled`` + ``launch``) is removed in favor of + one tri-valued ``container.lifetime``. Mutates and returns the passed dict. Resolution rules (first match wins): - - ``enabled`` present (any value) -> ``ValueError``. The field is removed - from the schema; the user must delete it and set ``lifetime``. + - ``enabled`` present (any value) -> ``ValueError`` (removed field). + - ``launch`` present (any value) -> ``ValueError`` (removed field). Both + removed fields fail loudly rather than being silently mapped, so a stale + flag can never quietly override an explicit ``lifetime``. - ``lifetime`` present -> validated, kept as-is. - - ``launch`` present (no enabled) -> ``DeprecationWarning``; ``True`` maps - to ``per_run``, ``False`` maps to ``external``. The legacy key is dropped. - none of the above -> default ``per_run``. An empty/absent container block is returned untouched (baremetal path). @@ -48,6 +47,13 @@ def _resolve_container_lifetime(container): "container.lifetime to one of 'external', 'per_run', 'persistent'" ) + if 'launch' in container: + raise ValueError( + "container.launch is removed; delete the field and set " + "container.lifetime ('launch: true' -> 'per_run', " + "'launch: false' -> 'external')" + ) + if 'lifetime' in container: lifetime = container['lifetime'] if lifetime not in VALID_CONTAINER_LIFETIMES: @@ -57,18 +63,6 @@ def _resolve_container_lifetime(container): ) return container - if 'launch' in container: - launch = container.pop('launch') - lifetime = 'per_run' if launch else 'external' - warnings.warn( - f"container.launch is deprecated, use container.lifetime " - f"(mapped launch={launch!r} -> lifetime={lifetime!r})", - DeprecationWarning, - stacklevel=2, - ) - container['lifetime'] = lifetime - return container - container['lifetime'] = 'per_run' return container diff --git a/cvs/core/orchestrators/unittests/test_container.py b/cvs/core/orchestrators/unittests/test_container.py index bd81e083..877ee0b1 100644 --- a/cvs/core/orchestrators/unittests/test_container.py +++ b/cvs/core/orchestrators/unittests/test_container.py @@ -37,8 +37,8 @@ "10.0.0.2": {"running": False, "exit_code": 0, "name": ""}, } _SHA_MATCH = { - "10.0.0.1": {"container_sha": "sha:abc", "image_sha": "sha:abc", "exit_code": 0}, - "10.0.0.2": {"container_sha": "sha:abc", "image_sha": "sha:abc", "exit_code": 0}, + "10.0.0.1": {"container_sha": "sha:abc", "image_sha": "sha:abc"}, + "10.0.0.2": {"container_sha": "sha:abc", "image_sha": "sha:abc"}, } @@ -168,7 +168,7 @@ def test_setup_containers_persistent_idempotent_on_resetup(self): "10.0.0.1": {"running": True, "exit_code": 0, "name": "cvs_iter_test"}, } runtime.image_sha_status.return_value = { - "10.0.0.1": {"container_sha": "sha:abc", "image_sha": "sha:abc", "exit_code": 0}, + "10.0.0.1": {"container_sha": "sha:abc", "image_sha": "sha:abc"}, } self.assertTrue(orch.setup_containers()) self.assertTrue(orch.setup_containers()) @@ -179,8 +179,8 @@ def test_setup_containers_persistent_cross_host_sha_skew_errors(self): orch, runtime = self._make(lifetime="persistent") runtime.is_running.return_value = _RUNNING runtime.image_sha_status.return_value = { - "10.0.0.1": {"container_sha": "sha:abc", "image_sha": "sha:abc", "exit_code": 0}, - "10.0.0.2": {"container_sha": "sha:def", "image_sha": "sha:def", "exit_code": 0}, + "10.0.0.1": {"container_sha": "sha:abc", "image_sha": "sha:abc"}, + "10.0.0.2": {"container_sha": "sha:def", "image_sha": "sha:def"}, } self.assertFalse(orch.setup_containers()) @@ -191,10 +191,35 @@ def test_setup_containers_persistent_stale_overlay_warns_but_passes(self): "10.0.0.1": {"running": True, "exit_code": 0, "name": "cvs_iter_test"}, } runtime.image_sha_status.return_value = { - "10.0.0.1": {"container_sha": "sha:old", "image_sha": "sha:new", "exit_code": 0}, + "10.0.0.1": {"container_sha": "sha:old", "image_sha": "sha:new"}, } self.assertTrue(orch.setup_containers()) + def test_setup_containers_persistent_partial_running_refuses(self): + # Running on some hosts but not all must NOT auto-relaunch (that would + # force-remove and rebuild the still-running hosts, destroying their + # overlay). It fails loudly and starts nothing. + orch, runtime = self._make(lifetime="persistent") + runtime.is_running.return_value = { + "10.0.0.1": {"running": True, "exit_code": 0, "name": "cvs_iter_test"}, + "10.0.0.2": {"running": False, "exit_code": 0, "name": ""}, + } + self.assertFalse(orch.setup_containers()) + runtime.setup_containers.assert_not_called() + # No image-SHA attach check either; it bailed before attaching. + runtime.image_sha_status.assert_not_called() + + def test_setup_containers_persistent_unreadable_sha_fails(self): + # Attach path: container is running on all hosts but the image-SHA probe + # yields no SHA -> cannot verify consistency, so fail rather than silently pass. + orch, runtime = self._make(lifetime="persistent") + runtime.is_running.return_value = _RUNNING + runtime.image_sha_status.return_value = { + "10.0.0.1": {"container_sha": "", "image_sha": ""}, + "10.0.0.2": {"container_sha": "", "image_sha": ""}, + } + self.assertFalse(orch.setup_containers()) + # ------------------------------------------------------------------ # teardown_containers lifetime branching # ------------------------------------------------------------------ @@ -418,17 +443,21 @@ def test_invalid_lifetime_raises(self): with self.assertRaises(ValueError): _resolve_container_lifetime({"lifetime": "forever", "image": "x"}) - def test_launch_true_maps_to_per_run_with_deprecation(self): - with self.assertWarns(DeprecationWarning): - out = _resolve_container_lifetime({"launch": True, "image": "x"}) - self.assertEqual(out["lifetime"], "per_run") - self.assertNotIn("launch", out) + def test_launch_present_raises(self): + # launch is a removed field: any value is a hard error (no silent mapping). + with self.assertRaises(ValueError) as ctx: + _resolve_container_lifetime({"launch": True, "image": "x"}) + self.assertIn("launch", str(ctx.exception)) - def test_launch_false_maps_to_external_with_deprecation(self): - with self.assertWarns(DeprecationWarning): - out = _resolve_container_lifetime({"launch": False, "image": "x"}) - self.assertEqual(out["lifetime"], "external") - self.assertNotIn("launch", out) + def test_launch_false_also_raises(self): + with self.assertRaises(ValueError): + _resolve_container_lifetime({"launch": False, "image": "x"}) + + def test_launch_alongside_lifetime_raises(self): + # A stale launch flag next to an explicit lifetime must fail loudly rather + # than being silently retained/ignored. + with self.assertRaises(ValueError): + _resolve_container_lifetime({"lifetime": "per_run", "launch": True, "image": "x"}) def test_no_policy_keys_defaults_to_per_run(self): out = _resolve_container_lifetime({"image": "x"}) @@ -439,18 +468,17 @@ def test_empty_block_untouched(self): self.assertEqual(_resolve_container_lifetime({}), {}) @patch("cvs.core.orchestrators.baremetal.Pssh") - def test_orchestratorconfig_init_resolves_launch_alias(self, _mock_pssh): + def test_orchestratorconfig_init_rejects_launch(self, _mock_pssh): # Direct construction routes through __init__ -> the same helper, so a - # legacy launch flag is resolved identically to from_configs. - with self.assertWarns(DeprecationWarning): - cfg = OrchestratorConfig( + # legacy launch flag is rejected identically to from_configs. + with self.assertRaises(ValueError): + OrchestratorConfig( orchestrator="container", node_dict={"1.1.1.1": {}}, username="u", priv_key_file="/dev/null", container={"launch": True, "image": "x"}, ) - self.assertEqual(cfg.container["lifetime"], "per_run") if __name__ == "__main__": diff --git a/cvs/core/runtimes/base.py b/cvs/core/runtimes/base.py index 322cbb4c..ba9bc4b4 100644 --- a/cvs/core/runtimes/base.py +++ b/cvs/core/runtimes/base.py @@ -24,8 +24,8 @@ def image_sha_status(self, container_name, image_name): Returns: dict: per-host result with keys 'container_sha' (image ID the running - container was created from), 'image_sha' (local image's current ID), - and 'exit_code' (the underlying probe command's exit code). + container was created from) and 'image_sha' (local image's current ID). + Either is '' when its probe could not read a value. """ ... diff --git a/cvs/core/runtimes/docker.py b/cvs/core/runtimes/docker.py index 4309a3b4..e96cac08 100644 --- a/cvs/core/runtimes/docker.py +++ b/cvs/core/runtimes/docker.py @@ -8,6 +8,13 @@ import shlex +# Timeout (seconds) for the `docker run` that starts a container. This is the +# step that implicitly pulls the image when it is not already present locally, +# so it must allow for a cold pull of a large image (multi-GB ROCm images can +# take several minutes). 15 minutes covers a fresh pull on launch. +CONTAINER_START_TIMEOUT_S = 900 + + class DockerRuntime: """Docker container runtime implementation.""" @@ -130,7 +137,9 @@ def setup_containers( remove_cmd = f"sudo docker rm -f {container_name} || true" self.orchestrator.all.exec(remove_cmd, timeout=30, print_console=False) - result = self.orchestrator.all.exec(cmd, timeout=60, detailed=True) + # CONTAINER_START_TIMEOUT_S (not 60s): `docker run` implicitly pulls the + # image on a cold node, which for a multi-GB image far exceeds a minute. + result = self.orchestrator.all.exec(cmd, timeout=CONTAINER_START_TIMEOUT_S, detailed=True) # Check if all hosts started successfully success = all(output['exit_code'] == 0 for output in result.values()) @@ -176,14 +185,19 @@ def image_sha_status(self, container_name, image_name): staleness and cross-host image skew. Runs both ``docker inspect`` probes in one fan-out and emits ``|`` per host. + A failed ``docker inspect`` (missing container/image) yields an empty + string for that SHA; callers treat an empty ``container_sha`` as a probe + failure. No exit code is returned because the wrapping ``echo`` always + succeeds, so it carries no signal. + Args: container_name: Name of the running container to inspect. image_name: Local image tag to compare against. Returns: dict: per-host result with keys 'container_sha' (image ID the running - container was created from, ``.Image``), 'image_sha' (local image's - current ID, ``.Id``), and 'exit_code' (the probe's exit code). + container was created from, ``.Image``) and 'image_sha' (local image's + current ID, ``.Id``); each is '' when its probe could not read a value. """ cmd = ( f"echo \"$(sudo docker inspect --format '{{{{.Image}}}}' {container_name} 2>/dev/null)" @@ -194,7 +208,6 @@ def image_sha_status(self, container_name, image_name): for host, res in raw.items(): parts = (res.get('output') or '').strip().split('|') out[host] = { - 'exit_code': res.get('exit_code'), 'container_sha': parts[0] if len(parts) > 0 else '', 'image_sha': parts[1] if len(parts) > 1 else '', } diff --git a/cvs/input/cluster_file/README.md b/cvs/input/cluster_file/README.md index fa4f558e..fbe7f4df 100644 --- a/cvs/input/cluster_file/README.md +++ b/cvs/input/cluster_file/README.md @@ -78,7 +78,7 @@ RDMA-ready container; the keys below are only needed to extend or override. | --- | --- | --- | | `external` | Verify the container is already running on every host; set `container_id`. Never starts anything. | No-op. CVS does not own externally managed containers. | | `per_run` (default) | Start a fresh container on every host (force-removing any stale same-named container first). | Force-remove the container CVS started. | -| `persistent` | Attach if the container is already running on every host (with a per-host image-SHA check; cross-host SHA skew is a hard error). Otherwise start fresh. Idempotent across runs. | No-op. The container is left running for the next run; remove it yourself when done. | +| `persistent` | Attach if the container is already running on every host (with a per-host image-SHA check; cross-host SHA skew or an unreadable SHA is a hard error). Start fresh only if it is running on no host. Running on some hosts but not all is a hard error (CVS will not force-remove the still-running hosts and destroy their overlay). Idempotent across runs. | No-op. The container is left running for the next run; remove it yourself when done. | ## Prerequisites on each cluster node diff --git a/docs/how-to/run-with-containers.rst b/docs/how-to/run-with-containers.rst index b978a0d8..a5827e38 100644 --- a/docs/how-to/run-with-containers.rst +++ b/docs/how-to/run-with-containers.rst @@ -105,7 +105,7 @@ Lifecycle and teardown The ``container.lifetime`` policy controls who owns the container lifecycle: - ``per_run`` (default) - CVS starts a fresh container at setup and force-removes it at teardown. Anything written to the container overlay is lost when the run ends. -- ``persistent`` - CVS starts the container if it is not already running, otherwise attaches to it. Teardown is a no-op, so the container (and its overlay) survives across runs. This unblocks install-then-run workflows: ``cvs run install_rvs`` followed by ``cvs run rvs_cvs`` in separate invocations. Pin ``container.name`` so a tag bump does not silently abandon the overlay. +- ``persistent`` - CVS attaches to the container if it is already running on every host, or starts it fresh if it is running on no host. Teardown is a no-op, so the container (and its overlay) survives across runs. This unblocks install-then-run workflows: ``cvs run install_rvs`` followed by ``cvs run rvs_cvs`` in separate invocations. If the container is running on some hosts but not all, CVS fails rather than rebuilding (which would destroy the overlay on the still-running hosts) -- remove it on all hosts and rerun, or restart it on the missing hosts. Pin ``container.name`` so a tag bump does not silently abandon the overlay. - ``external`` - CVS does not start anything. It verifies that a container with the configured name is already running on every host and reuses it. Teardown is a no-op. See the ``lifetime`` truth table in :doc:`/reference/configuration-files/cluster-file` for the full state matrix. diff --git a/docs/reference/configuration-files/cluster-file.rst b/docs/reference/configuration-files/cluster-file.rst index bfc4f9b5..e803ec13 100644 --- a/docs/reference/configuration-files/cluster-file.rst +++ b/docs/reference/configuration-files/cluster-file.rst @@ -218,7 +218,7 @@ When ``runtime.name`` is ``docker``, the keys below configure the underlying ``d - Start a fresh container on every host (force-removing any stale same-named container first). - Force-remove the container CVS started. * - ``persistent`` - - Attach if the container is already running on every host (with a per-host image-SHA check; cross-host SHA skew is a hard error). Otherwise start fresh. Idempotent across runs. + - Attach if the container is already running on every host (with a per-host image-SHA check; cross-host SHA skew or an unreadable SHA is a hard error). Start fresh only if it is running on no host. Running on some hosts but not all is a hard error (CVS will not force-remove the still-running hosts and destroy their overlay). Idempotent across runs. - No-op. The container is left running for the next run; remove it yourself when done. .. note:: From f950082559df745c7e4c791b236d96313b12255d Mon Sep 17 00:00:00 2001 From: Atul Nair Date: Sun, 31 May 2026 19:29:57 -0400 Subject: [PATCH 16/20] orchestrators: apply ruff format to satisfy the fmt-check gate `make test` runs `ruff format --check` over the tree. Collapse the multi-line calls/raises that fit the configured line length in container.py and factory.py (from the previous commit) and in test_factory.py (pre-existing drift). Formatting only, no behavior change. --- cvs/core/orchestrators/container.py | 8 ++------ cvs/core/orchestrators/factory.py | 10 ++-------- cvs/core/orchestrators/unittests/test_factory.py | 8 ++------ 3 files changed, 6 insertions(+), 20 deletions(-) diff --git a/cvs/core/orchestrators/container.py b/cvs/core/orchestrators/container.py index b2aebc51..8e1c7e86 100644 --- a/cvs/core/orchestrators/container.py +++ b/cvs/core/orchestrators/container.py @@ -334,14 +334,10 @@ def setup_containers( # Not running on any host: legitimate cold start, launch fresh on all. self.log.info("Persistent container not running on any host, launching...") - return self._launch_containers( - volumes, devices, capabilities, security_opts, environment, groups, ulimits - ) + return self._launch_containers(volumes, devices, capabilities, security_opts, environment, groups, ulimits) # 'per_run' (default): always start fresh. - return self._launch_containers( - volumes, devices, capabilities, security_opts, environment, groups, ulimits - ) + return self._launch_containers(volumes, devices, capabilities, security_opts, environment, groups, ulimits) def _launch_containers( self, diff --git a/cvs/core/orchestrators/factory.py b/cvs/core/orchestrators/factory.py index 7da4c8f8..c1981b39 100644 --- a/cvs/core/orchestrators/factory.py +++ b/cvs/core/orchestrators/factory.py @@ -57,10 +57,7 @@ def _resolve_container_lifetime(container): if 'lifetime' in container: lifetime = container['lifetime'] if lifetime not in VALID_CONTAINER_LIFETIMES: - raise ValueError( - f"container.lifetime must be one of {VALID_CONTAINER_LIFETIMES}, " - f"got {lifetime!r}" - ) + raise ValueError(f"container.lifetime must be one of {VALID_CONTAINER_LIFETIMES}, got {lifetime!r}") return container container['lifetime'] = 'per_run' @@ -90,10 +87,7 @@ def _resolve_container_setup_script(container): if setup_script: resolved = os.path.abspath(os.path.expanduser(setup_script)) if not os.path.isfile(resolved): - raise ValueError( - f"container.setup_script not found: {setup_script!r} " - f"(resolved to {resolved!r})" - ) + raise ValueError(f"container.setup_script not found: {setup_script!r} (resolved to {resolved!r})") container['setup_script'] = resolved return container diff --git a/cvs/core/orchestrators/unittests/test_factory.py b/cvs/core/orchestrators/unittests/test_factory.py index 75ccb72b..2957a2a9 100644 --- a/cvs/core/orchestrators/unittests/test_factory.py +++ b/cvs/core/orchestrators/unittests/test_factory.py @@ -290,9 +290,7 @@ def test_existing_user_path_kept_as_absolute(self): def test_missing_user_path_raises(self): with self.assertRaises(ValueError) as ctx: - _resolve_container_setup_script( - {"image": "x", "setup_script": "/no/such/setup_script.sh"} - ) + _resolve_container_setup_script({"image": "x", "setup_script": "/no/such/setup_script.sh"}) self.assertIn("setup_script", str(ctx.exception)) def test_missing_packaged_default_raises(self): @@ -323,9 +321,7 @@ def test_tilde_user_path_is_expanded(self): # A ~ path is expanded before the existence check (proven via the resolved # path in the error message since the file does not exist). with self.assertRaises(ValueError) as ctx: - _resolve_container_setup_script( - {"image": "x", "setup_script": "~/__cvs_missing_setup__.sh"} - ) + _resolve_container_setup_script({"image": "x", "setup_script": "~/__cvs_missing_setup__.sh"}) msg = str(ctx.exception) self.assertIn(os.path.expanduser("~"), msg) self.assertNotIn("~", msg.split("resolved to", 1)[-1]) From fa4ca32eb4de57ddbfbc9300709a038cf6441357 Mon Sep 17 00:00:00 2001 From: Atul Nair Date: Mon, 1 Jun 2026 17:23:50 -0400 Subject: [PATCH 17/20] orchestrators: rename container lifetime 'external' to 'no_launch' Rename the verify-only container lifetime value from 'external' to 'no_launch', which describes the behavior (CVS never launches the container) rather than an ownership model. Updates the validation tuple, branch checks, removed-field error messages, unit tests, the cluster_container.json sample comment, and the cluster-file docs / README truth tables. The feature is unreleased so no alias is kept. --- cvs/core/orchestrators/container.py | 14 +++++------ cvs/core/orchestrators/factory.py | 13 +++++----- .../orchestrators/unittests/test_container.py | 24 +++++++++---------- cvs/input/cluster_file/README.md | 4 ++-- cvs/input/cluster_file/cluster_container.json | 2 +- docs/how-to/run-with-containers.rst | 8 +++---- .../configuration-files/cluster-file.rst | 6 ++--- 7 files changed, 36 insertions(+), 35 deletions(-) diff --git a/cvs/core/orchestrators/container.py b/cvs/core/orchestrators/container.py index 8e1c7e86..a860e456 100644 --- a/cvs/core/orchestrators/container.py +++ b/cvs/core/orchestrators/container.py @@ -275,8 +275,8 @@ def setup_containers( This method should be called explicitly by tests when they need containers. Behavior branches on container.lifetime: - - 'external' : verify the (externally managed) container is running and - set container_id; never starts anything. + - 'no_launch' : verify a container with the configured name is running + and set container_id; never starts anything. - 'per_run' : start fresh containers on all hosts. - 'persistent' : attach to a container already running on all hosts (with an image-SHA check), otherwise start fresh. Idempotent. @@ -301,8 +301,8 @@ def setup_containers( return False container_name = self.get_container_name(self.container_config, image) - if lifetime == 'external': - # Externally managed: verify only, never start. + if lifetime == 'no_launch': + # CVS never launches it: verify only, never start. return self.verify_containers_running(container_name) if lifetime == 'persistent': @@ -406,7 +406,7 @@ def _launch_containers( # Provision the freshly-launched container (install packages on top of # the base image, e.g. openssh-server). Runs only on this fresh-start - # path, so 'external' and 'persistent'-attach skip it automatically. + # path, so 'no_launch' and 'persistent'-attach skip it automatically. return self._provision_container() def _provision_container(self): @@ -637,7 +637,7 @@ def teardown_containers(self): This method should be called explicitly by tests for cleanup. Behavior branches on container.lifetime: - - 'external' : no-op (CVS does not own externally managed containers). + - 'no_launch' : no-op (CVS does not own a container it did not launch). - 'persistent' : no-op (left running for the next run; user removes it explicitly). - 'per_run' : force-remove the container CVS started. @@ -647,7 +647,7 @@ def teardown_containers(self): """ lifetime = self.container_config.get('lifetime', 'per_run') - if lifetime in ('external', 'persistent'): + if lifetime in ('no_launch', 'persistent'): self.log.debug(f"lifetime={lifetime}, leaving containers running") return True diff --git a/cvs/core/orchestrators/factory.py b/cvs/core/orchestrators/factory.py index c1981b39..a0ee8891 100644 --- a/cvs/core/orchestrators/factory.py +++ b/cvs/core/orchestrators/factory.py @@ -11,7 +11,7 @@ from cvs.core.orchestrators.container import ContainerOrchestrator -VALID_CONTAINER_LIFETIMES = ("external", "per_run", "persistent") +VALID_CONTAINER_LIFETIMES = ("no_launch", "per_run", "persistent") # Packaged default provisioning script, run inside each freshly-launched # container when container.setup_script is not set. Installs openssh-server so @@ -44,14 +44,14 @@ def _resolve_container_lifetime(container): if 'enabled' in container: raise ValueError( "container.enabled is removed; delete the field and set " - "container.lifetime to one of 'external', 'per_run', 'persistent'" + "container.lifetime to one of 'no_launch', 'per_run', 'persistent'" ) if 'launch' in container: raise ValueError( "container.launch is removed; delete the field and set " "container.lifetime ('launch: true' -> 'per_run', " - "'launch: false' -> 'external')" + "'launch: false' -> 'no_launch')" ) if 'lifetime' in container: @@ -143,8 +143,9 @@ class OrchestratorConfig: } ``` lifetime: Container lifecycle policy [default: 'per_run'] - - 'external' : containers managed outside CVS. Setup verifies they - are running; teardown is a no-op. + - 'no_launch' : CVS never launches the container. Setup verifies a + container with the configured name is already running + on every host; teardown is a no-op. - 'per_run' : start at setup, remove at teardown (the default). - 'persistent' : start if absent / attach if present; never torn down by the run. Pin container.name explicitly under this @@ -209,7 +210,7 @@ def from_configs(cls, cluster_config, testsuite_config=None): Required keys: orchestrator, node_dict, username, priv_key_file Optional keys: container, head_node_dict, password (defaults provided for missing optional keys) - Container structure: {lifetime: 'external'|'per_run'|'persistent', runtime: {name: str, args: dict}, image: str, name: str, setup_script: str, ...} + Container structure: {lifetime: 'no_launch'|'per_run'|'persistent', runtime: {name: str, args: dict}, image: str, name: str, setup_script: str, ...} testsuite_config: Test suite specific configuration (dict or path to _config.json) Can override any keys from cluster_config diff --git a/cvs/core/orchestrators/unittests/test_container.py b/cvs/core/orchestrators/unittests/test_container.py index 877ee0b1..ae081d1b 100644 --- a/cvs/core/orchestrators/unittests/test_container.py +++ b/cvs/core/orchestrators/unittests/test_container.py @@ -11,7 +11,7 @@ # so tests run with no SSH or container runtime. # # The per-lifetime setup/teardown behavior is pinned here so any future change to the -# external / per_run / persistent contract has a loud canary. Pssh + RuntimeFactory are +# no_launch / per_run / persistent contract has a loud canary. Pssh + RuntimeFactory are # patched once in setUp (not per method); _make() returns a fresh orch + runtime mock. import base64 @@ -122,18 +122,18 @@ def test_setup_containers_per_run_delegates_to_runtime(self): self.assertTrue(orch.setup_containers()) runtime.setup_containers.assert_called_once() - def test_setup_containers_external_verifies_only(self): - # external never starts a container; it verifies and sets container_id. - orch, runtime = self._make(lifetime="external") + def test_setup_containers_no_launch_verifies_only(self): + # no_launch never starts a container; it verifies and sets container_id. + orch, runtime = self._make(lifetime="no_launch") runtime.is_running.return_value = _RUNNING self.assertTrue(orch.setup_containers()) runtime.setup_containers.assert_not_called() self.assertEqual(orch.container_id, "cvs_iter_test") - def test_setup_containers_external_not_running_fails(self): - # external + container not actually running -> verification fails; nothing + def test_setup_containers_no_launch_not_running_fails(self): + # no_launch + container not actually running -> verification fails; nothing # is launched and nothing is provisioned. - orch, runtime = self._make(lifetime="external") + orch, runtime = self._make(lifetime="no_launch") runtime.is_running.return_value = { "10.0.0.1": {"running": False, "exit_code": 0, "name": ""}, } @@ -224,9 +224,9 @@ def test_setup_containers_persistent_unreadable_sha_fails(self): # teardown_containers lifetime branching # ------------------------------------------------------------------ - def test_teardown_containers_short_circuits_when_lifetime_external(self): - # external means containers are externally managed; teardown is a no-op. - orch, runtime = self._make(lifetime="external") + def test_teardown_containers_short_circuits_when_lifetime_no_launch(self): + # no_launch means CVS did not launch the container; teardown is a no-op. + orch, runtime = self._make(lifetime="no_launch") orch.container_id = "test_container" self.assertTrue(orch.teardown_containers()) runtime.teardown_containers.assert_not_called() @@ -302,12 +302,12 @@ def test_setup_sshd_installs_and_validates_when_not_running(self, _mock_sleep): def test_provisioning_runs_only_on_fresh_launch(self): # Dispatch matrix: provisioning (one exec) happens only on a fresh launch - # (per_run, persistent cold-start); external and persistent-attach skip it. + # (per_run, persistent cold-start); no_launch and persistent-attach skip it. # (name, lifetime, is_running, image_sha, expect_provision) cases = [ ("per_run", "per_run", None, None, True), ("persistent_cold", "persistent", _NOT_RUNNING, None, True), - ("external", "external", _RUNNING, None, False), + ("no_launch", "no_launch", _RUNNING, None, False), ("persistent_attach", "persistent", _RUNNING, _SHA_MATCH, False), ] for name, lifetime, is_running, image_sha, expect in cases: diff --git a/cvs/input/cluster_file/README.md b/cvs/input/cluster_file/README.md index fbe7f4df..85a4b6f9 100644 --- a/cvs/input/cluster_file/README.md +++ b/cvs/input/cluster_file/README.md @@ -43,7 +43,7 @@ Consumed by `ContainerOrchestrator` in [`cvs/core/orchestrators/container.py`](. | Key | Type | Default | Purpose | | --- | --- | --- | --- | -| `lifetime` | str | `"per_run"` | Container lifecycle policy: `external`, `per_run`, or `persistent`. See the truth table below. | +| `lifetime` | str | `"per_run"` | Container lifecycle policy: `no_launch`, `per_run`, or `persistent`. See the truth table below. | | `image` | str | (required) | Image with the test dependencies (rvs, etc.) the suite invokes. Must be present locally on each node OR pullable from a reachable registry. An in-container sshd is **not** required up front -- `setup_script` installs it if missing. | | `name` | str | (required) | Container name on each host. For parallel runs make this per-iteration unique (e.g. `cvs_iter_`). | | `setup_script` | str | (packaged default) | Optional path to a shell script run inside each freshly-launched container (before sshd setup) to install packages on top of the base image. `null` or omitting the key both use the packaged default that installs `openssh-server` only (there is no value that disables provisioning). A non-existent path fails at config load. Delivered inline via `docker exec` (so the base image needs `bash` + `base64`, and the script must stay under ~16 KB); the default is apt-based and runs as the container's exec user (root) -- non-apt images need a custom script. | @@ -76,7 +76,7 @@ RDMA-ready container; the keys below are only needed to extend or override. | `lifetime` | `setup_containers` | `teardown_containers` | | --- | --- | --- | -| `external` | Verify the container is already running on every host; set `container_id`. Never starts anything. | No-op. CVS does not own externally managed containers. | +| `no_launch` | Verify the container is already running on every host; set `container_id`. Never starts anything. | No-op. CVS does not own a container it did not launch. | | `per_run` (default) | Start a fresh container on every host (force-removing any stale same-named container first). | Force-remove the container CVS started. | | `persistent` | Attach if the container is already running on every host (with a per-host image-SHA check; cross-host SHA skew or an unreadable SHA is a hard error). Start fresh only if it is running on no host. Running on some hosts but not all is a hard error (CVS will not force-remove the still-running hosts and destroy their overlay). Idempotent across runs. | No-op. The container is left running for the next run; remove it yourself when done. | diff --git a/cvs/input/cluster_file/cluster_container.json b/cvs/input/cluster_file/cluster_container.json index a7c65a7f..09055860 100644 --- a/cvs/input/cluster_file/cluster_container.json +++ b/cvs/input/cluster_file/cluster_container.json @@ -31,7 +31,7 @@ "_container_block_comment": "Container backend configuration. Consumed by ContainerOrchestrator in cvs/core/orchestrators/container.py. See README.md for the full schema and lifetime semantics.", "container": { - "_lifetime_comment": "Container lifecycle policy. 'external' = containers managed outside CVS (verify-only, never stopped). 'per_run' = CVS starts on setup and removes on teardown. 'persistent' = CVS starts if absent / attaches if present and leaves running on exit (unblocks install-then-run; pin 'name' explicitly).", + "_lifetime_comment": "Container lifecycle policy. 'no_launch' = CVS never launches the container (verify it is already running, never stopped). 'per_run' = CVS starts on setup and removes on teardown. 'persistent' = CVS starts if absent / attaches if present and leaves running on exit (unblocks install-then-run; pin 'name' explicitly).", "lifetime": "per_run", "_image_comment": "Image with the test dependencies (rvs, etc.) the suite invokes. Must be present locally on each node OR pullable from a reachable registry. An in-container sshd is NOT required up front -- it is installed by setup_script (see below) if missing.", diff --git a/docs/how-to/run-with-containers.rst b/docs/how-to/run-with-containers.rst index a5827e38..b65e9cf1 100644 --- a/docs/how-to/run-with-containers.rst +++ b/docs/how-to/run-with-containers.rst @@ -51,7 +51,7 @@ Open the copied file and edit: - ``container.image``: an image present on every node or pullable from a reachable registry. The image must include the workload binary (for example ``rvs``); ``openssh-server`` is installed at launch by ``setup_script``, not required in the image. - ``container.name``: container name on each host. For parallel runs make this per-iteration unique (for example ``cvs_iter_``). Pin it explicitly when using ``lifetime: persistent``. - ``container.setup_script`` (optional): path to a shell script run inside each freshly-launched container before sshd setup, to install packages on top of the base image. Omit to use the packaged default that installs ``openssh-server``. -- ``container.lifetime``: ``external``, ``per_run``, or ``persistent``. See the lifecycle note below. +- ``container.lifetime``: ``no_launch``, ``per_run``, or ``persistent``. See the lifecycle note below. For the full schema and runtime argument reference, see :doc:`/reference/configuration-files/cluster-file`. @@ -76,7 +76,7 @@ What happens during the run: - The ``orch`` fixture in ``rvs_cvs`` reads ``orchestrator: container`` from the cluster file and constructs a ``ContainerOrchestrator``. - With ``lifetime: per_run``, CVS removes any container with the same name on each host, runs the configured image with the merged ``runtime.args``, runs ``container.setup_script`` inside the new container (default: install ``openssh-server``), and starts an in-container ``sshd`` on port ``2224``. - With ``lifetime: persistent``, CVS attaches to a container already running on every host (skipping provisioning and ``sshd`` setup if it is already up), or starts one fresh -- provisioning it -- if none is running. -- With ``lifetime: external``, CVS verifies that a container with the configured name is already running on every host and reuses it. +- With ``lifetime: no_launch``, CVS verifies that a container with the configured name is already running on every host and reuses it. - All ``rvs`` invocations are routed through the container via ``docker exec`` and the in-container ``sshd``. Step 4: Verify the run actually used the container @@ -106,11 +106,11 @@ The ``container.lifetime`` policy controls who owns the container lifecycle: - ``per_run`` (default) - CVS starts a fresh container at setup and force-removes it at teardown. Anything written to the container overlay is lost when the run ends. - ``persistent`` - CVS attaches to the container if it is already running on every host, or starts it fresh if it is running on no host. Teardown is a no-op, so the container (and its overlay) survives across runs. This unblocks install-then-run workflows: ``cvs run install_rvs`` followed by ``cvs run rvs_cvs`` in separate invocations. If the container is running on some hosts but not all, CVS fails rather than rebuilding (which would destroy the overlay on the still-running hosts) -- remove it on all hosts and rerun, or restart it on the missing hosts. Pin ``container.name`` so a tag bump does not silently abandon the overlay. -- ``external`` - CVS does not start anything. It verifies that a container with the configured name is already running on every host and reuses it. Teardown is a no-op. +- ``no_launch`` - CVS does not start anything. It verifies that a container with the configured name is already running on every host and reuses it. Teardown is a no-op. See the ``lifetime`` truth table in :doc:`/reference/configuration-files/cluster-file` for the full state matrix. -To stop and remove a container that CVS left running (``persistent`` or ``external``), run on every node: +To stop and remove a container that CVS left running (``persistent`` or ``no_launch``), run on every node: .. code:: bash diff --git a/docs/reference/configuration-files/cluster-file.rst b/docs/reference/configuration-files/cluster-file.rst index e803ec13..830d5dd0 100644 --- a/docs/reference/configuration-files/cluster-file.rst +++ b/docs/reference/configuration-files/cluster-file.rst @@ -138,7 +138,7 @@ The ``container`` block configures the container backend. It is consumed by the - Description * - ``lifetime`` - ``per_run`` - - Container lifecycle policy: ``external``, ``per_run``, or ``persistent``. See the truth table below. + - Container lifecycle policy: ``no_launch``, ``per_run``, or ``persistent``. See the truth table below. * - ``image`` - (required) - Image with the test dependencies (for example ``rvs``) the suite invokes. Must be present locally on each node or pullable from a reachable registry. An in-container ``sshd`` is not required up front; it is installed at launch by ``setup_script``. @@ -211,9 +211,9 @@ When ``runtime.name`` is ``docker``, the keys below configure the underlying ``d * - ``lifetime`` - ``setup_containers`` - ``teardown_containers`` - * - ``external`` + * - ``no_launch`` - Verify a container with the configured name is already running on every host; set ``container_id``. Never starts anything. - - No-op. CVS does not own externally managed containers. + - No-op. CVS does not own a container it did not launch. * - ``per_run`` (default) - Start a fresh container on every host (force-removing any stale same-named container first). - Force-remove the container CVS started. From ec87e4f8ab41cf87b74da603432529d187960399 Mon Sep 17 00:00:00 2001 From: Atul Nair Date: Tue, 9 Jun 2026 16:02:13 -0400 Subject: [PATCH 18/20] orchestrators: replace container enabled/launch with container.lifetime Collapse the two-axis container config (enabled + launch) into a single tri-valued container.lifetime: no_launch, per_run (default), persistent. - factory: resolve/validate container.lifetime; enabled and launch are removed fields that hard-error with migration guidance. - container: branch setup_containers/teardown_containers on lifetime. persistent attaches when the container runs on every host (with a per-host image-SHA check), cold-starts when it runs on no host, and hard-fails when it runs on some hosts but not all (relaunching would force-remove and rebuild the still-running hosts, destroying their overlay). - runtimes: add image_sha_status to the ContainerRuntime protocol and the docker/enroot backends; drop the launch short-circuit so the runtime always renders docker run when invoked. - cluster file sample, README, and docs: document the lifetime truth table (replacing enabled/launch). - unittests: cover lifetime resolution and per-lifetime setup/teardown. --- cvs/core/orchestrators/container.py | 243 ++++----------- cvs/core/orchestrators/factory.py | 66 +--- .../scripts/default_container_setup.sh | 20 -- .../orchestrators/unittests/test_container.py | 285 ++++-------------- .../orchestrators/unittests/test_factory.py | 112 ------- cvs/core/runtimes/base.py | 10 - cvs/core/runtimes/docker.py | 46 +-- cvs/core/runtimes/enroot.py | 5 - cvs/input/cluster_file/README.md | 11 +- cvs/input/cluster_file/cluster_container.json | 5 +- docs/how-to/run-with-containers.rst | 11 +- .../configuration-files/cluster-file.rst | 10 +- 12 files changed, 152 insertions(+), 672 deletions(-) delete mode 100644 cvs/core/orchestrators/scripts/default_container_setup.sh diff --git a/cvs/core/orchestrators/container.py b/cvs/core/orchestrators/container.py index a860e456..3116a4e2 100644 --- a/cvs/core/orchestrators/container.py +++ b/cvs/core/orchestrators/container.py @@ -6,7 +6,6 @@ ''' from cvs.core.orchestrators.baremetal import BaremetalOrchestrator -import base64 import getpass import re from cvs.core.runtimes import RuntimeFactory @@ -43,14 +42,6 @@ "privileged": True, } -# Upper bound on a setup_script that can be delivered inline. _provision_container -# ships the script base64-encoded inside the `docker exec ... bash -c` command -# string, which rides the SSH exec channel (bounded to ~30 KB by libssh2 in -# cvs/lib/parallel/pssh.py). base64 inflates by ~4/3, so cap the raw script well -# under that so a too-large script fails with a clear message instead of an -# opaque SSH/exec truncation error. -MAX_INLINE_SETUP_SCRIPT_BYTES = 16384 - class ContainerOrchestrator(BaremetalOrchestrator): """ @@ -260,6 +251,35 @@ def is_privileged(self): runtime_args = runtime_config.get('args', {}) return runtime_args.get('privileged', DEFAULT_CONTAINER_ARGS['privileged']) + def _partition_by_status(self, status): + """Partition the expected hosts into (running, absent, probe_failed). + + The single source of truth for interpreting a runtime is_running() result. + Both the persistent setup branch and verify_containers_running consume this + so they cannot drift apart on what counts as running vs unreachable. + + running : probe succeeded and the named container is up. + absent : probe succeeded and no such container is running. + probe_failed : the probe cannot be trusted -- non-zero exit_code (an + SSH/sudo/docker error or a timeout) OR the host is missing + from status entirely (pruned as unreachable by an earlier + command). Either way the container's true state is unknown. + + Iterates the expected host set (self.hosts), not status.keys(), so a host + that dropped out of the probe is surfaced as probe_failed rather than + silently ignored. + """ + running, absent, probe_failed = [], [], [] + for host in self.hosts: + info = status.get(host) + if info is None or info.get('exit_code') != 0: + probe_failed.append(host) + elif info.get('running'): + running.append(host) + else: + absent.append(host) + return running, absent, probe_failed + def setup_containers( self, volumes=None, @@ -278,8 +298,8 @@ def setup_containers( - 'no_launch' : verify a container with the configured name is running and set container_id; never starts anything. - 'per_run' : start fresh containers on all hosts. - - 'persistent' : attach to a container already running on all hosts (with - an image-SHA check), otherwise start fresh. Idempotent. + - 'persistent' : attach to a container already running on all hosts, + otherwise start fresh. Idempotent. Args: volumes: Optional list of volume mounts (uses standards if not provided) @@ -307,16 +327,29 @@ def setup_containers( if lifetime == 'persistent': status = self.runtime.is_running(container_name) - running_hosts = [h for h, info in status.items() if info.get('running')] - missing_hosts = [h for h, info in status.items() if not info.get('running')] + running_hosts, absent_hosts, probe_failed_hosts = self._partition_by_status(status) + + if probe_failed_hosts: + # The probe could not be trusted on these hosts (SSH/sudo/docker + # error, timeout, or the host dropped out). We cannot tell "absent" + # from "actually running but unreachable", and treating unreachable + # as absent would let the cold-start path below force-remove a + # container that is in fact running -- destroying its overlay. Refuse. + self.log.error( + f"Cannot determine state of persistent container '{container_name}' on " + f"{probe_failed_hosts} (probe failed: SSH/sudo/docker error, timeout, or " + f"host unreachable). Refusing to act on an untrustworthy probe -- resolve " + f"host/daemon health and rerun." + ) + return False - if running_hosts and not missing_hosts: - # Running on every host: attach (with image-SHA check). + if running_hosts and not absent_hosts: + # Running on every host: attach. self.container_id = container_name self.log.info(f"Attaching to running container '{container_name}'") - return self._verify_persistent_image(container_name, image) + return True - if running_hosts and missing_hosts: + if running_hosts and absent_hosts: # Partial: refuse to auto-relaunch. _launch_containers force-removes # the same-named container on ALL hosts before recreating, which would # destroy the overlay (installs, clones) on the still-running hosts -- @@ -325,14 +358,14 @@ def setup_containers( # restart the container on the missing hosts to reattach. self.log.error( f"Persistent container '{container_name}' is running on {running_hosts} " - f"but missing on {missing_hosts}. Refusing to auto-relaunch: that would " + f"but missing on {absent_hosts}. Refusing to auto-relaunch: that would " f"force-remove and rebuild the containers on the still-running hosts, " f"destroying their overlay. Either remove '{container_name}' on all hosts " - f"and rerun, or restart it on {missing_hosts} to reattach." + f"and rerun, or restart it on {absent_hosts} to reattach." ) return False - # Not running on any host: legitimate cold start, launch fresh on all. + # Genuinely absent on every host: legitimate cold start, launch fresh on all. self.log.info("Persistent container not running on any host, launching...") return self._launch_containers(volumes, devices, capabilities, security_opts, environment, groups, ulimits) @@ -401,130 +434,7 @@ def _launch_containers( ulimits=ulimits, device_expansion=ib_device_expansion, ) - if not launched: - return False - - # Provision the freshly-launched container (install packages on top of - # the base image, e.g. openssh-server). Runs only on this fresh-start - # path, so 'no_launch' and 'persistent'-attach skip it automatically. - return self._provision_container() - - def _provision_container(self): - """Run the configured setup_script inside the freshly-launched container. - - Reads the resolved ``container.setup_script`` on the control host, - base64-encodes it, and executes it inside the container on every host via - ``docker exec`` (``self.exec``), which works before sshd exists -- the same - mechanism ``setup_sshd`` uses. The default script installs - ``openssh-server`` so the subsequent ``setup_sshd`` can start sshd; a - user-supplied script can install anything else the base image lacks. - - Returns: - bool: True if provisioning succeeded on all hosts (or no script is - configured), False otherwise. - """ - setup_script = self.container_config.get('setup_script') - if not setup_script: - # Defensive: the factory always resolves a default for container - # configs, so this only triggers for a hand-built config dict. - return True - - try: - with open(setup_script, 'rb') as f: - script_bytes = f.read() - except OSError as exc: - self.log.error(f"Cannot read container setup_script {setup_script!r}: {exc}") - return False - - if len(script_bytes) > MAX_INLINE_SETUP_SCRIPT_BYTES: - # Delivered inline over the SSH exec channel; a too-large script would - # otherwise fail with an opaque truncation error mid-run. - self.log.error( - f"container.setup_script {setup_script!r} is too large for inline " - f"delivery ({len(script_bytes)} bytes > {MAX_INLINE_SETUP_SCRIPT_BYTES}). " - f"Slim the script, or bake the packages into the image." - ) - return False - - self.log.info(f"Provisioning containers via setup_script: {setup_script}") - encoded = base64.b64encode(script_bytes).decode('ascii') - # docker exec already wraps the command in `bash -c`, so decode the - # script and pipe it straight into bash. base64 sidesteps all quoting and - # newline issues with arbitrary script content over pssh. `set -o pipefail` - # is required: without it the pipeline's exit code is bash's (the last - # stage), so a missing/failing `base64` in the image would exit 0 and the - # provisioning would silently no-op, surfacing later as an opaque sshd - # failure. pipefail makes that fail here with a diagnosable error instead. - cmd = f"set -o pipefail; echo {encoded} | base64 -d | bash" - result = self.exec(cmd, timeout=600, detailed=True) - - ok = True - for hostname, output in result.items(): - if output.get('exit_code') != 0: - # Surface the in-container stderr/stdout so a failed apt/bash is - # diagnosable from the log without re-running by hand. - detail = (output.get('output') or '').strip() - self.log.error( - f"Container provisioning failed on {hostname} " - f"(setup_script: {setup_script}, exit_code: {output.get('exit_code')})" - + (f": {detail}" if detail else "") - ) - ok = False - if not ok: - return False - self.log.info("Container provisioning succeeded on all hosts") - return True - - def _verify_persistent_image(self, container_name, image): - """Compare the running container's image SHA to the local image tag on - each host (persistent attach only). - - This runs only after ``is_running`` confirmed the container is up on every - host, so every host MUST yield a readable container SHA. - - - Unreadable SHA on any host (probe failed / container vanished): ERROR - and return False -- we cannot vouch for consistency, so do not silently - pass. - - Per-host mismatch (container created from an image older than the local - ```` tag): WARN and continue -- the overlay may be stale. - - Cross-host SHA skew (hosts running different image SHAs): ERROR and - return False -- a correctness problem, not mere staleness. - - Returns: - bool: False on unreadable SHA or cross-host skew, True otherwise. - """ - status = self.runtime.image_sha_status(container_name, image) - container_shas = set() - unreadable_hosts = [] - for host, info in status.items(): - container_sha = info.get('container_sha', '') - image_sha = info.get('image_sha', '') - if not container_sha: - unreadable_hosts.append(host) - continue - container_shas.add(container_sha) - if image_sha and container_sha != image_sha: - self.log.warning( - f"Container '{container_name}' on {host} runs image " - f"{container_sha[:19]} but local '{image}' is {image_sha[:19]}; " - f"overlay may be stale" - ) - - if unreadable_hosts: - self.log.error( - f"Could not read the running image SHA for container " - f"'{container_name}' on {unreadable_hosts}; cannot verify image " - f"consistency across hosts" - ) - return False - - if len(container_shas) > 1: - self.log.error( - f"Cross-host image SHA skew for container '{container_name}': " - f"hosts are running different images {sorted(container_shas)}" - ) - return False - return True + return launched def setup_sshd(self): """ @@ -547,19 +457,6 @@ def setup_sshd(self): self.log.info(f"Setting up SSH daemon in containers: {self.container_id}") - # Idempotency precheck (required by lifetime: persistent): on a second - # `cvs run` the container's sshd is already bound to 2224, so re-running - # `/usr/sbin/sshd -p2224` would fail with "address already in use". Skip - # the setup commands on any host that already has sshd on 2224. - # Pattern uses the `[s]shd` trick so pgrep -f does not match its own - # parent shell (whose argv contains the pattern); a literal 'sshd.*2224' - # self-matches and makes this precheck always report "already running". - precheck = self.exec("pgrep -f '[s]shd.*2224' > /dev/null 2>&1", timeout=10, detailed=True) - hosts_needing_sshd = [host for host, output in precheck.items() if output['exit_code'] != 0] - if not hosts_needing_sshd: - self.log.info("SSH daemon already running on all hosts, skipping setup") - return True - # Execute SSH setup commands # Note: Commands with shell operators must be wrapped in bash -c for proper execution inside container ssh_setup_commands = [ @@ -572,8 +469,8 @@ def setup_sshd(self): ] for cmd in ssh_setup_commands: - result = self.exec(cmd, hosts=hosts_needing_sshd, timeout=10, detailed=True) - # Check if command succeeded on all targeted hosts + result = self.exec(cmd, timeout=10, detailed=True) + # Check if command succeeded on all hosts for hostname, output in result.items(): if output['exit_code'] != 0: self.log.error(f"SSH setup command failed on {hostname}: {cmd}") @@ -584,9 +481,8 @@ def setup_sshd(self): time.sleep(2) - # Validate sshd is running by checking process. Same `[s]shd` trick as the - # precheck so this validates the real sshd, not pgrep's own parent shell. - check_cmd = "pgrep -f '[s]shd.*2224' > /dev/null 2>&1" + # Validate sshd is running by checking process + check_cmd = "pgrep -f 'sshd.*2224' > /dev/null 2>&1" result = self.exec(check_cmd, timeout=10, detailed=True) # Check if all hosts have sshd running @@ -610,21 +506,14 @@ def verify_containers_running(self, container_name): bool: True if container is running on all hosts, False otherwise """ self.log.debug(f"Checking if container '{container_name}' is running on all hosts") - result = self.runtime.is_running(container_name) - - # Verify container is running on all hosts - failed_hosts = [] - for host, info in result.items(): - exit_code = info.get('exit_code') - if exit_code != 0: - failed_hosts.append(f"{host} (exit code {exit_code})") - continue - if not info.get('running'): - running_name = info.get('name', '') - failed_hosts.append(f"{host} (container not running, found: '{running_name}')") - - if failed_hosts: - self.log.error(f"Container '{container_name}' not running on hosts: {failed_hosts}") + status = self.runtime.is_running(container_name) + _running, absent_hosts, probe_failed_hosts = self._partition_by_status(status) + + if absent_hosts or probe_failed_hosts: + self.log.error( + f"Container '{container_name}' not running on all hosts: " + f"not running on {absent_hosts}, probe failed on {probe_failed_hosts}" + ) return False self.container_id = container_name diff --git a/cvs/core/orchestrators/factory.py b/cvs/core/orchestrators/factory.py index a0ee8891..7aaa0258 100644 --- a/cvs/core/orchestrators/factory.py +++ b/cvs/core/orchestrators/factory.py @@ -5,22 +5,12 @@ All code contained here is Property of Advanced Micro Devices, Inc. ''' -import os - from cvs.core.orchestrators.baremetal import BaremetalOrchestrator from cvs.core.orchestrators.container import ContainerOrchestrator VALID_CONTAINER_LIFETIMES = ("no_launch", "per_run", "persistent") -# Packaged default provisioning script, run inside each freshly-launched -# container when container.setup_script is not set. Installs openssh-server so -# the in-container sshd can start on port 2224. Resolved __file__-relative so it -# works in both editable and wheel installs. -DEFAULT_CONTAINER_SETUP_SCRIPT = os.path.join( - os.path.dirname(os.path.abspath(__file__)), "scripts", "default_container_setup.sh" -) - def _resolve_container_lifetime(container): """Normalize a container config block to a single resolved ``lifetime`` key. @@ -64,46 +54,6 @@ def _resolve_container_lifetime(container): return container -def _resolve_container_setup_script(container): - """Resolve ``container.setup_script`` to a concrete, existing file path. - - The script is run inside each freshly-launched container (see - ``ContainerOrchestrator._provision_container``) to install packages on top - of the base image. Resolution rules: - - - empty/absent container block (baremetal path) -> returned untouched. - - ``setup_script`` set -> validated to exist on - the control host; ``ValueError`` (fail fast at config load) if missing. - - ``setup_script`` absent -> defaults to the - packaged ``default_container_setup.sh`` (installs openssh-server). - - Relative paths are resolved against the current working directory of the - process running ``cvs``. Mutates and returns the passed dict. - """ - if not container: - return container - - setup_script = container.get('setup_script') - if setup_script: - resolved = os.path.abspath(os.path.expanduser(setup_script)) - if not os.path.isfile(resolved): - raise ValueError(f"container.setup_script not found: {setup_script!r} (resolved to {resolved!r})") - container['setup_script'] = resolved - return container - - # No user-supplied script: fall back to the packaged default. Validate it - # exists here (same fail-fast as a user path) so a broken/incomplete install - # surfaces at config load rather than as an OSError mid-run inside - # _provision_container. - if not os.path.isfile(DEFAULT_CONTAINER_SETUP_SCRIPT): - raise ValueError( - f"packaged default container setup script is missing: " - f"{DEFAULT_CONTAINER_SETUP_SCRIPT!r} (broken CVS install?)" - ) - container['setup_script'] = DEFAULT_CONTAINER_SETUP_SCRIPT - return container - - class OrchestratorConfig: """ Configuration for orchestrator creation. @@ -138,8 +88,7 @@ class OrchestratorConfig: } }, "image": "rocm/cvs:latest", - "name": "myuser_rocm_cvs_latest", - "setup_script": "/path/to/setup.sh" + "name": "myuser_rocm_cvs_latest" } ``` lifetime: Container lifecycle policy [default: 'per_run'] @@ -150,11 +99,6 @@ class OrchestratorConfig: - 'persistent' : start if absent / attach if present; never torn down by the run. Pin container.name explicitly under this mode (the default _ name shifts on tag bumps). - setup_script: Optional path to a shell script run inside each freshly - launched container (per_run, and persistent when cold-started) before - sshd setup, to install packages on top of the base image. Omit to use - the packaged default that installs openssh-server. A non-existent path - fails at config load. """ def __init__(self, **kwargs): @@ -182,10 +126,8 @@ def __init__(self, **kwargs): # Normalize the container block. This is the single chokepoint: # from_configs constructs via cls(**required_config), so both file-driven # and direct programmatic construction hit the same normalization (and the - # same enabled-removed / launch-deprecated errors, and the same - # setup_script validation / default injection). - container = _resolve_container_lifetime(kwargs.get('container', {})) - self.container = _resolve_container_setup_script(container) + # same enabled-removed / launch-removed errors). + self.container = _resolve_container_lifetime(kwargs.get('container', {})) def get(self, key, default=None): """Get configuration value with default.""" @@ -210,7 +152,7 @@ def from_configs(cls, cluster_config, testsuite_config=None): Required keys: orchestrator, node_dict, username, priv_key_file Optional keys: container, head_node_dict, password (defaults provided for missing optional keys) - Container structure: {lifetime: 'no_launch'|'per_run'|'persistent', runtime: {name: str, args: dict}, image: str, name: str, setup_script: str, ...} + Container structure: {lifetime: 'no_launch'|'per_run'|'persistent', runtime: {name: str, args: dict}, image: str, name: str, ...} testsuite_config: Test suite specific configuration (dict or path to _config.json) Can override any keys from cluster_config diff --git a/cvs/core/orchestrators/scripts/default_container_setup.sh b/cvs/core/orchestrators/scripts/default_container_setup.sh deleted file mode 100644 index 5cb32cb8..00000000 --- a/cvs/core/orchestrators/scripts/default_container_setup.sh +++ /dev/null @@ -1,20 +0,0 @@ -#!/usr/bin/env bash -# Default CVS container provisioning script. -# -# Run inside each freshly-launched container (via `docker exec`) before -# setup_sshd. CVS's container exec model needs an in-container sshd on port -# 2224; many base images do not ship one. This installs only the sshd binary -# (openssh-server) so the existing setup_sshd can start `/usr/sbin/sshd -p2224`. -# -# Override with container.setup_script in the cluster file to install other -# packages (or to support non-apt base images -- this default is apt-only). -set -euo pipefail - -if command -v sshd >/dev/null 2>&1 || [ -x /usr/sbin/sshd ]; then - echo "default_container_setup: sshd already present, nothing to install" - exit 0 -fi - -export DEBIAN_FRONTEND=noninteractive -apt-get update -apt-get install -y --no-install-recommends openssh-server diff --git a/cvs/core/orchestrators/unittests/test_container.py b/cvs/core/orchestrators/unittests/test_container.py index ae081d1b..70c03f0b 100644 --- a/cvs/core/orchestrators/unittests/test_container.py +++ b/cvs/core/orchestrators/unittests/test_container.py @@ -14,20 +14,15 @@ # no_launch / per_run / persistent contract has a loud canary. Pssh + RuntimeFactory are # patched once in setUp (not per method); _make() returns a fresh orch + runtime mock. -import base64 import unittest import warnings -from unittest.mock import MagicMock, mock_open, patch +from unittest.mock import MagicMock, patch from cvs.core.orchestrators.factory import OrchestratorConfig, _resolve_container_lifetime -from cvs.core.orchestrators.container import ContainerOrchestrator, MAX_INLINE_SETUP_SCRIPT_BYTES +from cvs.core.orchestrators.container import ContainerOrchestrator -# Reusable runtime.exec / is_running / image_sha_status fixtures (two-host cluster). -_OK = { - "10.0.0.1": {"exit_code": 0, "output": ""}, - "10.0.0.2": {"exit_code": 0, "output": ""}, -} +# Reusable runtime.is_running fixtures (two-host cluster). _RUNNING = { "10.0.0.1": {"running": True, "exit_code": 0, "name": "cvs_iter_test"}, "10.0.0.2": {"running": True, "exit_code": 0, "name": "cvs_iter_test"}, @@ -36,9 +31,17 @@ "10.0.0.1": {"running": False, "exit_code": 0, "name": ""}, "10.0.0.2": {"running": False, "exit_code": 0, "name": ""}, } -_SHA_MATCH = { - "10.0.0.1": {"container_sha": "sha:abc", "image_sha": "sha:abc"}, - "10.0.0.2": {"container_sha": "sha:abc", "image_sha": "sha:abc"}, +# Probe itself failed on every host (non-zero exit: SSH/sudo/docker error or +# timeout). 'running' is False but it is NOT trustworthy -- state is unknown. +_PROBE_FAILED = { + "10.0.0.1": {"running": False, "exit_code": 1, "name": ""}, + "10.0.0.2": {"running": False, "exit_code": -1, "name": ""}, +} +# Container up on one host; probe aborted (SSH timeout, exit_code -1) on the +# other. The aborted host must be treated as unknown, never as "absent". +_RUNNING_PLUS_PROBE_FAILED = { + "10.0.0.1": {"running": True, "exit_code": 0, "name": "cvs_iter_test"}, + "10.0.0.2": {"running": False, "exit_code": -1, "name": ""}, } @@ -117,8 +120,6 @@ def test_init_requires_container_config(self): def test_setup_containers_per_run_delegates_to_runtime(self): orch, runtime = self._make(lifetime="per_run") runtime.setup_containers.return_value = True - # The fresh-launch path also provisions via self.exec (-> runtime.exec). - runtime.exec.return_value = _OK self.assertTrue(orch.setup_containers()) runtime.setup_containers.assert_called_once() @@ -132,19 +133,17 @@ def test_setup_containers_no_launch_verifies_only(self): def test_setup_containers_no_launch_not_running_fails(self): # no_launch + container not actually running -> verification fails; nothing - # is launched and nothing is provisioned. + # is launched. orch, runtime = self._make(lifetime="no_launch") runtime.is_running.return_value = { "10.0.0.1": {"running": False, "exit_code": 0, "name": ""}, } self.assertFalse(orch.setup_containers()) runtime.setup_containers.assert_not_called() - runtime.exec.assert_not_called() def test_setup_containers_persistent_attaches_when_running(self): orch, runtime = self._make(lifetime="persistent") runtime.is_running.return_value = _RUNNING - runtime.image_sha_status.return_value = _SHA_MATCH self.assertTrue(orch.setup_containers()) # Attach path: no new container started. runtime.setup_containers.assert_not_called() @@ -154,47 +153,18 @@ def test_setup_containers_persistent_starts_when_not_running(self): orch, runtime = self._make(lifetime="persistent") runtime.is_running.return_value = _NOT_RUNNING runtime.setup_containers.return_value = True - runtime.exec.return_value = _OK self.assertTrue(orch.setup_containers()) runtime.setup_containers.assert_called_once() - # No SHA check when launching fresh. - runtime.image_sha_status.assert_not_called() def test_setup_containers_persistent_idempotent_on_resetup(self): # Re-running setup against an already-running persistent container is a # no-op attach both times -- never starts a new container. orch, runtime = self._make(lifetime="persistent") - runtime.is_running.return_value = { - "10.0.0.1": {"running": True, "exit_code": 0, "name": "cvs_iter_test"}, - } - runtime.image_sha_status.return_value = { - "10.0.0.1": {"container_sha": "sha:abc", "image_sha": "sha:abc"}, - } + runtime.is_running.return_value = _RUNNING self.assertTrue(orch.setup_containers()) self.assertTrue(orch.setup_containers()) runtime.setup_containers.assert_not_called() - def test_setup_containers_persistent_cross_host_sha_skew_errors(self): - # Nodes running different image SHAs is a correctness error, not staleness. - orch, runtime = self._make(lifetime="persistent") - runtime.is_running.return_value = _RUNNING - runtime.image_sha_status.return_value = { - "10.0.0.1": {"container_sha": "sha:abc", "image_sha": "sha:abc"}, - "10.0.0.2": {"container_sha": "sha:def", "image_sha": "sha:def"}, - } - self.assertFalse(orch.setup_containers()) - - def test_setup_containers_persistent_stale_overlay_warns_but_passes(self): - # Per-host staleness (container older than local image tag) warns, does not fail. - orch, runtime = self._make(lifetime="persistent") - runtime.is_running.return_value = { - "10.0.0.1": {"running": True, "exit_code": 0, "name": "cvs_iter_test"}, - } - runtime.image_sha_status.return_value = { - "10.0.0.1": {"container_sha": "sha:old", "image_sha": "sha:new"}, - } - self.assertTrue(orch.setup_containers()) - def test_setup_containers_persistent_partial_running_refuses(self): # Running on some hosts but not all must NOT auto-relaunch (that would # force-remove and rebuild the still-running hosts, destroying their @@ -206,19 +176,61 @@ def test_setup_containers_persistent_partial_running_refuses(self): } self.assertFalse(orch.setup_containers()) runtime.setup_containers.assert_not_called() - # No image-SHA attach check either; it bailed before attaching. - runtime.image_sha_status.assert_not_called() - def test_setup_containers_persistent_unreadable_sha_fails(self): - # Attach path: container is running on all hosts but the image-SHA probe - # yields no SHA -> cannot verify consistency, so fail rather than silently pass. + def test_setup_containers_persistent_probe_failed_refuses_and_does_not_launch(self): + # Probe failed on every host (non-zero exit). The container's true state + # is unknown -- treating it as "absent" and cold-starting would force-remove + # a possibly-running container and destroy its overlay. Refuse, launch nothing. orch, runtime = self._make(lifetime="persistent") - runtime.is_running.return_value = _RUNNING - runtime.image_sha_status.return_value = { - "10.0.0.1": {"container_sha": "", "image_sha": ""}, - "10.0.0.2": {"container_sha": "", "image_sha": ""}, + runtime.is_running.return_value = _PROBE_FAILED + self.assertFalse(orch.setup_containers()) + runtime.setup_containers.assert_not_called() + + def test_setup_containers_persistent_running_plus_probe_failed_refuses(self): + # Up on one host, probe aborted on the other. The aborted host must not be + # bucketed as "absent" (which would trip the partial-relaunch path); an + # untrustworthy probe is its own hard stop. Nothing is launched. + orch, runtime = self._make(lifetime="persistent") + runtime.is_running.return_value = _RUNNING_PLUS_PROBE_FAILED + self.assertFalse(orch.setup_containers()) + runtime.setup_containers.assert_not_called() + + def test_setup_containers_persistent_host_dropped_from_status_refuses(self): + # A host expected by node_dict is absent from is_running's result entirely + # (pruned as unreachable by an earlier command). It must surface as a probe + # failure, not be silently ignored into a false "running on all" attach. + orch, runtime = self._make(lifetime="persistent") + runtime.is_running.return_value = { + "10.0.0.1": {"running": True, "exit_code": 0, "name": "cvs_iter_test"}, + # 10.0.0.2 missing } self.assertFalse(orch.setup_containers()) + runtime.setup_containers.assert_not_called() + self.assertIsNone(orch.container_id) + + def test_partition_by_status_three_buckets(self): + # Direct test of the shared partition helper: one host per bucket. + orch, _ = self._make(lifetime="persistent") + orch.hosts = ["h_run", "h_absent", "h_failed"] + status = { + "h_run": {"running": True, "exit_code": 0, "name": "cvs_iter_test"}, + "h_absent": {"running": False, "exit_code": 0, "name": ""}, + "h_failed": {"running": False, "exit_code": 1, "name": ""}, + } + running, absent, probe_failed = orch._partition_by_status(status) + self.assertEqual(running, ["h_run"]) + self.assertEqual(absent, ["h_absent"]) + self.assertEqual(probe_failed, ["h_failed"]) + + def test_partition_by_status_missing_host_is_probe_failed(self): + # A host expected in self.hosts but absent from status -> probe_failed. + orch, _ = self._make(lifetime="persistent") + orch.hosts = ["h_run", "h_gone"] + status = {"h_run": {"running": True, "exit_code": 0, "name": "cvs_iter_test"}} + running, absent, probe_failed = orch._partition_by_status(status) + self.assertEqual(running, ["h_run"]) + self.assertEqual(absent, []) + self.assertEqual(probe_failed, ["h_gone"]) # ------------------------------------------------------------------ # teardown_containers lifetime branching @@ -256,169 +268,6 @@ def test_teardown_containers_short_circuits_when_no_container_id(self): self.assertTrue(orch.teardown_containers()) runtime.teardown_containers.assert_not_called() - # ------------------------------------------------------------------ - # setup_sshd (required by lifetime: persistent) - # ------------------------------------------------------------------ - - def test_setup_sshd_idempotent_when_already_running(self): - # When the pgrep precheck reports sshd already on 2224 for every host, the - # start commands must NOT run (would re-bind the port and fail). - orch, runtime = self._make(lifetime="persistent") - orch.container_id = "cvs_iter_test" - runtime.exec.return_value = _OK - self.assertTrue(orch.setup_sshd()) - # Only the precheck exec happened; no setup commands were issued. - runtime.exec.assert_called_once() - # Guard the subtle self-match fix: the precheck must use the `[s]shd` - # trick, not a literal 'sshd.*2224' (which matches pgrep's own parent - # shell and makes this precheck always report "already running"). - precheck_cmd = runtime.exec.call_args[0][1] - self.assertIn("[s]shd.*2224", precheck_cmd) - self.assertNotIn("'sshd.*2224'", precheck_cmd) - - @patch("time.sleep") - def test_setup_sshd_installs_and_validates_when_not_running(self, _mock_sleep): - # The other setup_sshd branch: precheck reports NO sshd -> the start - # commands run (incl. /usr/sbin/sshd -p2224) and the post-start validation - # runs. Both pgrep sites must use the non-self-matching [s]shd pattern. - orch, runtime = self._make(lifetime="per_run") - orch.container_id = "cvs_iter_test" - need = { - "10.0.0.1": {"exit_code": 1, "output": ""}, - "10.0.0.2": {"exit_code": 1, "output": ""}, - } - # precheck (needs sshd) -> 6 ssh_setup_commands -> post-start validation. - runtime.exec.side_effect = [need] + [_OK] * 6 + [_OK] - self.assertTrue(orch.setup_sshd()) - issued = [c[0][1] for c in runtime.exec.call_args_list] - self.assertIn("/usr/sbin/sshd -p2224", issued) - # precheck (first) and post-start validation (last) both use the trick. - self.assertIn("[s]shd.*2224", issued[0]) - self.assertIn("[s]shd.*2224", issued[-1]) - - # ------------------------------------------------------------------ - # Container provisioning (setup_script) -- runs only on fresh launch. - # ------------------------------------------------------------------ - - def test_provisioning_runs_only_on_fresh_launch(self): - # Dispatch matrix: provisioning (one exec) happens only on a fresh launch - # (per_run, persistent cold-start); no_launch and persistent-attach skip it. - # (name, lifetime, is_running, image_sha, expect_provision) - cases = [ - ("per_run", "per_run", None, None, True), - ("persistent_cold", "persistent", _NOT_RUNNING, None, True), - ("no_launch", "no_launch", _RUNNING, None, False), - ("persistent_attach", "persistent", _RUNNING, _SHA_MATCH, False), - ] - for name, lifetime, is_running, image_sha, expect in cases: - with self.subTest(name): - orch, runtime = self._make(lifetime=lifetime) - runtime.setup_containers.return_value = True - runtime.exec.return_value = _OK - if is_running is not None: - runtime.is_running.return_value = is_running - if image_sha is not None: - runtime.image_sha_status.return_value = image_sha - self.assertTrue(orch.setup_containers()) - if expect: - runtime.exec.assert_called_once() - provision_cmd = runtime.exec.call_args[0][1] - self.assertIn("base64 -d", provision_cmd) - self.assertIn("| bash", provision_cmd) - else: - runtime.exec.assert_not_called() - - def test_provisioning_size_guard(self): - # Strict `>`: exactly MAX bytes proceeds; MAX+1 is rejected before any exec. - cases = [ - ("at_limit_ok", b"x" * MAX_INLINE_SETUP_SCRIPT_BYTES, True), - ("over_limit_rejected", b"x" * (MAX_INLINE_SETUP_SCRIPT_BYTES + 1), False), - ] - for name, payload, expect_ok in cases: - with self.subTest(name): - orch, runtime = self._make(lifetime="per_run") - orch.container_id = "cvs_iter_test" - runtime.exec.return_value = {"10.0.0.1": {"exit_code": 0, "output": ""}} - with patch("builtins.open", mock_open(read_data=payload)): - self.assertEqual(orch._provision_container(), expect_ok) - if expect_ok: - runtime.exec.assert_called_once() - else: - runtime.exec.assert_not_called() - - def test_provisioning_failure_fails_launch(self): - # A non-zero provisioning exit on any host fails setup_containers. - orch, runtime = self._make(lifetime="per_run") - runtime.setup_containers.return_value = True - runtime.exec.return_value = { - "10.0.0.1": {"exit_code": 0, "output": ""}, - "10.0.0.2": {"exit_code": 100, "output": "apt failed"}, - } - self.assertFalse(orch.setup_containers()) - - def test_provisioning_not_attempted_when_runtime_launch_fails(self): - # If the container failed to start, provisioning must not be attempted. - orch, runtime = self._make(lifetime="per_run") - runtime.setup_containers.return_value = False - self.assertFalse(orch.setup_containers()) - runtime.exec.assert_not_called() - - def test_provisioning_noop_when_no_setup_script(self): - # Defensive skip: a hand-built config with no setup_script provisions nothing. - orch, runtime = self._make(lifetime="per_run") - orch.container_id = "cvs_iter_test" - orch.container_config["setup_script"] = None - self.assertTrue(orch._provision_container()) - runtime.exec.assert_not_called() - - def test_provisioning_failure_logs_every_host_with_detail(self): - # F1: all failing hosts are logged (not just the first), each with its - # captured stderr/stdout so the failure is diagnosable from the log. - orch, runtime = self._make(lifetime="per_run") - orch.container_id = "cvs_iter_test" - runtime.exec.return_value = { - "10.0.0.1": {"exit_code": 100, "output": "apt boom one"}, - "10.0.0.2": {"exit_code": 5, "output": "apt boom two"}, - } - self.assertFalse(orch._provision_container()) - logged = " ".join(str(c) for c in orch.log.error.call_args_list) - self.assertIn("10.0.0.1", logged) - self.assertIn("apt boom one", logged) - self.assertIn("10.0.0.2", logged) - self.assertIn("apt boom two", logged) - - def test_provisioning_ships_the_user_supplied_script(self): - # A user-supplied setup_script (not just the default) is the payload shipped. - orch, runtime = self._make(lifetime="per_run") - orch.container_id = "cvs_iter_test" - custom = b"#!/bin/bash\necho custom-provision\napt-get install -y foo\n" - runtime.exec.return_value = {"10.0.0.1": {"exit_code": 0, "output": ""}} - with patch("builtins.open", mock_open(read_data=custom)): - self.assertTrue(orch._provision_container()) - cmd = runtime.exec.call_args[0][1] - encoded = cmd.split("echo ", 1)[1].split(" | base64", 1)[0] - self.assertEqual(base64.b64decode(encoded), custom) - - def test_provisioning_payload_is_the_resolved_script(self): - # Pin that the base64 actually carries the resolved setup_script bytes, - # not just that the command contains "base64 -d" (guards a broken encoder). - orch, runtime = self._make(lifetime="per_run") - runtime.setup_containers.return_value = True - runtime.exec.return_value = _OK - self.assertTrue(orch.setup_containers()) - cmd = runtime.exec.call_args[0][1] - encoded = cmd.split("echo ", 1)[1].split(" | base64", 1)[0] - with open(orch.container_config["setup_script"], "rb") as f: - self.assertEqual(base64.b64decode(encoded), f.read()) - - def test_provisioning_read_failure_returns_false(self): - # An unreadable setup_script fails the launch and never reaches docker exec. - orch, runtime = self._make(lifetime="per_run") - orch.container_id = "cvs_iter_test" - with patch("builtins.open", side_effect=OSError("boom")): - self.assertFalse(orch._provision_container()) - runtime.exec.assert_not_called() - class TestResolveContainerLifetime(unittest.TestCase): """One assertion per row of the lifetime resolution table.""" diff --git a/cvs/core/orchestrators/unittests/test_factory.py b/cvs/core/orchestrators/unittests/test_factory.py index 2957a2a9..bf96f01e 100644 --- a/cvs/core/orchestrators/unittests/test_factory.py +++ b/cvs/core/orchestrators/unittests/test_factory.py @@ -16,10 +16,8 @@ from unittest.mock import MagicMock, patch from cvs.core.orchestrators.factory import ( - DEFAULT_CONTAINER_SETUP_SCRIPT, OrchestratorConfig, OrchestratorFactory, - _resolve_container_setup_script, ) from cvs.core.orchestrators.baremetal import BaremetalOrchestrator from cvs.core.orchestrators.container import ContainerOrchestrator @@ -255,115 +253,5 @@ def test_from_configs_does_not_trip_changeme_in_testsuite(self): self.assertEqual(cfg.username, "literal_user") -class TestResolveContainerSetupScript(unittest.TestCase): - """container.setup_script default injection + path validation.""" - - def test_empty_block_untouched(self): - # Baremetal path: empty container block stays empty (no script injected). - self.assertEqual(_resolve_container_setup_script({}), {}) - - def test_falsy_setup_script_defaults(self): - # absent / None / "" are all falsy -> packaged default (no disable value). - for name, container in [ - ("absent", {"image": "x"}), - ("none", {"image": "x", "setup_script": None}), - ("empty", {"image": "x", "setup_script": ""}), - ]: - with self.subTest(name): - out = _resolve_container_setup_script(dict(container)) - self.assertEqual(out["setup_script"], DEFAULT_CONTAINER_SETUP_SCRIPT) - - def test_packaged_default_script_exists_on_disk(self): - # The default must actually be present (packaged + __file__-relative), - # otherwise every container run with no setup_script would fail to read it. - self.assertTrue(os.path.isfile(DEFAULT_CONTAINER_SETUP_SCRIPT)) - - def test_existing_user_path_kept_as_absolute(self): - with tempfile.NamedTemporaryFile("w", suffix=".sh", delete=False) as f: - f.write("#!/bin/bash\ntrue\n") - path = f.name - try: - out = _resolve_container_setup_script({"image": "x", "setup_script": path}) - self.assertEqual(out["setup_script"], os.path.abspath(path)) - finally: - os.unlink(path) - - def test_missing_user_path_raises(self): - with self.assertRaises(ValueError) as ctx: - _resolve_container_setup_script({"image": "x", "setup_script": "/no/such/setup_script.sh"}) - self.assertIn("setup_script", str(ctx.exception)) - - def test_missing_packaged_default_raises(self): - # The F2 fix branch: a broken install whose packaged default is missing - # must fail fast at resolution (ValueError), not as an OSError mid-run. - with patch( - "cvs.core.orchestrators.factory.DEFAULT_CONTAINER_SETUP_SCRIPT", - "/nonexistent/default_container_setup.sh", - ): - with self.assertRaises(ValueError) as ctx: - _resolve_container_setup_script({"image": "x"}) - self.assertIn("default", str(ctx.exception).lower()) - - def test_relative_user_path_resolved_to_abspath(self): - # A relative setup_script is resolved against the cwd and stored absolute. - with tempfile.NamedTemporaryFile("w", suffix=".sh", delete=False) as f: - f.write("#!/bin/bash\ntrue\n") - path = f.name - try: - rel = os.path.relpath(path, os.getcwd()) - out = _resolve_container_setup_script({"image": "x", "setup_script": rel}) - self.assertTrue(os.path.isabs(out["setup_script"])) - self.assertEqual(out["setup_script"], os.path.abspath(path)) - finally: - os.unlink(path) - - def test_tilde_user_path_is_expanded(self): - # A ~ path is expanded before the existence check (proven via the resolved - # path in the error message since the file does not exist). - with self.assertRaises(ValueError) as ctx: - _resolve_container_setup_script({"image": "x", "setup_script": "~/__cvs_missing_setup__.sh"}) - msg = str(ctx.exception) - self.assertIn(os.path.expanduser("~"), msg) - self.assertNotIn("~", msg.split("resolved to", 1)[-1]) - - @patch("cvs.core.orchestrators.baremetal.Pssh") - def test_orchestratorconfig_init_injects_default_setup_script(self, _mock_pssh): - # Direct construction routes through __init__ -> the same resolver, so a - # container config with no setup_script gets the packaged default. - cfg = OrchestratorConfig( - orchestrator="container", - node_dict={"1.1.1.1": {}}, - username="u", - priv_key_file="/dev/null", - container={"lifetime": "per_run", "image": "x"}, - ) - self.assertEqual(cfg.container["setup_script"], DEFAULT_CONTAINER_SETUP_SCRIPT) - - @patch("cvs.core.orchestrators.baremetal.Pssh") - def test_from_configs_null_setup_script_defaults(self, _mock_pssh): - # JSON null (Python None) for setup_script is treated like "absent" and - # resolves to the packaged default -- not "disable provisioning". - cluster = { - "orchestrator": "container", - "node_dict": {"1.1.1.1": {}}, - "username": "u", - "priv_key_file": "/dev/null", - "container": {"lifetime": "per_run", "image": "x", "setup_script": None}, - } - cfg = OrchestratorConfig.from_configs(cluster) - self.assertEqual(cfg.container["setup_script"], DEFAULT_CONTAINER_SETUP_SCRIPT) - - @patch("cvs.core.orchestrators.baremetal.Pssh") - def test_baremetal_init_does_not_inject_setup_script(self, _mock_pssh): - # Empty container block (baremetal) must not gain a setup_script key. - cfg = OrchestratorConfig( - orchestrator="baremetal", - node_dict={"1.1.1.1": {}}, - username="u", - priv_key_file="/dev/null", - ) - self.assertEqual(cfg.container, {}) - - if __name__ == "__main__": unittest.main() diff --git a/cvs/core/runtimes/base.py b/cvs/core/runtimes/base.py index ba9bc4b4..9df3f99c 100644 --- a/cvs/core/runtimes/base.py +++ b/cvs/core/runtimes/base.py @@ -19,16 +19,6 @@ def teardown_containers(self, container_name): """Tear down containers on all nodes.""" ... - def image_sha_status(self, container_name, image_name): - """Compare a running container's image SHA to a local image tag, per host. - - Returns: - dict: per-host result with keys 'container_sha' (image ID the running - container was created from) and 'image_sha' (local image's current ID). - Either is '' when its probe could not read a value. - """ - ... - def is_running(self, container_name): """Check whether a container with the given name is running on each host. diff --git a/cvs/core/runtimes/docker.py b/cvs/core/runtimes/docker.py index e96cac08..3b902072 100644 --- a/cvs/core/runtimes/docker.py +++ b/cvs/core/runtimes/docker.py @@ -8,13 +8,6 @@ import shlex -# Timeout (seconds) for the `docker run` that starts a container. This is the -# step that implicitly pulls the image when it is not already present locally, -# so it must allow for a cold pull of a large image (multi-GB ROCm images can -# take several minutes). 15 minutes covers a fresh pull on launch. -CONTAINER_START_TIMEOUT_S = 900 - - class DockerRuntime: """Docker container runtime implementation.""" @@ -137,9 +130,7 @@ def setup_containers( remove_cmd = f"sudo docker rm -f {container_name} || true" self.orchestrator.all.exec(remove_cmd, timeout=30, print_console=False) - # CONTAINER_START_TIMEOUT_S (not 60s): `docker run` implicitly pulls the - # image on a cold node, which for a multi-GB image far exceeds a minute. - result = self.orchestrator.all.exec(cmd, timeout=CONTAINER_START_TIMEOUT_S, detailed=True) + result = self.orchestrator.all.exec(cmd, timeout=60, detailed=True) # Check if all hosts started successfully success = all(output['exit_code'] == 0 for output in result.values()) @@ -178,41 +169,6 @@ def is_running(self, container_name): } return out - def image_sha_status(self, container_name, image_name): - """Compare a running container's image SHA to the local image tag, per host. - - Used by the ``lifetime: persistent`` attach path to detect overlay - staleness and cross-host image skew. Runs both ``docker inspect`` probes - in one fan-out and emits ``|`` per host. - - A failed ``docker inspect`` (missing container/image) yields an empty - string for that SHA; callers treat an empty ``container_sha`` as a probe - failure. No exit code is returned because the wrapping ``echo`` always - succeeds, so it carries no signal. - - Args: - container_name: Name of the running container to inspect. - image_name: Local image tag to compare against. - - Returns: - dict: per-host result with keys 'container_sha' (image ID the running - container was created from, ``.Image``) and 'image_sha' (local image's - current ID, ``.Id``); each is '' when its probe could not read a value. - """ - cmd = ( - f"echo \"$(sudo docker inspect --format '{{{{.Image}}}}' {container_name} 2>/dev/null)" - f"|$(sudo docker inspect --format '{{{{.Id}}}}' {image_name} 2>/dev/null)\"" - ) - raw = self.orchestrator.all.exec(cmd, timeout=30, detailed=True) - out = {} - for host, res in raw.items(): - parts = (res.get('output') or '').strip().split('|') - out[host] = { - 'container_sha': parts[0] if len(parts) > 0 else '', - 'image_sha': parts[1] if len(parts) > 1 else '', - } - return out - def teardown_containers(self, container_name): """Stop and remove Docker containers on all nodes.""" if not container_name: diff --git a/cvs/core/runtimes/enroot.py b/cvs/core/runtimes/enroot.py index cc3dc6b7..9d62bf4a 100644 --- a/cvs/core/runtimes/enroot.py +++ b/cvs/core/runtimes/enroot.py @@ -23,11 +23,6 @@ def teardown_containers(self, container_name): self.log.warning("Enroot runtime not yet implemented") return True - def image_sha_status(self, container_name, image_name): - """Compare image SHAs for Enroot containers - not yet implemented.""" - self.log.error("Enroot runtime not yet implemented") - return {} - def is_running(self, container_name): """Check Enroot container status - not yet implemented.""" self.log.error("Enroot runtime not yet implemented") diff --git a/cvs/input/cluster_file/README.md b/cvs/input/cluster_file/README.md index 85a4b6f9..9bc67bfb 100644 --- a/cvs/input/cluster_file/README.md +++ b/cvs/input/cluster_file/README.md @@ -44,9 +44,8 @@ Consumed by `ContainerOrchestrator` in [`cvs/core/orchestrators/container.py`](. | Key | Type | Default | Purpose | | --- | --- | --- | --- | | `lifetime` | str | `"per_run"` | Container lifecycle policy: `no_launch`, `per_run`, or `persistent`. See the truth table below. | -| `image` | str | (required) | Image with the test dependencies (rvs, etc.) the suite invokes. Must be present locally on each node OR pullable from a reachable registry. An in-container sshd is **not** required up front -- `setup_script` installs it if missing. | +| `image` | str | (required) | Image with the test dependencies (rvs, etc.) pre-installed and an sshd you can start on port 2224. Must be present locally on each node OR pullable from a reachable registry. | | `name` | str | (required) | Container name on each host. For parallel runs make this per-iteration unique (e.g. `cvs_iter_`). | -| `setup_script` | str | (packaged default) | Optional path to a shell script run inside each freshly-launched container (before sshd setup) to install packages on top of the base image. `null` or omitting the key both use the packaged default that installs `openssh-server` only (there is no value that disables provisioning). A non-existent path fails at config load. Delivered inline via `docker exec` (so the base image needs `bash` + `base64`, and the script must stay under ~16 KB); the default is apt-based and runs as the container's exec user (root) -- non-apt images need a custom script. | | `runtime.name` | str | `"docker"` | Container runtime. Supported: `docker` (concrete), `enroot` (stub). | | `runtime.args` | dict | `{}` | Backend-specific runtime arguments (see below). Defaults from `DEFAULT_CONTAINER_ARGS` in [`cvs/core/orchestrators/container.py`](../../core/orchestrators/container.py) apply for any key omitted here. | @@ -78,14 +77,14 @@ RDMA-ready container; the keys below are only needed to extend or override. | --- | --- | --- | | `no_launch` | Verify the container is already running on every host; set `container_id`. Never starts anything. | No-op. CVS does not own a container it did not launch. | | `per_run` (default) | Start a fresh container on every host (force-removing any stale same-named container first). | Force-remove the container CVS started. | -| `persistent` | Attach if the container is already running on every host (with a per-host image-SHA check; cross-host SHA skew or an unreadable SHA is a hard error). Start fresh only if it is running on no host. Running on some hosts but not all is a hard error (CVS will not force-remove the still-running hosts and destroy their overlay). Idempotent across runs. | No-op. The container is left running for the next run; remove it yourself when done. | +| `persistent` | Attach if the container is already running on every host. Start fresh only if it is running on no host. Running on some hosts but not all is a hard error (CVS will not force-remove the still-running hosts and destroy their overlay). Idempotent across runs. | No-op. The container is left running for the next run; remove it yourself when done. | ## Prerequisites on each cluster node - **Docker** installed and the SSH user has passwordless `sudo docker`. - **Host driver** loaded so `/dev/kfd` and `/dev/dri/*` (and `/dev/infiniband/*` if RDMA is in scope) are present for passthrough. - **`~/.ssh/`** of the SSH user is reachable; the orchestrator mounts it as `/host_ssh` and copies keys into `/root/.ssh` inside the container so `setup_sshd` can start an in-container sshd on port 2224. -- **The image** is either pre-loaded on every node (`docker load`) or pullable from a reachable registry. Inside the image you need: the workload binaries the suite invokes (e.g. `/opt/rocm/bin/rvs`) and any ROCm runtime libs the workload needs. `openssh-server` is **not** required in the image -- CVS provisions it at launch via `container.setup_script` (the packaged default installs it). On a non-apt base image, supply a custom `setup_script`. +- **The image** is either pre-loaded on every node (`docker load`) or pullable from a reachable registry. Inside the image you need: `openssh-server` (for the in-container sshd), the workload binaries the suite invokes (e.g. `/opt/rocm/bin/rvs`), and any ROCm runtime libs the workload needs. ## What happens when you run a backend-blind suite @@ -122,7 +121,7 @@ sequenceDiagram participant Sshd as container sshd:2224 TestFix->>ContainerOrch: setup_containers() - ContainerOrch->>Docker: is_running(name)? yes -> attach, image-SHA check + ContainerOrch->>Docker: is_running(name)? yes -> attach TestFix->>ContainerOrch: setup_sshd() ContainerOrch->>Sshd: pgrep sshd:2224 already up -> skip start TestFix->>ContainerOrch: orch.exec("rvs -c ...") @@ -137,7 +136,7 @@ file changes. The `orch` fixture in [`cvs/tests/health/rvs_cvs.py`](../../tests/ ## Common pitfalls -- **Base image cannot install `openssh-server`**. The default `setup_script` runs `apt-get install openssh-server` inside the container so `setup_sshd` can start sshd on port 2224. If the base image has no apt / no network to a package mirror, that install fails and `orch.exec` cannot connect. Fixes: bake `openssh-server` into the image, or point `container.setup_script` at a script that installs sshd the way that image expects (e.g. `dnf`/`microdnf`). +- **Image without `openssh-server`**. `setup_sshd` cannot start sshd on port 2224; `orch.exec` then fails to connect. Make sure your image installs `openssh-server` and exposes the binary at `/usr/sbin/sshd`. - **Image without the workload binary**. `cvs run rvs_cvs` will execute `rvs` inside the container; if the image lacks `/opt/rocm/bin/rvs` the test fails with a `command not found` flavor error. - **`lifetime: persistent` without a pinned `name`**. The default container name is `_`, which shifts when you bump the image tag. A tag bump silently abandons the previous container's overlay (installs, clones) and starts fresh. Pin `container.name` explicitly when using `persistent`. - **Port 2224 collision on the host**. With `network: host` the in-container sshd binds to 2224 in the host's network namespace. If something else on the host already listens on 2224 the bind fails. Stop the conflicting service or change the in-image sshd port (and update the orchestrator's port to match). diff --git a/cvs/input/cluster_file/cluster_container.json b/cvs/input/cluster_file/cluster_container.json index 09055860..da5e0ea5 100644 --- a/cvs/input/cluster_file/cluster_container.json +++ b/cvs/input/cluster_file/cluster_container.json @@ -34,15 +34,12 @@ "_lifetime_comment": "Container lifecycle policy. 'no_launch' = CVS never launches the container (verify it is already running, never stopped). 'per_run' = CVS starts on setup and removes on teardown. 'persistent' = CVS starts if absent / attaches if present and leaves running on exit (unblocks install-then-run; pin 'name' explicitly).", "lifetime": "per_run", - "_image_comment": "Image with the test dependencies (rvs, etc.) the suite invokes. Must be present locally on each node OR pullable from a reachable registry. An in-container sshd is NOT required up front -- it is installed by setup_script (see below) if missing.", + "_image_comment": "Image with test dependencies (rvs, etc.) pre-installed and an sshd you can start on port 2224. Must be present locally on each node OR pullable from a reachable registry.", "image": "rocm/cvs:latest", "_name_comment": "Container name on each host. For parallel runs make this per-iteration unique (e.g. cvs_iter_).", "name": "cvs_container", - "_setup_script_comment": "Optional path to a shell script run inside each freshly-launched container (before sshd setup) to install packages on top of the base image. null or omitting this key both use the packaged default that installs openssh-server (there is no 'disable provisioning' value). A non-existent path fails at config load. Delivered inline via docker exec, so the base image needs bash and base64; the default script is apt-based and runs as the container's exec user (root).", - "setup_script": null, - "runtime": { "_name_comment": "Supported: 'docker' (concrete), 'enroot' (stub).", "name": "docker", diff --git a/docs/how-to/run-with-containers.rst b/docs/how-to/run-with-containers.rst index b65e9cf1..7f9682eb 100644 --- a/docs/how-to/run-with-containers.rst +++ b/docs/how-to/run-with-containers.rst @@ -18,7 +18,7 @@ On every cluster node: - **Docker** installed, with passwordless ``sudo docker`` for the SSH user. - **Host driver** loaded so ``/dev/kfd``, ``/dev/dri/*``, and ``/dev/infiniband/*`` (when RDMA is in scope) are present for passthrough. - **SSH user home directory** reachable. The orchestrator mounts ``~/.ssh`` as ``/host_ssh`` inside the container so that the in-container ``sshd`` on port ``2224`` can authenticate. -- **Container image** either pre-loaded on every node (``docker load``) or pullable from a reachable registry. The image must contain the workload binaries the suite invokes (for example ``/opt/rocm/bin/rvs``). It does **not** need ``openssh-server`` baked in -- CVS provisions it at launch via ``container.setup_script`` (the packaged default installs it; a non-apt base image needs a custom script). +- **Container image** either pre-loaded on every node (``docker load``) or pullable from a reachable registry. The image must contain ``openssh-server`` and the workload binaries the suite invokes (for example ``/opt/rocm/bin/rvs``). On the head node where you launch ``cvs run``: @@ -48,9 +48,8 @@ Open the copied file and edit: - ``{user-id}``: your SSH user (or leave it for runtime resolution). - ``priv_key_file``: absolute path to your SSH private key. - ``head_node_dict.mgmt_ip`` and the keys of ``node_dict``: real IPs or hostnames of your cluster nodes. ``mgmt_ip`` must equal one of the keys in ``node_dict``. -- ``container.image``: an image present on every node or pullable from a reachable registry. The image must include the workload binary (for example ``rvs``); ``openssh-server`` is installed at launch by ``setup_script``, not required in the image. +- ``container.image``: an image present on every node or pullable from a reachable registry. The image must include ``openssh-server`` and the workload binary (for example ``rvs``). - ``container.name``: container name on each host. For parallel runs make this per-iteration unique (for example ``cvs_iter_``). Pin it explicitly when using ``lifetime: persistent``. -- ``container.setup_script`` (optional): path to a shell script run inside each freshly-launched container before sshd setup, to install packages on top of the base image. Omit to use the packaged default that installs ``openssh-server``. - ``container.lifetime``: ``no_launch``, ``per_run``, or ``persistent``. See the lifecycle note below. For the full schema and runtime argument reference, see :doc:`/reference/configuration-files/cluster-file`. @@ -74,8 +73,8 @@ Run ``rvs_cvs`` the same way you run any CVS test suite, but with the container What happens during the run: - The ``orch`` fixture in ``rvs_cvs`` reads ``orchestrator: container`` from the cluster file and constructs a ``ContainerOrchestrator``. -- With ``lifetime: per_run``, CVS removes any container with the same name on each host, runs the configured image with the merged ``runtime.args``, runs ``container.setup_script`` inside the new container (default: install ``openssh-server``), and starts an in-container ``sshd`` on port ``2224``. -- With ``lifetime: persistent``, CVS attaches to a container already running on every host (skipping provisioning and ``sshd`` setup if it is already up), or starts one fresh -- provisioning it -- if none is running. +- With ``lifetime: per_run``, CVS removes any container with the same name on each host, runs the configured image with the merged ``runtime.args``, and starts an in-container ``sshd`` on port ``2224``. +- With ``lifetime: persistent``, CVS attaches to a container already running on every host, or starts one fresh if none is running. - With ``lifetime: no_launch``, CVS verifies that a container with the configured name is already running on every host and reuses it. - All ``rvs`` invocations are routed through the container via ``docker exec`` and the in-container ``sshd``. @@ -122,7 +121,7 @@ Replace ``cvs_container`` with the actual ``container.name`` from your cluster f Common pitfalls =============== -- **Base image cannot install ``openssh-server``.** The default ``setup_script`` runs ``apt-get install openssh-server`` inside the container so ``setup_sshd`` can start ``sshd`` on port ``2224``. If the base image has no apt or no network to a package mirror, that install fails and ``orch.exec`` cannot connect. Bake ``openssh-server`` into the image, or point ``container.setup_script`` at a script that installs it the way that image expects. +- **Image without ``openssh-server``.** ``setup_sshd`` cannot start ``sshd`` on port ``2224`` and ``orch.exec`` fails to connect. Make sure the image installs ``openssh-server`` and exposes the binary at ``/usr/sbin/sshd``. - **Image without the workload binary.** ``cvs run rvs_cvs`` invokes ``rvs`` inside the container; if the image lacks ``/opt/rocm/bin/rvs`` the test fails with a "command not found" style error. - **``lifetime: persistent`` without a pinned ``name``.** The default container name is ``_``, which shifts when you bump the image tag. A tag bump silently abandons the previous container's overlay (installs, clones) and starts fresh. Pin ``container.name`` when using ``persistent``. - **Port ``2224`` already bound on the host.** With ``network: host`` the in-container ``sshd`` binds to ``2224`` in the host's network namespace. If something else on the host already listens on ``2224`` the bind fails. Stop the conflicting service. diff --git a/docs/reference/configuration-files/cluster-file.rst b/docs/reference/configuration-files/cluster-file.rst index 830d5dd0..4b6a61ab 100644 --- a/docs/reference/configuration-files/cluster-file.rst +++ b/docs/reference/configuration-files/cluster-file.rst @@ -80,7 +80,6 @@ Both templates share the same top-level shape. The ``container`` block and the ` "lifetime": "per_run", "image": "rocm/cvs:latest", "name": "cvs_container", - "setup_script": null, "runtime": { "name": "docker", "args": { @@ -141,13 +140,10 @@ The ``container`` block configures the container backend. It is consumed by the - Container lifecycle policy: ``no_launch``, ``per_run``, or ``persistent``. See the truth table below. * - ``image`` - (required) - - Image with the test dependencies (for example ``rvs``) the suite invokes. Must be present locally on each node or pullable from a reachable registry. An in-container ``sshd`` is not required up front; it is installed at launch by ``setup_script``. + - Image with the test dependencies (for example ``rvs``) pre-installed and an ``sshd`` you can start on port ``2224``. Must be present locally on each node or pullable from a reachable registry. * - ``name`` - (required) - Container name on each host. For parallel runs, make this per-iteration unique (for example ``cvs_iter_``). - * - ``setup_script`` - - (packaged default) - - Optional path to a shell script run inside each freshly-launched container (before sshd setup) to install packages on top of the base image. Omit to use the packaged default that installs ``openssh-server`` only. A non-existent path fails at config load. apt-based; non-apt images need a custom script. * - ``runtime.name`` - ``docker`` - Container runtime. Today only ``docker`` is implemented. ``enroot`` is registered as a stub and is not yet functional. @@ -218,7 +214,7 @@ When ``runtime.name`` is ``docker``, the keys below configure the underlying ``d - Start a fresh container on every host (force-removing any stale same-named container first). - Force-remove the container CVS started. * - ``persistent`` - - Attach if the container is already running on every host (with a per-host image-SHA check; cross-host SHA skew or an unreadable SHA is a hard error). Start fresh only if it is running on no host. Running on some hosts but not all is a hard error (CVS will not force-remove the still-running hosts and destroy their overlay). Idempotent across runs. + - Attach if the container is already running on every host. Start fresh only if it is running on no host. Running on some hosts but not all is a hard error (CVS will not force-remove the still-running hosts and destroy their overlay). Idempotent across runs. - No-op. The container is left running for the next run; remove it yourself when done. .. note:: @@ -233,7 +229,7 @@ To use the container backend, every cluster node must have: - **Docker installed** with passwordless ``sudo docker`` for the SSH user. - **Host driver loaded** so ``/dev/kfd``, ``/dev/dri/*``, and ``/dev/infiniband/*`` (when RDMA is in scope) are present for passthrough. - **SSH user home directory accessible**. The orchestrator mounts ``~/.ssh`` as ``/host_ssh`` and copies keys into ``/root/.ssh`` inside the container so that the in-container ``sshd`` on port ``2224`` can authenticate. -- **Container image** either pre-loaded on every node (``docker load``) or pullable from a reachable registry. The image must contain the workload binaries the suite invokes (for example ``/opt/rocm/bin/rvs``). ``openssh-server`` (for the in-container ``sshd``) is not required in the image -- it is installed at launch by ``container.setup_script``. +- **Container image** either pre-loaded on every node (``docker load``) or pullable from a reachable registry. The image must contain ``openssh-server`` (for the in-container ``sshd``) and the workload binaries the suite invokes (for example ``/opt/rocm/bin/rvs``). See also ======== From cd23180d03f7cb36b21400328b82385300810c89 Mon Sep 17 00:00:00 2001 From: Atul Nair Date: Tue, 9 Jun 2026 23:13:01 -0400 Subject: [PATCH 19/20] address review: trim normalize comment, carry lifetime in runtime test config --- cvs/core/orchestrators/factory.py | 5 +---- cvs/core/runtimes/unittests/test_docker.py | 9 ++++++--- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/cvs/core/orchestrators/factory.py b/cvs/core/orchestrators/factory.py index 7aaa0258..f780f08d 100644 --- a/cvs/core/orchestrators/factory.py +++ b/cvs/core/orchestrators/factory.py @@ -123,10 +123,7 @@ def __init__(self, **kwargs): self.priv_key_file = kwargs['priv_key_file'] self.password = kwargs.get('password') self.head_node_dict = kwargs.get('head_node_dict', {}) - # Normalize the container block. This is the single chokepoint: - # from_configs constructs via cls(**required_config), so both file-driven - # and direct programmatic construction hit the same normalization (and the - # same enabled-removed / launch-removed errors). + # Normalize here (not in from_configs) so direct construction is validated too. self.container = _resolve_container_lifetime(kwargs.get('container', {})) def get(self, key, default=None): diff --git a/cvs/core/runtimes/unittests/test_docker.py b/cvs/core/runtimes/unittests/test_docker.py index 98d8bd0a..18e7dbfc 100644 --- a/cvs/core/runtimes/unittests/test_docker.py +++ b/cvs/core/runtimes/unittests/test_docker.py @@ -43,15 +43,18 @@ def _fake_exec(cmd, timeout=None, detailed=False, print_console=True): return DockerRuntime(log, orchestrator) -def _container_config(image="img:test", extra_runtime_args=None, **extra): +def _container_config(image="img:test", lifetime="per_run", extra_runtime_args=None, **extra): """Minimal container config dict for setup_containers. - The runtime no longer reads enabled/launch -- lifetime resolution lives in - OrchestratorConfig -- so neither key is present here. + lifetime is carried for parity with a real config block, but the runtime never + reads it: lifecycle policy is resolved and branched on by the orchestrator, + which only calls runtime.setup_containers on start paths. The runtime is + policy-free, so the value here has no effect on its behavior. """ cfg = { "image": image, "name": "cvs_iter_test", + "lifetime": lifetime, "runtime": {"name": "docker", "args": dict(extra_runtime_args or {})}, } cfg.update(extra) From a643858eb86ffb26a3ec7d11009e5b5704fdaaf5 Mon Sep 17 00:00:00 2001 From: Atul Nair Date: Wed, 10 Jun 2026 00:08:49 -0400 Subject: [PATCH 20/20] tests: drop redundant persistent-idempotent test, strip dead warning guard --- .../orchestrators/unittests/test_container.py | 16 ++-------------- 1 file changed, 2 insertions(+), 14 deletions(-) diff --git a/cvs/core/orchestrators/unittests/test_container.py b/cvs/core/orchestrators/unittests/test_container.py index 70c03f0b..e71f582d 100644 --- a/cvs/core/orchestrators/unittests/test_container.py +++ b/cvs/core/orchestrators/unittests/test_container.py @@ -15,7 +15,6 @@ # patched once in setUp (not per method); _make() returns a fresh orch + runtime mock. import unittest -import warnings from unittest.mock import MagicMock, patch from cvs.core.orchestrators.factory import OrchestratorConfig, _resolve_container_lifetime @@ -156,15 +155,6 @@ def test_setup_containers_persistent_starts_when_not_running(self): self.assertTrue(orch.setup_containers()) runtime.setup_containers.assert_called_once() - def test_setup_containers_persistent_idempotent_on_resetup(self): - # Re-running setup against an already-running persistent container is a - # no-op attach both times -- never starts a new container. - orch, runtime = self._make(lifetime="persistent") - runtime.is_running.return_value = _RUNNING - self.assertTrue(orch.setup_containers()) - self.assertTrue(orch.setup_containers()) - runtime.setup_containers.assert_not_called() - def test_setup_containers_persistent_partial_running_refuses(self): # Running on some hosts but not all must NOT auto-relaunch (that would # force-remove and rebuild the still-running hosts, destroying their @@ -282,10 +272,8 @@ def test_enabled_false_also_raises(self): with self.assertRaises(ValueError): _resolve_container_lifetime({"enabled": False, "image": "x"}) - def test_explicit_lifetime_kept_no_warning(self): - with warnings.catch_warnings(): - warnings.simplefilter("error") # any warning would fail the test - out = _resolve_container_lifetime({"lifetime": "persistent", "image": "x"}) + def test_explicit_lifetime_kept(self): + out = _resolve_container_lifetime({"lifetime": "persistent", "image": "x"}) self.assertEqual(out["lifetime"], "persistent") def test_invalid_lifetime_raises(self):