Repository navigation
review round and refactor - #28
Merged
Merged
Conversation
The GCE zone and subnetwork URLs end in the segment path.Base returns, the region is the zone without its last dash suffix, and the status timestamp is UTC so RFC3339 prints the same string as the literal layout.
It rebuilt the fixture and recomputed ipsPerENI minus the count itself, so no change to the shortfall arithmetic in ProvisionNetwork could fail it.
…oop on cancel The attach-failure cleanup ran DeleteNetworkInterface on the same context whose cancellation had killed the attach, so a SIGTERM during init left an unattached ENI that teardown's attached-only listing never finds. Both orphan deletes now share the detached, time-bounded path the create arm already used, and a canceled attach returns ctx.Err() instead of reporting the shortfall as "no secondary IPs assigned".
CommandContext kills gcloud or ve, but CombinedOutput waited for the pipe to reach EOF; a helper process that inherited it kept init hanging after the operator's signal.
handleRelease validated the packet outside lifecycleMu and then freed whatever the MAC held at lock time, so a RELEASE/REQUEST pair from one guest could free the lease the REQUEST had just ACKed. The server now stamps every packet when it is read from the socket, records that stamp as the lease's grant time, validates the RELEASE under the lock, and drops a RELEASE received before the REQUEST that holds the lease; one received after it is honored even when the two overlapped in flight.
A missing file is the normal first start; an unreadable one was logged with the same 'starting fresh' warning and every later REQUEST then failed to persist, with nothing pointing at the file.
init and adopt persisted the raw list and the daemon silently dropped the entry it could not parse, so a typo cost the VMs a resolver with no error.
…aken RELEASE The --dns help string now names the IPv4 requirement the flag enforces, so the binary and the configuration page agree.
Both sites built the same six-argument gcloud network-interfaces update and differed only in the error text, which stays at the call sites.
…t and in pool.json ensureENIs appended to its result only after the attach-propagation sleep, so a cancel inside that window returned a result without the ENI it had just attached, ProvisionNetwork discarded the result on error, and the seed pool.json kept an empty eniIDs; the next teardown then fell back to deleting every non-primary ENI on the instance. The ENI is recorded before the wait, ProvisionNetwork returns the attached ids with its error, and init persists them before it fails.
Persisting the partial result rewrote pool.json from the fresh seed even when a working state existed, so a retried init that hit an ENI create failure erased the node's IPs, NICs and gateway. The attached ids are now merged into the loaded state (or the seed on a first run), and ProvisionNetwork carries them with every error raised after the attach.
Only init and adopt removed the guest agent's local alias route and installed the boot cron, so after a restart the cron raced the guest agent's own programming and the route could come back. The daemon now runs the GKE adopt step before node setup; Volcengine has nothing to do, so its platform is not constructed.
Removing the cloud allocation under a live DHCP server left every later lease unroutable with nothing telling the operator; teardown now checks the pidfile the way the daemon does and stops unless --force is passed.
node.Setup failed on the first pool NIC without a link, so one ENI detached out of band crash-looped the daemon and took every VM on the node offline. Setup now uses the NICs that are present, warns per missing one, fails only when none is present, and reports expected versus present through cocoon_net_secondary_nics so the degraded state is visible.
This was referenced Sep 21, 2026
On GCE the VPC MTU is 1460 while cni0 and every veth defaulted to 1500, so guests could emit frames the egress NIC cannot carry and the node relied on the doctor's TCPMSS clamp. Setup now reads the primary NIC MTU, creates or resets cni0 to it and writes it into the conflist; cocoon syncs the TAP to the veth and the hypervisor advertises the TAP MTU to the guest.
Both subcommands bound --pool-size to one package variable, and pflag writes the default into that variable at registration, so building the root command left the last registration's 253 in place and a bare init provisioned a GKE pool of 253 addresses while its help said 140. Each FlagSet now owns its storage and the RunE reads it.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Whole-repo review round, 2026-09-21. Every production file read in full; the DHCP/ENI flow lens and a test lens run by reader agents, every finding adjudicated against the source; Linux-gated paths verified in a linux/arm64 container.
Commits
fdb1a5freview: path.Base, strings.CutLast and time.RFC3339 for three renderings98c1882test: drop the shortfall test that never reached provision.gobd82bc1fix: delete an orphan ENI on a detached context and stop the create loop on cancela075c30fix: teardown keeps the detach errors a cut propagation wait would dropbdfbc69fix: bound the wait on a canceled subprocess's pipe43813dcfix: order a RELEASE against the REQUEST it raced by receive time68a70dafix: name the lease-file outcome the daemon starts fromefe4a1efix: reject a --dns entry that is not an IPv4 address616e237docs: --dns takes IPv4 entries, lease-file startup outcomes, an overtaken RELEASE990c3decut: one nic0 alias update for assign and teardown8daca2dfix: an ENI attached before a cut propagation wait stays in the result and in pool.json16964f5fix: a failed init merges the attached ENIs into the existing pool state50ed1d3fix: the daemon re-applies the GKE guest-agent route fix on every start99794e4fix: teardown refuses while the daemon holds the pidfilee12661ffix: the daemon keeps serving when a secondary NIC is missing3a5a9dereview: drop two err shadows and clean the pidfile pathbf5edddtest: the cut-wait ENI test cancels after the attach instead of racing a deadline0f1371areview: name the local logger in the two single-call warn sitesa1ca4aereview: the gauge's help string already carries the secondary NIC godocad7469cfix: the bridge and the CNI veths follow the primary NIC MTU306cd6fdocs: the conflist mtu follows the primary NIC and invalidates old FC snapshots4656ebbdocs: a snapshot after an MTU change needs the source VM recreated firsta944b1edocs: simplify README navigation and section orderaebaa6efix: init without --pool-size gets its own 140 default, not adopt's 253Findings
platform/volcengine/eni.goinitleaked a detached ENI thatteardownnever sees; a canceled attach was also swallowed intocontinueand reported as a provisioning shortfallctx.Err();TestEnsureENIsDeletesTheOrphanWhenTheAttachIsCanceleddhcp/release.gohandleReleasevalidated the packet outsidelifecycleMuand freed whatever the MAC held at lock time, so a RELEASE/REQUEST pair from one guest could free the lease the REQUEST had just ACKed; a first fix compared handler-start time to grant-commit time, which Codex showed still misorders under goroutine schedulingReadFrom(keeping server4's broadcast reply for an unspecified source); the stamp becomes the lease's grant time, orders a RELEASE against the REQUEST by receive time, and a REQUEST older than the current grant is ignored so the grant never moves backwards; explicit-stamp tests plus a loop test over loopback UDPplatform/volcengine/teardown.goerrors.Join;TestTeardownKeepsTheDetachErrorsACutWaitWouldDropplatform/subprocess.goWaitDelay, so a canceledgcloud/vecall blocked on a grandchild holding the pipeWaitDelay2 s;TestRunSubprocessReturnsWhenAGrandchildHoldsThePipedhcp/server.gocmd/utils.go--dnstypo was persisted silently and VMs got one resolverinit/adoptfailnode/node.gonode.Setup, so the daemon crash-looped and every VM on the node lost DHCP, not only the ones behind the missing NICusableSecondaryNICssets up the NICs that are present, warns per missing NIC and publishescocoon_net_secondary_nics{state=expected|present}; only an all-missing set still fails startup;TestUsableSecondaryNICsKeepsThePresentOnesnode/bridge_linux.go,node/node.gocni0and every veth defaulted to 1500 while the GCE VPC MTU is 1460, so a guest could emit frames the egress NIC cannot carry; the nodes only worked because every runbook runscocoon-check --fixfirst, which installs the mangle FORWARDTCPMSS --clamp-mss-to-pmturule (cocoon #135, the July GCP incident)Setupreads the primary NIC MTU, creates or resetscni0to it and writesmtuinto the conflist; cocoon syncs the TAP to the veth and the hypervisor advertises the TAP MTU to the guest, so no DHCP option is needed;TestCNIConflistCarriesThePrimaryNICMTU,TestLinkMTUReadsTheLoopback,TestSetupBridgeFollowsTheMTU(Linux, underCAP_NET_ADMIN); the docs state that a changed node MTU invalidates Firecracker snapshots captured at the old value, since clone and restore reject a target network MTU that differs from the snapshot's, and that the source VM must be recreated after the daemon restart before the snapshot is captured again (a running VM keeps its old veth and TAP)platform/volcengine/eni.go,provision.go,cmd/init.goProvisionNetworkdiscarded the result on error and the seedpool.jsonkept an emptyeniIDs, so the next teardown fell back to deleting every non-primary ENI on the instanceinitpersists them before failing;TestEnsureENIsKeepsAnAttachedENIWhenTheWaitIsCut(now cancel-driven: the ve stub marks the Attach call, the test cancels after it and fails only when the ENI is neither in the result nor deleted); Codex then showed the first cut of this fix rewrote a working node'spool.jsonfrom the seed on a retriedinit, so the attached ids are merged into the loaded state (mergeENIIDs,TestMergeENIIDsKeepsRecordedAndAddsNew) and every post-attach error carries them (closes #25)cmd/daemon.gostate.jsonsurvived, so the daemon came back without the route fix and VM egress stayed broken until a manualadoptgke.Adoptwith the persisted node name, subnet, gateway and primary NIC beforenode.Setup; the fix is idempotent, so a healthy node sees no changecmd/teardown.goteardowndeleted the pool state and leases while the daemon still served themteardownchecks the pidfile after the dry-run branch and returns "stop it first or pass --force";--forceskips the check;TestTeardownForceFlag,TestCheckExistingPIDRefusesALiveDaemonplatform/volcengine/eni.goreusable-ENI subnetdocs/volcengine.md), no changecmd/utils.go,cmd/init.go,cmd/adopt.go--pool-sizeto one package variable and pflag writes each default into it at registration, so building the root command (init then adopt) left 253 in place and a bare GKEinitprovisioned 253 addresses while its help said 140 (since c25d54c; raised by the owner's independent Codex pass)runInit/runAdoptread it;TestInitAndAdoptKeepTheirOwnPoolSizeDefaultsbuilds the real root command and failed on the previous head withinit pool-size = 253, <nil>, want 140Docs:
--dnsrow and help string, the lease-file startup outcomes, and the overtaken-RELEASE rule indocs/dhcp.md; thecocoon_net_secondary_nicsmetric and the--forceflag indocs/configuration.md; the missing-ENI troubleshooting row indocs/volcengine.md; the daemon's route re-apply indocs/gke.md; teardown's refusal while the daemon runs indocs/dhcp.md; the conflistmturule and the snapshot re-capture note indocs/architecture.mdanddocs/gke.md.Cut-list (owner decided 2026-09-21)
platform/volcengine/utils.gosleepCtx→commonk8s.SleepCtxcocoon-common/k8simports client-go, controller-runtime, clientcmd and record, and this daemon has no k8s.io dependency today; twelve lines do not buy thatplatform/gke/gcloud.go+teardown.go: oneupdateNic0Aliaseshelper for the two identicalnetwork-interfaces update --aliasesinvocationscut:commit, −4 net after the helper)dhcp/lease.goadd()other-MAC eviction branch + therequest.golog armisLeasedToOtherignores expired leases but the pool slot stays used until the sweep, which deletes the lease and frees the slot in one locked transaction), yetTestLeaseStore_AddOtherMACSameIPand…Expiredpin it as the store's own invariant with a stated reason (the sweep must never reclaim the new holder's IP); a cut would have to delete those tests, so it staysLOC (same counting on both ends)
Per commit (prod adds/dels · test adds/dels): review 7/9; test drop 0/27; orphan ENI 12/10 · 38/0; teardown 1/1 · 26/0; WaitDelay 4/0 · 26/0; RELEASE ordering (incl. the receive loop) see the commit stat; lease-file log 7/3;
--dns19/3 · 26/0; docs 1/1; cut 13/17; ENI result 13/6 · 34/0; ENI merge 18/5 · 10/0; route re-apply 6/0; teardown refusal 10/3 · 31/1; missing NIC 49/15 · 19/0; lint follow-up 3/3; cancel-driven test 0/0 · 22/8; logger names 4/2; godoc drop 0/1; MTU 53/24 · 67/0; pool-size default 11/4 · 14/0. Production comment lines end where they started (142).Gates
GOWORK=off make fmt-check,GOWORK=off make lintonGOOS=darwinandGOOS=linux: 0 issues.asl ./...on both GOOS: 0 findings.go test -race -count=1 ./...: green (seven packages).golang:1.27container on306cd6fand again onaebaa6ewith--cap-add=NET_ADMIN:go build ./... && go vet ./... && go test -race -count=1 ./...green (seven packages);TestSetupBridgeFollowsTheMTUruns there and was mutation-checked (without theLinkSetMTUbranch:cni0 mtu = 1500 after setupBridge(1460)).Codex
Round 1 on the committed diff raised the RELEASE-ordering residual (handler-start vs receive time) and three timing-dependent tests; round 2 raised the lost broadcast reply for an unspecified source, a backwards grant on a reordered REQUEST pair, and weak ordering tests. All were folded into their commits. Round 3 on dde9b7a: "No blockers: this diff converges." An independent re-read then raised the WaitDelay success-path regression, an import shadow and a coverage gap, all folded in; round 4 on 616e237: "No blockers: this diff converges." Round 5 on the ENI-persistence fix raised the seed-overwrite regression and the partial-ids gap on later errors; round 6 on 16964f5: "No blockers: this diff converges." Round 7 on bf5eddd (the #23/#26/#27 fixes, the lint follow-up and the cancel-driven test): "No blockers: this diff converges." Round 8 on a1ca4ae (the two review commits): "No blockers: this diff converges." Round 9 on 306cd6f (the MTU fix): "No blockers: this diff converges." Round 10 on 4656ebb (the re-capture wording): "No blockers: this diff converges." Round 11 on aebaa6e (the pool-size default): "No blockers: this diff converges."
Follow-ups
Closes #23, #24, #25, #26, #27.