Repository navigation
fcvm-fleet: host AMI from ejc3/fcvm main 2c965afe (IPv6 NTP, relay TCP_NODELAY) - #699
Conversation
On the wall's staging page every fcvm tile was drawn dimmed as behind the common time: each fcvm VM's clock was 0.2 to 1.0 s behind the host. The host AMI (bdaa0003) predates ejc3/fcvm#1301, so its fc-agent stepped a restored guest to the whole second, and a routed guest reached no NTP server to correct it (ejc3/fcvm#1343). - us-east-1 host AMI ami-0913aacc80394e1bd -> ami-0084954a5dfe26bf6, built from ejc3/fcvm main 47ec5fcf (fleet/ami/host, revised from the previous image; store under store/fcvm-fleet-host-x86_64-47ec5fcf-20261026/). - us-east-1 tile AMI ami-08f3cc41395353a2f -> ami-07bf015c4588aebc2, revised from the previous image with the host AMI's renderer image (same image id), so both halves draw with one renderer. Checked on a g4dn.metal launched from the new host AMI: store restored from S3 (16 parts, 9.5 s), a routed GPU restore landed within 5 ms of the host and chronyd selected an IPv6 NTP server; a clock set 700 ms back was within 7 ms again 10 s after chronyd's next poll. The tile builder's renderer reached its first frame (image phase). Tested: python3 -S -B scripts/test-fcvm-fleet.py OK, terraform fmt -check.
|
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
Update default GPU host AMI fcvm-fleet.tf |
The default host AMI changes to ami-00d8ecc3edfb8059e. Its description identifies commit 2c965afe and records the listed NTP, relay, and renderer details. |
Priority:
Estimated code review effort: 2 (Simple) | ~5 minutes
Change: Bug fix
Possibly related issues
- ejc3/aws issue 689 requests a GPU host AMI rebuild and fleet host AMI update, which this change addresses.
- ejc3/aws issue 700 describes this PR’s host AMI update and the same IPv6 NTP and IPv4 relay changes.
Possibly related PRs
- ejc3/aws#698: This PR replaces the host AMI introduced by PR
#698with an image built from2c965afe. - ejc3/aws#659: PR
#659introduced the IPv4 slot-port relay path that this change describes as using TCP_NODELAY.
Merge Risk: 
166bd
-
The host AMI update is correctly scoped and presents no identified merge-blocking risk.
Pre-merge checks: 
-
Check name Status Explanation Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title clearly identifies the host AMI update and the key changes in the new image, including the source commit, IPv6 NTP, and relay TCP_NODELAY. Docstring Coverage No eligible functions found in the changed files. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Documentation-only edits and deletions within survi… Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Full details: Docstring Coverage
-
Explanation
No eligible functions found in the changed files. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 0 functions across 0 files. (1 skipped: 1 unsupported.)
-
Finishing Touches
-
🧪 Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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 help to get the list of available commands.
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. |
…m 47ec5fcf #698 moved both AMIs to ejc3/fcvm 31f42d03, which has ejc3/fcvm#1301. 47ec5fcf is 31f42d03 plus ejc3/fcvm#1343 (IPv6 NTP servers for routed guests), which touches no renderer code, so this keeps #698's tile AMI and changes only the host AMI to ami-0084954a5dfe26bf6.
ejc3
changed the title
ami-0084954a5dfe26bf6 (47ec5fcf) -> ami-00d8ecc3edfb8059e (2c965afe), which adds ejc3/fcvm#1350: the relay behind a GPU host's IPv4 slot ports sets TCP_NODELAY, so a frame's second TLS record no longer waits for the browser's delayed ACK. The renderer source is unchanged from 31f42d03, so the tile AMI stays. Checked on a g4dn.metal launched from the new AMI: store restored from S3, a routed GPU restore within 4 to 8 ms of the host for 20 s, chronyd with an IPv6 NTP source.
ejc3
changed the title
|
@codex review |
|
Codex Review: Didn't find any major issues. Keep it up! 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". |
|
@coderabbitai review |
|
@codex security review |
|
@greptileai review |
✅ Action performed
|
🛡️ Codex Security ReviewSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
|
Source CI investigation: fcvm run 40411315700 at 2c965afe has six failed root/container jobs. All six hit the same precondition on both attempts in a_sigkilled_restore_leaves_its_mountpoint_and_runtime_dir_to_the_next_launch: the test expects restore to recreate the source VM directory. The drive overrides introduced by #1318 intentionally avoid creating that directory. Existing fcvm #1351 changes the test to require the clone runtime directory to exist and the source directory to remain absent, while retaining the next-launch runtime cleanup assertion. Its focused test passed; its full CI remains pending. This is an obsolete test assumption, not evidence that runtime cleanup failed. Independent source and security reviews found no blocking defect in the NTP, TCP_NODELAY, or related runtime changes. The new AMI is available, private, fleet-owned, x86_64, and its CloudTrail CreateImage event matches the expected builder and source-tagged image. AWS PR checks and Codex code/security reviews pass on 166bd13. No full-green source-CI claim is being made. |
Host AMI from ejc3/fcvm main 2c965afe: routed GPU guests get IPv6 NTP servers they can reach (ejc3/fcvm#1343), and the relay behind the IPv4 slot ports sets TCP_NODELAY (ejc3/fcvm#1350).
fcvm_fleet_host_amis)fcvm_fleet_tile_amis)git diff 31f42d03 2c965afe -- fleet/rendereris empty, so the tile AMI stays.Why, measured on the staging wall (frames' arrival minus their scene time, per tile):
Built with ejc3/fcvm
fleet/ami/build-ami.sh, revised from ami-0084954a5dfe26bf6: prep, build (fcvm at 2c965afe,make setup-gpu, renderer image, one GPU VM to first frame), bake, store parts understore/fcvm-fleet-host-x86_64-2c965afe-20261026/.Checked on a g4dn.metal launched from ami-00d8ecc3edfb8059e: store restored from S3; a routed GPU restore stayed 4 to 8 ms off the host for 20 s; chronyd had an IPv6 NTP source and its config two IPv6 and two IPv4 servers.
Tested:
python3 -S -B scripts/test-fcvm-fleet.pyOK,terraform fmt -check.Plan: only the host AMI default changes; running hosts keep theirs until the fleet next wakes.
Summary by CodeRabbit