feat(huntsman)!: Replace endpoint's IP address with general hostname.#401
Conversation
WalkthroughScheduler endpoint addressing now supports validated hosts instead of IP-only values. Configuration separates gRPC binding from advertised scheduler endpoints, while protobuf, gRPC, runtime, storage APIs, MariaDB persistence, and tests use host-plus-port fields. ChangesScheduler host registration
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant SchedulerRuntime
participant GrpcStorageClient
participant StorageRegistrationService
participant MariaDbStorageConnector
SchedulerRuntime->>GrpcStorageClient: register advertised Host and port
GrpcStorageClient->>StorageRegistrationService: send RegisterSchedulerRequest
StorageRegistrationService->>MariaDbStorageConnector: register_scheduler host and port
MariaDbStorageConnector-->>StorageRegistrationService: return SchedulerId
StorageRegistrationService-->>GrpcStorageClient: return registration response
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@components/spider-storage/src/db/mariadb.rs`:
- Around line 719-725: The scheduler schema change is not migrated for existing
databases, leaving the legacy address column incompatible with host-based
queries. Update the migration or initialization flow around the schedulers table
creation to transform existing tables from the address schema to the host
schema, while preserving correct creation for new databases; apply the change
consistently wherever SCHEDULERS_TABLE_NAME is created.
In `@components/spider-utils/src/config.rs`:
- Around line 71-73: Update Config::endpoint to format IPv6 hosts correctly:
when self.host contains a colon and is not already bracketed, surround it with
square brackets before appending the port; leave hostnames and already-bracketed
addresses unchanged. Ensure Endpoint::from_shared receives valid URIs such as
http://[::1]:50052.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: dc12d7bd-a4ab-40e8-93f8-667bbad4c190
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.lockcomponents/spider-proto-rust/src/generated/storage.rsis excluded by!**/generated/**
📒 Files selected for processing (19)
components/spider-core/src/types/scheduler.rscomponents/spider-proto-rust/Cargo.tomlcomponents/spider-proto-rust/src/scheduler_registration.rscomponents/spider-proto-rust/src/unpack/storage.rscomponents/spider-proto/storage/storage.protocomponents/spider-scheduler/src/bin/grpc_server.rscomponents/spider-scheduler/src/config.rscomponents/spider-scheduler/src/core_impl/round_robin/tests.rscomponents/spider-scheduler/src/runtime.rscomponents/spider-scheduler/src/storage_client/grpc.rscomponents/spider-scheduler/src/storage_client/mod.rscomponents/spider-storage/src/db/mariadb.rscomponents/spider-storage/src/db/protocol.rscomponents/spider-storage/src/grpc.rscomponents/spider-storage/src/state/service.rscomponents/spider-storage/src/state/test_utils.rscomponents/spider-storage/tests/mariadb_test.rscomponents/spider-utils/Cargo.tomlcomponents/spider-utils/src/config.rs
| r" | ||
| CREATE TABLE IF NOT EXISTS `{SCHEDULERS_TABLE_NAME}` ( | ||
| `id` BIGINT UNSIGNED NOT NULL AUTO_INCREMENT, | ||
| `ip_address` VARCHAR(45) NOT NULL, | ||
| `host` VARCHAR(255) NOT NULL, | ||
| `port` SMALLINT UNSIGNED NOT NULL, | ||
| `created_at` TIMESTAMP NOT NULL DEFAULT CURRENT_TIMESTAMP, | ||
| PRIMARY KEY (`id`) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check for existing database migration mechanisms.
fd -e sql
fd -t d migrations
rg -i 'ALTER TABLE'Repository: y-scope/spider
Length of output: 152
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## repo files related to migrations\n'
fd -a -t d migrations . || true
fd -a -e sql . || true
printf '\n## search for migration keywords\n'
rg -n -i 'ALTER TABLE|CREATE TABLE IF NOT EXISTS|migrat' . || true
printf '\n## inspect mariadb.rs around scheduler schema and queries\n'
sed -n '680,790p' components/spider-storage/src/db/mariadb.rs
printf '\n## inspect any migration-related files if present\n'
git ls-files | rg 'migrat|schema|sql$'Repository: y-scope/spider
Length of output: 8373
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## scheduler table definitions across repo\n'
rg -n 'schedulers|SCHEDULERS_TABLE_NAME|scheduler' components/spider-storage/src/db/mariadb.rs src/spider/storage/mysql/mysql_stmt.hpp tools/scripts/mariadb/wolf/init_db.py || true
printf '\n## search for upgrade/version/migration docs\n'
rg -n -i 'upgrade|migration|migrate|schema version|breaking change|database' README* docs .github components src tools || true
printf '\n## inspect MySQL init script around scheduler table\n'
sed -n '1,80p' tools/scripts/mariadb/wolf/init_db.py
printf '\n## inspect mysql stmt scheduler creation\n'
sed -n '1,40p' src/spider/storage/mysql/mysql_stmt.hppRepository: y-scope/spider
Length of output: 21596
Add a schedulers table migration
CREATE TABLE IF NOT EXISTS won’t alter existing tables, so upgraded databases will keep the old address schema and the new host queries will fail. Update the schema everywhere this table is created, or document that this release requires a fresh database.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@components/spider-storage/src/db/mariadb.rs` around lines 719 - 725, The
scheduler schema change is not migrated for existing databases, leaving the
legacy address column incompatible with host-based queries. Update the migration
or initialization flow around the schedulers table creation to transform
existing tables from the address schema to the host schema, while preserving
correct creation for new databases; apply the change consistently wherever
SCHEDULERS_TABLE_NAME is created.
| pub fn endpoint(&self) -> Result<Endpoint, tonic::transport::Error> { | ||
| Endpoint::from_shared(format!("http://{}", self.socket_addr())) | ||
| Endpoint::from_shared(format!("http://{}:{}", self.host, self.port)) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Format IPv6 addresses correctly in the endpoint URI.
If self.host contains a raw IPv6 address (e.g., ::1), formatting it directly into the URI will produce an invalid URI string (e.g., http://::1:50052) that will fail to parse. In standard URI formatting, IPv6 addresses must be enclosed in square brackets (e.g., http://[::1]:50052).
Since self.host is a NonEmptyString allowing both hostnames and IPs, you can safely wrap it in brackets if it contains a colon and isn't already bracketed.
🐛 Proposed fix
pub fn endpoint(&self) -> Result<Endpoint, tonic::transport::Error> {
- Endpoint::from_shared(format!("http://{}:{}", self.host, self.port))
+ let host_str = self.host.as_str();
+ let formatted_host = if host_str.contains(':') && !host_str.starts_with('[') {
+ format!("[{}]", host_str)
+ } else {
+ host_str.to_owned()
+ };
+ Endpoint::from_shared(format!("http://{}:{}", formatted_host, self.port))
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| pub fn endpoint(&self) -> Result<Endpoint, tonic::transport::Error> { | |
| Endpoint::from_shared(format!("http://{}", self.socket_addr())) | |
| Endpoint::from_shared(format!("http://{}:{}", self.host, self.port)) | |
| } | |
| pub fn endpoint(&self) -> Result<Endpoint, tonic::transport::Error> { | |
| let host_str = self.host.as_str(); | |
| let formatted_host = if host_str.contains(':') && !host_str.starts_with('[') { | |
| format!("[{}]", host_str) | |
| } else { | |
| host_str.to_owned() | |
| }; | |
| Endpoint::from_shared(format!("http://{}:{}", formatted_host, self.port)) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@components/spider-utils/src/config.rs` around lines 71 - 73, Update
Config::endpoint to format IPv6 hosts correctly: when self.host contains a colon
and is not already bracketed, surround it with square brackets before appending
the port; leave hostnames and already-bracketed addresses unchanged. Ensure
Endpoint::from_shared receives valid URIs such as http://[::1]:50052.
20001020ycx
left a comment
20001020ycx
left a comment
There was a problem hiding this comment.
Before diving into the code details, I wonder what is the new configuration looks like (do we need http attached to url? is this backward compatible with ipv4 or ipv6)? Can you show how this breaking change would affect user's configuration e2e?
Below is a configuration example. All |
Thanks for the explanation, can we also include this in the PR so the reference is more clear (unless this violate any policies you guys have in Spider). Also a few questions:
|
There was a problem hiding this comment.
Thanks for the work and get the PR out so promptly. I don't have too much comment on the actual code itself as most of them are straight update from ip to host, but I do have a few question regarding the failure model in networking:
While reviewing https://github.com/y-scope/spider/pull/401/changes#r3600548223, I am actually wondering about the failure cases. I think it would be best if we have unit test to cover the failure cases unless we can already clearly define the behavior themselves:
- Invalid ipv4/ipv6 IP
- DNS fails to resolve a domain name
I think what I am looking for is the e2e example of how an IP/host is passed through the configs, and our handle when the input are invalid (as I often config them incorrectly at the first few attempts).
| .ip_address | ||
| .parse::<IpAddr>() | ||
| .map_err(|error| invalid_argument(format!("invalid IP address: {error}")))?; | ||
| let host = Host::new(self.host) |
There was a problem hiding this comment.
What if DNS failed to resolve the URL. This failure is often seen in the networking, and we should be able to deal with the gracefully
There was a problem hiding this comment.
If the DNS name is syntactically wrong, then in EndpointConfig::endpoint function, the Endpoint::from_shared will return an error, and an error will be logged in the respecting grpc_server.rs and the binary exits with an error.
If the DNS name is syntactically correct, and not an existing address that is connectable, then the gRPC client's connect will return an error, an error will be logged and the binary exits with an error.
I am not sure if these are graceful enough in your opinion.
There was a problem hiding this comment.
Its good that you are thinking about this, but maybe its my problem that I dont really understand what exactly is this "error" you are talking about.
can you tell me say if I input an invalid ip or user name, what would the user see?
I am okay if you think what you presented to user can clearly show what is the root cause.
There was a problem hiding this comment.
For example, if user set an invalid storage hostname in scheduler config, scheduler will fail with "Failed to parse storage endpoint." or "Failed to connect to storage gRPC service.". It also includes the message from underlying library.
| let ip_address = self | ||
| .ip_address | ||
| .parse::<IpAddr>() | ||
| .map_err(|error| invalid_argument(format!("invalid IP address: {error}")))?; |
There was a problem hiding this comment.
So what if the input is an IP, and the IP is invalid, would deleting the check cause any regression
There was a problem hiding this comment.
There is no good way to tell if it is an invalid IP or just not an IP address. I think the best way is to leave it to the other functions mentioned in #401 (comment) to handle the failure.
There was a problem hiding this comment.
really? isnt that you are doing this in the deleted line?
Say user inputs 256.256.256.256, this is not a valid IP right? We can easily parse it (of course though some lib) and let the user knows
There was a problem hiding this comment.
Previously, it can only be an IPv4, Thus, we know for sure if it is not a valid IPv4 address, it is invalid. The AddrParseError is not specific enough to inform us if it is a invalid IP address out of range, or just something totally does not look like an IP address.
Thanks for the suggestion. I have added the example in the PR description. For your questions:
|
Agreed with all other points except this one, wouldn't this confuse user? |
We can provide The final solution should be a user doc that explains all the configuration, but we don't have time for it now. |
…ost/port for the y-scope#401 config schema. PR y-scope#401 split the scheduler's gRPC listen address (now top-level `host`/`port` on ServerConfig, like the storage service) from the endpoint it advertises to storage during registration (`runtime.advertised_endpoint`). The generator still emitted the pre-y-scope#401 flat `runtime.host`/`runtime.port`, so the scheduler failed to parse its config ("missing field advertised_endpoint", then "missing field host"). Emit both shapes from `scheduler_endpoint`.
3 participants
Description
Note
This PR is a breaking change because it changes configuration.
Background
Spider's storage and scheduler use the same IP address for them to listen on and for other components to connect to them. However, for deployment in docker compose and helm, the network are split and each service's network typically expose a hostname for other service to connect to, which is different from the IP address inside the network where each server bind on.
Solution
This PR resolves #400 by separating IP address to bind on with the connecting hostname.
Hostalias backed byNonEmptyString, representing a DNS name or an IP address.EndpointConfigto accept theHost, so all components useHostto connect to storage and scheduler.ServerConfig.hostandportremain the socket bind address.RuntimeConfig.advertised_endpointidentifies the scheduler endpoint registered with storage.Scheduler Configuration
This PR changes the shape of the scheduler's configuration. Below is an example of the configuration after change.
hostcan be a DNS name, an IPv4 address of an IPv6 address surrounded by bracket (e.g.[::1]).Checklist
breaking change.
Validation performed
Summary by CodeRabbit
New Features
Bug Fixes