feat: implement pod snapshot method#339
Conversation
✅ Deploy Preview for agent-sandbox canceled.
|
|
Hi @shrutiyam-glitch. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/ok-to-test |
clients/python/agentic-sandbox-client/k8s_agent_sandbox/gke_extensions/podsnapshot_client.py
Show resolved
Hide resolved
clients/python/agentic-sandbox-client/k8s_agent_sandbox/gke_extensions/podsnapshot_client.py
Show resolved
Hide resolved
clients/python/agentic-sandbox-client/k8s_agent_sandbox/gke_extensions/podsnapshot_client.py
Show resolved
Hide resolved
clients/python/agentic-sandbox-client/k8s_agent_sandbox/gke_extensions/podsnapshot_client.py
Outdated
Show resolved
Hide resolved
clients/python/agentic-sandbox-client/k8s_agent_sandbox/gke_extensions/podsnapshot_client.py
Outdated
Show resolved
Hide resolved
clients/python/agentic-sandbox-client/k8s_agent_sandbox/gke_extensions/podsnapshot_client.py
Show resolved
Hide resolved
|
/lgtm |
|
@SHRUTI6991: changing LGTM is restricted to collaborators DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
6dacc55 to
fd7a2c0
Compare
clients/python/agentic-sandbox-client/k8s_agent_sandbox/gke_extensions/podsnapshot_client.py
Show resolved
Hide resolved
clients/python/agentic-sandbox-client/k8s_agent_sandbox/gke_extensions/podsnapshot_client.py
Show resolved
Hide resolved
clients/python/agentic-sandbox-client/k8s_agent_sandbox/gke_extensions/podsnapshot_client.py
Show resolved
Hide resolved
clients/python/agentic-sandbox-client/k8s_agent_sandbox/gke_extensions/podsnapshot_client.py
Show resolved
Hide resolved
|
|
||
| import logging | ||
| from kubernetes import client | ||
| import os |
There was a problem hiding this comment.
import os seems to be unused in this file. Consider removing it to keep the imports clean.
| """Result of a snapshot processing operation.""" | ||
|
|
||
| snapshot_uid: str | ||
| snapshot_timestamp: str |
There was a problem hiding this comment.
snapshot_timestamp is extracted and stored in SnapshotResult, but it is never utilized or returned in SnapshotResponse. Should we add it to the final SnapshotResponse so the user knows when the snapshot occurred
| """Parses the object to extract snapshot details.""" | ||
| status = obj.get("status", {}) | ||
| conditions = status.get("conditions", []) | ||
| for condition in conditions: |
There was a problem hiding this comment.
If the the API responds with an explicit null for conditions ({"conditions": null}), status.get("conditions", []) will return None. Iterating over None on the next line will raise a TypeError. Please use conditions = status.get("conditions") or [] to ensure it always defaults to a list.
| and condition.get("status") == "True" | ||
| and condition.get("reason") == "Complete" | ||
| ): | ||
| snapshot_uid = status.get("snapshotCreated", {}).get("name") |
There was a problem hiding this comment.
It is safer to write: snapshot_created = status.get("snapshotCreated") or {} and then snapshot_uid = snapshot_created.get("name"). Similar case as the on ebaove where the API can return a null value...
| Returns: | ||
| SnapshotResponse: The result of the operation. | ||
| """ | ||
| timestamp = datetime.now().strftime("%Y%m%d-%H%M%S") |
There was a problem hiding this comment.
No, it is the local time of the system wherever the code is running. Should it be UTC ?
| self.assertIn("test-trigger", result.trigger_name) | ||
|
|
||
| # Verify create call was made | ||
| self.client.custom_objects_api.create_namespaced_custom_object.assert_called_once() |
There was a problem hiding this comment.
To improve test coverage, you could use assert_called_once_with(...) instead of assert_called_once(). This ensures that the generated manifest payload accurately includes the generated trigger_name and the correct targetPod.
|
|
||
| result = self.client.snapshot("test-retry") | ||
|
|
||
| self.assertTrue(result.success) |
There was a problem hiding this comment.
When asserting result.success, we could use self.assertTrue(result.success, result.error_reason). This will print the actual error message in the test output if the test fails unexpectedly.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: shrutiyam-glitch, vicentefb The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Depends on: #338 #337
This PR completes the Pod Snapshot extension for the Python SDK by implementing the
snapshot()method and the underlying logic to monitor snapshot completion.Core Logic Implementation:
snapshot()method creates aPodSnapshotManualTriggerresource for the target pod, using a unique hash suffix to avoid name collisions._wait_for_snapshot_to_be_completed, which uses the Kubernetes watch API to block until the snapshot controller marks the trigger asComplete. It specifically looks for the Triggered condition with aTruestatus andCompletereason.snapshot_uidfrom the resource status and returns a structuredSnapshotResponse, providing the user with the unique identifier needed for future restores.__exit__method to ensure that allPodSnapshotManualTriggerresources created during the session are automatically deleted, maintaining a clean namespace.Testing Done:
Integration Test: Added
test_podsnapshot_extension.pywhich verifies the full E2E flow:-- Starts a sandbox with a counter application.
-- Creates two sequential snapshots (test-snapshot-10 at 10 seconds and test-snapshot-20 at 20 seconds).
Unit tests are added
Follow-up PR:
Restore example, is_restored() check added in the following PR (#249)
List_snapshots, delete_snapshots and restoring from dedicated snapshot will be added in the following PRs.
Output: