From 290c542afe1663980df775a1b01e996b01b79408 Mon Sep 17 00:00:00 2001 From: YueGuobin Date: Fri, 16 Oct 2026 16:49:06 +0800 Subject: [PATCH 1/2] fix: roll back a node update the compute refused MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A compute 409 (for instance "must be stopped to change the console type to vnc") left every value that had already been applied locally on the controller node, because the update loop sets them before the PUT. Nothing was dumped at that point, but project.dump() serializes the whole topology: the next successful change anywhere in the project wrote the refused values to disk, and the next project open recreated the node with them — this time accepted by a stopped node. A refused change silently landed later. Restore the applied properties, the node properties, custom adapters, missing images and the derived ports before re-raising, the way the degraded-image create() path already cleans up after its own failure. --- gns3server/controller/node.py | 14 ++++++++++++++ tests/controller/test_node.py | 30 ++++++++++++++++++++++++++++++ 2 files changed, 44 insertions(+) diff --git a/gns3server/controller/node.py b/gns3server/controller/node.py index 6022ff982..fb36ee8ef 100644 --- a/gns3server/controller/node.py +++ b/gns3server/controller/node.py @@ -711,6 +711,9 @@ class Node: old_properties = copy.deepcopy(self._properties) old_custom_adapters = copy.deepcopy(self._custom_adapters) old_missing_images = copy.deepcopy(self._missing_images) + # Values applied locally below, kept to roll the node back if the + # compute refuses the update (see the ComputeConflictError handler). + old_prop_values = {prop: getattr(self, prop) for prop in kwargs if prop != "properties"} compute_properties = None # Update node properties with additional elements @@ -766,9 +769,20 @@ class Node: try: response = await self.put(None, data=data) except ComputeConflictError: + # The compute refused the update: roll back everything applied + # locally. Keeping the requested values would leave the + # controller describing a node the compute never accepted, and + # the next project dump persists them — a refused change would + # then silently land the next time the node is created. + for prop, old_value in old_prop_values.items(): + setattr(self, prop, old_value) if old_name != self.name: # special case when the new name is already updated on controller but refused by the compute self.name = old_name + self._properties = old_properties + self._custom_adapters = old_custom_adapters + self._missing_images = old_missing_images + self._list_ports() raise await self.parse_node_response(response.json) elif old_json != self.asdict(): diff --git a/tests/controller/test_node.py b/tests/controller/test_node.py index f17eaea3d..4d6a561db 100644 --- a/tests/controller/test_node.py +++ b/tests/controller/test_node.py @@ -811,6 +811,36 @@ async def test_update_properties(node, compute, controller): # controller._notification.emit.assert_called_with("node.updated", node_notif) +@pytest.mark.asyncio +async def test_update_rolls_back_when_compute_refuses(node, compute, project, controller): + """ + Values applied locally before the PUT must be rolled back when the compute + refuses the update: kept in memory they describe a node the compute never + accepted, and the next project dump persists them — the refused change + would then silently land the next time the node is created. + """ + + async def resp(*args, **kwargs): + raise ComputeConflictError( + f"/projects/{project.id}/vpcs/nodes/{node.id}", + {"message": '"demo" must be stopped to change the console type to telnet'}, + ) + + compute.put = AsyncioMagicMock(side_effect=resp) + controller._notification = AsyncioMagicMock() + project.dump = MagicMock() + old_json = node.asdict() + + with pytest.raises(ComputeConflictError): + await node.update(x=42, console_type="telnet", name="renamed", properties={"startup_script": "changed"}) + + assert node.name == "demo" + assert node.console_type == "vnc" + assert node.properties == {"startup_script": "echo test"} + assert node.asdict() == old_json + assert not project.dump.called + + @pytest.mark.asyncio async def test_update_only_controller(node, compute): """ From 1090a32bbb4ea9202f4d5d4e34f980b36f5a6eb4 Mon Sep 17 00:00:00 2001 From: YueGuobin Date: Fri, 16 Oct 2026 16:49:11 +0800 Subject: [PATCH 2/2] fix: refuse Docker node updates that recreate a running container MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Updating any Docker node property destroys and recreates the container (the route calls DockerVM.update() as soon as the property diff is non-empty), so it discards everything written inside it. On a running node that happened silently, as a side effect of an unrelated edit: the user only saw the node restart. Refuse the update while the container is up and say what stopping it is for, like QEMU, IOU, Dynamips, VPCS, VirtualBox and VMware already do for their own restart-requiring settings. The check runs before anything is applied — a refused update leaves the node untouched — and only when the PUT really changes a property, since Web clients send the whole node back on every edit. is_running() is not reusable for the question: it reports a paused container as not running and stops crashed containers as a side effect, so DockerVM gains is_up() for "running or paused". --- gns3server/api/routes/compute/docker_nodes.py | 28 ++++++-- gns3server/compute/docker/docker_vm.py | 15 +++++ tests/api/routes/compute/test_docker_nodes.py | 65 +++++++++++++++++++ 3 files changed, 101 insertions(+), 7 deletions(-) diff --git a/gns3server/api/routes/compute/docker_nodes.py b/gns3server/api/routes/compute/docker_nodes.py index a66084f5b..dc251fdac 100644 --- a/gns3server/api/routes/compute/docker_nodes.py +++ b/gns3server/api/routes/compute/docker_nodes.py @@ -28,6 +28,7 @@ from fastapi.responses import StreamingResponse from gns3server import schemas from gns3server.compute.docker import Docker +from gns3server.compute.docker.docker_error import DockerError from gns3server.compute.docker.docker_vm import DockerVM from .dependencies.authentication import compute_authentication, ws_compute_authentication @@ -154,15 +155,28 @@ async def update_docker_node(node_update: schemas.DockerUpdate, node: DockerVM = "cpus", ] - changed = False node_data = jsonable_encoder(node_update, exclude_unset=True) - for prop in props: - # hasattr: startup_config_content only exists on IOLDockerVM - if prop in node_data and hasattr(node, prop) and node_data[prop] != getattr(node, prop): - setattr(node, prop, node_data[prop]) - changed = True + # hasattr: startup_config_content only exists on IOLDockerVM + changed_props = [ + prop for prop in props if prop in node_data and hasattr(node, prop) and node_data[prop] != getattr(node, prop) + ] + + # A property change is applied by destroying and recreating the container, + # so it discards whatever was written inside it. Refuse that while the + # container is up (like QEMU, IOU, Dynamips, VPCS, VirtualBox and VMware do + # for their own restart-requiring settings) instead of resetting a running + # node behind the user's back. Nothing is applied before this check: a + # refused update must leave the node exactly as it was. + if changed_props and await node.is_up(): + raise DockerError( + f'"{node.name}" must be stopped to change {", ".join(sorted(changed_props))}: ' + "the container is recreated and changes made inside it are lost" + ) + + for prop in changed_props: + setattr(node, prop, node_data[prop]) # We don't call container.update for nothing because it will restart the container - if changed: + if changed_props: await node.update() node.updated() return node.asdict() diff --git a/gns3server/compute/docker/docker_vm.py b/gns3server/compute/docker/docker_vm.py index 9e99c9104..6528d73e7 100644 --- a/gns3server/compute/docker/docker_vm.py +++ b/gns3server/compute/docker/docker_vm.py @@ -1395,6 +1395,21 @@ class DockerVM(DockerKernelDatapathMixin, BaseNode): await self.stop() return False + async def is_up(self): + """ + Checks if the container exists in a running or paused state. + + Both mean the container holds live state that recreating it would + discard. is_running() is not usable for that question: it reports a + paused container as not running, and it stops a crashed container as + a side effect. + + :returns: True or False + :rtype: bool + """ + + return await self._get_container_state() in ("running", "paused") + async def restart(self): """ Restart this Docker container. diff --git a/tests/api/routes/compute/test_docker_nodes.py b/tests/api/routes/compute/test_docker_nodes.py index c706f34b3..cf81fc972 100644 --- a/tests/api/routes/compute/test_docker_nodes.py +++ b/tests/api/routes/compute/test_docker_nodes.py @@ -392,6 +392,71 @@ class TestDockerNodesRoutes: assert response.json()["start_command"] is None assert response.json()["console_http_path"] == "/" + async def test_docker_update_refused_while_container_is_up( + self, app: FastAPI, compute_client: AsyncClient, vm: dict + ) -> None: + """ + A property change is applied by recreating the container, which throws + away what was written inside it. While the container is up (running or + paused) the update is refused instead of resetting the node behind the + user's back, and the error says what stopping it would cost. + """ + + for state in ("running", "paused"): + with asyncio_patch("gns3server.compute.docker.docker_vm.DockerVM.update") as mock: + with asyncio_patch( + "gns3server.compute.docker.docker_vm.DockerVM._get_container_state", return_value=state + ): + response = await compute_client.put( + app.url_path_for( + "compute:update_docker_node", project_id=vm["project_id"], node_id=vm["node_id"] + ), + json={"console_type": "vnc"}, + ) + + assert response.status_code == 409 + assert "must be stopped" in response.json()["message"] + assert "console_type" in response.json()["message"] + assert not mock.called + + # the refused update left the node exactly as it was + response = await compute_client.get( + app.url_path_for("compute:get_docker_node", project_id=vm["project_id"], node_id=vm["node_id"]) + ) + assert response.json()["console_type"] == "telnet" + + async def test_docker_update_without_change_is_allowed_while_container_is_up( + self, app: FastAPI, compute_client: AsyncClient, vm: dict + ) -> None: + """ + Clients send the whole node back on every edit: a PUT that changes + nothing must not be refused just because the container is running. + """ + + with asyncio_patch("gns3server.compute.docker.docker_vm.DockerVM._get_container_state", return_value="running"): + response = await compute_client.put( + app.url_path_for("compute:update_docker_node", project_id=vm["project_id"], node_id=vm["node_id"]), + json={"name": "DOCKER-TEST-1", "console_type": "telnet"}, + ) + assert response.status_code == 200 + + async def test_docker_update_stopped_container_is_recreated( + self, app: FastAPI, compute_client: AsyncClient, vm: dict + ) -> None: + """A stopped container has no live state to lose: the change applies.""" + + with asyncio_patch("gns3server.compute.docker.docker_vm.DockerVM.update") as mock: + with asyncio_patch( + "gns3server.compute.docker.docker_vm.DockerVM._get_container_state", return_value="exited" + ): + response = await compute_client.put( + app.url_path_for("compute:update_docker_node", project_id=vm["project_id"], node_id=vm["node_id"]), + json={"console_type": "vnc"}, + ) + assert response.status_code == 200 + assert mock.called + assert response.json()["console_type"] == "vnc" + async def test_docker_start_capture(self, app: FastAPI, compute_client: AsyncClient, vm: dict) -> None: url = app.url_path_for(