fix(server): configure egress sidecar readiness timeout - #1522
Conversation
github-actions
Bot
added
component/server
size/S
|
@Pangjiping Could you please review this PR when you have time? If the changes look correct and the required checks pass, would you be willing to merge it? I will address any requested changes promptly. 简体中文@Pangjiping 方便时能否帮忙审核一下这个 PR？如果修改没有问题且必要检查通过，麻烦帮忙合并；如果需要调整，我会及时跟进。 |
![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: fea4b024e3
ℹ️ 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".
| timeout_seconds: float = Field( | ||
| default=30.0, | ||
| gt=0, |
There was a problem hiding this comment.
When timeout_seconds is set below one second, which this field explicitly permits, a slow health endpoint can still block for the hard-coded one-second urlopen timeout and then sleep another 0.2 seconds. Consequently, the operation can substantially exceed the documented maximum; cap each probe and sleep to the remaining deadline, or reject values below the polling granularity.
AGENTS.md reference: server/AGENTS.md:L42-L42
Useful? React with 👍 / 👎.
| "(e.g. IPv4-only CNI or experimenting with IPv6 egress despite gaps)." | ||
| ), | ||
| ) | ||
| timeout_seconds: float = Field( |
There was a problem hiding this comment.
Nit (non-blocking): timeout_seconds is a bit generic — the [egress] section may later grow other timeouts (e.g., per-request API timeouts), and this one specifically bounds the readiness wait. Since the PR is not merged yet, renaming is nearly free. Something like ready_timeout_seconds (matches _wait_for_egress_sidecar_ready) or readiness_timeout_seconds (K8s readinessProbe terminology) would make the intent explicit. If renamed, remember to update networking.py, server/configuration.md, both example TOMLs, docker-compose.example.yaml, and the test assertions.
2 participants
Summary
[egress].timeout_secondsso Docker deployments can configure how long the server waits for the egress sidecar health endpoint to become ready.Testing
pytest tests/test_config.py(90 passed)pytest tests/test_docker_service.py(133 passed)ruff check opensandbox_server/config.py opensandbox_server/services/docker/networking.py tests/test_config.py tests/test_docker_service.pyBreaking Changes
Checklist