mirror of
https://github.com/GNS3/gns3-server.git
synced 2026-10-28 11:12:26 +02:00
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.
This commit is contained in:
parent
888afdccbd
commit
8a8314ab29
@ -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).
|
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.
|
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, {})
|
return await asyncio.to_thread(_run_handler_sync, install_images_handler, {})
|
||||||
|
|
||||||
|
|||||||
@ -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]:
|
def install_images_handler(params: dict[str, Any], gns3_ctx: dict[str, Any]) -> dict[str, Any]:
|
||||||
conn = _get_connector(gns3_ctx)
|
conn = _get_connector(gns3_ctx)
|
||||||
# Returns 204 No Content on success (empty body, no .json())
|
response = conn.http_call("post", f"{conn.base_url}/images/install")
|
||||||
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"}
|
return {"message": "Image installation completed"}
|
||||||
|
|||||||
@ -206,19 +206,24 @@ async def prune_images(
|
|||||||
|
|
||||||
@router.post(
|
@router.post(
|
||||||
"/install",
|
"/install",
|
||||||
status_code=status.HTTP_204_NO_CONTENT,
|
status_code=status.HTTP_200_OK,
|
||||||
dependencies=[Depends(has_privilege("Image.Allocate"))]
|
dependencies=[Depends(has_privilege("Image.Allocate"))]
|
||||||
)
|
)
|
||||||
async def install_images(
|
async def install_images(
|
||||||
images_repo: ImagesRepository = Depends(get_repository(ImagesRepository)),
|
images_repo: ImagesRepository = Depends(get_repository(ImagesRepository)),
|
||||||
templates_repo: TemplatesRepository = Depends(get_repository(TemplatesRepository))
|
templates_repo: TemplatesRepository = Depends(get_repository(TemplatesRepository))
|
||||||
) -> None:
|
) -> dict:
|
||||||
"""
|
"""
|
||||||
Attempt to automatically create templates based on image checksums.
|
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
|
Required privilege: Image.Allocate
|
||||||
"""
|
"""
|
||||||
|
|
||||||
|
created = []
|
||||||
|
skipped = []
|
||||||
skip_images = get_builtin_disks()
|
skip_images = get_builtin_disks()
|
||||||
images = await images_repo.get_images()
|
images = await images_repo.get_images()
|
||||||
for image in images:
|
for image in images:
|
||||||
@ -229,8 +234,12 @@ async def install_images(
|
|||||||
if templates:
|
if templates:
|
||||||
# the image is already used by a template
|
# the image is already used by a template
|
||||||
log.warning(f"Image '{image.path}' is used by one or more templates")
|
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
|
continue
|
||||||
await Controller.instance().appliance_manager.install_appliances_from_image(
|
results = await Controller.instance().appliance_manager.install_appliances_from_image(
|
||||||
image.path,
|
image.path,
|
||||||
image.checksum,
|
image.checksum,
|
||||||
images_repo,
|
images_repo,
|
||||||
@ -239,6 +248,12 @@ async def install_images(
|
|||||||
None,
|
None,
|
||||||
os.path.dirname(image.path)
|
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(
|
@router.get(
|
||||||
|
|||||||
@ -245,11 +245,16 @@ class ApplianceManager:
|
|||||||
rbac_repo: RbacRepository,
|
rbac_repo: RbacRepository,
|
||||||
current_user: schemas.User,
|
current_user: schemas.User,
|
||||||
image_dir: str
|
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)
|
appliances_info = self._find_appliances_from_image_checksum(image_checksum)
|
||||||
for appliance, image_version in appliances_info:
|
for appliance, image_version in appliances_info:
|
||||||
try:
|
try:
|
||||||
@ -257,15 +262,48 @@ class ApplianceManager:
|
|||||||
ApplianceModel.model_validate(appliance.asdict())
|
ApplianceModel.model_validate(appliance.asdict())
|
||||||
except ValidationError as e:
|
except ValidationError as e:
|
||||||
log.warning(f"Could not validate appliance '{appliance.id}': {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:
|
if appliance.versions:
|
||||||
for version in appliance.versions:
|
for version in appliance.versions:
|
||||||
if version.get("name") == image_version:
|
if version.get("name") == image_version:
|
||||||
try:
|
try:
|
||||||
await self._find_appliance_version_images(appliance, version, images_repo, image_dir)
|
await self._find_appliance_version_images(appliance, version, images_repo, image_dir)
|
||||||
template_data = await self._appliance_to_template(appliance, version)
|
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:
|
except (ControllerError, InvalidImageError) as e:
|
||||||
log.warning(f"Could not automatically create template using image '{image_path}': {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(
|
async def install_appliance(
|
||||||
self,
|
self,
|
||||||
|
|||||||
@ -71,6 +71,17 @@ class TemplatesRepository(BaseRepository):
|
|||||||
result = await self._db_session.execute(query)
|
result = await self._db_session.execute(query)
|
||||||
return result.scalars().first()
|
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]:
|
async def get_templates(self) -> List[models.Template]:
|
||||||
|
|
||||||
query = select(models.Template).options(selectinload(models.Template.images))
|
query = select(models.Template).options(selectinload(models.Template.images))
|
||||||
|
|||||||
@ -523,6 +523,36 @@ class TestTemplate:
|
|||||||
assert "deleted" in str(result).lower()
|
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) ────────────────────────────────────────────
|
# ── Marker (traffic-insight) ────────────────────────────────────────────
|
||||||
|
|
||||||
|
|
||||||
|
|||||||
@ -328,10 +328,54 @@ class TestImageRoutes:
|
|||||||
with asyncio_patch("gns3server.api.routes.controller.images.get_builtin_disks", return_value=[]) as mock:
|
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"))
|
response = await client.post(app.url_path_for("install_images"))
|
||||||
assert mock.called
|
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_repo = TemplatesRepository(db_session)
|
||||||
templates = await templates_repo.get_templates()
|
templates = await templates_repo.get_templates()
|
||||||
assert len(templates) == 1
|
assert len(templates) == 1
|
||||||
assert templates[0].name == "Empty VM"
|
assert templates[0].name == "Empty VM"
|
||||||
assert templates[0].version == "100G"
|
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)
|
||||||
Loading…
x
Reference in New Issue
Block a user