From 8a8314ab297a46f67fd47f71d5cea3f6b87b272c Mon Sep 17 00:00:00 2001 From: YueGuobin Date: Wed, 26 Aug 2026 00:04:57 +0800 Subject: [PATCH] fix: dedupe and report automatic template creation from images install_appliances_from_image relied on the name+version pair check in TemplatesService, so the same appliance reached through a second image (the CSR1000v case) created a second template sharing the name. The auto path now skips when any template with the same name exists, whatever the version, and returns a manifest of created and skipped candidates; POST /images/install replies 200 with that manifest instead of an empty 204, and the image_install MCP tool surfaces it. --- gns3server/agent/mcp/__init__.py | 4 +- gns3server/agent/mcp/images.py | 7 +++- gns3server/api/routes/controller/images.py | 21 ++++++++-- gns3server/controller/appliance_manager.py | 44 ++++++++++++++++++-- gns3server/db/repositories/templates.py | 11 +++++ tests/agent/mcp/test_handlers.py | 30 ++++++++++++++ tests/api/routes/controller/test_images.py | 48 +++++++++++++++++++++- 7 files changed, 154 insertions(+), 11 deletions(-) diff --git a/gns3server/agent/mcp/__init__.py b/gns3server/agent/mcp/__init__.py index dac6031a4..2d9e39a89 100644 --- a/gns3server/agent/mcp/__init__.py +++ b/gns3server/agent/mcp/__init__.py @@ -1435,7 +1435,9 @@ async def image_install() -> list[dict[str, Any]]: This is NOT for downloading images. Images must be uploaded first (via the GNS3 Web UI). If an uploaded image matches a known appliance, a template is automatically created. - Images already referenced by existing templates are skipped. + Returns {"created": [...], "skipped": [...]}: images already referenced by existing + templates are skipped, and no template is auto-created when one with the same name + already exists (regardless of version). """ return await asyncio.to_thread(_run_handler_sync, install_images_handler, {}) diff --git a/gns3server/agent/mcp/images.py b/gns3server/agent/mcp/images.py index 3b8169a71..3fcbfd17d 100644 --- a/gns3server/agent/mcp/images.py +++ b/gns3server/agent/mcp/images.py @@ -72,6 +72,9 @@ def prune_images_handler(params: dict[str, Any], gns3_ctx: dict[str, Any]) -> di def install_images_handler(params: dict[str, Any], gns3_ctx: dict[str, Any]) -> dict[str, Any]: conn = _get_connector(gns3_ctx) - # Returns 204 No Content on success (empty body, no .json()) - conn.http_call("post", f"{conn.base_url}/images/install") + response = conn.http_call("post", f"{conn.base_url}/images/install") + if response.content: + # the install endpoint reports which templates were created or skipped + return response.json() + # tolerate an empty body in case an older server still replies with 204 return {"message": "Image installation completed"} diff --git a/gns3server/api/routes/controller/images.py b/gns3server/api/routes/controller/images.py index 26b9a2ed2..70c49b098 100644 --- a/gns3server/api/routes/controller/images.py +++ b/gns3server/api/routes/controller/images.py @@ -206,19 +206,24 @@ async def prune_images( @router.post( "/install", - status_code=status.HTTP_204_NO_CONTENT, + status_code=status.HTTP_200_OK, dependencies=[Depends(has_privilege("Image.Allocate"))] ) async def install_images( images_repo: ImagesRepository = Depends(get_repository(ImagesRepository)), templates_repo: TemplatesRepository = Depends(get_repository(TemplatesRepository)) -) -> None: +) -> dict: """ Attempt to automatically create templates based on image checksums. + Returns the list of created templates and the list of skipped + candidates (with the reason why they were skipped). + Required privilege: Image.Allocate """ + created = [] + skipped = [] skip_images = get_builtin_disks() images = await images_repo.get_images() for image in images: @@ -229,8 +234,12 @@ async def install_images( if templates: # the image is already used by a template log.warning(f"Image '{image.path}' is used by one or more templates") + skipped.append({ + "name": image.filename, + "reason": "image is already used by one or more templates", + }) continue - await Controller.instance().appliance_manager.install_appliances_from_image( + results = await Controller.instance().appliance_manager.install_appliances_from_image( image.path, image.checksum, images_repo, @@ -239,6 +248,12 @@ async def install_images( None, os.path.dirname(image.path) ) + for result in results: + if result.get("status") == "created": + created.append({k: v for k, v in result.items() if k != "status"}) + else: + skipped.append({k: v for k, v in result.items() if k != "status"}) + return {"created": created, "skipped": skipped} @router.get( diff --git a/gns3server/controller/appliance_manager.py b/gns3server/controller/appliance_manager.py index 7ee36a4d4..e411b9852 100644 --- a/gns3server/controller/appliance_manager.py +++ b/gns3server/controller/appliance_manager.py @@ -245,11 +245,16 @@ class ApplianceManager: rbac_repo: RbacRepository, current_user: schemas.User, image_dir: str - ) -> None: + ) -> List[dict]: """ - Install appliances using an image checksum + Install appliances using an image checksum. + + Returns a manifest of what happened: one entry per attempted template, + either {"status": "created", ...template fields} or + {"status": "skipped", "name", "reason"}. """ + results: List[dict] = [] appliances_info = self._find_appliances_from_image_checksum(image_checksum) for appliance, image_version in appliances_info: try: @@ -257,15 +262,48 @@ class ApplianceManager: ApplianceModel.model_validate(appliance.asdict()) except ValidationError as e: log.warning(f"Could not validate appliance '{appliance.id}': {e}") + results.append({ + "status": "skipped", + "name": appliance.name, + "reason": f"could not validate appliance '{appliance.id}': {e}", + }) + continue if appliance.versions: for version in appliance.versions: if version.get("name") == image_version: try: await self._find_appliance_version_images(appliance, version, images_repo, image_dir) template_data = await self._appliance_to_template(appliance, version) - await self._create_template(template_data, templates_repo, rbac_repo, current_user) + name = template_data.get("name") + existing = await templates_repo.get_template_by_name(name) if name else None + if existing is not None: + # never automatically create a second template with the same + # name: the name+version check in TemplatesService would allow + # duplicates when the appliance version differs, but two + # templates sharing a name is never what the user asked for here + log.warning(f"Template '{name}' already exists, skipping automatic template creation") + results.append({ + "status": "skipped", + "name": name, + "reason": f"a template named '{name}' already exists", + }) + continue + template = await self._create_template(template_data, templates_repo, rbac_repo, current_user) + results.append({ + "status": "created", + "template_id": str(template.get("template_id")), + "name": template.get("name"), + "version": template.get("version"), + "template_type": template.get("template_type"), + }) except (ControllerError, InvalidImageError) as e: log.warning(f"Could not automatically create template using image '{image_path}': {e}") + results.append({ + "status": "skipped", + "name": appliance.name, + "reason": str(e), + }) + return results async def install_appliance( self, diff --git a/gns3server/db/repositories/templates.py b/gns3server/db/repositories/templates.py index 0f03c74a3..a08b58053 100644 --- a/gns3server/db/repositories/templates.py +++ b/gns3server/db/repositories/templates.py @@ -71,6 +71,17 @@ class TemplatesRepository(BaseRepository): result = await self._db_session.execute(query) return result.scalars().first() + async def get_template_by_name(self, name: str) -> Union[None, models.Template]: + """ + Return the first template with this name, regardless of version. + """ + + query = select(models.Template).\ + options(selectinload(models.Template.images)).\ + where(models.Template.name == name) + result = await self._db_session.execute(query) + return result.scalars().first() + async def get_templates(self) -> List[models.Template]: query = select(models.Template).options(selectinload(models.Template.images)) diff --git a/tests/agent/mcp/test_handlers.py b/tests/agent/mcp/test_handlers.py index 91f1b9f72..5b84dae64 100644 --- a/tests/agent/mcp/test_handlers.py +++ b/tests/agent/mcp/test_handlers.py @@ -523,6 +523,36 @@ class TestTemplate: assert "deleted" in str(result).lower() +# ── Image ─────────────────────────────────────────────────────────────── + + +class TestImage: + + mod = "images" + + def test_install_manifest(self, ctx): + from gns3server.agent.mcp.images import install_images_handler + with patch(f"{BASE}.{self.mod}._get_connector") as m: + conn = _mock_conn({ + "created": [{"template_id": "t1", "name": "Empty VM", "version": "100G", "template_type": "qemu"}], + "skipped": [{"name": "csr1000v.qcow2", "reason": "image is already used by one or more templates"}], + }) + m.return_value = conn + result = install_images_handler({}, ctx) + assert result["created"][0]["name"] == "Empty VM" + assert result["skipped"][0]["name"] == "csr1000v.qcow2" + + def test_install_empty_body(self, ctx): + from gns3server.agent.mcp.images import install_images_handler + with patch(f"{BASE}.{self.mod}._get_connector") as m: + conn = _mock_conn() + conn.http_call.return_value.content = b"" + conn.http_call.return_value.json.side_effect = json.JSONDecodeError("Expecting value", "", 0) + m.return_value = conn + result = install_images_handler({}, ctx) + assert result == {"message": "Image installation completed"} + + # ── Marker (traffic-insight) ──────────────────────────────────────────── diff --git a/tests/api/routes/controller/test_images.py b/tests/api/routes/controller/test_images.py index 8760c9075..8a7a4d68c 100644 --- a/tests/api/routes/controller/test_images.py +++ b/tests/api/routes/controller/test_images.py @@ -328,10 +328,54 @@ class TestImageRoutes: with asyncio_patch("gns3server.api.routes.controller.images.get_builtin_disks", return_value=[]) as mock: response = await client.post(app.url_path_for("install_images")) assert mock.called - assert response.status_code == status.HTTP_204_NO_CONTENT + assert response.status_code == status.HTTP_200_OK + created = response.json()["created"] + assert len(created) == 1 + assert created[0]["name"] == "Empty VM" + assert created[0]["version"] == "100G" templates_repo = TemplatesRepository(db_session) templates = await templates_repo.get_templates() assert len(templates) == 1 assert templates[0].name == "Empty VM" - assert templates[0].version == "100G" \ No newline at end of file + assert templates[0].version == "100G" + await templates_repo.delete_template(templates[0].template_id) + + async def test_install_all_skips_existing_template_name( + self, app: FastAPI, + client: AsyncClient, + db_session: AsyncSession, + controller: Controller + ) -> None: + # two images matching two versions of the same appliance must not + # produce two templates with the same name + # + # earlier tests in this class uploaded the same filenames from different + # (function-scoped) images directories; drop those stale rows so the + # install route only sees this test's uploads + images_repo = ImagesRepository(db_session) + for image_name in ("empty30G.qcow2", "empty100G.qcow2"): + await images_repo.delete_image(image_name) + for image_path in ("tests/resources/empty30G.qcow2", "tests/resources/empty100G.qcow2"): + with open(image_path, "rb") as f: + image_data = f.read() + response = await client.post( + app.url_path_for("upload_image", image_path=os.path.basename(image_path)), + content=image_data) + assert response.status_code == status.HTTP_201_CREATED + + controller.appliance_manager.load_appliances() # make sure appliances are loaded + with asyncio_patch("gns3server.api.routes.controller.images.get_builtin_disks", return_value=[]): + response = await client.post(app.url_path_for("install_images")) + assert response.status_code == status.HTTP_200_OK + + manifest = response.json() + assert len(manifest["created"]) == 1 + assert manifest["created"][0]["name"] == "Empty VM" + assert any("already exists" in skipped["reason"] for skipped in manifest["skipped"]) + + templates_repo = TemplatesRepository(db_session) + templates = await templates_repo.get_templates() + assert len(templates) == 1 + assert templates[0].name == "Empty VM" + await templates_repo.delete_template(templates[0].template_id) \ No newline at end of file