From db2ae143c4ae96581b95cac9fd56a9abc8498055 Mon Sep 17 00:00:00 2001 From: CMGS Date: Thu, 23 Jul 2026 15:03:31 +0800 Subject: [PATCH 1/9] review: comment budget pass Drop lease.go add() comments that restate the adjacent condition. --- dhcp/lease.go | 2 -- 1 file changed, 2 deletions(-) diff --git a/dhcp/lease.go b/dhcp/lease.go index d8f6c70..4162ff6 100644 --- a/dhcp/lease.go +++ b/dhcp/lease.go @@ -54,12 +54,10 @@ func (s *leaseStore) add(mac net.HardwareAddr, ip net.IP, duration time.Duration newIP := ip.To4() var evicted []evictedLease - // Same MAC, different IP — surface the old IP so the caller can clean it up. if prev, ok := s.leases[key]; ok && !prev.IP.Equal(newIP) { evicted = append(evicted, evictedLease{MAC: key, IP: prev.IP}) } - // Other MAC, same IP — drop the conflicting active lease. for k, l := range s.leases { if l.IP.Equal(newIP) && k != key && now.Before(l.Expiry) { delete(s.leases, k) From 6cf4b9d62242116d09fe625aad8e2089663519fc Mon Sep 17 00:00:00 2001 From: CMGS Date: Thu, 23 Jul 2026 15:03:37 +0800 Subject: [PATCH 2/9] review: declaration layout and modern idioms Use cmp.Or for the primaryNIC fallback in place of the manual if-empty check. --- platform/gke/provision.go | 6 ++---- platform/volcengine/provision.go | 6 ++---- 2 files changed, 4 insertions(+), 8 deletions(-) diff --git a/platform/gke/provision.go b/platform/gke/provision.go index f8f51be..8dddeb9 100644 --- a/platform/gke/provision.go +++ b/platform/gke/provision.go @@ -1,6 +1,7 @@ package gke import ( + "cmp" "context" "fmt" "strings" @@ -13,10 +14,7 @@ import ( func (g *GKE) ProvisionNetwork(ctx context.Context, cfg *platform.Config) (*platform.NetworkResult, error) { logger := log.WithFunc("gke.ProvisionNetwork") - primaryNIC := cfg.PrimaryNIC - if primaryNIC == "" { - primaryNIC = platform.DefaultNIC(g.Name()) - } + primaryNIC := cmp.Or(cfg.PrimaryNIC, platform.DefaultNIC(g.Name())) instance, zone, project, subnet, err := fetchMetadata(ctx) if err != nil { diff --git a/platform/volcengine/provision.go b/platform/volcengine/provision.go index 59de891..e636d1a 100644 --- a/platform/volcengine/provision.go +++ b/platform/volcengine/provision.go @@ -1,6 +1,7 @@ package volcengine import ( + "cmp" "context" "fmt" @@ -12,10 +13,7 @@ import ( func (v *Volcengine) ProvisionNetwork(ctx context.Context, cfg *platform.Config) (*platform.NetworkResult, error) { logger := log.WithFunc("volcengine.ProvisionNetwork") - primaryNIC := cfg.PrimaryNIC - if primaryNIC == "" { - primaryNIC = platform.DefaultNIC(v.Name()) - } + primaryNIC := cmp.Or(cfg.PrimaryNIC, platform.DefaultNIC(v.Name())) vpcID, err := fetchMeta(ctx, "/vpc-id") if err != nil { From d9c46160b631b928cbb5d38ea985786fa8427e6f Mon Sep 17 00:00:00 2001 From: CMGS Date: Thu, 23 Jul 2026 15:03:42 +0800 Subject: [PATCH 3/9] review: simplify pass Reuse the existing filePerm constant instead of a duplicated literal. --- pool/pool_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pool/pool_test.go b/pool/pool_test.go index c9e7764..4129df9 100644 --- a/pool/pool_test.go +++ b/pool/pool_test.go @@ -74,7 +74,7 @@ func TestState_SaveAtomicTmpIgnored(t *testing.T) { } tmp := filepath.Join(dir, poolFileName+".tmp") - if err := os.WriteFile(tmp, []byte("{not json"), 0o644); err != nil { + if err := os.WriteFile(tmp, []byte("{not json"), filePerm); err != nil { t.Fatalf("write fake tmp: %v", err) } From def5b9c708e7a4ce057f5f3aeeb416f437f68cc7 Mon Sep 17 00:00:00 2001 From: CMGS Date: Thu, 23 Jul 2026 16:09:34 +0800 Subject: [PATCH 4/9] review: loc-justify cuts 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. --- dhcp/server.go | 12 ------------ 1 file changed, 12 deletions(-) diff --git a/dhcp/server.go b/dhcp/server.go index 86f7343..0837532 100644 --- a/dhcp/server.go +++ b/dhcp/server.go @@ -6,7 +6,6 @@ import ( "context" "fmt" "net" - "sync" "time" "github.com/insomniacslk/dhcp/dhcpv4" @@ -45,8 +44,6 @@ type Server struct { offers *pendingOffers linkIndex int // cached kernel interface index for route operations - mu sync.Mutex - stopped bool } // New creates a DHCP server. IPs are the allocatable pool (excluding gateway). @@ -97,9 +94,6 @@ func (s *Server) Run(ctx context.Context) error { select { case <-ctx.Done(): - s.mu.Lock() - s.stopped = true - s.mu.Unlock() _ = srv.Close() if err := s.leases.save(); err != nil { logger.Error(ctx, err, "persist leases on shutdown") @@ -107,12 +101,6 @@ func (s *Server) Run(ctx context.Context) error { logger.Info(ctx, "DHCP server stopped") return nil case err := <-errCh: - s.mu.Lock() - stopped := s.stopped - s.mu.Unlock() - if stopped { - return nil - } return fmt.Errorf("DHCP server: %w", err) } } From affdeaf16ba09011b2b384acea1233c586ad7a84 Mon Sep 17 00:00:00 2001 From: CMGS Date: Thu, 23 Jul 2026 16:29:18 +0800 Subject: [PATCH 5/9] review: keep the load-bearing WHY the comment pass dropped --- dhcp/lease.go | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/dhcp/lease.go b/dhcp/lease.go index 4162ff6..f10e2cd 100644 --- a/dhcp/lease.go +++ b/dhcp/lease.go @@ -24,9 +24,7 @@ type leaseEntry struct { Expiry string `json:"expiry"` } -// evictedLease describes a lease entry displaced by add(). Same-MAC -// rebind leaves the old IP's route + pool slot orphaned; other-MAC -// conflict is reported for logging. +// evictedLease is a lease displaced by add(): a same-MAC rebind orphans the old IP's route and pool slot, an other-MAC conflict is log-only. type evictedLease struct { MAC string IP net.IP From f6df678acc965a4c95457bbd1a5f2257eede21c6 Mon Sep 17 00:00:00 2001 From: CMGS Date: Thu, 23 Jul 2026 17:06:15 +0800 Subject: [PATCH 6/9] fix(platform): reject negative pool size in SubnetIPs 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. --- platform/ip.go | 4 ++++ platform/ip_test.go | 8 ++++++++ 2 files changed, 12 insertions(+) diff --git a/platform/ip.go b/platform/ip.go index 221f160..aa00a6c 100644 --- a/platform/ip.go +++ b/platform/ip.go @@ -85,6 +85,10 @@ func SubnetIPs(cidr, gateway string, count int) ([]string, error) { return nil, fmt.Errorf("gateway %s collides with network/broadcast of %s", gateway, cidr) } + if count < 0 { + return nil, fmt.Errorf("pool size %d is negative", count) + } + ips := make([]string, 0, count) for addr := prefix.Masked().Addr().Next(); prefix.Contains(addr) && len(ips) < count; addr = addr.Next() { if addr == gwAddr || addr == bcast { diff --git a/platform/ip_test.go b/platform/ip_test.go index e2db9df..1f6747c 100644 --- a/platform/ip_test.go +++ b/platform/ip_test.go @@ -136,6 +136,14 @@ func TestSubnetIPs_CountZero(t *testing.T) { } } +func TestSubnetIPs_NegativeCountIsError(t *testing.T) { + t.Parallel() + + if _, err := SubnetIPs("10.0.0.0/24", "10.0.0.1", -1); err == nil { + t.Fatal("expected error for negative count") + } +} + func TestFirstHostIP(t *testing.T) { t.Parallel() From 99802a8066b0e06dd0cc9c9a83c25a910f421b4b Mon Sep 17 00:00:00 2001 From: CMGS Date: Thu, 23 Jul 2026 17:06:24 +0800 Subject: [PATCH 7/9] fix(volcengine): reuse existing ENIs instead of creating a second batch 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. --- platform/volcengine/eni.go | 41 +++++++++---- platform/volcengine/eni_test.go | 100 +++++++++++++++++++++++++++++++ platform/volcengine/provision.go | 32 ++++++---- 3 files changed, 152 insertions(+), 21 deletions(-) create mode 100644 platform/volcengine/eni_test.go diff --git a/platform/volcengine/eni.go b/platform/volcengine/eni.go index 369dcd6..f106e53 100644 --- a/platform/volcengine/eni.go +++ b/platform/volcengine/eni.go @@ -26,11 +26,19 @@ type networkInterface struct { } `json:"PrivateIpSets"` } -func createAndAttachENIs(ctx context.Context, subnetID, sgID, instanceID, prefix string, count int) ([]string, error) { - logger := log.WithFunc("volcengine.createAndAttachENIs") - eniIDs := make([]string, 0, count) +func ensureENIs(ctx context.Context, subnetID, sgID, instanceID, prefix string, count int) ([]networkInterface, error) { + logger := log.WithFunc("volcengine.ensureENIs") - for i := 1; i <= count; i++ { + existing, err := listENIs(ctx, instanceID) + if err != nil { + return nil, fmt.Errorf("list existing ENIs: %w", err) + } + result := reusableENIs(existing, count) + if len(result) > 0 { + logger.Infof(ctx, "reusing %d existing ENI(s)", len(result)) + } + + for i := len(result) + 1; i <= count; i++ { out, err := veRun( ctx, "vpc", "CreateNetworkInterface", "--SubnetId", subnetID, @@ -38,7 +46,7 @@ func createAndAttachENIs(ctx context.Context, subnetID, sgID, instanceID, prefix "--NetworkInterfaceName", fmt.Sprintf("%s-eni-%d", prefix, i), ) if err != nil { - return nil, fmt.Errorf("create ENI %d: %w", i, err) + return result, fmt.Errorf("create ENI %d: %w", i, err) } var resp struct { Result struct { @@ -46,12 +54,12 @@ func createAndAttachENIs(ctx context.Context, subnetID, sgID, instanceID, prefix } `json:"Result"` } if unmarshalErr := json.Unmarshal(out, &resp); unmarshalErr != nil { - return nil, fmt.Errorf("parse create ENI %d response: %w", i, unmarshalErr) + return result, fmt.Errorf("parse create ENI %d response: %w", i, unmarshalErr) } eniID := resp.Result.NetworkInterfaceID if err := sleepCtx(ctx, createPropagationDelay); err != nil { - return eniIDs, err + return result, err } _, attachErr := veRun( @@ -72,13 +80,26 @@ func createAndAttachENIs(ctx context.Context, subnetID, sgID, instanceID, prefix } if err := sleepCtx(ctx, attachPropagationDelay); err != nil { - return eniIDs, err + return result, err } - eniIDs = append(eniIDs, eniID) + result = append(result, networkInterface{NetworkInterfaceID: eniID}) logger.Infof(ctx, "created and attached ENI %s (%d/%d)", eniID, i, count) } - return eniIDs, nil + return result, nil +} + +func reusableENIs(enis []networkInterface, count int) []networkInterface { + var reusable []networkInterface + for _, e := range enis { + if e.Type != eniTypePrimary { + reusable = append(reusable, e) + } + } + if len(reusable) > count { + reusable = reusable[:count] + } + return reusable } func assignSecondaryIPs(ctx context.Context, eniID string, count int) ([]string, error) { diff --git a/platform/volcengine/eni_test.go b/platform/volcengine/eni_test.go new file mode 100644 index 0000000..d3e51da --- /dev/null +++ b/platform/volcengine/eni_test.go @@ -0,0 +1,100 @@ +package volcengine + +import ( + "encoding/json" + "testing" +) + +const ( + eniList1Primary7Secondary = `{ + "Result": { + "NetworkInterfaceSets": [ + {"NetworkInterfaceId": "eni-primary", "Type": "primary", "PrivateIpSets": {"PrivateIpSet": [{"Primary": true, "PrivateIpAddress": "10.0.0.1"}]}}, + {"NetworkInterfaceId": "eni-1", "Type": "secondary", "PrivateIpSets": {"PrivateIpSet": [{"Primary": false, "PrivateIpAddress": "10.0.0.2"}]}}, + {"NetworkInterfaceId": "eni-2", "Type": "secondary", "PrivateIpSets": {"PrivateIpSet": [{"Primary": false, "PrivateIpAddress": "10.0.0.3"}]}}, + {"NetworkInterfaceId": "eni-3", "Type": "secondary", "PrivateIpSets": {"PrivateIpSet": [{"Primary": false, "PrivateIpAddress": "10.0.0.4"}]}}, + {"NetworkInterfaceId": "eni-4", "Type": "secondary", "PrivateIpSets": {"PrivateIpSet": [{"Primary": false, "PrivateIpAddress": "10.0.0.5"}]}}, + {"NetworkInterfaceId": "eni-5", "Type": "secondary", "PrivateIpSets": {"PrivateIpSet": [{"Primary": false, "PrivateIpAddress": "10.0.0.6"}]}}, + {"NetworkInterfaceId": "eni-6", "Type": "secondary", "PrivateIpSets": {"PrivateIpSet": [{"Primary": false, "PrivateIpAddress": "10.0.0.7"}]}}, + {"NetworkInterfaceId": "eni-7", "Type": "secondary", "PrivateIpSets": {"PrivateIpSet": [{"Primary": false, "PrivateIpAddress": "10.0.0.8"}]}} + ] + } +}` + + eniList1Primary3Secondary = `{ + "Result": { + "NetworkInterfaceSets": [ + {"NetworkInterfaceId": "eni-primary", "Type": "primary", "PrivateIpSets": {"PrivateIpSet": [{"Primary": true, "PrivateIpAddress": "10.0.0.1"}]}}, + {"NetworkInterfaceId": "eni-1", "Type": "secondary", "PrivateIpSets": {"PrivateIpSet": [{"Primary": false, "PrivateIpAddress": "10.0.0.2"}]}}, + {"NetworkInterfaceId": "eni-2", "Type": "secondary", "PrivateIpSets": {"PrivateIpSet": [{"Primary": false, "PrivateIpAddress": "10.0.0.3"}]}}, + {"NetworkInterfaceId": "eni-3", "Type": "secondary", "PrivateIpSets": {"PrivateIpSet": [{"Primary": false, "PrivateIpAddress": "10.0.0.4"}]}} + ] + } +}` +) + +func TestReusableENIs(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + fixture string + count int + want int + }{ + {"exact match runs zero shortfall", eniList1Primary7Secondary, 7, 7}, + {"fewer than count returns all", eniList1Primary3Secondary, 7, 3}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + enis := unmarshalENIList(t, tt.fixture) + if got := len(reusableENIs(enis, tt.count)); got != tt.want { + t.Errorf("got %d reusable ENIs, want %d", got, tt.want) + } + }) + } +} + +func TestIPShortfall_FullENIIsZero(t *testing.T) { + t.Parallel() + + eni := newENIWithSecondaryIPs("eni-full", ipsPerENI) + + var existing int + for _, pip := range eni.PrivateIPSets.PrivateIPSet { + if !pip.Primary { + existing++ + } + } + if shortfall := ipsPerENI - existing; shortfall != 0 { + t.Errorf("got shortfall %d, want 0", shortfall) + } +} + +func unmarshalENIList(t *testing.T, fixture string) []networkInterface { + t.Helper() + + var resp struct { + Result struct { + NetworkInterfaceSets []networkInterface `json:"NetworkInterfaceSets"` + } `json:"Result"` + } + if err := json.Unmarshal([]byte(fixture), &resp); err != nil { + t.Fatalf("unmarshal fixture: %v", err) + } + return resp.Result.NetworkInterfaceSets +} + +func newENIWithSecondaryIPs(id string, n int) networkInterface { + eni := networkInterface{NetworkInterfaceID: id, Type: "secondary"} + for range n { + eni.PrivateIPSets.PrivateIPSet = append(eni.PrivateIPSets.PrivateIPSet, struct { + Primary bool `json:"Primary"` + PrivateIPAddress string `json:"PrivateIpAddress"` + }{PrivateIPAddress: "10.0.1.1"}) + } + return eni +} diff --git a/platform/volcengine/provision.go b/platform/volcengine/provision.go index e636d1a..e3977f5 100644 --- a/platform/volcengine/provision.go +++ b/platform/volcengine/provision.go @@ -37,24 +37,34 @@ func (v *Volcengine) ProvisionNetwork(ctx context.Context, cfg *platform.Config) } logger.Infof(ctx, "instance id: %s", instanceID) - eniIDs, err := createAndAttachENIs(ctx, subnetID, sgID, instanceID, cfg.NodeName, enisPerNode) + enis, err := ensureENIs(ctx, subnetID, sgID, instanceID, cfg.NodeName, enisPerNode) if err != nil { - return nil, fmt.Errorf("create/attach ENIs: %w", err) + return nil, fmt.Errorf("ensure ENIs: %w", err) } - logger.Infof(ctx, "attached %d ENIs", len(eniIDs)) + logger.Infof(ctx, "%d ENIs ready", len(enis)) var allIPs []string - for _, eniID := range eniIDs { - ips, assignErr := assignSecondaryIPs(ctx, eniID, ipsPerENI) - if assignErr != nil { - // One ENI's failure is tolerable; the len(allIPs)==0 guard below aborts only if every ENI failed. - logger.Warnf(ctx, "assign secondary IPs to %s: %v", eniID, assignErr) - continue + for _, eni := range enis { + var existing []string + for _, pip := range eni.PrivateIPSets.PrivateIPSet { + if !pip.Primary { + existing = append(existing, pip.PrivateIPAddress) + } + } + allIPs = append(allIPs, existing...) + + if shortfall := ipsPerENI - len(existing); shortfall > 0 { + ips, assignErr := assignSecondaryIPs(ctx, eni.NetworkInterfaceID, shortfall) + if assignErr != nil { + // One ENI's failure is tolerable; the len(allIPs)==0 guard below aborts only if every ENI failed. + logger.Warnf(ctx, "assign secondary IPs to %s: %v", eni.NetworkInterfaceID, assignErr) + continue + } + allIPs = append(allIPs, ips...) } - allIPs = append(allIPs, ips...) } if len(allIPs) == 0 { - return nil, fmt.Errorf("no secondary IPs assigned across %d ENIs", len(eniIDs)) + return nil, fmt.Errorf("no secondary IPs assigned across %d ENIs", len(enis)) } logger.Infof(ctx, "assigned %d secondary IPs", len(allIPs)) From a42eaa8d65218b682509100f2389d16af9888e12 Mon Sep 17 00:00:00 2001 From: CMGS Date: Thu, 23 Jul 2026 18:04:24 +0800 Subject: [PATCH 8/9] fix(volcengine): delete the created ENI when cancellation lands before 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. --- platform/volcengine/eni.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/platform/volcengine/eni.go b/platform/volcengine/eni.go index f106e53..9bafb29 100644 --- a/platform/volcengine/eni.go +++ b/platform/volcengine/eni.go @@ -59,6 +59,9 @@ func ensureENIs(ctx context.Context, subnetID, sgID, instanceID, prefix string, eniID := resp.Result.NetworkInterfaceID if err := sleepCtx(ctx, createPropagationDelay); err != nil { + if _, delErr := veRun(context.WithoutCancel(ctx), "vpc", "DeleteNetworkInterface", "--NetworkInterfaceId", eniID); delErr != nil { + logger.Warnf(ctx, "delete orphan ENI %s: %v", eniID, delErr) + } return result, err } From 9e1ff001c75b4cf892988fdde81fb1150c11d7aa Mon Sep 17 00:00:00 2001 From: CMGS Date: Thu, 23 Jul 2026 18:16:15 +0800 Subject: [PATCH 9/9] fix(volcengine): bound the orphan-ENI cleanup so shutdown cannot hang on it --- platform/volcengine/eni.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/platform/volcengine/eni.go b/platform/volcengine/eni.go index 9bafb29..6f28063 100644 --- a/platform/volcengine/eni.go +++ b/platform/volcengine/eni.go @@ -13,6 +13,7 @@ import ( const ( createPropagationDelay = 2 * time.Second attachPropagationDelay = 4 * time.Second + orphanDeleteTimeout = 15 * time.Second ) type networkInterface struct { @@ -59,9 +60,11 @@ func ensureENIs(ctx context.Context, subnetID, sgID, instanceID, prefix string, eniID := resp.Result.NetworkInterfaceID if err := sleepCtx(ctx, createPropagationDelay); err != nil { - if _, delErr := veRun(context.WithoutCancel(ctx), "vpc", "DeleteNetworkInterface", "--NetworkInterfaceId", eniID); delErr != nil { + delCtx, cancel := context.WithTimeout(context.WithoutCancel(ctx), orphanDeleteTimeout) + if _, delErr := veRun(delCtx, "vpc", "DeleteNetworkInterface", "--NetworkInterfaceId", eniID); delErr != nil { logger.Warnf(ctx, "delete orphan ENI %s: %v", eniID, delErr) } + cancel() return result, err }