-
Notifications
You must be signed in to change notification settings - Fork 14
Add SnapshotableContainer protocol and Docker snapshot implementation #94
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -1,12 +1,15 @@ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| """An interface for interacting with local Docker containers.""" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import asyncio | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import contextlib | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import dataclasses | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import functools | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import io | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import logging | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import pathlib | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import tarfile | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| from typing import cast | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import uuid | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import docker | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import docker.errors | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -15,6 +18,8 @@ | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| from ares.containers import containers | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| _LOGGER = logging.getLogger(__name__) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| def _make_docker_client() -> docker.DockerClient: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| try: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -24,7 +29,7 @@ def _make_docker_client() -> docker.DockerClient: | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| @dataclasses.dataclass(kw_only=True) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| class DockerContainer(containers.Container): | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| class DockerContainer(containers.SnapshotableContainer): | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| image: str | None = None | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| dockerfile_path: pathlib.Path | str | None = None | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| name: str | None = None | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -183,6 +188,46 @@ async def download_files(self, remote_paths: list[str], local_paths: list[pathli | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| with open(local_path, "wb") as f: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| f.write(file_data.read()) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| async def snapshot(self) -> str: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| """Commit current container state as a Docker image.""" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if self._container is None: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| raise RuntimeError("Container not started, snapshot is not possible.") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| tag = f"ares-snapshot-{uuid.uuid4().hex[:12]}" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| image = await asyncio.to_thread( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| self._container.commit, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| repository="ares-go-explore", | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| tag=tag, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| conf={"Labels": {"ares-go-explore": "true"}}, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| _LOGGER.info("Snapshot created: %s (tag: %s)", image.id, tag) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return image.id | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| async def delete_snapshot(self, snapshot_id: str) -> None: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| """Delete a Docker image created by snapshot().""" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| try: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| await asyncio.to_thread(self._client.images.remove, snapshot_id) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| _LOGGER.info("Snapshot deleted: %s", snapshot_id) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| except docker.errors.ImageNotFound: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| _LOGGER.debug("Snapshot %s already deleted.", snapshot_id) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| def delete_snapshot_sync(self, snapshot_id: str) -> None: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| """Synchronous version for atexit cleanup.""" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| with contextlib.suppress(docker.errors.ImageNotFound): | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| self._client.images.remove(snapshot_id) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+196
to
+217
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 Analysis chain🏁 Script executed: cat -n src/ares/containers/docker.py | sed -n '190,225p'Repository: withmartian/ares Length of output: 1691 🏁 Script executed: # Get more context around the snapshot-related methods
cat -n src/ares/containers/docker.py | sed -n '170,230p'Repository: withmartian/ares Length of output: 2987 🏁 Script executed: # Search for the snapshot method implementation
rg -n "def snapshot" src/ares/containers/docker.py -A 15Repository: withmartian/ares Length of output: 765 🏁 Script executed: # Check if label validation exists anywhere
rg -n "ares-go-explore" src/ares/containers/docker.pyRepository: withmartian/ares Length of output: 167 🏁 Script executed: # Check the full class structure for context
rg -n "class DockerContainer" src/ares/containers/docker.py -A 50Repository: withmartian/ares Length of output: 2559 Use the snapshot label as a delete guard.
🛡️ Suggested guard+ def _assert_snapshot_image(self, snapshot_id: str) -> None:
+ image = self._client.images.get(snapshot_id)
+ labels = image.attrs.get("Config", {}).get("Labels") or {}
+ if labels.get("ares-go-explore") != "true":
+ raise ValueError(f"{snapshot_id} is not an ARES snapshot")
+
async def delete_snapshot(self, snapshot_id: str) -> None:
"""Delete a Docker image created by snapshot()."""
try:
+ await asyncio.to_thread(self._assert_snapshot_image, snapshot_id)
await asyncio.to_thread(self._client.images.remove, snapshot_id)
_LOGGER.info("Snapshot deleted: %s", snapshot_id)
except docker.errors.ImageNotFound:
_LOGGER.debug("Snapshot %s already deleted.", snapshot_id)
def delete_snapshot_sync(self, snapshot_id: str) -> None:
"""Synchronous version for atexit cleanup."""
with contextlib.suppress(docker.errors.ImageNotFound):
+ self._assert_snapshot_image(snapshot_id)
self._client.images.remove(snapshot_id)📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| @classmethod | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| def from_snapshot( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| cls, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| snapshot_id: str, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| *, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| name: str | None = None, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| resources: containers.Resources | None = None, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| default_workdir: str | None = None, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) -> "DockerContainer": | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| """Create a DockerContainer from a previously captured snapshot.""" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return DockerContainer(image=snapshot_id, name=name, resources=resources, default_workdir=default_workdir) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| @classmethod | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| def from_image( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| cls, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,86 @@ | ||
| """Tests for Docker container snapshotting.""" | ||
|
|
||
| import unittest.mock | ||
|
|
||
| import pytest | ||
|
|
||
| from ares.containers import docker | ||
|
|
||
|
|
||
| @pytest.fixture | ||
| def mock_docker_client(): | ||
| """Create a mock Docker client.""" | ||
| with unittest.mock.patch.object(docker, "_make_docker_client") as mock_fn: | ||
| client = unittest.mock.MagicMock() | ||
| mock_fn.return_value = client | ||
| yield client | ||
|
|
||
|
|
||
| @pytest.mark.asyncio | ||
| async def test_snapshot_creates_image(mock_docker_client): # noqa: ARG001 | ||
| """Test that snapshot() commits the container and returns an image ID.""" | ||
| container = docker.DockerContainer(image="test:latest") | ||
|
|
||
| # Set up mock container | ||
| mock_inner = unittest.mock.MagicMock() | ||
| container._container = mock_inner | ||
|
|
||
| mock_image = unittest.mock.MagicMock() | ||
| mock_image.id = "sha256:abc123" | ||
| mock_inner.commit.return_value = mock_image | ||
|
|
||
| snapshot_id = await container.snapshot() | ||
|
|
||
| assert snapshot_id == "sha256:abc123" | ||
| mock_inner.commit.assert_called_once() | ||
| call_kwargs = mock_inner.commit.call_args | ||
| assert call_kwargs[1]["repository"] == "ares-go-explore" | ||
| assert call_kwargs[1]["conf"]["Labels"]["ares-go-explore"] == "true" | ||
|
|
||
|
|
||
| @pytest.mark.asyncio | ||
| async def test_snapshot_raises_if_not_started(): | ||
| """Test that snapshot() raises if container isn't started.""" | ||
| container = docker.DockerContainer(image="test:latest") | ||
| with pytest.raises(RuntimeError, match="not started"): | ||
| await container.snapshot() | ||
|
|
||
|
|
||
| def test_from_snapshot_creates_container(): | ||
| """Test that from_snapshot() creates a DockerContainer with the snapshot as image.""" | ||
| container = docker.DockerContainer.from_snapshot( | ||
| "sha256:abc123", | ||
| name="restored", | ||
| default_workdir="/workspace", | ||
| ) | ||
|
|
||
| assert isinstance(container, docker.DockerContainer) | ||
| assert container.image == "sha256:abc123" | ||
| assert container.name == "restored" | ||
| assert container.default_workdir == "/workspace" | ||
|
|
||
|
|
||
| @pytest.mark.asyncio | ||
| async def test_delete_snapshot(mock_docker_client): | ||
| """Test that delete_snapshot() removes the Docker image.""" | ||
| container = docker.DockerContainer(image="test:latest") | ||
| await container.delete_snapshot("sha256:abc123") | ||
| mock_docker_client.images.remove.assert_called_once_with("sha256:abc123") | ||
|
|
||
|
|
||
| @pytest.mark.asyncio | ||
| async def test_delete_snapshot_ignores_not_found(mock_docker_client): | ||
| """Test that delete_snapshot() handles already-deleted images.""" | ||
| import docker as docker_lib | ||
|
|
||
| mock_docker_client.images.remove.side_effect = docker_lib.errors.ImageNotFound("not found") | ||
| container = docker.DockerContainer(image="test:latest") | ||
| # Should not raise | ||
| await container.delete_snapshot("sha256:abc123") | ||
|
|
||
|
|
||
| def test_delete_snapshot_sync(mock_docker_client): | ||
| """Test synchronous snapshot deletion for atexit cleanup.""" | ||
| container = docker.DockerContainer(image="test:latest") | ||
| container.delete_snapshot_sync("sha256:abc123") | ||
| mock_docker_client.images.remove.assert_called_once_with("sha256:abc123") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
snapshot()raises rawRuntimeErrorwhen_containerisNone, violating the repo's error-handling guideline (CLAUDE.md) and preventing callers from distinguishing container lifecycle failures. Can we introduce or reuse a container-specific exception (e.g.ContainerNotStartedError) that derives from the repo exception hierarchy?Finding type:
AI Coding Guidelines| Severity: 🟢 LowWant Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents: