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(