From 3cc382aace2e7092892c3d091fcaf3c3c8bd3470 Mon Sep 17 00:00:00 2001 From: CMGS Date: Mon, 5 Oct 2026 01:40:25 +0800 Subject: [PATCH 1/3] fix: retain ENIs when orphan cleanup fails --- cmd/init.go | 7 +- cmd/init_test.go | 107 +++++++++++++++++++++++++++ platform/volcengine/eni.go | 21 ++++-- platform/volcengine/eni_test.go | 89 +++++++++++++--------- platform/volcengine/teardown_test.go | 19 +++-- 5 files changed, 192 insertions(+), 51 deletions(-) create mode 100644 cmd/init_test.go diff --git a/cmd/init.go b/cmd/init.go index d549ffa..9c0945b 100644 --- a/cmd/init.go +++ b/cmd/init.go @@ -77,7 +77,6 @@ func runInit(cmd *cobra.Command, _ []string) error { } existing, loadErr := pool.Load(ctx, flagStateDir) if loadErr != nil { - // a first provisioning that fails midway still leaves teardown something to act on if err = state.Save(ctx); err != nil { return fmt.Errorf("save seed pool state: %w", err) } @@ -90,12 +89,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 +105,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 { diff --git a/cmd/init_test.go b/cmd/init_test.go new file mode 100644 index 0000000..e761a01 --- /dev/null +++ b/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 +} diff --git a/platform/volcengine/eni.go b/platform/volcengine/eni.go index 1c0c56e..132cdc6 100644 --- a/platform/volcengine/eni.go +++ b/platform/volcengine/eni.go @@ -3,6 +3,7 @@ package volcengine import ( "context" "encoding/json" + "errors" "fmt" "slices" "strconv" @@ -61,10 +62,13 @@ func ensureENIs(ctx context.Context, subnetID, sgID, instanceID, prefix string, return result, fmt.Errorf("parse create ENI %d response: %w", i, unmarshalErr) } eniID := resp.Result.NetworkInterfaceID + result = append(result, networkInterface{NetworkInterfaceID: eniID}) if err := sleepCtx(ctx, createPropagationDelay); err != nil { - deleteOrphanENI(ctx, eniID) - return result, err + if cleanupErr := deleteOrphanENI(ctx, eniID); cleanupErr != nil { + return result, errors.Join(err, cleanupErr) + } + return result[:len(result)-1], err } _, attachErr := runVe( @@ -73,16 +77,17 @@ 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 result, errors.Join(attachErr, cleanupErr, ctx.Err()) + } + result = result[:len(result)-1] if ctx.Err() != nil { return result, ctx.Err() } continue } - result = append(result, networkInterface{NetworkInterfaceID: eniID}) if err := sleepCtx(ctx, attachPropagationDelay); err != nil { return result, err } @@ -91,13 +96,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 { diff --git a/platform/volcengine/eni_test.go b/platform/volcengine/eni_test.go index 74d850d..f320994 100644 --- a/platform/volcengine/eni_test.go +++ b/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) + } + }) } } diff --git a/platform/volcengine/teardown_test.go b/platform/volcengine/teardown_test.go index bc276fb..6193dcc 100644 --- a/platform/volcengine/teardown_test.go +++ b/platform/volcengine/teardown_test.go @@ -12,15 +12,20 @@ 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 { @@ -28,10 +33,13 @@ esac } 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) @@ -39,8 +47,9 @@ esac 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) From 02eb7a86a94f31e761303c10cf01557c4696231d Mon Sep 17 00:00:00 2001 From: CMGS Date: Mon, 5 Oct 2026 12:55:17 +0800 Subject: [PATCH 2/3] simplify: append the orphan ENI only where its cleanup fails --- platform/volcengine/eni.go | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/platform/volcengine/eni.go b/platform/volcengine/eni.go index 132cdc6..bf795ff 100644 --- a/platform/volcengine/eni.go +++ b/platform/volcengine/eni.go @@ -62,13 +62,12 @@ func ensureENIs(ctx context.Context, subnetID, sgID, instanceID, prefix string, return result, fmt.Errorf("parse create ENI %d response: %w", i, unmarshalErr) } eniID := resp.Result.NetworkInterfaceID - result = append(result, networkInterface{NetworkInterfaceID: eniID}) if err := sleepCtx(ctx, createPropagationDelay); err != nil { if cleanupErr := deleteOrphanENI(ctx, eniID); cleanupErr != nil { - return result, errors.Join(err, cleanupErr) + return append(result, networkInterface{NetworkInterfaceID: eniID}), errors.Join(err, cleanupErr) } - return result[:len(result)-1], err + return result, err } _, attachErr := runVe( @@ -79,15 +78,15 @@ func ensureENIs(ctx context.Context, subnetID, sgID, instanceID, prefix string, if attachErr != nil { logger.Warnf(ctx, "attach ENI %s: %v", eniID, attachErr) if cleanupErr := deleteOrphanENI(ctx, eniID); cleanupErr != nil { - return result, errors.Join(attachErr, cleanupErr, ctx.Err()) + return append(result, networkInterface{NetworkInterfaceID: eniID}), errors.Join(attachErr, cleanupErr, ctx.Err()) } - result = result[:len(result)-1] if ctx.Err() != nil { return result, ctx.Err() } continue } + result = append(result, networkInterface{NetworkInterfaceID: eniID}) if err := sleepCtx(ctx, attachPropagationDelay); err != nil { return result, err } From a484dfd913180b407cf2b5b144374d7bd8548beb Mon Sep 17 00:00:00 2001 From: CMGS Date: Mon, 5 Oct 2026 12:55:24 +0800 Subject: [PATCH 3/3] init: restore the seed-state comment --- cmd/init.go | 1 + 1 file changed, 1 insertion(+) diff --git a/cmd/init.go b/cmd/init.go index 9c0945b..22a0b77 100644 --- a/cmd/init.go +++ b/cmd/init.go @@ -77,6 +77,7 @@ func runInit(cmd *cobra.Command, _ []string) error { } existing, loadErr := pool.Load(ctx, flagStateDir) if loadErr != nil { + // a first provisioning that fails midway still leaves teardown something to act on if err = state.Save(ctx); err != nil { return fmt.Errorf("save seed pool state: %w", err) }