feat(orion-scheduler): support multi-VM keyed by server_ws domain#2146
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
mega/orion-scheduler/src/handlers.rs
Line 323 in b79c31b
Use collision-resistant VM IDs
With multi-VM support, this seconds-only ID can be generated for multiple different domains started in the same second. Those VMs are stored under different domain keys but later /vms/{id}, get_machine_by_id, and domain_for_vm_id search by id and return the first matching map entry, so polling or shutdown by the returned vm_id can target the wrong VM. Include a nonce/UUID or otherwise guarantee uniqueness across concurrent starts.
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| // Conflict / idempotency checks (hold update lock briefly). | ||
| { | ||
| let _guard = state.lock_update().await; | ||
| if let Some(existing) = state.get_vm_by_domain(&domain).await { |
There was a problem hiding this comment.
Holding the update lock only for these read checks leaves state unchanged until the background handle_update later reacquires the lock and calls set_vm_provisioning. If two same-domain POSTs arrive in that window, both return 202 instead of the second seeing Provisioning; the later background task then unconditionally shuts down the first VM despite replace=false. The same window lets two new domains both pass a max_vms check before either is counted, so admission needs to reserve/recheck the slot atomically with the decision.
Useful? React with 👍 / 👎.
| match tokio::process::Command::new("kill") | ||
| .args(["-9", &pid.to_string()]) | ||
| .output() |
There was a problem hiding this comment.
When a stale qemu.pid remains after qemu exits, Linux can reuse that numeric PID for an unrelated process before the scheduler restarts. This path only checks that /proc/{pid} exists and then sends kill -9, so startup or /shutdown/all can kill a non-QEMU process owned by the service user. Check /proc/{pid}/cmdline (or equivalent) for qemu-system-* before killing the recorded PID.
Useful? React with 👍 / 👎.
Allow concurrent runners for different domains with same-domain idempotency/409/replace, and wire mono client + OC Start Runner through the updated scheduler APIs and docs.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 41a8f97d7a
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| vms.values() | ||
| .find(|e| e.info.id == id) |
There was a problem hiding this comment.
With multi-VM support this lookup is now used by /vms/{id} and shutdown-by-id, but vm_id is still generated as orion-vm-{chrono_lite_timestamp()} with only second precision in webhook_post_handler. If two different domains are accepted in the same second, both map entries have the same id and this values().find(...) returns whichever HashMap entry it encounters first, so clients can poll or shut down the wrong VM. Please make the generated id unique per request, or include the domain in id-based lookups.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7169776615
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| pub async fn vm_count(&self) -> usize { | ||
| self.vms.read().await.len() |
There was a problem hiding this comment.
When a provisioning attempt fails, set_vm_failed keeps a Failed entry without a machine, but vm_count counts every map entry. With max_vms configured, failed domains can consume all slots; subsequent requests for different domains hit the max_vms check in webhook_post_handler and get 503 even though no VM is running. Count only Running/Provisioning entries or remove failed tombstones before enforcing capacity.
Useful? React with 👍 / 👎.
|
@genedna 可以合并 |
2 participants
Allow concurrent runners for different domains with same-domain idempotency/409/replace, and wire mono client + OC Start Runner through the updated scheduler APIs and docs.