Repository navigation
fix(mesh): stop advertising 0.0.0.0 to peers - #883
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughSplit mesh addressing into bind vs. advertise roles: added Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant CLI as CLI
participant Python as Python Entrypoint
participant Gateway as ModelGateway
participant Mesh as MeshServer
participant Gossip as GossipService
CLI->>Python: pass --mesh-host & --mesh-advertise-host
Python->>Gateway: forward parsed bind_addr & advertise_addr
Gateway->>Mesh: start(bind_addr, advertise_addr, init_peer)
Mesh->>Gossip: new(listen_addr=bind_addr, advertise_addr)
Gossip->>Peers: advertise(advertise_addr) / respond to pings
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
github-actions
Bot
added
documentation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request resolves a critical networking issue where mesh nodes, when configured to bind to the unspecified address Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
2d43365 to
5c699fe
Compare
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new mesh_advertise_host configuration option, allowing users to specify a routable IP address for mesh communication that is distinct from the bind address (mesh_host). This is particularly important when the mesh server binds to an unspecified address like 0.0.0.0. The changes include updates to the Router struct, CLI arguments, configuration parsing, and documentation across Python bindings, Rust crates, and CLI tools. A new validation ensures that the advertised address is always a specified and routable IP. The review comments suggest improving the clarity of error messages related to unspecified advertise addresses.
5c699fe to
e84ab67
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/concepts/architecture/high-availability.md`:
- Around line 171-177: The peer bootstrap addresses are inconsistent: examples
set --mesh-advertise-host to explicit IPs but --mesh-peer-urls still use
hostnames (node1/node2); update the --mesh-peer-urls values to use the
corresponding IP addresses (e.g., replace "node1:39527"/"node2:39527" with the
matching advertise-host IPs and ports) so copy/paste setups without DNS work
reliably, and apply the same change to the other occurrence of the same example.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: c6ba2355-7aab-4728-8909-fbe335efc7a5
📒 Files selected for processing (11)
bindings/python/src/lib.rsbindings/python/src/smg/router_args.pybindings/python/tests/test_arg_parser.pycrates/mesh/src/README.mdcrates/mesh/src/controller.rscrates/mesh/src/ping_server.rscrates/mesh/src/service.rsdocs/concepts/architecture/high-availability.mddocs/reference/configuration.mdmodel_gateway/src/main.rsmodel_gateway/src/server.rs
e84ab67 to
5669e02
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/reference/configuration.md (1)
388-399:⚠️ Potential issue | 🟡 MinorUpdate all HA command examples to include
--mesh-advertise-host.This section was updated, but the later “High-Availability Mesh” examples still omit
--mesh-advertise-host. With the new validation, those example commands can fail when--mesh-hostremains0.0.0.0.📘 Suggested doc patch
# Router 1 smg \ --enable-mesh \ --mesh-server-name router-1 \ + --mesh-advertise-host 192.168.1.10 \ --mesh-port 39527 \ --mesh-peer-urls 192.168.1.11:39527 \ --worker-urls http://worker1:8000 # Router 2 smg \ --enable-mesh \ --mesh-server-name router-2 \ + --mesh-advertise-host 192.168.1.11 \ --mesh-port 39527 \ --mesh-peer-urls 192.168.1.10:39527 \ --worker-urls http://worker2:8000🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/reference/configuration.md` around lines 388 - 399, Update all High-Availability Mesh example commands to include the --mesh-advertise-host flag when --mesh-host is an unspecified bind address (e.g., 0.0.0.0); specifically edit the example command blocks (the Example: block and any "High-Availability Mesh" examples) to add a sensible advertised address (matching the node’s routable IP) using --mesh-advertise-host so they won't fail under the new validation that requires an advertise host when --mesh-host is unspecified.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@bindings/python/src/lib.rs`:
- Around line 1170-1174: The error text produced when
advertise_addr.ip().is_unspecified() uses advertise_field and advertise_host and
currently tells users to "set {advertise_field} to a routable node IP", which is
confusing when mesh_advertise_host is unset and mesh_host is 0.0.0.0; update the
message in that branch to explicitly recommend setting mesh_advertise_host
(e.g., "set mesh_advertise_host to a routable node IP") so users aren’t prompted
to change mesh_host. Locate the conditional around
advertise_addr.ip().is_unspecified() and replace or augment the format string
that references advertise_field/advertise_host to always suggest
mesh_advertise_host as the actionable fix.
In `@docs/concepts/architecture/high-availability.md`:
- Around line 171-179: The Node 2 example command in the high-availability doc
omits the --mesh-host option present in Node 1 and Node 3; update the Node 2 smg
command (the block starting with "smg --enable-mesh" and the mesh flags) to
include --mesh-host 0.0.0.0 so it matches the other node examples and prevents
copy-paste confusion.
- Around line 207-209: Insert a blank line immediately before the fenced code
block that contains the SMG_MESH_* environment variable exports (the block that
starts with the lines "export SMG_MESH_ADVERTISE_HOST=10.0.0.11" etc.); update
the docs/concepts/architecture/high-availability.md so there is an empty line
separating the preceding paragraph and the code fence to satisfy the MD031 rule
and ensure proper Markdown rendering.
In `@model_gateway/src/main.rs`:
- Around line 839-848: In the branch that checks
advertise_addr.ip().is_unspecified() (the code creating a
ConfigError::InvalidValue using advertise_field and advertise_host), change the
error message so it always recommends setting --mesh-advertise-host to a
routable node IP (instead of suggesting --mesh-host when mesh_host is 0.0.0.0);
update the format string passed to ConfigError::InvalidValue to explicitly
mention "--mesh-advertise-host" (you can still include
advertise_field/adverise_host values for context) so users are guided to set the
mesh-advertise-host flag.
- Around line 824-827: The current parsing of mesh_peer_urls swallows parse
errors (let peer = self.mesh_peer_urls.first().and_then(|url|
url.parse::<std::net::SocketAddr>().ok())), which can hide configuration
mistakes; change this to explicitly handle the Result from url.parse for the
peer (e.g., match or map_err) and either log a warning with the invalid URL and
parse error (using the project's logger, e.g., tracing::warn! or processLogger
equivalent) or propagate a configuration error from the surrounding function so
invalid peer URLs are not silently ignored—align this behavior with the Python
binding logic that returns an error on parse failure.
---
Outside diff comments:
In `@docs/reference/configuration.md`:
- Around line 388-399: Update all High-Availability Mesh example commands to
include the --mesh-advertise-host flag when --mesh-host is an unspecified bind
address (e.g., 0.0.0.0); specifically edit the example command blocks (the
Example: block and any "High-Availability Mesh" examples) to add a sensible
advertised address (matching the node’s routable IP) using --mesh-advertise-host
so they won't fail under the new validation that requires an advertise host when
--mesh-host is unspecified.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 23b1eb45-fda3-4749-82db-dfb416993c90
📒 Files selected for processing (11)
bindings/python/src/lib.rsbindings/python/src/smg/router_args.pybindings/python/tests/test_arg_parser.pycrates/mesh/src/README.mdcrates/mesh/src/controller.rscrates/mesh/src/ping_server.rscrates/mesh/src/service.rsdocs/concepts/architecture/high-availability.mddocs/reference/configuration.mdmodel_gateway/src/main.rsmodel_gateway/src/server.rs
fa36843 to
c2b9ef7
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/concepts/architecture/high-availability.md`:
- Around line 188-196: Add the missing explicit --mesh-host 0.0.0.0 flag to the
Node 3 example command so it matches Node 1/Node 2; update the smg invocation
for node3 (the block showing --enable-mesh, --mesh-server-name node3,
--mesh-advertise-host 10.0.0.13, --mesh-port 39527, --mesh-peer-urls
"10.0.0.11:39527,10.0.0.12:39527", --host 0.0.0.0, --port 8000) by inserting
--mesh-host 0.0.0.0 alongside the other mesh flags for consistency.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8dd8ee8d-149f-4ed1-a41a-9f1bf9ae9a1a
📒 Files selected for processing (11)
bindings/python/src/lib.rsbindings/python/src/smg/router_args.pybindings/python/tests/test_arg_parser.pycrates/mesh/src/README.mdcrates/mesh/src/controller.rscrates/mesh/src/ping_server.rscrates/mesh/src/service.rsdocs/concepts/architecture/high-availability.mddocs/reference/configuration.mdmodel_gateway/src/main.rsmodel_gateway/src/server.rs
c2b9ef7 to
ecd31cc
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/concepts/architecture/high-availability.md (1)
411-429:⚠️ Potential issue | 🟡 MinorMissing
POD_IPenvironment variable definition.Line 415 references
$(POD_IP)for--mesh-advertise-host, but onlyPOD_NAMEis defined in theenvsection (lines 420-423). The StatefulSet needs aPOD_IPenvironment variable sourced fromstatus.podIP.📘 Suggested doc fix
env: - name: POD_NAME valueFrom: fieldRef: fieldPath: metadata.name + - name: POD_IP + valueFrom: + fieldRef: + fieldPath: status.podIP ports:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/concepts/architecture/high-availability.md` around lines 411 - 429, The StatefulSet manifest references POD_IP in the container arg --mesh-advertise-host=$(POD_IP) but does not define POD_IP in the env block; add an env entry named POD_IP that sources status.podIP (similar to the existing POD_NAME section) so the container arg can expand correctly; update the env for the container that sets POD_IP using valueFrom.fieldRef.fieldPath=status.podIP to ensure --mesh-advertise-host receives the pod IP at runtime.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@docs/concepts/architecture/high-availability.md`:
- Around line 411-429: The StatefulSet manifest references POD_IP in the
container arg --mesh-advertise-host=$(POD_IP) but does not define POD_IP in the
env block; add an env entry named POD_IP that sources status.podIP (similar to
the existing POD_NAME section) so the container arg can expand correctly; update
the env for the container that sets POD_IP using
valueFrom.fieldRef.fieldPath=status.podIP to ensure --mesh-advertise-host
receives the pod IP at runtime.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 633a7871-360b-4d45-8a29-547743dcaffd
📒 Files selected for processing (11)
bindings/python/src/lib.rsbindings/python/src/smg/router_args.pybindings/python/tests/test_arg_parser.pycrates/mesh/src/README.mdcrates/mesh/src/controller.rscrates/mesh/src/ping_server.rscrates/mesh/src/service.rsdocs/concepts/architecture/high-availability.mddocs/reference/configuration.mdmodel_gateway/src/main.rsmodel_gateway/src/server.rs
|
Hi @jshanson7, this PR has merge conflicts that must be resolved before it can be merged. Please rebase your branch: git fetch origin main
git rebase origin/main
# resolve any conflicts, then:
git push --force-with-lease |
mergify
Bot
added
the
needs-rebase
ecd31cc to
a5a2b71
Compare
mergify
Bot
removed
the
needs-rebase
a5a2b71 to
8240bc0
Compare
Signed-off-by: Jeff Hanson <jeff@thinkingmachines.ai>
8240bc0 to
c5e46d7
Compare
Signed-off-by: Jeff Hanson <jeff@thinkingmachines.ai>
2 participants
Description
Problem
Mesh nodes used the same address for both listening and peer advertisement. When
--mesh-hostwas left at0.0.0.0, gossip could propagate0.0.0.0:<port>back into cluster state and cause peers to dial an unroutable address.Solution
Split mesh bind and advertise addresses. The mesh server now listens on a bind address but advertises a separate routable address to peers. Add
--mesh-advertise-hostto the Rust and Python entrypoints, reject unspecified advertised addresses, and update HA docs/examples accordingly.Changes
--mesh-advertise-hostto the Rust CLI and Python router args0.0.0.0Router(...)constructorTest Plan
cargo +nightly fmt --all --checkcargo clippy --all-targets --all-features -- -D warningscargo test -p smg-mesh test_ping_advertises_configured_address -- --nocapturecargo check -p smg -p smg-python --all-targetspytest -v -s tests/test_arg_parser.pySummary by CodeRabbit
New Features
Bug Fixes / Validation
Documentation
Tests