[Serve] Bound the load balancer port search to the declared range - #10607
Open
roulbac wants to merge 3 commits into
Open
[Serve] Bound the load balancer port search to the declared range#10607roulbac wants to merge 3 commits into
roulbac wants to merge 3 commits into
Conversation
The SkyServe controller task opens ports=['30001-30020'], but the allocator on the controller called find_free_port(30001), which scans up to 65535. Once 20 services were up, service 21 bound a port that was never exposed: it reported READY and was unreachable. serve.controller.load_balancer_port_range (default 30001-30020) is now the single source for both the controller task's ports= and the port search, so the two cannot drift. Exhausting the range raises and names that config key instead of silently binding out of range. find_free_port grows an end_port bound whose default preserves today's behavior for every other caller. A changed range only applies to a newly launched controller; existing controllers keep the ports they opened at launch.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
- An admin policy that rewrites the range was still able to desynchronize the two ends this PR set out to keep in lockstep. impl.up read the range from the ambient pre-policy config while the controller is launched with the policy's mutated config, so a policy-set range would open one range and allocate from another. get_load_balancer_port_range now takes an optional config, and impl.up passes the mutated one it already uploads. - Bounding the search regressed pool launches. _allocate_load_balancer_port runs before the branch that skips load balancer creation for pools, and impl.up does not open the range for pools at all, so an exhausted range refused a pool that needs no port. The bound now applies only to services that actually start a load balancer; for a pool the port stays the placeholder the service row requires.
The pool branch called find_free_port(start) without an end, so it inherited the 65534 default cap. A configured range of 65535-65535 passes validation, and the search span for a pool then ran from 65535 to 65534: empty, raising a bare OSError before a single bind was tried. That failed the launch on the one path the branch exists to exempt from the range, and skipped the error message naming the config key. Ask for the top of the port space explicitly. find_free_port keeps its 65534 default, so behavior is unchanged for every other caller, and the default is now pinned by a test.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The SkyServe controller task opens ports=['30001-30020'], but the
allocator on the controller called find_free_port(30001), which scans up
to 65535. Once 20 services were up, service 21 bound a port that was
never exposed: it reported READY and was unreachable.
serve.controller.load_balancer_port_range (default 30001-30020) is now
the single source for both the controller task's ports= and the port
search, so the two cannot drift. Exhausting the range raises and names
that config key instead of silently binding out of range. find_free_port
grows an end_port bound whose default preserves today's behavior for
every other caller.
A changed range only applies to a newly launched controller; existing
controllers keep the ports they opened at launch.
Tested (run the relevant ones):
bash format.sh/smoke-test(CI) orpytest tests/test_smoke.py(local)/smoke-test -k test_name(CI) orpytest tests/test_smoke.py::test_name(local)/quicktest-core(CI) orpytest tests/smoke_tests/test_backward_compat.py(local)