review: batch-end quality round - #8
Merged
Merged
Conversation
Drop lease.go add() comments that restate the adjacent condition.
Use cmp.Or for the primaryNIC fallback in place of the manual if-empty check.
Reuse the existing filePerm constant instead of a duplicated literal.
Remove Server.mu and Server.stopped from dhcp/server.go: the flag is written only in the ctx.Done() branch (which returns immediately) and read only in the errCh branch of the same non-looping select, so no execution path can observe its own write, and Run has no retry wrapper. Also drops the now-unused sync import.
A negative --pool-size reaches make([]string, 0, count) with a negative capacity, which panics instead of returning a clean validation error to the caller.
ProvisionNetwork previously created and attached enisPerNode ENIs unconditionally on every call. A retry after a mid-provision failure left the first batch attached and orphaned, then hit the ENI quota and could never succeed. ensureENIs now lists what is already attached, reuses non-primary ENIs, and only creates the shortfall; IP assignment likewise tops up each ENI to ipsPerENI instead of reassigning a full batch.
…e attach An ENI created but not yet attached is invisible to the instance-filtered list that both retry reuse and teardown rely on, so a SIGTERM in the propagation window leaked it and its quota permanently.
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.
Batch-end quality round: style walkthrough, comment budget, simplify lenses, and a loc-justify pass with adversarial verification of every candidate.
Removed
dhcp/server.go:Server.muandServer.stopped(12 lines). The flag is written only in thectx.Done()case body and read only in theerrChcase body of a single non-loopingselect, so no execution can observe its own write.Runis single-shot (srv.Run(ctx)is a direct return incmd/daemon.go, no retry wrapper), andsrv.Close()runs only in the branch that returns immediately, so the post-Close serve error is unreachable. Even when both cases are ready simultaneously andselectpickserrCh,stoppedis false — today's behaviour already returns the wrapped error. The mutex guarded nothing shared: the metrics goroutine reads through the pool/lease locks.Kept, after verification
node/iptables_linux.go:iptEnsure/iptInsert. Merging them is safe but not a simplification: it deduplicates ~6 lines of boilerplate and pays with a positional boolean at all 5 call sites, hiding the append-at-tail vs insert-at-head distinction that the surrounding comment says is correctness-relevant (DROP must precede ACCEPT).Style and comments
cmp.Orreplaces hand-rolled empty-string fallbacks in the GKE and Volcengine provisioners.pool_test.goreuses the package'sfilePermconstant instead of a literal0o644.evictedLeaserationale was condensed to one line rather than dropped, since it carries the WHY (a same-MAC rebind orphans the old IP's route and pool slot).Gates
GOWORK=off make lint(GOOS=linux and darwin): 0 issuesGOWORK=off go test -race -count=1 ./...: all packages passgolangci-lint fmt --diff ./...: cleango.mod/go.sumunmodifiedNet: -20 prod, 0 test.