docker: harden the shm/devices/extra_configs/masking work (code review)

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.

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

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)

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