Merge pull request #2983 from yueguobin/fix/docker-image-digest-translation

fix(docker): accept the pinned image id when the two daemons report different ids for the same content

gns3server/api/routes/compute/images.py

@@ -20,9 +20,9 @@ API routes for images.
 
 import os
 import urllib.parse
-from typing import List
+from typing import List, Optional
 
-from fastapi import APIRouter, Body, HTTPException, Request, status
+from fastapi import APIRouter, Body, HTTPException, Query, Request, status
 from fastapi.responses import FileResponse
 
 from gns3server.compute.docker import Docker
@@ -54,13 +54,23 @@ async def pull_docker_image(image: str = Body(..., embed=True, min_length=1, pat
 
 
 @router.post("/docker/images/load", status_code=status.HTTP_204_NO_CONTENT)
-async def load_docker_image(request: Request) -> None:
+async def load_docker_image(
+    request: Request,
+    image: Optional[str] = Query(None, pattern=r"^\S+$", description="Tag of the image in the archive"),
+    digest: Optional[str] = Query(
+        None, pattern=r"^\S+$", description="Image id reported by the controller host daemon"
+    ),
+) -> None:
     """
     Load a Docker image into the Docker daemon from a docker save tar stream.
+
+    The optional image tag and digest let the compute remember which image the
+    controller pushed and how the local daemon identifies it (see
+    Docker.load_image).
     """
 
     docker_manager = Docker.instance()
-    await docker_manager.load_image(request.stream())
+    await docker_manager.load_image(request.stream(), image=image, digest=digest)
 
 
 @router.get("/dynamips/images")

gns3server/compute/docker/__init__.py

@@ -66,6 +66,11 @@ class Docker(BaseManager):
         self._session = None
         self._api_version = DOCKER_MINIMUM_API_VERSION
         self._host_checked = False
+        # Image name -> (controller digest, local id) for the images synced from
+        # the controller. The controller pins the id its own daemon reports,
+        # which is not comparable with the id this daemon reports for the same
+        # content (see load_image()).
+        self._image_id_translations = {}
 
     def _select_node_class(self, **kwargs):
         """
@@ -419,11 +424,16 @@ class Docker(BaseManager):
             progress_callback(f"Success pulling image {image}")
 
     @locking
-    async def load_image(self, stream, progress_callback=None):
+    async def load_image(self, stream, image=None, digest=None, progress_callback=None):
         """
         Load a Docker image into the Docker daemon from a docker save tar stream
 
         :param stream: An async iterable of bytes (the tar produced by docker save)
+        :param image: Tag of the image in the archive. Together with digest, the
+            (controller digest, local id) pair is remembered so that node creation
+            can accept the digest the controller pinned for this image
+        :param digest: Image id as reported by the Docker daemon on the controller
+            host (the value the controller pins in the node create payload)
         :param progress_callback: A function that receive a log message about image load progress
         """
 
@@ -470,6 +480,39 @@ class Docker(BaseManager):
         if progress_callback:
             progress_callback("Docker image loaded")
 
+        if image and digest:
+            await self._remember_image_id_translation(image, digest)
+
+    async def _remember_image_id_translation(self, image, controller_digest):
+        """
+        Remember how the local daemon identifies an image that was just loaded
+        from the controller.
+
+        Docker ids are not comparable across image stores: for the same content
+        a classic store reports the config digest, while a containerd store (or
+        an image loaded from an OCI archive) reports the manifest digest. The
+        controller pins the id of its own daemon, so recording the pair lets
+        DockerVM accept that pin for the image it actually received instead of
+        asking for another full sync on every node creation.
+        """
+
+        try:
+            image_infos = await self.query("GET", f"images/{image}/json")
+        except DockerError as e:
+            log.warning(f"Image '{image}' has been loaded but cannot be inspected: {e}")
+            return
+        local_id = image_infos.get("Id")
+        if local_id:
+            self._image_id_translations[image] = (controller_digest, local_id)
+
+    def image_id_translation(self, image):
+        """
+        :returns: the (controller digest, local id) pair recorded the last time
+            this image was loaded from the controller, None otherwise
+        """
+
+        return self._image_id_translations.get(image)
+
     async def list_images(self):
         """
         Gets Docker image list.

gns3server/compute/docker/docker_vm.py

@@ -591,15 +591,20 @@ class DockerVM(DockerKernelDatapathMixin, BaseNode):
         if image_infos is None:
             raise DockerError(f"Cannot get information for image '{self._image}', please try again.")
 
-        if self._image_digest and image_infos.get("Id") != self._image_digest:
+        local_id = image_infos.get("Id")
+        if self._image_digest and local_id != self._image_digest:
             # the tag exists but points to a different image (e.g. a moved :latest):
-            # report it as missing so the controller re-syncs the expected version
-            local_id = image_infos.get("Id")
-            log.info(
-                f"Image '{self._image}' version mismatch on this compute: "
-                f"local id '{local_id}' != expected '{self._image_digest}'"
-            )
-            raise ImageMissingError(self._image)
+            # report it as missing so the controller re-syncs the expected version.
+            # Image ids are not comparable across image stores (a classic store
+            # reports the config digest where a containerd store, or an image
+            # loaded from an OCI archive, reports the manifest digest), so accept
+            # the pin for the image this compute received from the controller.
+            if self.manager.image_id_translation(self._image) != (self._image_digest, local_id):
+                log.info(
+                    f"Image '{self._image}' version mismatch on this compute: "
+                    f"local id '{local_id}' != expected '{self._image_digest}'"
+                )
+                raise ImageMissingError(self._image)
 
         available_cpus = psutil.cpu_count(logical=True)
         if self._cpus > available_cpus:

gns3server/controller/node.py

@@ -527,7 +527,9 @@ class Node:
                     res = False
                     if last_missing_image:
                         try:
-                            res = await self._upload_missing_image(self._node_type, last_missing_image)
+                            res = await self._upload_missing_image(
+                                self._node_type, last_missing_image, data.get("image_digest")
+                            )
                         except ControllerError as upload_error:
                             # For Docker the image is pulled from a registry: a
                             # failed pull (unknown tag, no network, private image)
@@ -1075,14 +1077,18 @@ class Node:
                 f"/projects/{self._project.id}/{self._node_type}/nodes/{self._id}{path}", **kwargs
             )
 
-    async def _upload_missing_image(self, type, img):
+    async def _upload_missing_image(self, type, img, image_digest=None):
         """
         Search an image on local computer and upload it to remote compute
         if the image exists
+
+        :param image_digest: digest pinned for this image in the create payload,
+            forwarded so the compute can translate it (see
+            _sync_missing_docker_image)
         """
 
         if self._node_type == "docker":
-            return await self._sync_missing_docker_image(img)
+            return await self._sync_missing_docker_image(img, image_digest)
 
         image = self._find_local_image(type, img)
         if image is None:
@@ -1119,11 +1125,17 @@ class Node:
         if image_id:
             data["image_digest"] = image_id
 
-    async def _sync_missing_docker_image(self, image):
+    async def _sync_missing_docker_image(self, image, image_digest=None):
         """
         Export a Docker image from the Docker daemon on the controller host and
         stream it to the remote compute, or ask the compute to pull it when the
         image is not available locally
+
+        :param image_digest: digest pinned for this image in the create payload.
+            It is sent along with the archive so the compute can remember how its
+            own daemon identifies the image it received: image ids are not
+            comparable across image stores, so the pin alone would keep being
+            reported as a mismatch after every sync.
         """
 
         from gns3server.compute.docker import Docker
@@ -1147,8 +1159,11 @@ class Node:
         self.project.emit_notification(
             "log.info", {"message": f"Syncing Docker image '{image}' to compute '{self._compute.name}'"}
         )
+        params = {"image": image}
+        if image_digest:
+            params["digest"] = image_digest
         try:
-            await self._compute.post("/docker/images/load", data=response.content, timeout=None)
+            await self._compute.post("/docker/images/load", data=response.content, timeout=None, params=params)
         finally:
             response.close()
         self.project.emit_notification(

tests/api/routes/compute/test_images.py

@@ -54,3 +54,16 @@ class TestImagesRoutes:
             stream = mock.call_args[0][0]
             assert hasattr(stream, "__aiter__")
             assert response.status_code == status.HTTP_204_NO_CONTENT
+
+    async def test_load_docker_image_with_image_and_digest(self, app: FastAPI, compute_client: AsyncClient) -> None:
+
+        digest = "sha256:" + "a" * 64
+        with asyncio_patch("gns3server.compute.docker.Docker.load_image") as mock:
+            response = await compute_client.post(
+                app.url_path_for("compute:load_docker_image"),
+                params={"image": "gns3/frr:latest", "digest": digest},
+                content=b"docker-save-tar-bytes",
+            )
+            assert response.status_code == status.HTTP_204_NO_CONTENT
+            # the tag and the digest pinned by the controller reach the manager
+            assert mock.call_args[1] == {"image": "gns3/frr:latest", "digest": digest}

tests/compute/docker/test_docker.py

@@ -276,6 +276,38 @@ async def test_load_image():
     assert any("Loaded image" in message for message in messages)
 
 
+@pytest.mark.asyncio
+async def test_load_image_records_image_id_translation():
+    """
+    The digest pinned by the controller is the id of its own daemon and is not
+    comparable with the id this daemon reports for the same content, so the pair
+    is remembered to accept the pin during node creation.
+    """
+
+    class Content:
+        def __init__(self):
+            self._chunks = [b'{"stream": "Loaded image: ubuntu:latest"}', b""]
+
+        async def read(self, size):
+            return self._chunks.pop(0)
+
+    response = MagicMock()
+    response.content = Content()
+
+    async def tar_stream():
+        yield b"tar-bytes"
+
+    controller_digest = "sha256:" + "a" * 64
+    local_id = "sha256:" + "b" * 64
+    with asyncio_patch("gns3server.compute.docker.Docker.http_query", return_value=response):
+        with asyncio_patch("gns3server.compute.docker.Docker.query", return_value={"Id": local_id}) as query_mock:
+            await Docker.instance().load_image(tar_stream(), image="ubuntu:latest", digest=controller_digest)
+
+    query_mock.assert_called_once_with("GET", "images/ubuntu:latest/json")
+    assert Docker.instance().image_id_translation("ubuntu:latest") == (controller_digest, local_id)
+    assert Docker.instance().image_id_translation("nginx:latest") is None
+
+
 @pytest.mark.asyncio
 async def test_load_image_error():
 

tests/compute/docker/test_docker_vm.py

@@ -538,6 +538,49 @@ async def test_create_image_digest_mismatch(compute_project, manager):
         query_mock.assert_called_once_with("GET", "images/ubuntu:latest/json")
 
 
+@pytest.mark.asyncio
+async def test_create_image_digest_translated_for_this_daemon(compute_project, manager):
+    """
+    Image ids are not comparable across image stores: the daemon on the
+    controller host may pin the config digest while this daemon reports the
+    manifest digest for the same content. An image this compute received from
+    the controller is accepted through the translation recorded at load time.
+    """
+
+    controller_digest = "sha256:" + "a" * 64
+    local_id = "sha256:" + "b" * 64
+    manager._image_id_translations["ubuntu:latest"] = (controller_digest, local_id)
+
+    response = {"Id": local_id, "Warnings": []}
+    with asyncio_patch("gns3server.compute.docker.Docker.query", return_value=response) as mock:
+        vm = DockerVM(
+            "test", str(uuid.uuid4()), compute_project, manager, "ubuntu:latest", image_digest=controller_digest
+        )
+        await vm.create()
+        # the last query is the container creation: the translated pin let it through
+        assert mock.call_args[0] == ("POST", f"containers/create?name={vm.docker_name}")
+        assert vm._cid == local_id
+
+
+@pytest.mark.asyncio
+async def test_create_image_digest_translated_only_for_the_synced_content(compute_project, manager):
+    """
+    A tag that now points to some other image on this daemon stays missing, even
+    when this image name was synced from the controller before.
+    """
+
+    controller_digest = "sha256:" + "a" * 64
+    manager._image_id_translations["ubuntu:latest"] = (controller_digest, "sha256:" + "c" * 64)
+
+    response = {"Id": "sha256:" + "b" * 64, "Warnings": []}
+    vm = DockerVM("test", str(uuid.uuid4()), compute_project, manager, "ubuntu:latest", image_digest=controller_digest)
+    with asyncio_patch("gns3server.compute.docker.Docker.query", return_value=response) as query_mock:
+        with pytest.raises(ImageMissingError, match="ubuntu:latest"):
+            await vm.create()
+        # only the image inspect happened: no container was created from the stale image
+        query_mock.assert_called_once_with("GET", "images/ubuntu:latest/json")
+
+
 @pytest.mark.asyncio
 async def test_create_with_user(compute_project, manager):
 

tests/controller/test_node.py

@@ -1110,10 +1110,89 @@ async def test_sync_missing_docker_image_from_controller_daemon(compute, control
     with asyncio_patch("gns3server.compute.docker.Docker.http_query", return_value=response) as save_mock:
         assert await node._upload_missing_image("docker", "gns3/frr:latest") is True
         save_mock.assert_called_with("GET", "images/gns3/frr:latest/get", timeout=None)
-    compute.post.assert_called_with("/docker/images/load", data=response.content, timeout=None)
+    compute.post.assert_called_with(
+        "/docker/images/load", data=response.content, timeout=None, params={"image": "gns3/frr:latest"}
+    )
     response.close.assert_called_once_with()
 
 
+@pytest.mark.asyncio
+async def test_sync_missing_docker_image_forwards_pinned_digest(compute, controller):
+
+    project = Project(str(uuid.uuid4()), controller=controller)
+    node = Node(
+        project, compute, "demo", node_id=str(uuid.uuid4()), node_type="docker", properties={"image": "gns3/frr:latest"}
+    )
+
+    digest = "sha256:" + "a" * 64
+    response = MagicMock()
+    response.content = MagicMock()
+    with asyncio_patch("gns3server.compute.docker.Docker.http_query", return_value=response):
+        assert await node._upload_missing_image("docker", "gns3/frr:latest", digest) is True
+    compute.post.assert_called_with(
+        "/docker/images/load",
+        data=response.content,
+        timeout=None,
+        params={"image": "gns3/frr:latest", "digest": digest},
+    )
+
+
+@pytest.mark.asyncio
+async def test_create_docker_node_syncs_missing_image_with_the_pinned_digest(compute, controller):
+    """
+    When the compute reports a Docker image as missing, the create retries after
+    the sync and the load call carries the pinned digest, so the compute can
+    translate it into the id of its own daemon (image ids are not comparable
+    across image stores).
+    """
+
+    project = Project(str(uuid.uuid4()), controller=controller)
+    node = Node(
+        project,
+        compute,
+        "demo",
+        node_id=str(uuid.uuid4()),
+        node_type="docker",
+        properties={"image": "gns3/frr:latest", "adapters": 1},
+    )
+
+    controller_image_id = "sha256:" + "a" * 64
+    conflict = ComputeConflictError(
+        f"/projects/{project.id}/docker/nodes",
+        {
+            "message": "The image 'gns3/frr:latest' is missing",
+            "image": "gns3/frr:latest",
+            "exception": "ImageMissingError",
+        },
+    )
+    created = MagicMock()
+    created.status = 200
+    created.json = {}
+
+    # the create call, the image sync, then the create retry
+    responses = [conflict, MagicMock(), created]
+
+    async def resp(*args, **kwargs):
+        result = responses.pop(0)
+        if isinstance(result, Exception):
+            raise result
+        return result
+
+    compute.post = AsyncioMagicMock(side_effect=resp)
+
+    load_response = MagicMock()
+    load_response.content = MagicMock()
+    with asyncio_patch("gns3server.compute.docker.Docker.query", return_value={"Id": controller_image_id}):
+        with asyncio_patch("gns3server.compute.docker.Docker.http_query", return_value=load_response):
+            assert await node.create() is True
+
+    # first call is the node creation, then the image sync, then the retry
+    sync_call = compute.post.call_args_list[1]
+    assert sync_call[0][0] == "/docker/images/load"
+    assert sync_call[1]["params"] == {"image": "gns3/frr:latest", "digest": controller_image_id}
+    assert compute.post.call_args_list[2][1]["data"]["image_digest"] == controller_image_id
+
+
 @pytest.mark.asyncio
 async def test_sync_missing_docker_image_pull_fallback(compute, controller):