Skip to content

Commit 7e57438

Browse files
Pangjipingclaude
andauthored
fix(server): k8s patch_sandbox_metadata correctly deletes keys and returns post-patch state (#899)
* fix(server): k8s patch_sandbox_metadata correctly deletes keys and returns post-patch state Two bugs in KubernetesSandboxService.patch_sandbox_metadata caused the nightly k8s mini E2E test_02_metadata_filter_and_logic to fail: 1. JSON merge patch (RFC 7396) on metadata.labels merges keys recursively — keys absent from the patch body are kept. The previous code computed the desired final labels dict (with deleted keys removed) and sent it, so deleted keys were never actually removed on the API server. 2. After the PATCH, the code re-fetched the workload via _get_workload_or_404, which goes through K8sClient.get_custom_object that prefers the informer cache. The informer is eventually consistent, so the read could land before the watch event arrived and return the pre-patch labels. Fix both by: - Building the merge-patch body with explicit None for deleted keys. - Using the API server's PATCH response (returned from patch_labels) directly, instead of re-reading via the cache. WorkloadProvider.patch_labels now accepts Dict[str, Optional[str]] and returns the patched workload dict. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * chore(ci): touch trigger comment to run k8s mini E2E on this PR The k8s-nightly-build workflow only runs on PRs that touch one of its trigger paths. This PR fixes server-side k8s logic but does not modify those paths, so add a date stamp to the existing trigger comment to include this PR in the matrix. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
1 parent a0f66f3 commit 7e57438

4 files changed

Lines changed: 80 additions & 11 deletions

File tree

scripts/python-k8s-e2e.sh

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
#!/bin/bash
2-
# trigger k8s e2e
2+
# trigger k8s e2e (2026-05-17)
33
# Copyright 2026 Alibaba Group Holding Ltd.
44
#
55
# Licensed under the Apache License, Version 2.0 (the "License");

server/opensandbox_server/services/k8s/kubernetes_service.py

Lines changed: 11 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -791,21 +791,25 @@ def patch_sandbox_metadata(self, sandbox_id: str, patch: PatchSandboxMetadataReq
791791

792792
new_labels = self._apply_metadata_patch(labels, patch)
793793

794+
# JSON merge patch (RFC 7396) on metadata.labels treats keys absent
795+
# from the body as kept. To delete a label we must send the key with
796+
# an explicit null. Build the merge body from the desired final labels
797+
# plus null markers for keys removed by this patch.
798+
label_patch: Dict[str, Optional[str]] = dict(new_labels)
799+
for key, value in patch.items():
800+
if value is None:
801+
label_patch[key] = None
802+
794803
try:
795-
self.workload_provider.patch_labels(
804+
updated = self.workload_provider.patch_labels(
796805
name=name,
797806
namespace=self.namespace,
798-
labels=new_labels,
807+
labels=label_patch,
799808
)
800809
except Exception as e:
801810
logger.error("Error patching labels for sandbox %s: %s", sandbox_id, e)
802811
raise _build_k8s_api_error("patch sandbox labels", e) from e
803812

804-
updated = _get_workload_or_404(
805-
self.workload_provider,
806-
self.namespace,
807-
sandbox_id,
808-
)
809813
return _build_sandbox_from_workload(updated, self.workload_provider)
810814

811815
def get_endpoint(

server/opensandbox_server/services/k8s/workload_provider.py

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -202,10 +202,16 @@ def resume_sandbox(self, sandbox_id: str, namespace: str) -> None:
202202
"""
203203
raise NotImplementedError("Resume is not supported by this provider")
204204

205-
def patch_labels(self, name: str, namespace: str, labels: Dict[str, str]) -> None:
206-
"""Patch workload metadata.labels via JSON merge patch."""
205+
def patch_labels(
206+
self, name: str, namespace: str, labels: Dict[str, Optional[str]]
207+
) -> Dict[str, Any]:
208+
"""Patch workload metadata.labels via JSON merge patch.
209+
210+
A None value for a label key deletes that label per RFC 7396.
211+
Returns the API server response (the patched workload).
212+
"""
207213
body = {"metadata": {"labels": labels}}
208-
self.k8s_client.patch_custom_object(
214+
return self.k8s_client.patch_custom_object(
209215
group=self.group,
210216
version=self.version,
211217
namespace=namespace,

server/tests/k8s/test_kubernetes_service.py

Lines changed: 59 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1297,3 +1297,62 @@ def test_signed_endpoint_different_expires_produces_different_endpoints(self, k8
12971297
ep2 = k8s_service.get_endpoint("sbx-001", 8080, expires=2000000500)
12981298

12991299
assert ep1.endpoint != ep2.endpoint
1300+
1301+
1302+
class TestPatchSandboxMetadata:
1303+
"""Verify patch_sandbox_metadata builds the JSON merge-patch body correctly
1304+
and uses the API server's PATCH response (not a cache-prone re-fetch)."""
1305+
1306+
@staticmethod
1307+
def _workload(labels: dict) -> dict:
1308+
return {
1309+
"metadata": {
1310+
"name": "sandbox-sbx-001",
1311+
"labels": dict(labels),
1312+
"creationTimestamp": datetime(2026, 1, 1, tzinfo=timezone.utc),
1313+
},
1314+
"spec": {},
1315+
"status": {"conditions": []},
1316+
}
1317+
1318+
@staticmethod
1319+
def _stub_provider_status(k8s_service) -> None:
1320+
k8s_service.workload_provider.get_status.return_value = {
1321+
"state": "Running",
1322+
"reason": None,
1323+
"message": None,
1324+
"last_transition_at": None,
1325+
}
1326+
k8s_service.workload_provider.get_expiration.return_value = None
1327+
1328+
def test_patch_body_sends_null_for_deleted_keys(self, k8s_service):
1329+
initial = {"opensandbox.io/id": "sbx-001", "team": "infra", "env": "dev"}
1330+
patched = {"opensandbox.io/id": "sbx-001", "env": "stage"}
1331+
1332+
k8s_service.workload_provider.get_workload.return_value = self._workload(initial)
1333+
k8s_service.workload_provider.patch_labels.return_value = self._workload(patched)
1334+
self._stub_provider_status(k8s_service)
1335+
1336+
k8s_service.patch_sandbox_metadata("sbx-001", {"env": "stage", "team": None})
1337+
1338+
k8s_service.workload_provider.patch_labels.assert_called_once()
1339+
body_labels = k8s_service.workload_provider.patch_labels.call_args.kwargs["labels"]
1340+
assert body_labels["env"] == "stage"
1341+
assert body_labels["team"] is None
1342+
assert body_labels["opensandbox.io/id"] == "sbx-001"
1343+
1344+
def test_returns_sandbox_from_patch_response(self, k8s_service):
1345+
"""The PATCH response is authoritative; re-reading via get_workload
1346+
could hit a stale informer cache."""
1347+
initial = {"opensandbox.io/id": "sbx-001", "env": "dev"}
1348+
patched = {"opensandbox.io/id": "sbx-001", "env": "stage"}
1349+
1350+
k8s_service.workload_provider.get_workload.return_value = self._workload(initial)
1351+
k8s_service.workload_provider.patch_labels.return_value = self._workload(patched)
1352+
self._stub_provider_status(k8s_service)
1353+
1354+
sandbox = k8s_service.patch_sandbox_metadata("sbx-001", {"env": "stage"})
1355+
1356+
assert sandbox.metadata == {"env": "stage"}
1357+
# Pre-patch read only; no second get_workload after patch_labels.
1358+
assert k8s_service.workload_provider.get_workload.call_count == 1

0 commit comments

Comments
 (0)