Repository navigation
fleet: publish the host aggregator under agg-<host>-<token> names - #1267
Conversation
A host aggregator's stream was reachable only at the plain URL its agent reported, which an https page cannot open, so AggregateView.frame_url was unusable and frame_url_v4 was always null. Agent (`--aggregate-tls-port <port>`, with --tls-secret): - runs the aggregator with --tls-file (the renderers' tls.json) and --tls-listen on <publish ip>:<port>, and with an IPv6 prefix also on [<prefix>::1]:443. <prefix>::1 is the host's side of the prefix, which fcvm never gives a VM; the agent puts it on the host's default IPv6 interface (ip -6 addr replace ... nodad) before it starts. - reports ipv6, and with --public-ipv4-from-imds ipv4 and v4_port, in the aggregate message (new optional fields). fleet-core: - Host keeps the reported addresses and its published names (aggregate_published). Event::Published for `agg-<host>` applies only to the addresses the host reports now. - aggregate_id(host): `agg-<host>` when the host name fits a DNS label. - canonical_ip and Published::names replace the two copies of the canonical comparison (dns.rs, view::frame_urls). - view: with published names an aggregate's frame_url/frame_url_v4 are its published names, else null; without DNS, the reported URL as before. Coordinator: - dns::facts includes each host aggregator (generation 0: one launch while its addresses stay the same), so its names go through the same mint/write/confirm/publish path and zone as slot names; owned() recognises agg-<host>-<token>[-v4]. The write/confirm loop is unchanged. - The poster reads a host aggregator at its published name when it has one: the reported URL can be an address only that host reaches. Staging is unchanged until an agent is given --aggregate-port and --aggregate-tls-port. Tests: - a_host_aggregator_is_published_under_its_own_names (tests/test_fleet_coord.rs): no URL for a plain aggregator, records written, URLs only after confirmation, a new address hides the old URLs and the old records go. With the view returning the reported URL it fails: "the plain URL reached an https page". - the_agent_runs_and_feeds_its_aggregator now runs the aggregator with TLS, checks the reported ipv4/v4_port, https /healthz and 404 for /layout over TLS. - only_fleet_names_are_owned, labels_are_owned_and_not_reused, a_host_aggregator_is_named_only_when_its_host_name_fits_a_label, a_slot_address_is_the_gpu_index_plus_two_in_the_prefix.
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 54 minutes. View limit details
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 |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
@codex review |
|
Codex Review: Didn't find any major issues. Swish! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
NOT-A-DEFECT: the CodeRabbit comment is a review-limit notice, not a finding. |
|
NOT-A-DEFECT: the CodeRabbit comment is a review-limit notice. The head is a clean automatic merge of the layer below (its tree equals |
…ggregate # Conflicts: # fleet/PROTOCOL.md # tests/test_fleet_coord.rs
Second of three stacked PRs for #1232. A GPU host's aggregator serves browsers with TLS and the coordinator names it like a slot launch, so
AggregateView.frame_url/frame_url_v4are reachable from the https page.Stacked on:
fix/issue-1232(#1266). Next: #1268 (EC2 aggregator names).What changed
Agent,
--aggregate-tls-port <port>(needs--aggregate-portand--tls-secret):--tls-file(the renderers'tls.json) and--tls-listen <publish ip>:<port>(the IPv4 path: one address per host, a port per stream), and with an IPv6 prefix also on[<prefix>::1]:443.<prefix>::1is the host's side of the delegated /80, which fcvm already refuses to give a VM; the agent puts it on the host's default IPv6 interface (ip -6 addr replace <addr>/128 dev <iface> nodad) before it starts. One IPv6 address for the aggregator, as each slot has its own.ipv6, and with--public-ipv4-from-imdsipv4andv4_port, as new optional fields of theaggregatemessage.fleet-core:
Hostkeeps the reported addresses and its published names.Event::Publishedforagg-<host>applies only to the addresses the host reports now.aggregate_id(host):agg-<host>when the host name fits a DNS label (lowercase letters, digits, inner hyphens, at most 40 characters; an EC2 instance id is 19; notec2).canonical_ipandPublished::namesreplace the two copies of the canonical address comparison (dns.rs andview::frame_urls).wss://agg-<host>-<token>.<zone>/frames,wss://agg-<host>-<token>-v4.<zone>:<port>/frames) once confirmed, else null. Without the DNS writer, the reported URL as before.Coordinator:
dns::factsincludes each host aggregator (generation 0: one launch while its addresses stay the same), so its names go through the same mint, write, confirm and publish path and the same zone setting as slot names.owned()recognisesagg-<host>-<token>[-v4]. The write and confirm loop is unchanged.Staging is unchanged until an agent is given
--aggregate-portand--aggregate-tls-port(ejc3/aws#671, switch off).Contract and evidence
a_host_aggregator_is_published_under_its_own_names(tests/test_fleet_coord.rs): an aggregator with no public address gives the page no URL and gets no records; with addresses its AAAA and A records are written, no URL reaches the page before the authoritative check passes, then both names do (the IPv4 one with its port); a new address hides the old URLs at once, publishes new names, and the old records are removed. With the view returning the reported URL it fails:the plain URL reached an https page: {"frame_url":"ws://gpu-a.internal:20300/frames",...}.the_agent_runs_and_feeds_its_aggregatornow runs the aggregator with TLS through the agent: the report carriesipv4andv4_port,https://.../healthzanswers,/layoutover TLS is 404.only_fleet_names_are_owned(agg names in and out, includingdnstest-agg-...),labels_are_owned_and_not_reused,a_host_aggregator_is_named_only_when_its_host_name_fits_a_label,a_slot_address_is_the_gpu_index_plus_two_in_the_prefix(<prefix>::1).Not exercised here: adding the address on a real host (needs root and a delegated prefix); the unit test covers the address, and
add_host_addressfails the agent's start with theiperror if it cannot.Part of #1232.