Conversation
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Keeps this branch's Local provider decision (always localhost, no host/user override) over main's generalization -- Local/_runtime.py resolved to our side, and the three tests that only existed to cover main's now-dropped host/user override + api_port-conflict-detection feature (_is_local_host/_port_bound) were removed or trimmed to match. test_global_controller_redis_reuse.py's host/user-override test kept both fields from each side, since neither was actually in conflict -- the test already exercises exactly what it's named for either way. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Unresolved SSH dependency, region-specific AMI, SSH-user, and provider-name issues block reliable deployment.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 1
Open (5)
What changed in this PR
Updates EC2 deployment with automatic AMI/key handling, simplified configuration, improved local behavior, and expanded documentation.
Changes:
- Adds EC2 defaults and SSH key generation.
- Updates runtime, Redis, cleanup, and provider handling.
- Refreshes examples, tests, documentation, and Docker tooling.
| File | Summary |
|---|---|
tests/test_runtime_ec2.py |
Tests EC2 defaults and key handling. |
tests/test_instance_manager_runtime.py |
Updates local runtime tests. |
tests/test_global_controller_redis_reuse.py |
Tests local Redis targeting. |
tests/test_global_controller_cleanup.py |
Tests local cleanup. |
README.md |
Links EC2 documentation. |
examples/portfolio/config/global_controller.yaml |
Updates provider and EC2 configuration. |
examples/portfolio/.env.example |
Adds EC2 variables. |
examples/helloworld/config/global_controller.yaml |
Updates SSH key configuration. |
cli/canyonos/deploy.py |
Improves status output. |
canyonos_core/Dockerfile |
Adds zstd support. |
canyonos_core/controller/utils/redis_client.py |
Prefers IPv4 Redis resolution. |
canyonos_core/controller/instance_manager.py |
Updates local deployment behavior. |
canyonos_core/controller/global_controller.py |
Adjusts provider-specific placement and cleanup. |
canyonos_core/controller/cloud_provider_logic/README.md |
Documents provider backends. |
canyonos_core/controller/cloud_provider_logic/Local/README.md |
Documents local deployment. |
canyonos_core/controller/cloud_provider_logic/Local/_runtime.py |
Simplifies local runtime provisioning. |
canyonos_core/controller/cloud_provider_logic/EC2/README.md |
Adds EC2 setup guidance. |
canyonos_core/controller/cloud_provider_logic/EC2/_runtime.py |
Adds EC2 defaults and key management. |
canyonos_core/cli.py |
Relaxes required EC2 configuration. |
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
- Install openssh-client in the controller image for ssh/ssh-keygen - Stop remote Redis with the same SSH user used to launch it - Restore lowercase provider: local in the portfolio example - Note the default AMI is us-east-1 only; fix comment typo Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
|
After the rebase there is a direct contradiction to resolve: packages/core/canyonos_core/schema/manifest.py still requires ami_id and ssh_user in the ec2: block (_EC2_REQUIRED_KEYS) and defaults ssh_private_key_path to |
| @@ -0,0 +1,6 @@ | |||
| # Local | |||
There was a problem hiding this comment.
Yeah, idk this looks wrong...
I would do ec2.md and local.md or self-host.md
this readme looks not necessary and also, I think we should copy what https://github.com/langfuse/langfuse does
many other OSS repos do like this, e.g: https://github.com/posthog/posthog
| @@ -82,9 +140,9 @@ def provision_instance(spec, replica_index, next_host_port=None): | |||
| "InstanceType": spec["instance_type"], | |||
| "SubnetId": cfg["subnet_id"], | |||
There was a problem hiding this comment.
This now depends on the instance getting a public IP, but run_instances never asks for one. In a subnet with MapPublicIpOnLaunch=false (the default for non-default VPCs) describe_instances returns only PrivateIpAddress, so the loop below exits with a 10.x host and SSH fails about 4 minutes later with "SSH never became ready". Nothing in the error points at the subnet.
To reproduce: deploy with provider: EC2 into a subnet without auto-assign public IP.
Passing NetworkInterfaces=[{DeviceIndex: 0, SubnetId, Groups, AssociatePublicIpAddress: True}] instead of SubnetId/SecurityGroupIds would fix it. Alternatively, fail fast when the subnet has auto-assign off.
| # deploy`) is outside the VPC in the common case, so SSH/build traffic | ||
| # needs the public IP. Kept alongside `host` for callers that want it | ||
| # explicitly regardless of which one `host` ended up being. | ||
| public_host = instance.get("PublicIpAddress") if instance else None |
There was a problem hiding this comment.
With host now being the public IP, redis_host and CANYONOS_AGENT_HOST below also become the public IP. The agent container then reaches the Redis on its own instance via :6379, which leaves through the IGW with the public IP as source. The self-referencing security group rule the README suggests does not match that traffic, so _wait_for_redis in the agent times out.
To reproduce: security group with 6379 and 50051 open only to the group itself, as the README describes, then deploy one EC2 agent. It only works if 6379 is open to 0.0.0.0/0.
The container should get the private IP (or host.docker.internal) for Redis, keeping the public IP for SSH and the external endpoint.
| fingerprint = hashlib.sha256(public_key).hexdigest()[:8] | ||
| key_name = f"canyonos-ec2-{project_id}-{fingerprint}" | ||
| try: | ||
| client.import_key_pair(KeyName=key_name, PublicKeyMaterial=public_key) |
There was a problem hiding this comment.
_aws_clients now runs ssh-keygen -y and ImportKeyPair every time it is called, and it is called from provision_instance, bootstrap_instance and terminate_instance. That is 3 subprocesses and 3 ImportKeyPair calls per replica.
The bigger problem is the error path: bootstrap_instance calls terminate_instance on failure, which calls _aws_clients again. If import_key_pair raises there (UnauthorizedOperation on ImportKeyPair, RequestLimitExceeded with several replicas at once, ssh-keygen missing), the EC2 instance is never terminated and the user sees the key pair error instead of the original bootstrap error.
Memoizing the key pair setup per fingerprint, or doing it once at the start of provision_instance and passing the name along, fixes both.
| ["docker", "rm", "-f", runtime_id], host, user | ||
| ["docker", "rm", "-f", runtime_id], DEFAULT_HOST, None | ||
| ) | ||
| host_port += 1 |
There was a problem hiding this comment.
Removing _port_bound means a conflict on the workflow api_port now lands in this retry loop, which increments the wrong port. docker run fails with "port is already allocated" because of 8080, the loop bumps host_port, and after 50 attempts it raises "no free port found after 50 attempts" without ever mentioning 8080.
To reproduce: bind 8080 with any process, then add a workflow with api_port: 8080 via reload_config (that path does not go through port_in_use in cli/canyonos/deploy.py). Each attempt is a full docker run plus docker rm -f.
The two tests that covered this (test_workflow_bootstrap_fails_fast_on_an_occupied_api_port and test_plain_agent_bootstrap_ignores_api_port_conflicts) were deleted along with the guard. Keeping the guard on api_port only would preserve the fast failure.
| import redis | ||
|
|
||
|
|
||
| def _prefer_ipv4(host): |
There was a problem hiding this comment.
The premise in the docstring does not hold for the pinned redis-py: Connection._connect iterates every getaddrinfo result and falls back to the next one on failure. What actually hurts with host.docker.internal is socket_connect_timeout=None, so the unreachable AAAA blocks until the OS timeout before the IPv4 fallback runs.
Resolving the name once in the constructor also breaks reconnection. redis-py reconnects using self.host, which is now the IP literal, so if the Redis container is recreated with a different IP on the docker network the client stays stuck on the old one with ConnectionRefused until the process restarts.
To reproduce: start the controller against canyonos-redis-localhost, docker rm and recreate the Redis container, watch the client never recover.
Passing socket_connect_timeout to redis.Redis and leaving the hostname intact addresses the original symptom without the pinning.
| replicas = ctrl.get("replicas", 1) | ||
| default_host = ctrl.get("host", "localhost") | ||
| is_ec2 = ctrl.get("provider", "local").upper() == "EC2" | ||
| default_host = ctrl.get("host", "localhost") if is_ec2 else "localhost" |
There was a problem hiding this comment.
host: and user: are now silently ignored for provider: local agents. An existing config with host: 10.0.0.5 / user: ubuntu on a local agent starts the containers and the Redis on localhost with no error or warning; the user only finds out because the agent never appears on the remote machine.
The root README (line 195) still documents multi-host deploys over SSH for local agents.
Rejecting host/user for non-EC2 agents at config validation (or dropping that README paragraph if multi-host local is gone) would make the change visible instead of spreading is_ec2 checks across three methods.
| """Return the `ssh ... target` prefix used to reach a remote host.""" | ||
| ssh_key_path = os.path.expanduser( | ||
| self.config.get("ec2", {}).get("ssh_private_key_path", "~/.ssh/ventis_ec2") | ||
| self.config.get("ec2", {}).get("ssh_private_key_path", "~/.ssh/canyonos_ec2") |
There was a problem hiding this comment.
~/.ssh/canyonos_ec2 is duplicated here and in EC2/_runtime.DEFAULT_SSH_KEY_PATH, and _ssh_args does not go through _ssh_key_path. If the two ever diverge (this PR already had to rename both from ventis_ec2), the runtime generates the key in one path and the controller reads another, and remote docker via _run_cmd fails with "Permission denied (publickey)".
Importing the constant from the runtime here keeps a single source.
|
|
||
| for ctrl in self.controllers: | ||
| user = ctrl.get("user") | ||
| is_ec2 = ctrl.get("provider", "local").upper() == "EC2" |
There was a problem hiding this comment.
ctrl.get("provider", "local").upper() == "EC2" is now repeated in this file at lines 176, 298, 434 and 522, plus canyonos_core/cli.py and instance_manager._provider_runtime. A single helper (is_ec2(ctrl) or a normalized provider accessor) would keep them from drifting when a new provider or casing is added.



Deploying specifically to EC2 does not have good documentation, and the process was outdated. I updated the docs to make it easier for someone to launch their project on EC2.
While doing that, I noticed a bunch of old code that could be simplified/removed entirely, and simplified the EC2 path to reduce user requirements, as well as make the process more seamless.
Additions: