Fix Dynamips identifier is already used by another router

Fix #327

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):
         """

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):

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