feat(workloads): Add generic Docker workload execution - #580
Conversation
Introduce an isolated workload API for non-Frappe containers. Secrets stay out of Docker command arguments and queued job payloads, while failed health checks restore the previous container.
|
Tick the box to add this pull request to the merge queue (same as
|
Confidence Score: 4/5The PR is not yet safe to merge because cancellation can race worker startup and delete state required by a running deployment. Cancellation chooses between cancel and stop using unsynchronized job state, then unconditionally removes the queued environment file even if the worker has begun consuming it. Files Needing Attention: agent/job.py, agent/workload.py Reviews (3): Last reviewed commit: "fix(workloads): Remove cancelled deploym..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
This PR introduces a new “workload” subsystem to deploy and manage non-Frappe Docker containers via the agent API, aiming to keep secrets out of Docker command arguments and queued job payloads while providing rollback behavior on failed health checks.
Changes:
- Added
agent/workload.pyimplementing workload config validation, environment-file handling, container replacement with health checks, and rollback. - Exposed new workload management endpoints in
agent/web.py(deploy,status,logs,rollback). - Added unit tests for
WorkloadConfigto validate command construction and secret redaction.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| agent/workload.py | New workload deploy/rollback implementation using Docker + env-file and health checks. |
| agent/web.py | New HTTP endpoints to trigger and inspect workloads. |
| agent/tests/test_workload.py | New tests covering WorkloadConfig validation and secret handling expectations. |
Suppressed comments (1)
agent/workload.py:136
_replace_containerremoves the-previouscontainer before checking whether a current container exists. If the current container has been removed but a-previousrollback target exists, this deletes the only fallback and the exception handler won’t restore anything (sincecurrent_existsis false).
def _replace_container(self, deployment: WorkloadConfig, environment_file: Path):
previous = f"{deployment.name}-previous"
candidate = f"{deployment.name}-candidate"
self._remove_container(candidate)
self._remove_container(previous)
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| def validate(self): | ||
| validate_name(self.name) | ||
| if not IMAGE_REFERENCE.fullmatch(self.image): | ||
| raise InvalidWorkload("Invalid Docker image reference") | ||
| self._validate_port(self.container_port, "container_port") | ||
| self._validate_port(self.host_port, "host_port") | ||
| if not isinstance(self.health_path, str) or not self.health_path.startswith("/"): | ||
| raise InvalidWorkload("Health path must start with /") | ||
| if not isinstance(self.health_timeout, int) or not 1 <= self.health_timeout <= 300: | ||
| raise InvalidWorkload("Health timeout must be between 1 and 300 seconds") | ||
| self._validate_environment() | ||
|
|
||
| def _validate_port(self, port, field: str): | ||
| if not isinstance(port, int) or not 1024 <= port <= 65535: | ||
| raise InvalidWorkload(f"{field} must be between 1024 and 65535") | ||
|
|
| @job("Rollback Workload", priority="low") | ||
| def rollback(self, name: str): | ||
| validate_name(name) | ||
| previous = f"{name}-previous" | ||
| failed = f"{name}-failed" | ||
| if not self._container_exists(previous): | ||
| raise InvalidWorkload("No previous workload deployment is available") | ||
| self.server.execute(["docker", "stop", name]) | ||
| try: | ||
| self.server.execute(["docker", "rename", name, failed]) | ||
| except Exception: | ||
| self.server.execute(["docker", "start", name]) | ||
| raise | ||
| try: | ||
| self.server.execute(["docker", "rename", previous, name]) | ||
| self.server.execute(["docker", "start", name]) | ||
| except Exception: | ||
| if self._container_exists(name): | ||
| self.server.execute(["docker", "rename", name, previous]) | ||
| self.server.execute(["docker", "rename", failed, name]) | ||
| self.server.execute(["docker", "start", name]) | ||
| raise | ||
| self._remove_container(failed) | ||
|
|
| class TestWorkloadConfig(unittest.TestCase): | ||
| def valid_config(self): | ||
| return { | ||
| "name": "xassida-search", | ||
| "image": "ghcr.io/jvrlc/xassida-search:abc123", | ||
| "container_port": 3000, | ||
| "host_port": 13000, | ||
| "health_path": "/api/health", | ||
| } | ||
|
|
||
| def test_valid_config_builds_loopback_only_docker_command(self): | ||
| config = WorkloadConfig(self.valid_config(), {"SUPABASE_URL": "safe"}) | ||
| config.validate() | ||
|
|
||
| command = config.run_command("xassida-search-candidate", Path("/secure/environment")) | ||
|
|
||
| self.assertIn("127.0.0.1:13000:3000", command) | ||
| self.assertNotIn("safe", command) | ||
|
|
Create one protected environment file per request and serialize deployments per workload. Always remove the temporary secret file after execution or enqueue failure.
Keep queued and cancelled deployment environments encrypted at rest. Materialize plaintext only while Docker starts, clean runtime files in a finally block, and expire orphaned encrypted envelopes.
Delete a queued workload's encrypted environment immediately after RQ cancellation. Keep path-boundary validation and age-based cleanup as safeguards for abandoned envelopes.
| self.job.cancel() | ||
| self._cleanup_cancelled_workload_environment() |
There was a problem hiding this comment.
If a worker starts the deployment after cancel_or_stop() checks is_started, cancel() does not stop that running job but still deletes its queued environment file, causing the worker to fail with FileNotFoundError when decrypting it.
Context Used: Guidelines for reviewing Frappe Framework applicat... (source)
Knowledge Base Used: Job Execution: RQ Workers, Steps, and Callbacks
Introduce an isolated workload API for non-Frappe containers. Secrets stay out of Docker command arguments and queued job payloads, while failed health checks restore the previous container.