Merge pull request #3004 from Xeriioo/fix/docker-vnc-display-race

fix(docker): give each VNC node its own X display

gns3server/compute/docker/docker_vm.py

@@ -72,6 +72,11 @@ class DockerVM(DockerKernelDatapathMixin, BaseNode):
     :param extra_volumes: Additional directories to make persistent
     """
 
+    # Displays already given to a node of this server: nodes are created in
+    # parallel when a project is opened, and the X socket only appears once
+    # Xvnc has started, so the filesystem alone cannot tell they are taken.
+    _reserved_displays: set[int] = set()
+
     # systemd units masked by GNS3_MASK_UDEV=1: the udev daemon, its activation
     # sockets and the coldplug/settle triggers. Masking them stops a privileged
     # systemd container from replaying device events on the host.
@@ -220,17 +225,32 @@ class DockerVM(DockerKernelDatapathMixin, BaseNode):
             "cpus": self.cpus,
         }
 
+    @staticmethod
+    def _display_in_use(display):
+        """
+        True if an X server owns this display (socket, or lock of a live process)
+        """
+        if os.path.exists(f"/tmp/.X11-unix/X{display}"):
+            return True
+        try:
+            with open(f"/tmp/.X{display}-lock") as f:
+                return psutil.pid_exists(int(f.read().strip()))
+        except (OSError, ValueError):
+            return False
+
     def _get_free_display_port(self):
         """
-        Search a free display port
+        Search a free display port and reserve it for this node
         """
         display = 100
-        if not os.path.exists("/tmp/.X11-unix/"):
-            return display
-        while True:
-            if not os.path.exists(f"/tmp/.X11-unix/X{display}"):
-                return display
+        while display in DockerVM._reserved_displays or self._display_in_use(display):
             display += 1
+        DockerVM._reserved_displays.add(display)
+        return display
+
+    def _release_display(self):
+        if self._display is not None:
+            DockerVM._reserved_displays.discard(self._display)
 
     @property
     def name(self):
@@ -918,8 +938,18 @@ class DockerVM(DockerKernelDatapathMixin, BaseNode):
             return
         else:
             if self._console_type == "vnc" and not self._vnc_process:
-                # restart the vnc process in case it had previously crashed
+                # restart the vnc process in case it had previously crashed, on
+                # the display the container was created with; the socket left by
+                # the crashed process must go, the container bind-mounts the new
+                # one when it starts
+                x11_socket = f"/tmp/.X11-unix/X{self._display}"
+                with contextlib.suppress(FileNotFoundError):
+                    os.remove(x11_socket)
                 await self._start_vnc_process(restart=True)
+                try:
+                    await wait_for_file_creation(x11_socket)
+                except asyncio.TimeoutError:
+                    raise DockerError(f'x11 socket file "{x11_socket}" does not exist')
                 monitor_process(self._vnc_process, self._vnc_callback)
 
             if self._console_websocket:
@@ -1178,7 +1208,9 @@ class DockerVM(DockerKernelDatapathMixin, BaseNode):
         Starts the VNC process.
         """
 
-        self._display = self._get_free_display_port()
+        # on restart, keep the display the container was created with
+        if self._display is None:
+            self._display = self._get_free_display_port()
         tigervnc_path = shutil.which("Xtigervnc") or shutil.which("Xvnc")
 
         if not tigervnc_path:
