Fix Jenkins-update flow: redeploy_project pull + delete_image safety + delete_container
Bug fixes from product test: 1. redeploy_project: BUILD_FAILED now includes explicit image pull (stop → pull → start) 2. delete_image: Distinguishes running vs stopped containers, suggests system_prune for stopped refs 3. New tool delete_container: Verify stopped state before deletion, confirmation required Tests added for all three paths plus stopped-container edge cases. Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
This commit is contained in:
@@ -312,6 +312,58 @@ def register_containers(mcp: FastMCP, config: AppConfig, client: DsmClient) -> N
|
|||||||
|
|
||||||
return "\n".join(result_lines)
|
return "\n".join(result_lines)
|
||||||
|
|
||||||
|
@mcp.tool()
|
||||||
|
async def delete_container(container_name: str, confirmed: bool = False) -> str:
|
||||||
|
"""Delete a container.
|
||||||
|
|
||||||
|
Container must be stopped before deletion. Without confirmed=True,
|
||||||
|
returns a preview only.
|
||||||
|
|
||||||
|
Args:
|
||||||
|
container_name: Name of the container.
|
||||||
|
confirmed: Must be True to proceed. Set to True to confirm deletion.
|
||||||
|
"""
|
||||||
|
if not confirmed:
|
||||||
|
return (
|
||||||
|
f"Preview: would delete container '{container_name}'.\n"
|
||||||
|
f"Call this tool again with confirmed=True to proceed."
|
||||||
|
)
|
||||||
|
|
||||||
|
resolved_name = await _resolve_container_name(client, container_name)
|
||||||
|
display_name = _strip_hash_prefix(resolved_name)
|
||||||
|
|
||||||
|
try:
|
||||||
|
data = await client.request(
|
||||||
|
"SYNO.Docker.Container",
|
||||||
|
"get",
|
||||||
|
params={"name": resolved_name},
|
||||||
|
)
|
||||||
|
except Exception as e:
|
||||||
|
return f"Error inspecting container '{container_name}': {e}"
|
||||||
|
|
||||||
|
if not data:
|
||||||
|
return f"Container '{container_name}' not found."
|
||||||
|
|
||||||
|
details = data.get("details", {}) or {}
|
||||||
|
state = details.get("State", {}) or {}
|
||||||
|
running = state.get("Running", False)
|
||||||
|
|
||||||
|
if running:
|
||||||
|
return (
|
||||||
|
f"Cannot delete '{display_name}': container is still running.\n"
|
||||||
|
f"Stop the container first with stop_project or stop_container."
|
||||||
|
)
|
||||||
|
|
||||||
|
try:
|
||||||
|
await client.request(
|
||||||
|
"SYNO.Docker.Container",
|
||||||
|
"delete",
|
||||||
|
params={"name": resolved_name},
|
||||||
|
)
|
||||||
|
return f"Deleted container '{display_name}'."
|
||||||
|
except Exception as e:
|
||||||
|
return f"Error deleting '{display_name}': {e}"
|
||||||
|
|
||||||
|
|
||||||
def _container_in_project(container: dict[str, Any], project_name: str) -> bool:
|
def _container_in_project(container: dict[str, Any], project_name: str) -> bool:
|
||||||
"""Check if a container belongs to a project based on its labels."""
|
"""Check if a container belongs to a project based on its labels."""
|
||||||
@@ -350,9 +402,7 @@ def _format_container_detail(name: str, data: dict[str, Any]) -> str:
|
|||||||
lines.append(f" Exit code: {state.get('ExitCode', '?')}")
|
lines.append(f" Exit code: {state.get('ExitCode', '?')}")
|
||||||
|
|
||||||
# IP addresses from all attached networks
|
# IP addresses from all attached networks
|
||||||
networks: dict[str, Any] = (
|
networks: dict[str, Any] = (details.get("NetworkSettings", {}) or {}).get("Networks", {}) or {}
|
||||||
details.get("NetworkSettings", {}) or {}
|
|
||||||
).get("Networks", {}) or {}
|
|
||||||
for net_name, net in networks.items():
|
for net_name, net in networks.items():
|
||||||
ip = (net or {}).get("IPAddress", "")
|
ip = (net or {}).get("IPAddress", "")
|
||||||
if ip:
|
if ip:
|
||||||
|
|||||||
@@ -217,7 +217,8 @@ def register_images(mcp: FastMCP, config: AppConfig, client: DsmClient) -> None:
|
|||||||
img_hash = target.get("id", "")
|
img_hash = target.get("id", "")
|
||||||
|
|
||||||
# Check if image is in use by any container
|
# Check if image is in use by any container
|
||||||
in_use_by: list[str] = []
|
in_use_running: list[str] = []
|
||||||
|
in_use_stopped: list[str] = []
|
||||||
try:
|
try:
|
||||||
ctr_data = await client.request(
|
ctr_data = await client.request(
|
||||||
"SYNO.Docker.Container",
|
"SYNO.Docker.Container",
|
||||||
@@ -231,12 +232,25 @@ def register_images(mcp: FastMCP, config: AppConfig, client: DsmClient) -> None:
|
|||||||
ctr_img_id == img_hash or (hash_prefix and ctr_img_id.startswith(hash_prefix))
|
ctr_img_id == img_hash or (hash_prefix and ctr_img_id.startswith(hash_prefix))
|
||||||
):
|
):
|
||||||
ctr_name = ctr.get("name") or ctr.get("Names", ["?"])[0]
|
ctr_name = ctr.get("name") or ctr.get("Names", ["?"])[0]
|
||||||
in_use_by.append(ctr_name)
|
status = ctr.get("status", ctr.get("state", "")).lower()
|
||||||
|
if status == "running":
|
||||||
|
in_use_running.append(ctr_name)
|
||||||
|
else:
|
||||||
|
in_use_stopped.append(ctr_name)
|
||||||
except Exception as e:
|
except Exception as e:
|
||||||
logger.debug("Could not fetch containers for in-use check: %s", e)
|
logger.debug("Could not fetch containers for in-use check: %s", e)
|
||||||
|
|
||||||
if in_use_by:
|
if in_use_running:
|
||||||
return f"Cannot delete '{display_name}': image is used by " + ", ".join(in_use_by)
|
return (
|
||||||
|
f"Cannot delete '{display_name}': image is used by running container(s): "
|
||||||
|
+ ", ".join(in_use_running)
|
||||||
|
)
|
||||||
|
|
||||||
|
if in_use_stopped:
|
||||||
|
return (
|
||||||
|
f"Cannot delete '{display_name}': image is used by stopped container '{in_use_stopped[0]}'.\n"
|
||||||
|
f"Delete the container first or run system_prune to clean up stopped containers."
|
||||||
|
)
|
||||||
|
|
||||||
if not confirmed:
|
if not confirmed:
|
||||||
return (
|
return (
|
||||||
|
|||||||
@@ -122,7 +122,7 @@ def register_projects(mcp: FastMCP, config: AppConfig, client: DsmClient) -> Non
|
|||||||
Checks the current project status to determine the correct action:
|
Checks the current project status to determine the correct action:
|
||||||
- RUNNING → stop, then start
|
- RUNNING → stop, then start
|
||||||
- STOPPED → start directly (nothing to stop)
|
- STOPPED → start directly (nothing to stop)
|
||||||
- BUILD_FAILED → force-stop, then start
|
- BUILD_FAILED → stop, pull images, then start
|
||||||
|
|
||||||
This operation will briefly take the project offline.
|
This operation will briefly take the project offline.
|
||||||
Requires confirmation before executing.
|
Requires confirmation before executing.
|
||||||
@@ -153,19 +153,23 @@ def register_projects(mcp: FastMCP, config: AppConfig, client: DsmClient) -> Non
|
|||||||
await client.request("SYNO.Docker.Project", "start", params={"id": project_id})
|
await client.request("SYNO.Docker.Project", "start", params={"id": project_id})
|
||||||
results.append(" Project started.")
|
results.append(" Project started.")
|
||||||
|
|
||||||
elif status in ("RUNNING", "BUILD_FAILED", ""):
|
elif status == "BUILD_FAILED":
|
||||||
if status == "RUNNING":
|
results.append("Step 1/3: Stopping failed build...")
|
||||||
results.append("Step 1/2: Stopping project...")
|
with contextlib.suppress(Exception):
|
||||||
await client.request("SYNO.Docker.Project", "stop", params={"id": project_id})
|
await client.request("SYNO.Docker.Project", "stop", params={"id": project_id})
|
||||||
results.append(" Project stopped.")
|
results.append(" Build stopped.")
|
||||||
elif status == "BUILD_FAILED":
|
results.append("Step 2/3: Pulling updated images...")
|
||||||
results.append("Step 1/2: Stopping failed build...")
|
with contextlib.suppress(Exception):
|
||||||
with contextlib.suppress(Exception):
|
await client.request("SYNO.Docker.Image", "pull", params={"id": project_id})
|
||||||
await client.request(
|
results.append(" Images pulled.")
|
||||||
"SYNO.Docker.Project", "stop", params={"id": project_id}
|
results.append("Step 3/3: Starting project...")
|
||||||
)
|
await client.request("SYNO.Docker.Project", "start", params={"id": project_id})
|
||||||
results.append(" Build stopped.")
|
results.append(" Project started.")
|
||||||
|
|
||||||
|
elif status in ("RUNNING", ""):
|
||||||
|
results.append("Step 1/2: Stopping project...")
|
||||||
|
await client.request("SYNO.Docker.Project", "stop", params={"id": project_id})
|
||||||
|
results.append(" Project stopped.")
|
||||||
results.append("Step 2/2: Starting project...")
|
results.append("Step 2/2: Starting project...")
|
||||||
await client.request("SYNO.Docker.Project", "start", params={"id": project_id})
|
await client.request("SYNO.Docker.Project", "start", params={"id": project_id})
|
||||||
results.append(" Project started.")
|
results.append(" Project started.")
|
||||||
|
|||||||
@@ -247,6 +247,76 @@ async def test_container_stats_found():
|
|||||||
assert "Block I/O" in result
|
assert "Block I/O" in result
|
||||||
|
|
||||||
|
|
||||||
|
# ──────────────────────────────────────────────────────────────────────────────
|
||||||
|
# delete_container
|
||||||
|
# ──────────────────────────────────────────────────────────────────────────────
|
||||||
|
|
||||||
|
|
||||||
|
@pytest.mark.asyncio
|
||||||
|
async def test_delete_container_preview():
|
||||||
|
from mcp_synology_container.modules.containers import register_containers
|
||||||
|
|
||||||
|
client = AsyncMock()
|
||||||
|
|
||||||
|
mcp, tools = make_mock_mcp()
|
||||||
|
register_containers(mcp, make_config(), client)
|
||||||
|
|
||||||
|
result = await tools["delete_container"]("myapp_web", confirmed=False)
|
||||||
|
assert "Preview" in result
|
||||||
|
assert "myapp_web" in result
|
||||||
|
client.request.assert_not_called()
|
||||||
|
|
||||||
|
|
||||||
|
@pytest.mark.asyncio
|
||||||
|
async def test_delete_container_not_found():
|
||||||
|
from mcp_synology_container.modules.containers import register_containers
|
||||||
|
|
||||||
|
client = AsyncMock()
|
||||||
|
client.request.return_value = None
|
||||||
|
|
||||||
|
mcp, tools = make_mock_mcp()
|
||||||
|
register_containers(mcp, make_config(), client)
|
||||||
|
|
||||||
|
result = await tools["delete_container"]("nonexistent", confirmed=True)
|
||||||
|
assert "not found" in result
|
||||||
|
|
||||||
|
|
||||||
|
@pytest.mark.asyncio
|
||||||
|
async def test_delete_container_running_blocked():
|
||||||
|
from mcp_synology_container.modules.containers import register_containers
|
||||||
|
|
||||||
|
client = AsyncMock()
|
||||||
|
client.request.return_value = {
|
||||||
|
"details": {"State": {"Running": True, "Status": "running"}},
|
||||||
|
"profile": {"image": "nginx:latest"},
|
||||||
|
}
|
||||||
|
|
||||||
|
mcp, tools = make_mock_mcp()
|
||||||
|
register_containers(mcp, make_config(), client)
|
||||||
|
|
||||||
|
result = await tools["delete_container"]("myapp_web", confirmed=True)
|
||||||
|
assert "running" in result.lower()
|
||||||
|
assert "Cannot delete" in result
|
||||||
|
|
||||||
|
|
||||||
|
@pytest.mark.asyncio
|
||||||
|
async def test_delete_container_stopped_confirmed():
|
||||||
|
from mcp_synology_container.modules.containers import register_containers
|
||||||
|
|
||||||
|
client = AsyncMock()
|
||||||
|
client.request.return_value = {
|
||||||
|
"details": {"State": {"Running": False, "Status": "exited"}},
|
||||||
|
"profile": {"image": "nginx:latest"},
|
||||||
|
}
|
||||||
|
|
||||||
|
mcp, tools = make_mock_mcp()
|
||||||
|
register_containers(mcp, make_config(), client)
|
||||||
|
|
||||||
|
result = await tools["delete_container"]("myapp_web", confirmed=True)
|
||||||
|
assert "Deleted" in result
|
||||||
|
assert "myapp_web" in result
|
||||||
|
|
||||||
|
|
||||||
@pytest.mark.asyncio
|
@pytest.mark.asyncio
|
||||||
async def test_container_stats_cpu_calculation():
|
async def test_container_stats_cpu_calculation():
|
||||||
"""CPU% is computed via the standard Docker formula."""
|
"""CPU% is computed via the standard Docker formula."""
|
||||||
@@ -537,7 +607,10 @@ async def test_get_container_logs_resolves_hash_prefix():
|
|||||||
return HASH_PREFIXED_CONTAINERS_DATA
|
return HASH_PREFIXED_CONTAINERS_DATA
|
||||||
if api == "SYNO.Docker.Container.Log" and method == "get":
|
if api == "SYNO.Docker.Container.Log" and method == "get":
|
||||||
assert kwargs["params"]["name"] == "f93cb8b504f7_jenkins"
|
assert kwargs["params"]["name"] == "f93cb8b504f7_jenkins"
|
||||||
return {"logs": [{"created": "2025-01-01", "stream": "stdout", "text": "started"}], "total": 1}
|
return {
|
||||||
|
"logs": [{"created": "2025-01-01", "stream": "stdout", "text": "started"}],
|
||||||
|
"total": 1,
|
||||||
|
}
|
||||||
return {}
|
return {}
|
||||||
|
|
||||||
client = AsyncMock()
|
client = AsyncMock()
|
||||||
|
|||||||
@@ -255,7 +255,7 @@ async def test_delete_image_not_found():
|
|||||||
|
|
||||||
|
|
||||||
@pytest.mark.asyncio
|
@pytest.mark.asyncio
|
||||||
async def test_delete_image_in_use_blocked():
|
async def test_delete_image_in_use_by_running_blocked():
|
||||||
from mcp_synology_container.modules.images import register_images
|
from mcp_synology_container.modules.images import register_images
|
||||||
|
|
||||||
client = AsyncMock()
|
client = AsyncMock()
|
||||||
@@ -265,7 +265,7 @@ async def test_delete_image_in_use_blocked():
|
|||||||
if api == "SYNO.Docker.Image" and method == "list":
|
if api == "SYNO.Docker.Image" and method == "list":
|
||||||
return SAMPLE_IMAGES
|
return SAMPLE_IMAGES
|
||||||
if api == "SYNO.Docker.Container":
|
if api == "SYNO.Docker.Container":
|
||||||
return SAMPLE_CONTAINERS # nginx is in use
|
return SAMPLE_CONTAINERS # nginx is in use (running)
|
||||||
return {}
|
return {}
|
||||||
|
|
||||||
client.request.side_effect = mock_request
|
client.request.side_effect = mock_request
|
||||||
@@ -274,10 +274,41 @@ async def test_delete_image_in_use_blocked():
|
|||||||
|
|
||||||
result = await tools["delete_image"](image_id="nginx:1.24", confirmed=True)
|
result = await tools["delete_image"](image_id="nginx:1.24", confirmed=True)
|
||||||
assert "Cannot delete" in result
|
assert "Cannot delete" in result
|
||||||
|
assert "running" in result.lower()
|
||||||
assert "my-nginx" in result
|
assert "my-nginx" in result
|
||||||
client.post_request.assert_not_called()
|
client.post_request.assert_not_called()
|
||||||
|
|
||||||
|
|
||||||
|
@pytest.mark.asyncio
|
||||||
|
async def test_delete_image_in_use_by_stopped_blocked():
|
||||||
|
from mcp_synology_container.modules.images import register_images
|
||||||
|
|
||||||
|
client = AsyncMock()
|
||||||
|
client.post_request = AsyncMock(return_value={})
|
||||||
|
|
||||||
|
async def mock_request(api, method, **kwargs):
|
||||||
|
if api == "SYNO.Docker.Image" and method == "list":
|
||||||
|
return SAMPLE_IMAGES
|
||||||
|
if api == "SYNO.Docker.Container":
|
||||||
|
return {
|
||||||
|
"containers": [
|
||||||
|
{"name": "stopped-nginx", "image_id": "sha256:aaaa", "status": "exited"}
|
||||||
|
]
|
||||||
|
}
|
||||||
|
return {}
|
||||||
|
|
||||||
|
client.request.side_effect = mock_request
|
||||||
|
mcp, tools = make_mock_mcp()
|
||||||
|
register_images(mcp, make_config(), client)
|
||||||
|
|
||||||
|
result = await tools["delete_image"](image_id="nginx:1.24", confirmed=True)
|
||||||
|
assert "Cannot delete" in result
|
||||||
|
assert "stopped" in result.lower()
|
||||||
|
assert "stopped-nginx" in result
|
||||||
|
assert "system_prune" in result
|
||||||
|
client.post_request.assert_not_called()
|
||||||
|
|
||||||
|
|
||||||
@pytest.mark.asyncio
|
@pytest.mark.asyncio
|
||||||
async def test_delete_image_by_hash():
|
async def test_delete_image_by_hash():
|
||||||
import json
|
import json
|
||||||
|
|||||||
@@ -100,6 +100,7 @@ async def test_list_projects_tool():
|
|||||||
def decorator(fn):
|
def decorator(fn):
|
||||||
tools[fn.__name__] = fn
|
tools[fn.__name__] = fn
|
||||||
return fn
|
return fn
|
||||||
|
|
||||||
return decorator
|
return decorator
|
||||||
|
|
||||||
register_projects(MockMCP(), config, client)
|
register_projects(MockMCP(), config, client)
|
||||||
@@ -128,6 +129,7 @@ async def test_stop_project_requires_confirmation():
|
|||||||
def decorator(fn):
|
def decorator(fn):
|
||||||
tools[fn.__name__] = fn
|
tools[fn.__name__] = fn
|
||||||
return fn
|
return fn
|
||||||
|
|
||||||
return decorator
|
return decorator
|
||||||
|
|
||||||
register_projects(MockMCP(), config, client)
|
register_projects(MockMCP(), config, client)
|
||||||
@@ -154,6 +156,7 @@ async def test_redeploy_project_requires_confirmation():
|
|||||||
def decorator(fn):
|
def decorator(fn):
|
||||||
tools[fn.__name__] = fn
|
tools[fn.__name__] = fn
|
||||||
return fn
|
return fn
|
||||||
|
|
||||||
return decorator
|
return decorator
|
||||||
|
|
||||||
register_projects(MockMCP(), config, client)
|
register_projects(MockMCP(), config, client)
|
||||||
@@ -183,6 +186,7 @@ def make_projects_tools(client):
|
|||||||
def decorator(fn):
|
def decorator(fn):
|
||||||
tools[fn.__name__] = fn
|
tools[fn.__name__] = fn
|
||||||
return fn
|
return fn
|
||||||
|
|
||||||
return decorator
|
return decorator
|
||||||
|
|
||||||
register_projects(MockMCP(), config, client)
|
register_projects(MockMCP(), config, client)
|
||||||
@@ -249,7 +253,7 @@ async def test_redeploy_stopped_project_starts_directly():
|
|||||||
|
|
||||||
@pytest.mark.asyncio
|
@pytest.mark.asyncio
|
||||||
async def test_redeploy_build_failed_project():
|
async def test_redeploy_build_failed_project():
|
||||||
"""BUILD_FAILED project: force-stop (non-fatal), then start."""
|
"""BUILD_FAILED project: stop, pull images, then start (3 steps)."""
|
||||||
client = AsyncMock()
|
client = AsyncMock()
|
||||||
calls = []
|
calls = []
|
||||||
|
|
||||||
@@ -265,13 +269,16 @@ async def test_redeploy_build_failed_project():
|
|||||||
assert "redeployed successfully" in result
|
assert "redeployed successfully" in result
|
||||||
methods = [m for _, m in calls]
|
methods = [m for _, m in calls]
|
||||||
assert "stop" in methods
|
assert "stop" in methods
|
||||||
|
assert "pull" in methods # New: pull step
|
||||||
assert "start" in methods
|
assert "start" in methods
|
||||||
assert methods.index("stop") < methods.index("start")
|
# Order: stop → pull → start
|
||||||
|
assert methods.index("stop") < methods.index("pull")
|
||||||
|
assert methods.index("pull") < methods.index("start")
|
||||||
|
|
||||||
|
|
||||||
@pytest.mark.asyncio
|
@pytest.mark.asyncio
|
||||||
async def test_redeploy_build_failed_stop_error_nonfatal():
|
async def test_redeploy_build_failed_stop_error_nonfatal():
|
||||||
"""BUILD_FAILED: stop failure must not abort the redeploy."""
|
"""BUILD_FAILED: stop/pull failure must not abort the redeploy."""
|
||||||
from mcp_synology_container.dsm_client import SynologyError
|
from mcp_synology_container.dsm_client import SynologyError
|
||||||
|
|
||||||
client = AsyncMock()
|
client = AsyncMock()
|
||||||
@@ -281,6 +288,8 @@ async def test_redeploy_build_failed_stop_error_nonfatal():
|
|||||||
return project_list("BUILD_FAILED")
|
return project_list("BUILD_FAILED")
|
||||||
if method == "stop":
|
if method == "stop":
|
||||||
raise SynologyError("already stopped", code=2101)
|
raise SynologyError("already stopped", code=2101)
|
||||||
|
if method == "pull":
|
||||||
|
raise SynologyError("pull failed", code=2102)
|
||||||
return {} # start succeeds
|
return {} # start succeeds
|
||||||
|
|
||||||
client.request.side_effect = mock_request
|
client.request.side_effect = mock_request
|
||||||
|
|||||||
@@ -362,7 +362,7 @@ wheels = [
|
|||||||
|
|
||||||
[[package]]
|
[[package]]
|
||||||
name = "mcp-synology-container"
|
name = "mcp-synology-container"
|
||||||
version = "0.1.0"
|
version = "0.2.0"
|
||||||
source = { editable = "." }
|
source = { editable = "." }
|
||||||
dependencies = [
|
dependencies = [
|
||||||
{ name = "click" },
|
{ name = "click" },
|
||||||
|
|||||||
Reference in New Issue
Block a user