Added logging during Ventis deploy logs - #13
Conversation
Cleaned up changed code EC2 path working now
fixed the lint/type checking errors
…ls more logs are available
📝 WalkthroughWalkthroughThe change adds Redis-to-SQLAlchemy runtime persistence, records future lifecycle metadata, wires persistence into controller polling, configures SQLite storage, adds coverage, and improves EC2 image-transfer diagnostics and ignore rules. ChangesRuntime persistence
EC2 bootstrap diagnostics
Repository hygiene
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Future
participant Redis
participant GlobalController
participant SQLAlchemy
participant RuntimeInformation
Future->>Redis: Store created_at
GlobalController->>Redis: Pull future hashes
GlobalController->>SQLAlchemy: Send rows and resources
SQLAlchemy->>Redis: Read workflow metadata
SQLAlchemy->>RuntimeInformation: Upsert runtime_information
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ventis/controller/local_controller.py (1)
415-417: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winRestore thread-local request context after execution.
Executor threads are reused, but this context is never cleared. A later request lacking
request_idcan inherit the prior request’s ID, causing nested futures to be persisted under the wrong session. Reset or restore the prior context in afinallyblock.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ventis/controller/local_controller.py` around lines 415 - 417, Update the worker-thread flow around ventis_context.set_request_id to capture the prior request context, apply request_id when present, and restore the previous value in a finally block after execution. Ensure reused executor threads cannot retain a request ID when the current request lacks one.
🧹 Nitpick comments (1)
ventis/controller/cloud_provider_logic/EC2/_runtime.py (1)
231-247: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd regression coverage for transfer failures.
The existing bootstrap test exercises only a successful
subprocess.run. Add a failure case asserting that the raised error includes captured diagnostics, and verify the pipeline invokes Bash withpipefail.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ventis/controller/cloud_provider_logic/EC2/_runtime.py` around lines 231 - 247, Add regression coverage for the image-transfer failure path around the subprocess invocation: configure the mocked result with a nonzero return code and captured stderr, assert the raised RuntimeError includes that diagnostic text, and verify subprocess.run is called with executable="/bin/bash" so the pipeline uses pipefail. Preserve the existing successful-transfer test.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@ventis/controller/cloud_provider_logic/EC2/_runtime.py`:
- Around line 232-241: Update the image-transfer subprocess.run invocation to
enforce a finite timeout, covering both SSH connection and stalled docker
save/load operations. Use the existing command-timeout configuration or an
appropriate bounded value, and preserve the current pipeline, output capture,
and Bash execution behavior.
In `@ventis/controller/global_controller.py`:
- Around line 405-410: Update the send_data call within _poll_controllers to
catch database/persistence exceptions, log the failure, and allow polling to
continue so later polls retry persistence. Keep controller health checks and
routing maintenance running, and do not let the exception propagate through
run().
- Around line 405-410: Update the flow around send_data and pull_data so
completed request records are acknowledged as persisted before asynchronous
cleanup removes their future:* Redis hashes. Add a persistence
acknowledgement/queue, or defer deletion until send_data confirms storage,
ensuring requests completing between polls still produce canonical runtime rows.
In `@ventis/controller/local_controller_frontend.py`:
- Around line 43-45: Update the forwarded-future handling around the Redis hset
and pull_data lifecycle flow so the target controller does not create a partial
future:<id> hash. Keep the source controller’s hash as the single canonical
lifecycle record, and route completion updates back to that owner while
preserving request_id, workflow, and created_at.
In `@ventis/controller/utils/sqlalchemy.py`:
- Around line 11-30: Initialize or migrate the runtime_information table before
the _UPSERT statement can execute, including its columns and future_id conflict
constraint. Integrate this schema setup into the module’s existing database
startup or persistence flow so fresh sqlite:///ventis_runtime.db databases work
without test-only table creation.
---
Outside diff comments:
In `@ventis/controller/local_controller.py`:
- Around line 415-417: Update the worker-thread flow around
ventis_context.set_request_id to capture the prior request context, apply
request_id when present, and restore the previous value in a finally block after
execution. Ensure reused executor threads cannot retain a request ID when the
current request lacks one.
---
Nitpick comments:
In `@ventis/controller/cloud_provider_logic/EC2/_runtime.py`:
- Around line 231-247: Add regression coverage for the image-transfer failure
path around the subprocess invocation: configure the mocked result with a
nonzero return code and captured stderr, assert the raised RuntimeError includes
that diagnostic text, and verify subprocess.run is called with
executable="/bin/bash" so the pipeline uses pipefail. Preserve the existing
successful-transfer test.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: b2fb95f4-311b-4e43-936d-b4dbd6529d57
📒 Files selected for processing (13)
.gitignorepyproject.tomlrequirements.txttests/test_runtime_ec2.pytests/test_runtime_sqlalchemy.pyventis/controller/cloud_provider_logic/EC2/_runtime.pyventis/controller/global_controller.pyventis/controller/local_controller.pyventis/controller/local_controller_frontend.pyventis/controller/utils/sqlalchemy.pyventis/deploy.pyventis/future.pyventis/templates/config/global_controller.yaml
💤 Files with no reviewable changes (1)
- .gitignore
| result = subprocess.run( | ||
| "set -o pipefail; " | ||
| f"docker save {shlex.quote(image)} | ssh -o StrictHostKeyChecking=no " | ||
| f"-o IdentitiesOnly=yes -i {shlex.quote(key)} " | ||
| f"{shlex.quote(f'{ssh_user}@{host}')} 'sudo docker load'", | ||
| shell=True, | ||
| capture_output=True, | ||
| text=True, | ||
| executable="/bin/bash", | ||
| ) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Bound the image-transfer subprocess.
This direct SSH pipeline bypasses global_controller.py:_run_cmd, which supplies ConnectTimeout=10, and subprocess.run has no timeout. An unreachable host or stalled docker load can therefore block EC2 bootstrapping indefinitely.
Proposed fix
f"docker save {shlex.quote(image)} | ssh -o StrictHostKeyChecking=no "
+ f"-o ConnectTimeout=10 "
f"-o IdentitiesOnly=yes -i {shlex.quote(key)} "
f"{shlex.quote(f'{ssh_user}@{host}')} 'sudo docker load'",
...
executable="/bin/bash",
+ timeout=cfg.get("image_transfer_timeout", 300),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| result = subprocess.run( | |
| "set -o pipefail; " | |
| f"docker save {shlex.quote(image)} | ssh -o StrictHostKeyChecking=no " | |
| f"-o IdentitiesOnly=yes -i {shlex.quote(key)} " | |
| f"{shlex.quote(f'{ssh_user}@{host}')} 'sudo docker load'", | |
| shell=True, | |
| capture_output=True, | |
| text=True, | |
| executable="/bin/bash", | |
| ) | |
| result = subprocess.run( | |
| "set -o pipefail; " | |
| f"docker save {shlex.quote(image)} | ssh -o StrictHostKeyChecking=no " | |
| f"-o ConnectTimeout=10 " | |
| f"-o IdentitiesOnly=yes -i {shlex.quote(key)} " | |
| f"{shlex.quote(f'{ssh_user}@{host}')} 'sudo docker load'", | |
| shell=True, | |
| capture_output=True, | |
| text=True, | |
| executable="/bin/bash", | |
| timeout=cfg.get("image_transfer_timeout", 300), | |
| ) |
🧰 Tools
🪛 OpenGrep (1.25.0)
[ERROR] 232-241: Dynamic command passed to subprocess with shell=True. Use a command list without shell=True, or use shlex.quote() to sanitize input.
(coderabbit.command-injection.python-shell-true)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@ventis/controller/cloud_provider_logic/EC2/_runtime.py` around lines 232 -
241, Update the image-transfer subprocess.run invocation to enforce a finite
timeout, covering both SSH connection and stalled docker save/load operations.
Use the existing command-timeout configuration or an appropriate bounded value,
and preserve the current pipeline, output capture, and Bash execution behavior.
| send_data( | ||
| pull_data(node_redis), | ||
| {c["name"]: c.get("resources", {}) for c in self.controllers}, | ||
| node_redis, | ||
| self.config.get("database", {}).get("url"), | ||
| ) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Do not let database failures stop controller polling.
send_data() exceptions propagate through _poll_controllers() and out of run(), terminating health checks and routing maintenance when the database is unavailable or misconfigured. Isolate persistence failures, log them, and retry on later polls.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@ventis/controller/global_controller.py` around lines 405 - 410, Update the
send_data call within _poll_controllers to catch database/persistence
exceptions, log the failure, and allow polling to continue so later polls retry
persistence. Keep controller health checks and routing maintenance running, and
do not let the exception propagate through run().
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Persist before cleanup deletes the Redis source records.
Persistence is periodic, but completed requests are asynchronously cleaned up and their future:* hashes deleted. A request that completes after one poll can be removed before the next, leaving no runtime row. Add a persistence acknowledgement/queue or defer deletion until the canonical records have been stored.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@ventis/controller/global_controller.py` around lines 405 - 410, Update the
flow around send_data and pull_data so completed request records are
acknowledged as persisted before asynchronous cleanup removes their future:*
Redis hashes. Add a persistence acknowledgement/queue, or defer deletion until
send_data confirms storage, ensuring requests completing between polls still
produce canonical runtime rows.
| data = json.loads(request.resonse) | ||
| future_id = data.get("future_id") | ||
| self.redis.hset(f"future:{future_id}", "agent", data.get("service")) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Do not persist a partial duplicate for forwarded futures.
The target controller creates future:<id> with only agent. pull_data() still treats it as a row, and its later finished_at write is upserted over the source record for the same future_id. That replacement loses request_id/workflow and uses created_at=0, producing incorrect execution times. Keep lifecycle metadata in one canonical Redis hash, or replicate the complete record and propagate completion back to that owner.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@ventis/controller/local_controller_frontend.py` around lines 43 - 45, Update
the forwarded-future handling around the Redis hset and pull_data lifecycle flow
so the target controller does not create a partial future:<id> hash. Keep the
source controller’s hash as the single canonical lifecycle record, and route
completion updates back to that owner while preserving request_id, workflow, and
created_at.
| _UPSERT = text( | ||
| """ | ||
| INSERT INTO runtime_information ( | ||
| future_id, session_id, workflow, agent, execution_time, | ||
| cpu_resource, gpu_resource, created_at, updated_at | ||
| ) VALUES ( | ||
| :future_id, :session_id, :workflow, :agent, :execution_time, | ||
| :cpu_resource, :gpu_resource, :created_at, :updated_at | ||
| ) | ||
| ON CONFLICT(future_id) DO UPDATE SET | ||
| session_id=excluded.session_id, | ||
| workflow=excluded.workflow, | ||
| agent=excluded.agent, | ||
| execution_time=excluded.execution_time, | ||
| cpu_resource=excluded.cpu_resource, | ||
| gpu_resource=excluded.gpu_resource, | ||
| created_at=excluded.created_at, | ||
| updated_at=excluded.updated_at | ||
| """ | ||
| ) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Create or migrate runtime_information before issuing UPSERTs.
A fresh sqlite:///ventis_runtime.db has no table, while this module only executes inserts. The test hides this by manually creating the table. Add a migration or startup schema initialization; otherwise the first persisted future raises a database error.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@ventis/controller/utils/sqlalchemy.py` around lines 11 - 30, Initialize or
migrate the runtime_information table before the _UPSERT statement can execute,
including its columns and future_id conflict constraint. Integrate this schema
setup into the module’s existing database startup or persistence flow so fresh
sqlite:///ventis_runtime.db databases work without test-only table creation.
|
These changes were made and merged in PR#16 |
1 participant
Currently, when ventis deploy is running and docker container is run from global controller to a local controller, there is no logging if that command fails, and only says that that docker run command was the cause, instead of more micro details.
I added basic logging for that command.
~10 lines changes in ventis/controller/cloud_provider_logic/EC2/_runtime.py
Summary by CodeRabbit
New Features
Bug Fixes
Tests