Merge pull request #2953 from markparonyan/api-issue-04-burx2g

fix(links): reserve ports on add, accept port name and auto, add port_in_use code

gns3server/api/routes/controller/links.py

@@ -32,7 +32,7 @@ from fastapi.responses import FileResponse, StreamingResponse
 from gns3server import schemas
 from gns3server.agent.web_wireshark.manager import WebWiresharkManager
 from gns3server.controller import Controller
-from gns3server.controller.controller_error import ControllerError
+from gns3server.controller.controller_error import ControllerError, ControllerNotFoundError
 from gns3server.controller.link import _UNSET, Link
 from gns3server.db.repositories.rbac import RbacRepository
 from gns3server.utils.http_client import HTTPClient
@@ -59,6 +59,29 @@ responses: dict[int | str, dict[str, Any]] = {
 router = APIRouter(responses=responses)
 
 
+def _select_port(node, endpoint, other_port):
+    if endpoint.get("port_name") is not None:
+        port = node.get_port_by_name(endpoint["port_name"])
+        if port is None:
+            raise ControllerNotFoundError(f"Port named {endpoint['port_name']} for {node.name} not found")
+        return port
+    if endpoint.get("port") == "auto":
+        port = node.get_free_port(other_port.link_type if other_port else None)
+        if port is None:
+            raise ControllerError(f"No free port available on {node.name}", code="no_free_port")
+        return port
+    return None
+
+
+def _peer_port(project, endpoint):
+    if endpoint.get("port") == "auto":
+        return None
+    node = project.get_node(endpoint["node_id"])
+    if endpoint.get("port_name") is not None:
+        return node.get_port_by_name(endpoint["port_name"])
+    return node.get_port(endpoint["adapter_number"], endpoint["port_number"])
+
+
 async def dep_link(project_id: UUID, link_id: UUID) -> Link:
     """
     Dependency to retrieve a link.
@@ -118,14 +141,18 @@ async def create_link(project_id: UUID, link_create: schemas.LinkCreate) -> sche
     if "show_filters_icon" in link_data:
         await link.update_show_filters_icon(link_data["show_filters_icon"])
     try:
-        for node in link_data["nodes"]:
+        endpoints = link_data["nodes"]
-            await link.add_node(
+        attached_port = None
-                project.get_node(node["node_id"]),
+        for index, endpoint in enumerate(endpoints):
-                node.get("adapter_number", 0),
+            node = project.get_node(endpoint["node_id"])
-                node.get("port_number", 0),
+            peer_port = attached_port or _peer_port(project, endpoints[1 - index])
-                label=node.get("label"),
+            port = _select_port(node, endpoint, peer_port)
-            )
+            adapter_number = port.adapter_number if port else endpoint["adapter_number"]
+            port_number = port.port_number if port else endpoint["port_number"]
+            await link.add_node(node, adapter_number, port_number, label=endpoint.get("label"))
+            attached_port = node.get_port(adapter_number, port_number)
     except ControllerError as e:
+        link.release_ports()
         await project.delete_link(link.id)
         raise e
     return link.asdict()

gns3server/controller/link.py

@@ -326,17 +326,29 @@ class Link:
             {"node": node, "adapter_number": adapter_number, "port_number": port_number, "port": port, "label": label}
         )
 
+        if not batch:
+            port.link = self
+
         if len(self._nodes) == 2 and not batch:
-            await self.create()
+            try:
+                await self.create()
+            except BaseException:
+                self.release_ports()
+                raise
             for n in self._nodes:
                 n["node"].add_link(self)
-                n["port"].link = self
             self._created = True
             self._project.emit_notification("link.created", self.asdict())
 
         if dump:
             self._project.dump()
 
+    def release_ports(self):
+        for n in self._nodes:
+            if n["port"].link == self:
+                n["port"].link = None
+            n["node"].links.discard(self)
+
     async def update_nodes(self, nodes):
         for node_data in nodes:
             node = self._project.get_node(node_data["node_id"])
@@ -365,11 +377,7 @@ class Link:
         """
         Delete the link
         """
-        for n in self._nodes:
+        self.release_ports()
-            # It could be different of self if we rollback an already existing link
-            if n["port"].link == self:
-                n["port"].link = None
-                n["node"].remove_link(self)
 
     async def reset(self):
         """

gns3server/controller/node.py

@@ -1195,6 +1195,27 @@ class Node:
                 return port
         return None
 
+    def get_port_by_name(self, name):
+        """
+        Return the port with this name or None if the port is not found
+        """
+        for port in self.ports:
+            if port.name == name:
+                return port
+        return None
+
+    def get_free_port(self, link_type=None):
+        """
+        Return the lowest free port, optionally restricted to a link type,
+        or None if there is no such port
+        """
+        free_ports = [
+            port for port in self.ports if port.link is None and (link_type is None or port.link_type == link_type)
+        ]
+        if not free_ports:
+            return None
+        return min(free_ports, key=lambda port: (port.adapter_number, port.port_number))
+
     def _list_ports(self):
         """
         Generate the list of port display in the client

gns3server/controller/ports/port.py

@@ -41,6 +41,10 @@ class Port:
     def link(self, val):
         self._link = val
 
+    @property
+    def name(self):
+        return self._name
+
     @property
     def adapter_number(self):
         return self._adapter_number

gns3server/schemas/controller/links.py

@@ -18,7 +18,7 @@ from enum import Enum
 from typing import Annotated, List, Literal, Optional, Tuple
 from uuid import UUID, uuid4
 
-from pydantic import BaseModel, ConfigDict, Field, field_validator
+from pydantic import BaseModel, ConfigDict, Field, field_validator, model_validator
 
 from ..update import PartialUpdateModel
 from .labels import Label
@@ -35,6 +35,32 @@ class LinkNode(BaseModel):
     label: Optional[Label] = None
 
 
+class LinkNodeCreate(BaseModel):
+    """
+    Link node data for link creation.
+
+    The port is given either by adapter_number and port_number, by port_name
+    or by port set to "auto" (first free compatible port).
+    """
+
+    node_id: UUID
+    adapter_number: Optional[int] = None
+    port_number: Optional[int] = None
+    port_name: Optional[str] = Field(None, description="Name of the port on the node")
+    port: Optional[Literal["auto"]] = Field(None, description='Use "auto" to select the first free compatible port')
+    label: Optional[Label] = None
+
+    @model_validator(mode="after")
+    def check_port_selector(self):
+        numbered = self.adapter_number is not None or self.port_number is not None
+        if numbered and (self.adapter_number is None or self.port_number is None):
+            raise ValueError("adapter_number and port_number must be given together")
+        forms = sum([numbered, self.port_name is not None, self.port is not None])
+        if forms != 1:
+            raise ValueError("exactly one of adapter_number and port_number, port_name or port must be given")
+        return self
+
+
 class LinkType(str, Enum):
     """
     Link type.
@@ -142,7 +168,8 @@ class LinkBase(BaseModel):
 
 class LinkCreate(LinkBase):
     link_id: UUID = Field(default_factory=uuid4)
-    nodes: List[LinkNode] = Field(..., min_length=2, max_length=2)
+    # LinkNodeCreate narrows the port selector, so it cannot be a LinkNode subclass
+    nodes: List[LinkNodeCreate] = Field(..., min_length=2, max_length=2)  # type: ignore[assignment]
 
 
 class LinkUpdate(PartialUpdateModel, LinkBase):

tests/api/routes/controller/test_links.py

@@ -115,6 +115,110 @@ class TestLinkRoutes:
         assert response.status_code == status.HTTP_409_CONFLICT
         assert len(project.links) == 0
 
+    async def test_create_link_by_port_name(
+        self, app: FastAPI, client: AsyncClient, project: Project, nodes: Tuple[Node, Node]
+    ) -> None:
+
+        node1, node2 = nodes
+
+        with asyncio_patch("gns3server.controller.udp_link.UDPLink.create"):
+            response = await client.post(
+                app.url_path_for("create_link", project_id=project.id),
+                json={
+                    "nodes": [
+                        {"node_id": node1.id, "port_name": "E0"},
+                        {"node_id": node2.id, "adapter_number": 2, "port_number": 4},
+                    ]
+                },
+            )
+
+        assert response.status_code == status.HTTP_201_CREATED
+        first, second = response.json()["nodes"]
+        assert (first["adapter_number"], first["port_number"]) == (0, 3)
+        assert (second["adapter_number"], second["port_number"]) == (2, 4)
+
+    async def test_create_link_unknown_port_name(
+        self, app: FastAPI, client: AsyncClient, project: Project, nodes: Tuple[Node, Node]
+    ) -> None:
+
+        node1, node2 = nodes
+
+        response = await client.post(
+            app.url_path_for("create_link", project_id=project.id),
+            json={
+                "nodes": [
+                    {"node_id": node1.id, "port_name": "nope"},
+                    {"node_id": node2.id, "adapter_number": 2, "port_number": 4},
+                ]
+            },
+        )
+
+        assert response.status_code == status.HTTP_404_NOT_FOUND
+        assert len(project.links) == 0
+
+    async def test_create_link_auto_port(
+        self, app: FastAPI, client: AsyncClient, project: Project, nodes: Tuple[Node, Node]
+    ) -> None:
+
+        node1, node2 = nodes
+        node1._ports = [EthernetPort("E1", 0, 0, 5), EthernetPort("E0", 0, 0, 3)]
+
+        with asyncio_patch("gns3server.controller.udp_link.UDPLink.create"):
+            response = await client.post(
+                app.url_path_for("create_link", project_id=project.id),
+                json={"nodes": [{"node_id": node1.id, "port": "auto"}, {"node_id": node2.id, "port": "auto"}]},
+            )
+            assert response.status_code == status.HTTP_201_CREATED
+            assert response.json()["nodes"][0]["port_number"] == 3
+
+            response = await client.post(
+                app.url_path_for("create_link", project_id=project.id),
+                json={"nodes": [{"node_id": node1.id, "port": "auto"}, {"node_id": node2.id, "port": "auto"}]},
+            )
+
+        assert response.status_code == status.HTTP_409_CONFLICT
+        assert response.json()["code"] == "no_free_port"
+        assert len(project.links) == 1
+
+    async def test_create_link_port_in_use_code(
+        self, app: FastAPI, client: AsyncClient, project: Project, nodes: Tuple[Node, Node]
+    ) -> None:
+
+        node1, node2 = nodes
+        body = {
+            "nodes": [
+                {"node_id": node1.id, "adapter_number": 0, "port_number": 3},
+                {"node_id": node2.id, "adapter_number": 2, "port_number": 4},
+            ]
+        }
+
+        with asyncio_patch("gns3server.controller.udp_link.UDPLink.create"):
+            first = await client.post(app.url_path_for("create_link", project_id=project.id), json=body)
+            second = await client.post(app.url_path_for("create_link", project_id=project.id), json=body)
+
+        assert first.status_code == status.HTTP_201_CREATED
+        assert second.status_code == status.HTTP_409_CONFLICT
+        assert second.json()["code"] == "port_in_use"
+        assert len(project.links) == 1
+
+    async def test_create_link_invalid_port_selector(
+        self, app: FastAPI, client: AsyncClient, project: Project, nodes: Tuple[Node, Node]
+    ) -> None:
+
+        node1, node2 = nodes
+        valid = {"node_id": node2.id, "adapter_number": 2, "port_number": 4}
+
+        for bad in (
+            {"node_id": node1.id},
+            {"node_id": node1.id, "adapter_number": 0},
+            {"node_id": node1.id, "port_name": "E0", "port": "auto"},
+            {"node_id": node1.id, "adapter_number": 0, "port_number": 3, "port_name": "E0"},
+        ):
+            response = await client.post(
+                app.url_path_for("create_link", project_id=project.id), json={"nodes": [bad, valid]}
+            )
+            assert response.status_code == status.HTTP_422_UNPROCESSABLE_ENTITY
+
     async def test_get_link(
         self, app: FastAPI, client: AsyncClient, project: Project, nodes: Tuple[Node, Node]
     ) -> None:

tests/controller/test_link.py

@@ -15,6 +15,7 @@
 # You should have received a copy of the GNU General Public License
 # along with this program.  If not, see <http://www.gnu.org/licenses/>.
 
+import asyncio
 from unittest.mock import MagicMock
 
 import pytest
@@ -417,3 +418,67 @@ async def test_update_link_style(project, compute):
     }
     link._project.emit_notification.assert_called_with("link.updated", link.asdict())
     assert project.dump.called
