From 9604c85fdafc8347036cd1212b6cd648db939d3e Mon Sep 17 00:00:00 2001 From: YueGuobin Date: Sat, 15 Aug 2026 00:52:55 +0800 Subject: [PATCH] docker: harden the shm/devices/extra_configs/masking work (code review) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Nine fixes from a review of the docker-shm-devices diff: * GNS3_STOP_TIMEOUT >300 s aborted at the manager's default HTTP timeout before Docker finished the stop — the stop query now gets a timeout with a margin over the grace period. * Overlapping bind targets (GNS3_MASK_UDEV + GNS3_MASK_SYSTEMD on the same unit, a unit named twice, an extra_configs target equal to a masked unit) made Docker reject the create with 'Duplicate mount point' — Mounts are deduplicated by target. * ExtraConfig.target now carries a pydantic validator (absolute file path, no '..'), so bad targets 422 at template-save time instead of failing at node-create time after a multi-GB image pull; directory forms ('/', '/etc/') are also rejected by the runtime guard instead of raising IsADirectoryError (raw 500). * _check_host_readiness skipped every remaining check when one /proc/sys key was unreadable (mid-loop return) — now continues. * The base-class GNS3_* env parser strips trailing commas like the vendor parser, so 'GNS3_MASK_UDEV=1,' composed from a list still activates. * Vendor env knobs are re-parsed on every create(), so a PUT to the node's environment takes effect on the next (re)create. * The graceful SIGTERM stop is now limited to the explicit user stop route; delete/update/close/crash-cleanup keep the immediate kill (those paths force-delete or recreate the container right after). * An extra_configs target beneath a persisted volume is shadowed by the volume bind — warn at create time. --- docs/features/vendor-nos-xrd.md | 3 +- gns3server/api/routes/compute/docker_nodes.py | 6 +- gns3server/compute/docker/__init__.py | 4 +- gns3server/compute/docker/docker_vm.py | 48 +++++++-- gns3server/compute/docker/vendor_docker_vm.py | 53 ++++++++-- gns3server/schemas/common.py | 18 +++- tests/compute/docker/test_docker.py | 31 ++++++ tests/compute/docker/test_docker_vm.py | 100 ++++++++++++++++++ tests/compute/docker/test_vendor_docker_vm.py | 77 ++++++++++---- 9 files changed, 297 insertions(+), 43 deletions(-) diff --git a/docs/features/vendor-nos-xrd.md b/docs/features/vendor-nos-xrd.md index e0610d4be..cc18c5019 100644 --- a/docs/features/vendor-nos-xrd.md +++ b/docs/features/vendor-nos-xrd.md @@ -65,7 +65,7 @@ graph TB | config injection | `extra_configs: [{target, content}]` (template/node/appliance schema field) | content written under the node dir, bind-mounted **read-only** at `target` — seeds NOS startup configs without rebuilding the image | `docker_vm.py` `_mount_binds()`; persisted in `docker_templates.extra_configs` (Alembic migration) | | udev masking | `GNS3_MASK_UDEV=1` | `/dev/null` over the 5 udev systemd units **and** `/bin|/sbin|/usr/bin/udevadm` | `docker_vm.py` `create()` | | generic unit mask | `GNS3_MASK_SYSTEMD=u1,u2` | `/dev/null` over arbitrary `/etc/systemd/system/` | `docker_vm.py` `create()` | -| graceful stop | `GNS3_STOP_TIMEOUT=60` (seconds; default 60) | vendor containers are stopped with SIGTERM + a grace period (Docker SIGKILLs once it expires) instead of the base class' immediate kill — systemd NOS images require a graceful shutdown | `vendor_docker_vm.py` `_terminate_container()` | +| graceful stop | `GNS3_STOP_TIMEOUT=60` (seconds; default 60, max 600) | explicit user stop sends SIGTERM + a grace period (Docker SIGKILLs once it expires) instead of the base class' immediate kill — systemd NOS images require a graceful shutdown; internal paths (delete/update/close) keep the immediate kill since the container is force-deleted right after | `vendor_docker_vm.py` `_terminate_container()` | | host check | automatic at Docker connect | read-only `/proc` check of inotify/file-max/FUSE; warns with exact fix commands (server is unprivileged — it can only check) | `compute/docker/__init__.py` `_check_host_readiness()` | `GNS3_*` variables are consumed host-side only and never forwarded into the @@ -201,6 +201,7 @@ sequenceDiagram | Version | Date | Changes | |---------|------|---------| +| 1.4 | 2026-08-15 | Code-review hardening: stop-query HTTP timeout scales with `GNS3_STOP_TIMEOUT` (values >300 s no longer abort); overlapping mask/config bind targets deduplicated (Docker "Duplicate mount point"); `ExtraConfig.target` validated at save time and directory forms rejected; host-readiness check no longer aborts on one unreadable `/proc/sys` key; base env parser strips trailing commas; vendor env knobs re-parsed on create (PUT environment takes effect); graceful stop limited to explicit user stop (delete/update/close keep the immediate kill); extra_configs under a persisted volume warns. | | 1.3 | 2026-08-14 | Persistence corrected: XR's live data layer is `/xr-storage` (the image's symlink farm is materialized into real directories at bootstrap; `/xr-storage-shadow` is a pristine spare) — the appliance persists both. Vendor containers now stop gracefully (SIGTERM + `GNS3_STOP_TIMEOUT` grace, default 60 s) instead of being SIGKILLed on the spot. | | 1.2 | 2026-08-14 | Link UDP self-loop root-caused to a port-allocation race and fixed — see [link-udp-self-loop](../bugs/link-udp-self-loop.md) (now marked Fixed). | | 1.1 | 2026-08-14 | Datapath validated end-to-end (XRd brings its own interfaces up; ARP/ICMP bidirectional). Add troubleshooting entry for the one-way-link symptom (GNS3 link UDP self-loop bug, see bugs/link-udp-self-loop.md). | diff --git a/gns3server/api/routes/compute/docker_nodes.py b/gns3server/api/routes/compute/docker_nodes.py index 528925ff9..be55ec0f4 100644 --- a/gns3server/api/routes/compute/docker_nodes.py +++ b/gns3server/api/routes/compute/docker_nodes.py @@ -177,10 +177,12 @@ async def start_docker_node(node: DockerVM = Depends(dep_node)) -> None: ) async def stop_docker_node(node: DockerVM = Depends(dep_node)) -> None: """ - Stop a Docker node. + Stop a Docker node. This is the explicit user stop — the only path that + asks for a graceful SIGTERM shutdown (vendor NOS override); internal + paths (delete/update/close) keep the immediate kill. """ - await node.stop() + await node.stop(graceful=True) @router.post( diff --git a/gns3server/compute/docker/__init__.py b/gns3server/compute/docker/__init__.py index 28c76e09b..ed37ca626 100644 --- a/gns3server/compute/docker/__init__.py +++ b/gns3server/compute/docker/__init__.py @@ -190,7 +190,9 @@ class Docker(BaseManager): with open(f"/proc/sys/{key.replace('.', '/')}") as f: current = int(f.read().strip()) except (OSError, ValueError): - return # Not Linux or unreadable -- nothing to check. + # One unreadable key must not discard the warnings already + # collected nor skip the FUSE check — skip just this key. + continue if current < minimum: low.append((key, current, minimum)) diff --git a/gns3server/compute/docker/docker_vm.py b/gns3server/compute/docker/docker_vm.py index 5e48a5e6c..fa45c43e6 100644 --- a/gns3server/compute/docker/docker_vm.py +++ b/gns3server/compute/docker/docker_vm.py @@ -433,10 +433,21 @@ class DockerVM(BaseNode): for cfg in self._extra_configs: target = cfg["target"] if isinstance(cfg, dict) else cfg.target content = cfg["content"] if isinstance(cfg, dict) else cfg.content - if not target.startswith("/") or ".." in target.split("/"): + if not target.startswith("/") or target.endswith("/") or ".." in target.split("/"): raise DockerError( - f"Extra config target '{target}' must be an absolute path and not contain '..'." + f"Extra config target '{target}' must be an absolute file path and not contain '..'." ) + for volume in self._volumes: + # A single-file bind gets covered by the volume's bind mount at + # start (init.sh or the vendor volume bridge), so the injected + # content would never be seen — or worse, frozen at whatever + # the first-start seed copied. + if target == volume or target.startswith(volume.rstrip("/") + "/"): + log.warning( + "Extra config target '%s' on container '%s' is shadowed by persisted volume '%s' " + "and will not take effect; pick a target outside persisted volumes.", + target, self._name, volume, + ) host_path = os.path.join(self.working_dir, "configs", target.lstrip("/")) os.makedirs(os.path.dirname(host_path), exist_ok=True) with open(host_path, "w") as f: @@ -556,7 +567,10 @@ class DockerVM(BaseNode): # when set, so ordinary nodes keep the default Docker behaviour. if self._environment: for line in self._environment.splitlines(): - line = line.strip() + # Strip a trailing comma like the vendor-class parser does, so + # "GNS3_MASK_UDEV=1," composed from a comma-separated list + # still activates (values are never comma-separated here). + line = line.strip().rstrip(",") if line.startswith("GNS3_SHM_SIZE="): try: params["HostConfig"]["ShmSize"] = int(line.split("=", 1)[1].strip()) * (1024 * 1024) @@ -594,6 +608,19 @@ class DockerVM(BaseNode): "ReadOnly": True, }) + # Overlapping bind targets (GNS3_MASK_UDEV together with a + # GNS3_MASK_SYSTEMD entry for the same unit, an extra_configs target + # equal to a masked unit, a unit named twice in the list) make Docker + # reject the create outright ("Duplicate mount point") — keep only + # the first occurrence of each target. + seen_targets = set() + deduped_mounts = [] + for mount in params["HostConfig"]["Mounts"]: + if mount["Target"] not in seen_targets: + seen_targets.add(mount["Target"]) + deduped_mounts.append(mount) + params["HostConfig"]["Mounts"] = deduped_mounts + if params["Entrypoint"] is None: params["Entrypoint"] = [] if self._start_command: @@ -1179,9 +1206,14 @@ class DockerVM(BaseNode): await telnet_server.wait_closed() self._telnet_servers = [] - async def stop(self): + async def stop(self, graceful: bool = False): """ Stops this Docker container. + + :param graceful: request a graceful SIGTERM shutdown (honoured by the + vendor NOS override). The default immediate kill is used on the + internal paths (delete, update, close, crash cleanup), where the + container is force-deleted or recreated right after anyway. """ try: @@ -1207,7 +1239,7 @@ class DockerVM(BaseNode): state = await self._get_container_state() if state != "stopped" and state != "exited": try: - await self._terminate_container() + await self._terminate_container(graceful=graceful) log.debug(f"Docker container '{self._name}' [{self._image}] stopped") except DockerHttp409Error: # Container is already stopped @@ -1218,14 +1250,16 @@ class DockerVM(BaseNode): return self.status = "stopped" - async def _terminate_container(self): + async def _terminate_container(self, graceful: bool = False): """ Final termination of a still-running container: immediate SIGKILL. GNS3 has already persisted container state (permissions via _fix_permissions, /gns3volumes) before this point, and the business process (often an interactive shell) ignores SIGTERM — a stop grace period buys nothing but latency. Vendor NOS containers override this - with a graceful SIGTERM shutdown (see VendorDockerVM). + with a graceful SIGTERM shutdown when asked (see VendorDockerVM); + the ``graceful`` flag is accepted here only for signature + compatibility. """ await self.manager.query("POST", f"containers/{self._cid}/kill") diff --git a/gns3server/compute/docker/vendor_docker_vm.py b/gns3server/compute/docker/vendor_docker_vm.py index ca85c1813..7d4729b67 100644 --- a/gns3server/compute/docker/vendor_docker_vm.py +++ b/gns3server/compute/docker/vendor_docker_vm.py @@ -62,14 +62,22 @@ class VendorDockerVM(DockerVM): def __init__(self, *args, **kwargs): super().__init__(*args, **kwargs) - # Prototype knobs — parsed from GNS3_-prefixed entries in create(). - # Parse eagerly so _get_container_ifname can return the right name. + self._console_exec_writer = None + # Parsed eagerly so _get_container_ifname can return the right name, + # and re-parsed on every create() so a PUT to the node's environment + # takes effect on the next (re)create instead of the next reload. + self._parse_vendor_environment() + + def _parse_vendor_environment(self): + """ + (Re)parse the GNS3_* knobs from the current ``environment`` value, + resetting to defaults first so removed entries stop applying. + """ + self._gns3_init = True self._interface_names = [] self._console_cmd = None - self._console_exec_writer = None self._stop_timeout = 60 - if self._environment: for _line in self._environment.splitlines(): _line = _line.strip().rstrip(",") @@ -89,6 +97,12 @@ class VendorDockerVM(DockerVM): except ValueError: pass + async def create(self): + # The environment may have changed since __init__ (PUT on the node) — + # re-parse the knobs so the recreated container picks them up. + self._parse_vendor_environment() + return await super().create() + # ---- hook overrides --------------------------------------------------- def _mount_binds(self, image_info): @@ -148,18 +162,35 @@ class VendorDockerVM(DockerVM): pass self._console_exec_writer = None - async def _terminate_container(self): + async def _terminate_container(self, graceful: bool = False): """ Override: vendor NOS containers run systemd and require a graceful shutdown (e.g. Cisco XRd treats an abrupt SIGKILL as an unclean - shutdown). Send SIGTERM and wait up to ``GNS3_STOP_TIMEOUT`` seconds - (default 60) for the services to stop; Docker SIGKILLs the container - itself once the grace period expires, so no fallback kill is needed. - The blocking stop call sits well inside the manager's default 300 s - query timeout. + shutdown). + + With ``graceful`` (explicit user stop), send SIGTERM and wait up to + ``GNS3_STOP_TIMEOUT`` seconds (default 60, 1-600) for the services to + stop; Docker SIGKILLs the container itself once the grace period + expires, so no fallback kill is needed. The stop query gets an HTTP + timeout with a margin over the grace period — the manager's default + 300 s would abort first for values above it. + + Without ``graceful`` (delete/update/close/crash cleanup), fall back to + the base immediate kill: those paths force-delete or recreate the + container right after anyway, so a grace period buys nothing but + latency. """ + if not graceful: + await super()._terminate_container(graceful=False) + return try: - await self.manager.query("POST", f"containers/{self._cid}/stop", params={"t": self._stop_timeout}) + response = await self.manager.http_query( + "POST", + f"containers/{self._cid}/stop", + params={"t": self._stop_timeout}, + timeout=self._stop_timeout + 30, + ) + response.close() except DockerHttp304Error: pass # already stopped diff --git a/gns3server/schemas/common.py b/gns3server/schemas/common.py index fbf8ddf11..68ca0b861 100644 --- a/gns3server/schemas/common.py +++ b/gns3server/schemas/common.py @@ -14,7 +14,7 @@ # You should have received a copy of the GNU General Public License # along with this program. If not, see . -from pydantic import BaseModel, Field +from pydantic import BaseModel, Field, field_validator from typing import Optional from enum import Enum @@ -60,6 +60,22 @@ class ExtraConfig(BaseModel): target: str = Field(..., description="Absolute path inside the container where the file is mounted") content: str = Field("", description="File content written by GNS3 and bind-mounted read-only into the container") + @field_validator("target") + @classmethod + def target_is_an_absolute_file_path(cls, v): + """ + Reject at save time (template/appliance/node PUT) what would only + blow up at node-create time — after a potentially multi-GB image + pull: relative paths, '..' components and directory forms ('/', + '/etc/'). + """ + if not v.startswith("/") or v.endswith("/") or ".." in v.split("/"): + raise ValueError( + "target must be an absolute file path inside the container " + "(start with '/', name a file, no '..' components)" + ) + return v + class ConsoleType(str, Enum): """ diff --git a/tests/compute/docker/test_docker.py b/tests/compute/docker/test_docker.py index 11b610bb7..7fbb49c06 100644 --- a/tests/compute/docker/test_docker.py +++ b/tests/compute/docker/test_docker.py @@ -415,3 +415,34 @@ async def test_check_host_readiness_silent_when_ok(caplog): docker._check_host_readiness() assert not [r for r in caplog.records if r.levelno == logging.WARNING] + + +@pytest.mark.asyncio +async def test_check_host_readiness_continues_past_unreadable_key(caplog): + """ + One unreadable /proc/sys key must not discard the warnings already + collected nor skip the FUSE check (that was a mid-loop return). + """ + + import logging + from io import StringIO + + docker = Docker() + files = { + "/proc/sys/fs/inotify/max_user_instances": "128", # low -> must warn + # max_user_watches and fs.file-max: unreadable -> skipped + "/proc/filesystems": "nodev ext4\n", # no fuse -> must warn + } + + def fake_open(path, *args, **kwargs): + if path not in files: + raise OSError("masked") + return StringIO(files[path]) + + with patch("builtins.open", side_effect=fake_open): + with caplog.at_level(logging.WARNING, logger="gns3server.compute.docker"): + docker._check_host_readiness() + + message = " ".join(r.message for r in caplog.records) + assert "max_user_instances=128" in message # collected before the gap + assert "modprobe fuse" in message # FUSE check still ran diff --git a/tests/compute/docker/test_docker_vm.py b/tests/compute/docker/test_docker_vm.py index 53bfb2445..70d72b3bf 100644 --- a/tests/compute/docker/test_docker_vm.py +++ b/tests/compute/docker/test_docker_vm.py @@ -1998,3 +1998,103 @@ async def test_stop_exited_container_no_stop_query(vm): for call in mock_query.mock_calls ) assert vm.status == "stopped" + + +@pytest.mark.asyncio +async def test_create_dedups_overlapping_mount_targets(compute_project, manager): + """ + GNS3_MASK_UDEV overlapping a GNS3_MASK_SYSTEMD entry (or a unit named + twice) must not produce duplicate bind targets — Docker rejects the + create outright with "Duplicate mount point". + """ + + environment = "GNS3_MASK_UDEV=1\nGNS3_MASK_SYSTEMD=systemd-udevd.service,foo.service,foo.service" + response = {"Id": "e90e34656806", "Warnings": []} + + with asyncio_patch("gns3server.compute.docker.Docker.list_images", return_value=[{"image": "ubuntu"}]): + with asyncio_patch("gns3server.compute.docker.Docker.query", return_value=response) as mock: + vm = DockerVM("test", str(uuid.uuid4()), compute_project, manager, "ubuntu", environment=environment) + await vm.create() + mounts = mock.call_args[1]["data"]["HostConfig"]["Mounts"] + targets = [m["Target"] for m in mounts] + assert len(targets) == len(set(targets)), "duplicate bind targets in Mounts" + # the overlapping unit is present (masked) exactly once + assert targets.count("/etc/systemd/system/systemd-udevd.service") == 1 + assert targets.count("/etc/systemd/system/foo.service") == 1 + + +@pytest.mark.asyncio +async def test_create_env_trailing_comma_still_parsed(compute_project, manager): + """ + A trailing comma (environment composed from comma-separated lists) must + not silently disable the knobs — the base parser strips it like the + vendor parser does. + """ + + environment = "GNS3_MASK_UDEV=1,\nGNS3_SHM_SIZE=256," + response = {"Id": "e90e34656806", "Warnings": []} + + with asyncio_patch("gns3server.compute.docker.Docker.list_images", return_value=[{"image": "ubuntu"}]): + with asyncio_patch("gns3server.compute.docker.Docker.query", return_value=response) as mock: + vm = DockerVM("test", str(uuid.uuid4()), compute_project, manager, "ubuntu", environment=environment) + await vm.create() + host_config = mock.call_args[1]["data"]["HostConfig"] + assert host_config["ShmSize"] == 256 * 1024 * 1024 + masked = {m["Target"] for m in host_config["Mounts"] if m.get("Source") == "/dev/null"} + assert "/etc/systemd/system/systemd-udevd.service" in masked + + +@pytest.mark.asyncio +async def test_create_with_extra_configs_directory_target_rejected(compute_project, manager): + """ + Directory-form targets ('/', '/etc/', '///') would make the content write + fail with IsADirectoryError (a raw 500) — they must be rejected as + DockerError at create time. + """ + + response = {"Id": "e90e34656806", "Warnings": []} + for bad in ("/", "/etc/", "///"): + with asyncio_patch("gns3server.compute.docker.Docker.list_images", return_value=[{"image": "ubuntu"}]): + with asyncio_patch("gns3server.compute.docker.Docker.query", return_value=response): + vm = DockerVM("test", str(uuid.uuid4()), compute_project, manager, "ubuntu", + extra_configs=[{"target": bad, "content": "x"}]) + with pytest.raises(DockerError): + await vm.create() + + +def test_extra_config_schema_rejects_bad_targets(): + """ + The pydantic model rejects bad targets at template-save time (a 422) + instead of at node-create time (after a potentially multi-GB image pull). + """ + + from pydantic import ValidationError + from gns3server.schemas.common import ExtraConfig + + for bad in ("relative/path", "/has/../dots", "/", "/etc/", "no-leading-slash"): + with pytest.raises(ValidationError): + ExtraConfig(target=bad, content="x") + ok = ExtraConfig(target="/firstboot.cfg", content="x") + assert ok.target == "/firstboot.cfg" + + +@pytest.mark.asyncio +async def test_create_warns_when_extra_config_under_volume(compute_project, manager, caplog): + """ + An extra_configs target beneath a persisted volume is covered by the + volume bind at start — the injection would silently not take effect, so + warn at create time. + """ + + import logging + extra_configs = [{"target": "/xr-storage/config/foo.cfg", "content": "x"}] + response = {"Id": "e90e34656806", "Warnings": []} + + with asyncio_patch("gns3server.compute.docker.Docker.list_images", return_value=[{"image": "ubuntu"}]): + with asyncio_patch("gns3server.compute.docker.Docker.query", return_value=response): + vm = DockerVM("test", str(uuid.uuid4()), compute_project, manager, "ubuntu", + extra_configs=extra_configs, extra_volumes=["/xr-storage"]) + with caplog.at_level(logging.WARNING, logger="gns3server.compute.docker.docker_vm"): + await vm.create() + + assert any("shadowed by persisted volume" in r.message for r in caplog.records) diff --git a/tests/compute/docker/test_vendor_docker_vm.py b/tests/compute/docker/test_vendor_docker_vm.py index 33c5ce726..a4e8e92e3 100644 --- a/tests/compute/docker/test_vendor_docker_vm.py +++ b/tests/compute/docker/test_vendor_docker_vm.py @@ -620,17 +620,36 @@ async def test_create_exec_cmd_has_no_while_true(compute_project, manager): @pytest.mark.asyncio async def test_terminate_container_graceful_stop(compute_project, manager): - """Vendor containers must be SIGTERMed with a grace period, not SIGKILLed - on the spot: systemd NOS images (e.g. Cisco XRd) require a graceful - shutdown, and Docker itself SIGKILLs the container once the grace period - expires.""" + """With graceful=True (explicit user stop) vendor containers are SIGTERMed + with a grace period, not SIGKILLed on the spot: systemd NOS images + (e.g. Cisco XRd) require a graceful shutdown, and Docker itself SIGKILLs + the container once the grace period expires.""" vm = _make_vm(compute_project, manager) manager.query = AsyncioMagicMock() + manager.http_query = AsyncioMagicMock(return_value=MagicMock()) + + await vm._terminate_container(graceful=True) + + manager.http_query.assert_called_once_with( + "POST", "containers/e90e34656842/stop", params={"t": 60}, timeout=90) + manager.query.assert_not_called() # no kill on the graceful path + + +@pytest.mark.asyncio +async def test_terminate_container_default_is_kill(compute_project, manager): + """Without graceful (delete/update/close/crash cleanup) the vendor + container gets the base immediate kill — those paths force-delete or + recreate the container right after anyway.""" + + vm = _make_vm(compute_project, manager) + manager.query = AsyncioMagicMock() + manager.http_query = AsyncioMagicMock(return_value=MagicMock()) await vm._terminate_container() - manager.query.assert_called_once_with("POST", "containers/e90e34656842/stop", params={"t": 60}) + manager.query.assert_called_once_with("POST", "containers/e90e34656842/kill") + manager.http_query.assert_not_called() @pytest.mark.asyncio @@ -641,15 +660,16 @@ async def test_terminate_container_already_stopped_is_silent(compute_project, ma from gns3server.compute.docker.docker_error import DockerHttp304Error vm = _make_vm(compute_project, manager) - manager.query = AsyncioMagicMock( + manager.http_query = AsyncioMagicMock( side_effect=DockerHttp304Error("Docker has returned an error: 304")) - await vm._terminate_container() # must not raise + await vm._terminate_container(graceful=True) # must not raise @pytest.mark.asyncio async def test_stop_uses_graceful_termination(compute_project, manager): """The full stop() path must route through _terminate_container (the - vendor override), not the base class' immediate kill.""" + vendor override); the default is the fast kill — only the explicit user + stop route passes graceful=True.""" vm = _make_vm(compute_project, manager) with patch.object(DockerVM, "_clean_servers", new=AsyncioMagicMock()): @@ -657,16 +677,12 @@ async def test_stop_uses_graceful_termination(compute_project, manager): with patch.object( DockerVM, "_get_container_state", new=AsyncioMagicMock(return_value="running") ): + vm._permissions_fixed = True with patch.object( - VendorDockerVM, "_fix_permissions", new=AsyncioMagicMock() - ) as mock_perms: - mock_perms.return_value = None - vm._permissions_fixed = True - with patch.object( - VendorDockerVM, "_terminate_container", new=AsyncioMagicMock() - ) as mock_term: - await vm.stop() - mock_term.assert_called_once() + VendorDockerVM, "_terminate_container", new=AsyncioMagicMock() + ) as mock_term: + await vm.stop() + mock_term.assert_called_once_with(graceful=False) def test_env_stop_timeout(compute_project, manager): @@ -689,8 +705,29 @@ def test_env_stop_timeout_invalid_keeps_default(compute_project, manager): @pytest.mark.asyncio async def test_terminate_container_uses_env_timeout(compute_project, manager): vm = _make_vm(compute_project, manager, environment="GNS3_STOP_TIMEOUT=120") - manager.query = AsyncioMagicMock() + manager.http_query = AsyncioMagicMock(return_value=MagicMock()) - await vm._terminate_container() + await vm._terminate_container(graceful=True) - manager.query.assert_called_once_with("POST", "containers/e90e34656842/stop", params={"t": 120}) + manager.http_query.assert_called_once_with( + "POST", "containers/e90e34656842/stop", params={"t": 120}, timeout=150) + + +@pytest.mark.asyncio +async def test_create_reparse_refreshes_env_knobs(compute_project, manager): + """A PUT to the node's environment must take effect on the next create(), + not on the next project reload: create() re-parses the vendor knobs.""" + + response = _create_response(None, entrypoint=["/init"]) + vm = _make_vm(compute_project, manager, + environment="GNS3_SKIP_INIT=1\nGNS3_STOP_TIMEOUT=120") + assert vm._gns3_init is False and vm._stop_timeout == 120 + + vm._environment = "GNS3_STOP_TIMEOUT=5" # knob removed + value changed + with asyncio_patch("gns3server.compute.docker.Docker.list_images", + return_value=[{"image": "srlinux"}]): + with asyncio_patch("gns3server.compute.docker.Docker.query", return_value=response): + await vm.create() + + assert vm._stop_timeout == 5 + assert vm._gns3_init is True # removed entry reset to default