diff --git a/gns3server/compute/docker/docker_vm.py b/gns3server/compute/docker/docker_vm.py index 9e99c9104..9c511ff4c 100644 --- a/gns3server/compute/docker/docker_vm.py +++ b/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: @@ -1549,13 +1582,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. diff --git a/tests/compute/docker/test_docker_vm.py b/tests/compute/docker/test_docker_vm.py index efc7b0437..a491008da 100644 --- a/tests/compute/docker/test_docker_vm.py +++ b/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):