+
+
+@pytest.mark.asyncio
+async def test_add_node_concurrent_same_port(project, compute):
+    project.dump = MagicMock()
+    node1 = Node(project, compute, "node1", node_type="qemu")
+    node1._ports = [EthernetPort("E0", 0, 0, 4)]
+    node2 = Node(project, compute, "node2", node_type="qemu")
+    node2._ports = [EthernetPort("E0", 0, 0, 4), EthernetPort("E1", 0, 0, 5)]
+    node3 = Node(project, compute, "node3", node_type="qemu")
+    node3._ports = [EthernetPort("E0", 0, 0, 4)]
+
+    async def slow_create():
+        await asyncio.sleep(0)
+
+    async def connect(other, other_port):
+        link = Link(project)
+        link.create = slow_create
+        try:
+            await link.add_node(other, 0, other_port)
+            await link.add_node(node1, 0, 4)
+        except ControllerError as e:
+            return e
+        return link
+
+    results = await asyncio.gather(connect(node2, 4), connect(node3, 4))
+    errors = [r for r in results if isinstance(r, ControllerError)]
+    assert len(errors) == 1
+    assert errors[0].code == "port_in_use"
+    assert node1._ports[0].link in results
+
+
+@pytest.mark.asyncio
+async def test_add_node_create_failure_releases_ports(project, compute):
+    project.dump = MagicMock()
+    node1 = Node(project, compute, "node1", node_type="qemu")
+    node1._ports = [EthernetPort("E0", 0, 0, 4)]
+    node2 = Node(project, compute, "node2", node_type="qemu")
+    node2._ports = [EthernetPort("E0", 0, 0, 4)]
+
+    link = Link(project)
+    link.create = AsyncioMagicMock(side_effect=ControllerError("boom"))
+    await link.add_node(node1, 0, 4)
+    with pytest.raises(ControllerError):
+        await link.add_node(node2, 0, 4)
+
+    assert node1._ports[0].link is None
+    assert node2._ports[0].link is None
+    assert not node1.links
+    assert not node2.links
+
+
+def test_node_get_port_by_name_and_free_port(project, compute):
+    node = Node(project, compute, "node1", node_type="qemu")
+    node._ports = [EthernetPort("E1", 0, 0, 1), EthernetPort("E0", 0, 0, 0), SerialPort("S0", 0, 1, 0)]
+
+    assert node.get_port_by_name("E0") is node._ports[1]
+    assert node.get_port_by_name("nope") is None
+    assert node.get_free_port() is node._ports[1]
+    assert node.get_free_port("serial") is node._ports[2]
+    node._ports[1].link = object()
+    assert node.get_free_port("ethernet") is node._ports[0]
+    node._ports[0].link = object()
+    assert node.get_free_port("ethernet") is None