review: batch-end quality round

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
@@ -54,12 +52,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)

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,22 +94,13 @@ 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")
 		}
 		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)
 	}
 }

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 {

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 {

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()
 

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 {
@@ -26,32 +27,45 @@ 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,
 			"--SecurityGroupIds.1", sgID,
 			"--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 {
 				NetworkInterfaceID string `json:"NetworkInterfaceId"`
 			} `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
+			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
 		}
 
 		_, attachErr := veRun(
@@ -72,13 +86,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) {

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
+}

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 {
@@ -39,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))
 

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)
 	}