@@ -1213,10 +1245,11 @@ class DockerVM(DockerKernelDatapathMixin, BaseNode):
         Starts a VNC server for this container
         """
 
-        self._display = self._get_free_display_port()
         tigervnc_path = shutil.which("Xtigervnc") or shutil.which("Xvnc")
         if not tigervnc_path:
             raise DockerError("Please install TigerVNC server before using VNC support")
+        self._release_display()
+        self._display = self._get_free_display_port()
         await self._start_vnc_process()
         x11_socket = os.path.join("/tmp/.X11-unix/", f"X{self._display}")
         try:
@@ -1564,13 +1597,16 @@ class DockerVM(DockerKernelDatapathMixin, BaseNode):
                     except ProcessLookupError:
                         pass
 
-                if self._display:
-                    display = f"/tmp/.X11-unix/X{self._display}"
-                    try:
-                        if os.path.exists(display):
-                            os.remove(display)
-                    except OSError as e:
-                        log.warning(f"Could not remove display {display}: {e}")
+                    # only remove the socket of our own Xvnc, never another node's
+                    if self._display:
+                        display = f"/tmp/.X11-unix/X{self._display}"
+                        try:
+                            if os.path.exists(display):
+                                os.remove(display)
+                        except OSError as e:
+                            log.warning(f"Could not remove display {display}: {e}")
+                self._release_display()
+                self._display = None
 
             # v – 1/True/true or 0/False/false, Remove the volumes associated to the container. Default false.
             # force - 1/True/true or 0/False/false, Kill then remove the container. Default false.

tests/compute/docker/test_docker_vm.py

@@ -19,7 +19,7 @@ import os
 import shutil
 import uuid
 from types import SimpleNamespace
-from unittest.mock import AsyncMock, MagicMock, call, patch
+from unittest.mock import AsyncMock, MagicMock, call, mock_open, patch
 
 import aiohttp
 import pytest
@@ -1889,6 +1889,103 @@ async def test_start_vnc_missing(vm):
             await vm._start_vnc()
 
 
+@pytest.mark.asyncio
+async def test_start_vnc_concurrent_nodes_get_distinct_displays(compute_project, manager, monkeypatch):
+    """
+    Nodes are created in parallel when a project is opened: the X socket of the
+    first node does not exist yet when the second node picks its display.
+    """
+
+    monkeypatch.setattr(DockerVM, "_reserved_displays", set())
+    vms = [DockerVM(f"PC{i}", str(uuid.uuid4()), compute_project, manager, "ubuntu:latest") for i in range(3)]
+
+    async def wait_for_socket(path):
+        await asyncio.sleep(0)  # let the other nodes run, like Xvnc starting
+
+    with patch("shutil.which", return_value="/bin/Xtigervnc"), patch("os.path.exists", return_value=False):
+        with (
+            asyncio_patch("gns3server.compute.docker.docker_vm.wait_for_file_creation", side_effect=wait_for_socket),
+            patch("psutil.pid_exists", return_value=False),
+        ):
+            with asyncio_patch("asyncio.create_subprocess_exec"):
+                with patch("gns3server.compute.docker.docker_vm.monitor_process"):
+                    await asyncio.gather(*(vm._start_vnc() for vm in vms))
+
+    assert sorted(vm._display for vm in vms) == [100, 101, 102]
+
+
+def test_get_free_display_port_skips_live_lock(vm, monkeypatch):
+
+    monkeypatch.setattr(DockerVM, "_reserved_displays", set())
+
+    def fake_open(path, *args, **kwargs):
+        if path == "/tmp/.X100-lock":
+            return mock_open(read_data=f"{os.getpid():>10}\n")()
+        raise FileNotFoundError(path)
+
+    with patch("os.path.exists", return_value=False), patch("builtins.open", side_effect=fake_open):
+        assert vm._get_free_display_port() == 101
+
+
+def test_get_free_display_port_ignores_stale_lock(vm, monkeypatch):
+
+    monkeypatch.setattr(DockerVM, "_reserved_displays", set())
+    with patch("os.path.exists", return_value=False):
+        with patch("builtins.open", mock_open(read_data="     99999\n")):
+            with patch("psutil.pid_exists", return_value=False):
+                assert vm._get_free_display_port() == 100
+
+
+@pytest.mark.asyncio
+async def test_close_vnc_crashed_keeps_other_node_socket(vm, monkeypatch):
+    """
+    When Xvnc failed, the display may belong to another node: its socket must stay.
+    """
+
+    monkeypatch.setattr(DockerVM, "_reserved_displays", {100})
+    vm._console_type = "vnc"
+    vm._display = 100
+    vm._vnc_process = None
+
+    with asyncio_patch("gns3server.compute.docker.DockerVM._get_container_state", return_value="stopped"):
+        with asyncio_patch("gns3server.compute.docker.Docker.query"):
+            with patch("os.remove") as mock_remove:
+                await vm.close()
+
+    assert not mock_remove.called
+    assert vm._display is None
+    assert DockerVM._reserved_displays == set()
+
+
+@pytest.mark.asyncio
+async def test_start_restarts_crashed_vnc_on_same_display(vm):
+    """
+    The container is bound to the X socket of the display it was created with.
+    """
+
+    vm._console_type = "vnc"
+    vm._display = 105
+    vm._vnc_process = None
+    vm._get_container_state = AsyncioMagicMock(return_value="stopped")
+    vm._start_ubridge = AsyncioMagicMock()
+    vm._get_namespace = AsyncioMagicMock(return_value=42)
+    vm._start_console = AsyncioMagicMock()
+    vm._add_ubridge_connection = AsyncioMagicMock()
+
+    with patch("shutil.which", return_value="/bin/Xtigervnc"), patch("os.remove") as mock_remove:
+        with asyncio_patch("gns3server.compute.docker.docker_vm.wait_for_file_creation") as mock_wait:
+            with asyncio_patch("asyncio.create_subprocess_exec") as mock_exec:
+                with patch("gns3server.compute.docker.docker_vm.monitor_process"):
+                    with patch("gns3server.compute.docker.Docker.install_busybox"):
+                        with asyncio_patch("gns3server.compute.docker.Docker.query"):
+                            await vm.start()
+
+    assert vm._display == 105
+    assert mock_exec.call_args[0][-1] == ":105"
+    mock_remove.assert_any_call("/tmp/.X11-unix/X105")
+    mock_wait.assert_called_with("/tmp/.X11-unix/X105")
+
+
 @pytest.mark.asyncio
 async def test_start_aux(vm):