fix: retain ENIs when orphan cleanup fails

cmd/init.go

@@ -90,12 +90,12 @@ func runInit(cmd *cobra.Command, _ []string) error {
 	logger.Infof(ctx, "platform: %s", plat.Name())
 
 	result, err := plat.ProvisionNetwork(ctx, cfg)
+	recorded := cmp.Or(existing, state)
 	if err != nil {
 		if result != nil && len(result.ENIIDs) > 0 {
-			recorded := cmp.Or(existing, state)
 			recorded.ENIIDs = mergeENIIDs(recorded.ENIIDs, result.ENIIDs)
 			if saveErr := recorded.Save(ctx); saveErr != nil {
-				logger.Error(ctx, saveErr, "save the attached ENIs after the failed provisioning")
+				logger.Error(ctx, saveErr, "save ENIs after failed provisioning")
 			}
 		}
 		return fmt.Errorf("provision network: %w", err)
@@ -106,7 +106,7 @@ func runInit(cmd *cobra.Command, _ []string) error {
 	state.Gateway = result.Gateway
 	state.PrimaryNIC = result.PrimaryNIC
 	state.SecondaryNICs = result.SecondaryNICs
-	state.ENIIDs = result.ENIIDs
+	state.ENIIDs = mergeENIIDs(recorded.ENIIDs, result.ENIIDs)
 	state.IPs = result.IPs
 	state.AliasRangeName = result.AliasRangeName
 	if err := state.Save(ctx); err != nil {

cmd/init_test.go

@@ -0,0 +1,107 @@
+package cmd
+
+import (
+	"fmt"
+	"io"
+	"net/http"
+	"os"
+	"path/filepath"
+	"slices"
+	"strings"
+	"testing"
+
+	"github.com/cocoonstack/cocoon-net/pool"
+)
+
+func TestInitRetainsFailedENICleanupAcrossSuccessfulProvisioning(t *testing.T) {
+	dir := t.TempDir()
+	logPath := filepath.Join(dir, "calls.log")
+	script := `#!/bin/sh
+printf '%s\n' "$*" >> "$VE_TEST_LOG"
+case "$2" in
+  DescribeSecurityGroups) printf '%s\n' '{"Result":{"SecurityGroups":[{"SecurityGroupId":"sg-1"}]}}' ;;
+  DescribeSubnets) printf '%s\n' '{"Result":{"Subnets":[{"SubnetId":"subnet-1","CidrBlock":"10.0.0.0/24"}]}}' ;;
+  DescribeNetworkInterfaces)
+    printf '%s' '{"Result":{"NetworkInterfaceSets":['
+    if [ -f "$VE_TEST_RECOVERED" ]; then
+      sep=''
+      for i in 1 2 3 4 5 6 7; do
+        printf '%s{"NetworkInterfaceId":"eni-%s","Type":"secondary","DeviceId":"i-1"}' "$sep" "$i"
+        sep=','
+      done
+    fi
+    printf '%s\n' ']}}'
+    ;;
+  CreateNetworkInterface) printf '%s\n' '{"Result":{"NetworkInterfaceId":"eni-orphan"}}' ;;
+  AttachNetworkInterface) echo 'attach unavailable' >&2; exit 1 ;;
+  DeleteNetworkInterface) : > "$VE_TEST_RECOVERED"; echo 'delete unavailable' >&2; exit 1 ;;
+  AssignPrivateIpAddresses) printf '{"Result":{"PrivateIpSet":["10.0.0.%s"]}}\n' "${4#eni-}" ;;
+  *) echo 'unexpected API call' >&2; exit 1 ;;
+esac
+`
+	if err := os.WriteFile(filepath.Join(dir, "ve"), []byte(script), 0o755); err != nil {
+		t.Fatal(err)
+	}
+	t.Setenv("PATH", dir)
+	t.Setenv("VE_TEST_LOG", logPath)
+	t.Setenv("VE_TEST_RECOVERED", filepath.Join(dir, "recovered"))
+	t.Setenv("VOLCENGINE_ACCESS_KEY_ID", "test")
+	transport := http.DefaultTransport
+	http.DefaultTransport = initMetadataTransport{}
+	t.Cleanup(func() { http.DefaultTransport = transport })
+
+	stateDir := filepath.Join(dir, "state")
+	prior := &pool.State{StateDir: stateDir, ENIIDs: []string{"eni-deleted"}, IPs: []string{"10.0.0.20"}}
+	if err := prior.Save(t.Context()); err != nil {
+		t.Fatal(err)
+	}
+	args := []string{"--platform", "volcengine", "--node-name", "test", "--subnet", "10.0.0.0/24", "--state-dir", stateDir, "--primary-nic", "cocoon-net-test-missing"}
+	first := newInitCmd()
+	first.SetArgs(args)
+	if err := first.ExecuteContext(t.Context()); err == nil || !strings.Contains(err.Error(), "attach unavailable") || !strings.Contains(err.Error(), "delete orphan ENI eni-orphan") {
+		t.Fatalf("init error = %v, want attach and cleanup failures", err)
+	}
+	failed, err := pool.Load(t.Context(), stateDir)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if !slices.Equal(failed.ENIIDs, []string{"eni-deleted", "eni-orphan"}) || !slices.Equal(failed.IPs, prior.IPs) {
+		t.Fatalf("failed init lost ownership or changed the existing pool: %+v", failed)
+	}
+	calls, err := os.ReadFile(logPath)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if strings.Contains(string(calls), "AssignPrivateIpAddresses") {
+		t.Fatalf("failed ENI was used for IP allocation: %s", calls)
+	}
+
+	retry := newInitCmd()
+	retry.SetArgs(args)
+	if err := retry.ExecuteContext(t.Context()); err == nil || !strings.Contains(err.Error(), "node setup: primary NIC:") {
+		t.Fatalf("retry error = %v, want the deliberately absent NIC after provisioning", err)
+	}
+	provisioned, err := pool.Load(t.Context(), stateDir)
+	if err != nil {
+		t.Fatal(err)
+	}
+	want := []string{"eni-deleted", "eni-orphan", "eni-1", "eni-2", "eni-3", "eni-4", "eni-5", "eni-6", "eni-7"}
+	if !slices.Equal(provisioned.ENIIDs, want) || len(provisioned.IPs) != 7 {
+		t.Fatalf("successful provisioning lost prior cleanup ownership: %+v", provisioned)
+	}
+}
+
+type initMetadataTransport struct{}
+
+func (initMetadataTransport) RoundTrip(req *http.Request) (*http.Response, error) {
+	var body string
+	switch req.URL.String() {
+	case "http://100.96.0.96/latest/meta-data/vpc-id":
+		body = "vpc-1"
+	case "http://100.96.0.96/latest/meta-data/instance-id":
+		body = "i-1"
+	default:
+		return nil, fmt.Errorf("unexpected metadata request: %s", req.URL)
+	}
+	return &http.Response{StatusCode: http.StatusOK, Body: io.NopCloser(strings.NewReader(body)), Header: make(http.Header)}, nil
+}

platform/volcengine/eni.go

@@ -3,6 +3,7 @@ package volcengine
 import (
 	"context"
 	"encoding/json"
+	"errors"
 	"fmt"
 	"slices"
 	"strconv"
@@ -63,7 +64,9 @@ func ensureENIs(ctx context.Context, subnetID, sgID, instanceID, prefix string,
 		eniID := resp.Result.NetworkInterfaceID
 
 		if err := sleepCtx(ctx, createPropagationDelay); err != nil {
-			deleteOrphanENI(ctx, eniID)
+			if cleanupErr := deleteOrphanENI(ctx, eniID); cleanupErr != nil {
+				return append(result, networkInterface{NetworkInterfaceID: eniID}), errors.Join(err, cleanupErr)
+			}
 			return result, err
 		}
 
@@ -73,9 +76,10 @@ func ensureENIs(ctx context.Context, subnetID, sgID, instanceID, prefix string,
 			"--InstanceId", instanceID,
 		)
 		if attachErr != nil {
-			// best-effort delete so one attach failure neither leaks quota nor stalls the pool build
 			logger.Warnf(ctx, "attach ENI %s: %v", eniID, attachErr)
-			deleteOrphanENI(ctx, eniID)
+			if cleanupErr := deleteOrphanENI(ctx, eniID); cleanupErr != nil {
+				return append(result, networkInterface{NetworkInterfaceID: eniID}), errors.Join(attachErr, cleanupErr, ctx.Err())
+			}
 			if ctx.Err() != nil {
 				return result, ctx.Err()
 			}
@@ -91,13 +95,13 @@ func ensureENIs(ctx context.Context, subnetID, sgID, instanceID, prefix string,
 	return result, nil
 }
 
-func deleteOrphanENI(ctx context.Context, eniID string) {
-	logger := log.WithFunc("volcengine.deleteOrphanENI")
+func deleteOrphanENI(ctx context.Context, eniID string) error {
 	delCtx, cancel := context.WithTimeout(context.WithoutCancel(ctx), orphanDeleteTimeout)
 	defer cancel()
 	if _, err := runVe(delCtx, "vpc", "DeleteNetworkInterface", "--NetworkInterfaceId", eniID); err != nil {
-		logger.Warnf(ctx, "delete orphan ENI %s: %v", eniID, err)
+		return fmt.Errorf("delete orphan ENI %s: %w", eniID, err)
 	}
+	return nil
 }
 
 func selectReusableENIs(enis []networkInterface, count int) []networkInterface {

platform/volcengine/eni_test.go

@@ -4,8 +4,10 @@ import (
 	"context"
 	"encoding/json"
 	"errors"
+	"fmt"
 	"os"
 	"path/filepath"
+	"strconv"
 	"strings"
 	"testing"
 	"time"
@@ -95,7 +97,6 @@ esac
 		done <- outcome{result, err}
 	}()
 	waitForFile(t, attached)
-	time.Sleep(50 * time.Millisecond)
 	cancel()
 	got := <-done
 	if !errors.Is(got.err, context.Canceled) {
@@ -112,43 +113,63 @@ esac
 	}
 }
 
-func TestEnsureENIsDeletesTheOrphanWhenTheAttachIsCanceled(t *testing.T) {
-	dir := t.TempDir()
-	logPath := filepath.Join(dir, "calls.log")
-	attached := filepath.Join(dir, "attached")
-	script := `#!/bin/sh
+func TestEnsureENIsRetainsTheOrphanUntilCleanupSucceeds(t *testing.T) {
+	for _, cleanupFails := range []bool{false, true} {
+		t.Run(fmt.Sprintf("cleanup-fails=%t", cleanupFails), func(t *testing.T) {
+			dir := t.TempDir()
+			logPath := filepath.Join(dir, "calls.log")
+			started := filepath.Join(dir, "started")
+			script := `#!/bin/sh
 printf '%s\n' "$*" >> "$VE_TEST_LOG"
-case "$*" in
-  *CreateNetworkInterface*) printf '%s\n' '{"Result":{"NetworkInterfaceId":"eni-orphan"}}' ;;
-  *AttachNetworkInterface*) : > "$VE_TEST_ATTACHED"; sleep 5 ;;
-  *DescribeNetworkInterfaces*) printf '%s\n' '{"Result":{"NetworkInterfaceSets":[]}}' ;;
+case "$2" in
+  CreateNetworkInterface) printf '%s\n' '{"Result":{"NetworkInterfaceId":"eni-orphan"}}' ;;
+  AttachNetworkInterface) : > "$VE_TEST_STARTED"; exec sleep 5 ;;
+  DeleteNetworkInterface)
+    if [ "$VE_TEST_DELETE_FAIL" = true ]; then echo 'delete unavailable' >&2; exit 1; fi
+    ;;
+  DescribeNetworkInterfaces) printf '%s\n' '{"Result":{"NetworkInterfaceSets":[]}}' ;;
 esac
 `
-	if err := os.WriteFile(filepath.Join(dir, "ve"), []byte(script), 0o755); err != nil {
-		t.Fatalf("write ve stub: %v", err)
-	}
-	t.Setenv("PATH", dir+":/bin:/usr/bin")
-	t.Setenv("VE_TEST_LOG", logPath)
-	t.Setenv("VE_TEST_ATTACHED", attached)
-	ctx, cancel := context.WithCancel(t.Context())
-	defer cancel()
+			if err := os.WriteFile(filepath.Join(dir, "ve"), []byte(script), 0o755); err != nil {
+				t.Fatalf("write ve stub: %v", err)
+			}
+			t.Setenv("PATH", dir+":/bin:/usr/bin")
+			t.Setenv("VE_TEST_LOG", logPath)
+			t.Setenv("VE_TEST_STARTED", started)
+			t.Setenv("VE_TEST_DELETE_FAIL", strconv.FormatBool(cleanupFails))
+			ctx, cancel := context.WithCancel(t.Context())
+			defer cancel()
 
-	done := make(chan error, 1)
-	go func() {
-		_, err := ensureENIs(ctx, "subnet-1", "sg-1", "i-1", "cocoon-pool", 1)
-		done <- err
-	}()
-	waitForFile(t, attached)
-	cancel()
-	if err := <-done; !errors.Is(err, context.Canceled) {
-		t.Fatalf("err = %v, want the cancellation", err)
-	}
-	calls, readErr := os.ReadFile(logPath)
-	if readErr != nil {
-		t.Fatalf("read calls: %v", readErr)
-	}
-	if !strings.Contains(string(calls), "DeleteNetworkInterface --NetworkInterfaceId eni-orphan") {
-		t.Fatalf("orphan ENI not deleted after the canceled attach, calls:\n%s", calls)
+			type outcome struct {
+				result []networkInterface
+				err    error
+			}
+			done := make(chan outcome, 1)
+			go func() {
+				result, err := ensureENIs(ctx, "subnet-1", "sg-1", "i-1", "cocoon-pool", 1)
+				done <- outcome{result, err}
+			}()
+			waitForFile(t, started)
+			cancel()
+			got := <-done
+			if !errors.Is(got.err, context.Canceled) {
+				t.Fatalf("err = %v, want the cancellation", got.err)
+			}
+			if cleanupFails {
+				if len(got.result) != 1 || got.result[0].NetworkInterfaceID != "eni-orphan" || !strings.Contains(got.err.Error(), "delete orphan ENI eni-orphan") {
+					t.Fatalf("lost failed cleanup: result=%v err=%v", got.result, got.err)
+				}
+			} else if len(got.result) != 0 {
+				t.Fatalf("deleted orphan remains in result: %v", got.result)
+			}
+			calls, readErr := os.ReadFile(logPath)
+			if readErr != nil {
+				t.Fatalf("read calls: %v", readErr)
+			}
+			if !strings.Contains(string(calls), "DeleteNetworkInterface --NetworkInterfaceId eni-orphan") {
+				t.Fatalf("orphan ENI not deleted after cancellation, calls:\n%s", calls)
+			}
+		})
 	}
 }
 

platform/volcengine/teardown_test.go

@@ -12,35 +12,44 @@ import (
 	"github.com/cocoonstack/cocoon-net/platform"
 )
 
-func TestTeardownDeletesDetachedPersistedENI(t *testing.T) {
+func TestTeardownDeletesDetachedPersistedENIAndIgnoresMissingIDs(t *testing.T) {
 	dir := t.TempDir()
 	logPath := filepath.Join(dir, "calls.log")
 	script := `#!/bin/sh
 printf '%s\n' "$*" >> "$VE_TEST_LOG"
 case "$*" in
   *DescribeNetworkInterfaces*)
-    printf '%s\n' '{"Result":{"NetworkInterfaceSets":[{"NetworkInterfaceId":"eni-detached","Type":"secondary","DeviceId":""}]}}'
+    if [ -f "$VE_TEST_DELETED" ]; then
+      printf '%s\n' '{"Result":{"NetworkInterfaceSets":[]}}'
+    else
+      printf '%s\n' '{"Result":{"NetworkInterfaceSets":[{"NetworkInterfaceId":"eni-detached","Type":"secondary","DeviceId":""}]}}'
+    fi
     ;;
+  *DeleteNetworkInterface*) : > "$VE_TEST_DELETED" ;;
 esac
 `
 	if err := os.WriteFile(filepath.Join(dir, "ve"), []byte(script), 0o755); err != nil {
 		t.Fatalf("write ve stub: %v", err)
 	}
 	t.Setenv("PATH", dir)
 	t.Setenv("VE_TEST_LOG", logPath)
+	t.Setenv("VE_TEST_DELETED", filepath.Join(dir, "deleted"))
 
 	v := &Volcengine{}
-	if err := v.Teardown(t.Context(), &platform.TeardownConfig{ENIIDs: []string{"eni-detached"}}); err != nil {
-		t.Fatalf("teardown: %v", err)
+	for range 2 {
+		if err := v.Teardown(t.Context(), &platform.TeardownConfig{ENIIDs: []string{"eni-deleted", "eni-detached"}}); err != nil {
+			t.Fatalf("teardown: %v", err)
+		}
 	}
 
 	calls, err := os.ReadFile(logPath)
 	if err != nil {
 		t.Fatalf("read calls: %v", err)
 	}
 	want := strings.Join([]string{
-		"vpc DescribeNetworkInterfaces --NetworkInterfaceIds.1 eni-detached --PageSize 100",
+		"vpc DescribeNetworkInterfaces --NetworkInterfaceIds.1 eni-deleted --NetworkInterfaceIds.2 eni-detached --PageSize 100",
 		"vpc DeleteNetworkInterface --NetworkInterfaceId eni-detached",
+		"vpc DescribeNetworkInterfaces --NetworkInterfaceIds.1 eni-deleted --NetworkInterfaceIds.2 eni-detached --PageSize 100",
 	}, "\n")
 	if got := strings.TrimSpace(string(calls)); got != want {
 		t.Fatalf("got calls %q, want %q", got, want)