Add netSoftnetExpose to the VM spec and refuse exposed-port conflicts on a worker - #484
Add netSoftnetExpose to the VM spec and refuse exposed-port conflicts on a worker#484stratakis wants to merge 3 commits into
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
Here are some automated review suggestions for this pull request.
Reviewed commit: de530fd6e0
ℹ️ 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".
sylvesterkaczmarek
left a comment
sylvesterkaczmarek
left a comment
There was a problem hiding this comment.
The two-level conflict check is the right ownership model: the lagging WorkerInfos view filters obvious conflicts, then the scheduling transaction rechecks current VM state before assignment. Reusing the same external-port conflict helper for scheduled spec edits also keeps API and scheduler semantics aligned, including canonical forms such as 02222 versus 2222. The leading-zero regression is worth retaining because a textual comparison here would be easy to reintroduce.
de530fd to
8ef76a4
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8ef76a41c8
ℹ️ 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".
8ef76a4 to
a7b3fc0
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a7b3fc09b1
ℹ️ 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".
a7b3fc0 to
0ec251c
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0ec251c5f6
ℹ️ 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".
Softnet can forward host ports to a VM, but Orchard had no way to ask for it, so a VM behind Softnet was reachable only through the controller's SSH proxy.
Register the workers with the notifier as well, otherwise every placement waits a second for its worker to connect.
0ec251c to
060665b
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 060665b84d
ℹ️ 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".
060665b to
cf0410d
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cf0410d2c1
ℹ️ 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".
Two VMs exposing the same external port on one worker both reach the running state, and the second silently does not get the port. Endpoint listeners take worker ports too, so they count on both sides, and a scheduled VM's ports cannot change while the worker still has them bound.
cf0410d to
2bf9cec
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2bf9cec9f0
ℹ️ 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".
| return true | ||
| } | ||
|
|
||
| takenPorts[port] = struct{}{} |
There was a problem hiding this comment.
When a scheduled endpoint is still pending in range 2222–2223 and a later Softnet VM requests 2222, this accepts the placement by hypothetically assigning 2223 to the endpoint. The new evidence is that the selected port is recorded only in this local takenPorts map; the actual listener independently scans upward from the range minimum (internal/worker/endpoint/endpoint.go:84-93). If that endpoint starts first, it can therefore bind 2222 and make the later Tart launch fail despite the scheduler having accepted both VMs. Persist the selected port or otherwise make the worker honor the scheduler's reservation.
Useful? React with 👍 / 👎.
| for _, endpoint := range vm.Endpoints { | ||
| portRange := endpoint.WorkerPortRange | ||
|
|
||
| if portRange == nil || portRange.HasFreePort(seenExternalPorts) { | ||
| continue |
There was a problem hiding this comment.
When one VM exposes Softnet port 2222 and declares two endpoints whose ranges are both 2222–2223, each endpoint independently sees 2223 as free and validation succeeds. The scheduler's joint range matching then correctly determines that two listeners cannot share that sole remaining port, so every worker is rejected and the accepted VM remains pending indefinitely. Consume distinct free ports while validating all of the VM's endpoint ranges so this intrinsic specification conflict is rejected by the API.
Useful? React with 👍 / 👎.
2 participants
Softnet can forward host ports to a VM (
tart run --net-softnet-expose), but Orchard had no way to ask for it, so a VM behind Softnet was reachable only through the controller's SSH proxy.netSoftnetExposeon the VM spec, validated the way Softnet parses it, exposed inorchard create vmandorchard get vm.I wrote this before #478 and ran it on an Apple silicon host with Testing Farm's Artemis driver. Now that endpoints exist I am moving to them, so this is here only in case Softnet-level forwarding is worth having on its own and I didn't wanna throw that work away but if you feel it's not worth having it feel free to close it.