oxidecomputer / oxidecomputer/omicron
OmicronZoneType variants don't describe all the addresses in use by those zones
Nobody has claimed this yet.
- Dominant language
- Rust
- Stars
- 572
- Forks
- 97
- Avg merge
- 2d 12h
- Merged PRs (30d)
- 96
Description
OmicronZoneType::InternalDns contains two socket addresses because it's effectively running two services (a dropshot server for administration, and a DNS server): https://github.com/oxidecomputer/omicron/blob/f3ab3387b640de27875502209a55211087e9b23d/nexus-sled-agent-shared/src/inventory.rs#L232-L233
We have several other variants that are running multiple services but which only contain a single socket address:
OmicronZoneType::Cockroachhas the address for cockroach itself, but does not describe the address for thecockroach-adminserver running alongside- Similarly,
OmicronZoneType::{Clickhouse,ClickhouseServer,ClickhouseKeeper}have the address for clickhouse itself, but do not describe the address for theclickhouse-adminserver
sled-agent (and DNS, probably?) is forced to combine the IP from one address (e.g., Cockroach.address) with a hard-coded port number (COCKROACH_ADMIN_PORT) in order to configure these supplementary services.
This doesn't seem particularly urgent or terrible, as constant ports are all we use today, but is somewhat inconsistent. #6794 removed OmicronZoneConfig::underlay_address (the parent struct of OmicronZoneType) to remove some duplication; I think there are at least two possible ways to address this issue (one of which would undo #6794, and both of which would require some care to support compatibility across upgrades):
- Add new
SocketAddrV6fields to the relevant variants for their supplementary services - Restore
OmicronZoneConfig::underlay_address, and replace the existingOmicronZoneTypeSocketAddrV6fields with just port numbers (and add port numbers for the missing services while we're there)
These aren't equivalent: the first (which matches how our DNS zone types are described) allows different services in the zone to run on different IPs; the second would require all services to use the same IP. We've sprinkled assumptions that a given Omicron service zone only has one underlay IP throughout the codebase, but I'm not sure whether that was an intentional design choice or something we may want to change in the future.
Contributor guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Start with OmicronZoneType in nexus-sled-agent-shared/src/inventory.rs, especially InternalDns, Cockroach, Clickhouse, ClickhouseServer, and ClickhouseKeeper, then trace how sled-agent derives supplementary service addresses. Compare adding SocketAddrV6 fields with restoring OmicronZoneConfig::underlay_address, including compatibility across upgrades; done means the chosen design describes all service addresses without hard-coded ports.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- rust
- Domain
- backend, networking
- Issue type
- Feature
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Stale
- Clarity
- Mostly clear
- Newbie friendliness
- 35/100