From 9b740e85d0837d416c63c3c9dbf21989b993fb95 Mon Sep 17 00:00:00 2001 From: Julien Duponchelle Date: Wed, 7 Oct 2015 11:34:27 +0200 Subject: [PATCH] Fix Dynamips identifier is already used by another router Fix #327 --- gns3server/modules/dynamips/__init__.py | 53 ++++++++++++++++--- gns3server/modules/dynamips/nodes/router.py | 19 ++----- .../modules/dynamips/test_dynamips_manager.py | 51 ++++++++++++++++++ 3 files changed, 102 insertions(+), 21 deletions(-) diff --git a/gns3server/modules/dynamips/__init__.py b/gns3server/modules/dynamips/__init__.py index 2b00849b..2759d09d 100644 --- a/gns3server/modules/dynamips/__init__.py +++ b/gns3server/modules/dynamips/__init__.py @@ -116,6 +116,43 @@ class Dynamips(BaseManager): self._devices = {} self._ghost_files = set() self._dynamips_path = None + self._dynamips_ids = {} + + def get_dynamips_id(self, project_id): + """ + :param project_id: UUID of the project + :returns: a free dynamips id + """ + self._dynamips_ids.setdefault(project_id, set()) + dynamips_id = 0 + for dynamips_id in range(1, 4097): + if dynamips_id not in self._dynamips_ids[project_id]: + self._dynamips_ids[project_id].add(dynamips_id) + return dynamips_id + raise DynamipsError("Maximum number of Dynamips instances reached") + + def take_dynamips_id(self, project_id, dynamips_id): + """ + Reserve a dynamips id or raise an error + + :param project_id: UUID of the project + :param dynamips_id: Asked id + """ + self._dynamips_ids.setdefault(project_id, set()) + if dynamips_id in self._dynamips_ids[project_id]: + raise DynamipsError("Dynamips identifier {} is already used by another router".format(dynamips_id)) + self._dynamips_ids[project_id].add(dynamips_id) + + def release_dynamips_id(self, project_id, dynamips_id): + """ + A dynamips id can be reused by another vm + + :param project_id: UUID of the project + :param dynamips_id: Asked id + """ + self._dynamips_ids.setdefault(project_id, set()) + if dynamips_id in self._dynamips_ids[project_id]: + self._dynamips_ids[project_id].remove(dynamips_id) @asyncio.coroutine def unload(self): @@ -165,15 +202,14 @@ class Dynamips(BaseManager): :param project: Project instance """ - yield from super().project_closed(project) # delete useless Dynamips files project_dir = project.module_working_path(self.module_name.lower()) - files = glob.glob(glob_escape(os.path.join(project_dir, "*.ghost"))) - files += glob.glob(glob_escape(os.path.join(project_dir, "*_lock"))) - files += glob.glob(glob_escape(os.path.join(project_dir, "ilt_*"))) - files += glob.glob(glob_escape(os.path.join(project_dir, "c[0-9][0-9][0-9][0-9]_i[0-9]*_rommon_vars"))) - files += glob.glob(glob_escape(os.path.join(project_dir, "c[0-9][0-9][0-9][0-9]_i[0-9]*_log.txt"))) + files = glob.glob(os.path.join(glob_escape(project_dir), "*.ghost")) + files += glob.glob(os.path.join(glob_escape(project_dir), "*_lock")) + files += glob.glob(os.path.join(glob_escape(project_dir), "ilt_*")) + files += glob.glob(os.path.join(glob_escape(project_dir), "c[0-9][0-9][0-9][0-9]_i[0-9]*_rommon_vars")) + files += glob.glob(os.path.join(glob_escape(project_dir), "c[0-9][0-9][0-9][0-9]_i[0-9]*_log.txt")) for file in files: try: log.debug("Deleting file {}".format(file)) @@ -184,6 +220,11 @@ class Dynamips(BaseManager): log.warn("Could not delete file {}: {}".format(file, e)) continue + # Release the dynamips ids if we want to reload the same project + # later + if project.id: + del self._dynamips_ids[project.id] + @asyncio.coroutine def project_moved(self, project): """ diff --git a/gns3server/modules/dynamips/nodes/router.py b/gns3server/modules/dynamips/nodes/router.py index bf25b6a6..b896e488 100644 --- a/gns3server/modules/dynamips/nodes/router.py +++ b/gns3server/modules/dynamips/nodes/router.py @@ -53,7 +53,6 @@ class Router(BaseVM): :param platform: Platform of this router """ - _dynamips_ids = {} _status = {0: "inactive", 1: "shutting down", 2: "running", @@ -95,20 +94,11 @@ class Router(BaseVM): self._ghost_flag = ghost_flag if not ghost_flag: - self._dynamips_ids.setdefault(project.id, list()) if not dynamips_id: - # find a Dynamips ID if none is provided (0 < id <= 4096) - self._dynamips_id = 0 - for identifier in range(1, 4097): - if identifier not in self._dynamips_ids[project.id]: - self._dynamips_id = identifier - break - if self._dynamips_id == 0: - raise DynamipsError("Maximum number of Dynamips instances reached") + self._dynamips_id = manager.get_dynamips_id(project.id) else: - if dynamips_id in self._dynamips_ids[project.id]: - raise DynamipsError("Dynamips identifier {} is already used by another router".format(dynamips_id)) - self._dynamips_ids[project.id].append(self._dynamips_id) + self._dynamips_id = dynamips_id + manager.take_dynamips_id(project.id, dynamips_id) if self._aux is not None: self._aux = self._manager.port_manager.reserve_tcp_port(self._aux, self._project) @@ -1608,8 +1598,7 @@ class Router(BaseVM): log.warn("Could not delete file {}: {}".format(file, e)) continue - if self._dynamips_id in self._dynamips_ids[self._project.id]: - self._dynamips_ids[self._project.id].remove(self._dynamips_id) + self.manager.release_dynamips_id(self._project.id, self._dynamips_id) @asyncio.coroutine def clean_delete(self): diff --git a/tests/modules/dynamips/test_dynamips_manager.py b/tests/modules/dynamips/test_dynamips_manager.py index 00d48150..c744b29d 100644 --- a/tests/modules/dynamips/test_dynamips_manager.py +++ b/tests/modules/dynamips/test_dynamips_manager.py @@ -19,6 +19,9 @@ import pytest import tempfile import sys +import uuid +import os +import asyncio from gns3server.modules.dynamips import Dynamips from gns3server.modules.dynamips.dynamips_error import DynamipsError @@ -37,9 +40,57 @@ def test_vm_invalid_dynamips_path(manager): with pytest.raises(DynamipsError): manager.find_dynamips() + @pytest.mark.skipif(sys.platform.startswith("win"), reason="Not supported by Windows") def test_vm_non_executable_dynamips_path(manager): tmpfile = tempfile.NamedTemporaryFile() with patch("gns3server.config.Config.get_section_config", return_value={"dynamips_path": tmpfile.name}): with pytest.raises(DynamipsError): manager.find_dynamips() + + +def test_get_dynamips_id(manager): + project_1 = str(uuid.uuid4()) + project_2 = str(uuid.uuid4()) + project_3 = str(uuid.uuid4()) + + assert manager.get_dynamips_id(project_1) == 1 + assert manager.get_dynamips_id(project_1) == 2 + assert manager.get_dynamips_id(project_2) == 1 + with pytest.raises(DynamipsError): + for dynamips_id in range(1, 4098): + manager.get_dynamips_id(project_3) + + +def test_take_dynamips_id(manager): + project_1 = str(uuid.uuid4()) + + manager.take_dynamips_id(project_1, 1) + assert manager.get_dynamips_id(project_1) == 2 + with pytest.raises(DynamipsError): + manager.take_dynamips_id(project_1, 1) + + +def test_release_dynamips_id(manager): + project_1 = str(uuid.uuid4()) + project_2 = str(uuid.uuid4()) + + manager.take_dynamips_id(project_1, 1) + manager.release_dynamips_id(project_1, 1) + assert manager.get_dynamips_id(project_1) == 1 + # Should not crash for 0 id + manager.release_dynamips_id(project_2, 0) + + +def test_project_closed(manager, project, loop): + + manager._dynamips_ids[project.id] = set([1, 2, 3]) + + project_dir = project.module_working_path(manager.module_name.lower()) + os.makedirs(project_dir) + open(os.path.join(project_dir, "test.ghost"), "w+").close() + + loop.run_until_complete(asyncio.async(manager.project_closed(project))) + + assert not os.path.exists(os.path.join(project_dir, "test.ghost")) + assert project.id not in manager._dynamips_ids