diff --git a/gns3server/compute/base_node.py b/gns3server/compute/base_node.py index 65b816e5c..30fa856c5 100644 --- a/gns3server/compute/base_node.py +++ b/gns3server/compute/base_node.py @@ -340,14 +340,43 @@ class BaseNode: """ def set_rw(operation, name, exc): - os.chmod(name, stat.S_IWRITE) + # Add the missing user permissions instead of replacing the whole mode: chmod'ing + # a directory to S_IWRITE removes the search permission on POSIX systems, making + # the directory impossible to traverse or to delete. Also note that S_IWRITE + # clears the read-only attribute on Windows. + try: + if os.path.isdir(name) and not os.path.islink(name): + os.chmod(name, os.stat(name).st_mode | stat.S_IRWXU) + elif not os.path.islink(name): + os.chmod(name, os.stat(name).st_mode | stat.S_IRUSR | stat.S_IWUSR) + # retry the failed operation now that the permissions are fixed + # (retrying os.scandir is not possible, the directory is handled by the + # retry loop below) + if operation in (os.unlink, os.rmdir): + operation(name) + except OSError: + pass directory = self.project.node_working_directory(self) - if os.path.exists(directory): + # Retry the deletion: a concurrent task (e.g. a MD5 checksum computation caching + # its result in the node directory) can recreate a file while the directory is + # being deleted, and shutil.rmtree silently gives up when its error handler returns + for attempt in range(3): + if not os.path.exists(directory): + return try: await wait_run_in_executor(shutil.rmtree, directory, onerror=set_rw) except OSError as e: raise ComputeError(f"Could not delete the node working directory: {e}") + if not os.path.exists(directory): + return + if attempt == 2: + raise ComputeError( + f"Could not delete the node working directory '{directory}': a file may have been " + "recreated in it or could not be removed" + ) + log.warning(f"Could not completely delete the node working directory '{directory}', retrying") + await asyncio.sleep(0.1) def start(self): """ diff --git a/tests/compute/docker/test_docker_vm.py b/tests/compute/docker/test_docker_vm.py index 51359b024..5ad4eb4f8 100644 --- a/tests/compute/docker/test_docker_vm.py +++ b/tests/compute/docker/test_docker_vm.py @@ -21,6 +21,7 @@ import pytest import pytest_asyncio import uuid import os +import shutil from types import SimpleNamespace from unittest.mock import patch @@ -2111,12 +2112,22 @@ async def test_close_reclaims_node_directory(vm, port_manager): @pytest.mark.asyncio async def test_delete_retries_after_reclaim(vm): + real_rmtree = shutil.rmtree + calls = 0 + + def rmtree_first_fails_then_deletes(directory, onerror=None, **kwargs): + nonlocal calls + calls += 1 + if calls == 1: + raise OSError("permission denied") + real_rmtree(directory, onerror=onerror) + with patch.object(vm, "close", new_callable=AsyncioMagicMock): with patch.object(vm, "_reclaim_directory_ownership", new_callable=AsyncioMagicMock, return_value=True) as mock_reclaim: # First rmtree hits the root-owned leftovers, the retry (after - # the reclaim) succeeds. + # the reclaim) succeeds and really deletes the directory. with patch("gns3server.compute.base_node.shutil.rmtree", - side_effect=[OSError("permission denied"), None]) as mock_rmtree: + side_effect=rmtree_first_fails_then_deletes) as mock_rmtree: await vm.delete() assert mock_rmtree.call_count == 2 mock_reclaim.assert_called_once_with(vm.working_dir) diff --git a/tests/compute/test_base_node.py b/tests/compute/test_base_node.py index 7eca945e1..6420d905b 100644 --- a/tests/compute/test_base_node.py +++ b/tests/compute/test_base_node.py @@ -16,6 +16,7 @@ # along with this program. If not, see . import os +import shutil from collections import OrderedDict import pytest @@ -24,6 +25,7 @@ import pytest_asyncio from tests.utils import asyncio_patch, AsyncioMagicMock from unittest.mock import patch, MagicMock +from gns3server.compute.compute_error import ComputeError from gns3server.compute.vpcs.vpcs_vm import VPCSVM from gns3server.compute.docker.docker_vm import DockerVM from gns3server.compute.error import NodeError @@ -507,3 +509,93 @@ async def test_console_websocket_client_disconnect_while_node_output_streams( "has disconnected from compute console WebSocket while node output" in r.message for r in caplog.records ) + + +@pytest.mark.asyncio +async def test_delete_node_working_directory(node): + + working_dir = node.working_dir + with open(os.path.join(working_dir, "test.txt"), "w") as f: + f.write("TEST") + await node.delete() + assert not os.path.exists(working_dir) + + +@pytest.mark.asyncio +async def test_delete_directory_without_user_permissions(node): + # regression test: a failed deletion must not chmod the directory to S_IWRITE (0o200), + # which removes the search permission and makes the directory undeletable + working_dir = node.working_dir + with open(os.path.join(working_dir, "test.txt"), "w") as f: + f.write("TEST") + os.chmod(working_dir, 0o200) + try: + await node.delete() + assert not os.path.exists(working_dir) + finally: + # restore the permissions so a failed test does not leave an undeletable + # directory behind on the shared project path + if os.path.exists(working_dir): + os.chmod(working_dir, 0o700) + + +@pytest.mark.asyncio +async def test_delete_directory_with_readonly_entries(node): + + working_dir = node.working_dir + with open(os.path.join(working_dir, "test.txt"), "w") as f: + f.write("TEST") + os.chmod(os.path.join(working_dir, "test.txt"), 0o000) + os.chmod(working_dir, 0o500) # remove the write permission + try: + await node.delete() + assert not os.path.exists(working_dir) + finally: + if os.path.exists(working_dir): + os.chmod(working_dir, 0o700) + + +@pytest.mark.asyncio +async def test_delete_directory_with_file_recreated_during_deletion(node, monkeypatch): + # regression test: a concurrent MD5 checksum computation can cache its result in the + # node directory while it is being deleted, recreating a file after shutil.rmtree + # has listed the directory (rmtree then silently gives up on the final rmdir) + working_dir = node.working_dir + with open(os.path.join(working_dir, "hda_disk_image.md5sum"), "w") as f: + f.write("0" * 32) + real_rmtree = shutil.rmtree + calls = 0 + + def rmtree_recreating_a_file(directory, onerror=None, **kwargs): + nonlocal calls + calls += 1 + if calls == 1: + # delete the entries but leave one behind, as if the final rmdir + # had failed with ENOTEMPTY after the error handler returned + for entry in os.listdir(directory): + os.remove(os.path.join(directory, entry)) + with open(os.path.join(directory, "hda_disk_image.md5sum"), "w") as f: + f.write("0" * 32) + return + real_rmtree(directory, onerror=onerror, **kwargs) + + monkeypatch.setattr("gns3server.compute.base_node.shutil.rmtree", rmtree_recreating_a_file) + await node.delete() + assert calls == 2 + assert not os.path.exists(working_dir) + + +@pytest.mark.asyncio +async def test_delete_directory_failure_raises(node, monkeypatch): + + working_dir = node.working_dir + with open(os.path.join(working_dir, "test.txt"), "w") as f: + f.write("TEST") + + def rmtree_not_deleting(directory, onerror=None, **kwargs): + pass # simulate a persistent deletion failure + + monkeypatch.setattr("gns3server.compute.base_node.shutil.rmtree", rmtree_not_deleting) + with pytest.raises(ComputeError): + await node.delete() + assert os.path.exists(working_dir)