diff --git a/spinifex/handlers/ecs/eni.go b/spinifex/handlers/ecs/eni.go index 84728bb58..d79882520 100644 --- a/spinifex/handlers/ecs/eni.go +++ b/spinifex/handlers/ecs/eni.go @@ -128,10 +128,11 @@ func eniCmdSubject(instanceID string) string { } // reclaimTaskENI releases an awsvpc task's ENI on the single-writer teardown -// path. Success clears the identity; failure leaves it, so a non-empty ENIID -// always means "still owed" and the stopped-task sweep is what retries it. +// path. Success marks ENIReleased so a second call is a no-op; failure leaves +// it unset, so the stopped-task sweep keeps retrying from it. ENIID itself is +// never cleared: DescribeTasks still needs it for the stopped-task attachment. func (s *Service) reclaimTaskENI(ctx context.Context, accountID string, task *TaskRecord) { - if s.eni == nil || task == nil || task.NetworkMode != NetworkModeAwsvpc || task.ENIID == "" { + if s.eni == nil || task == nil || task.NetworkMode != NetworkModeAwsvpc || task.ENIID == "" || task.ENIReleased { return } if err := s.eni.Release(ctx, accountID, task); err != nil { @@ -139,8 +140,7 @@ func (s *Service) reclaimTaskENI(ctx context.Context, accountID string, task *Ta "task", task.TaskID, "eni", task.ENIID, "err", err) return } - task.ENIID = "" - task.ENIAttachmentID = "" + task.ENIReleased = true } // isENINotFound reports whether err is an idempotent already-gone signal — a diff --git a/spinifex/handlers/ecs/eni_test.go b/spinifex/handlers/ecs/eni_test.go index 06ae50104..dac411ad9 100644 --- a/spinifex/handlers/ecs/eni_test.go +++ b/spinifex/handlers/ecs/eni_test.go @@ -196,7 +196,7 @@ func singleTaskRecord(t *testing.T, svc *Service, cluster string) (TaskRecord, b return rec, found } -func TestReclaimTaskENI_ReleaseSucceeds_ClearsIdentity(t *testing.T) { +func TestReclaimTaskENI_ReleaseSucceeds_MarksReleased(t *testing.T) { svc, _ := newTestService(t) eni := &stubENI{} svc.eni = eni @@ -205,8 +205,13 @@ func TestReclaimTaskENI_ReleaseSucceeds_ClearsIdentity(t *testing.T) { svc.reclaimTaskENI(context.Background(), testAccountID, task) assert.Equal(t, 1, eni.releaseCalls) - assert.Empty(t, task.ENIID, "a successful release clears the identity") - assert.Empty(t, task.ENIAttachmentID) + assert.Equal(t, "eni-1", task.ENIID, "the identity survives release as the forensic record") + assert.Equal(t, "att-1", task.ENIAttachmentID) + assert.True(t, task.ENIReleased, "a successful release marks the task released") + + // A second call is a no-op: no repeat delete. + svc.reclaimTaskENI(context.Background(), testAccountID, task) + assert.Equal(t, 1, eni.releaseCalls, "an already-released record must not be released again") } func TestReclaimTaskENI_ReleaseFails_LeavesIdentity(t *testing.T) { @@ -220,6 +225,7 @@ func TestReclaimTaskENI_ReleaseFails_LeavesIdentity(t *testing.T) { assert.Equal(t, 1, eni.releaseCalls) assert.Equal(t, "eni-1", task.ENIID, "a failed release is still owed, so the identity must survive for the sweep") assert.Equal(t, "att-1", task.ENIAttachmentID) + assert.False(t, task.ENIReleased, "a failed release must not be marked released") } func TestRunTask_Awsvpc_AttachFailure_ReleaseSucceeds_NoRecordPersisted(t *testing.T) { @@ -257,6 +263,7 @@ func TestRunTask_Awsvpc_AttachFailure_ReleaseFails_PersistsStoppedRecord(t *test assert.Equal(t, TaskStatusStopped, rec.LastStatus) assert.Equal(t, TaskStatusStopped, rec.DesiredStatus) assert.Equal(t, "eni-stub", rec.ENIID, "the ENI identity must survive so the sweep owns the retry") + assert.False(t, rec.ENIReleased, "a failed release must not be marked released") assert.False(t, rec.StoppedAt.IsZero()) // The reservation was already given back to the instance. @@ -335,6 +342,32 @@ func TestRunTask_BridgeMode_NoENI(t *testing.T) { assert.Empty(t, out.Tasks[0].Attachments) } +// TestTaskToAWS_StoppedAwsvpc_ReleasedENI_ReportsDeletedAttachment pins the +// regression this fix closes: a STOPPED awsvpc task whose ENI has already been +// released must still carry its ElasticNetworkInterface attachment, with +// Status DELETED rather than an empty attachments list. +func TestTaskToAWS_StoppedAwsvpc_ReleasedENI_ReportsDeletedAttachment(t *testing.T) { + svc, _ := newTestService(t) + rec := &TaskRecord{ + TaskID: "t-1", Cluster: "web", ARN: TaskARN(testRegion, testAccountID, "web", "t-1"), + LastStatus: TaskStatusStopped, DesiredStatus: TaskStatusStopped, + NetworkMode: NetworkModeAwsvpc, + ENIID: "eni-1", + ENIAttachmentID: "att-1", + ENIPrivateIP: "172.31.0.50", + ENIReleased: true, + } + + task := svc.taskToAWS(testAccountID, rec) + + require.Len(t, task.Attachments, 1, "a released ENI must still be reported, not dropped") + att := task.Attachments[0] + assert.Equal(t, "ElasticNetworkInterface", aws.StringValue(att.Type)) + assert.Equal(t, "eni-1", detailValue(att, "networkInterfaceId")) + assert.Equal(t, "172.31.0.50", detailValue(att, "privateIPv4Address")) + assert.Equal(t, "DELETED", aws.StringValue(att.Status)) +} + func TestResolveNetworkMode(t *testing.T) { assert.Equal(t, NetworkModeAwsvpc, resolveNetworkMode(&TaskDefRecord{NetworkMode: "awsvpc"})) assert.Equal(t, NetworkModeAwsvpc, resolveNetworkMode(&TaskDefRecord{NetworkMode: "AWSVPC"})) diff --git a/spinifex/handlers/ecs/records.go b/spinifex/handlers/ecs/records.go index 3499acf5d..c8b384035 100644 --- a/spinifex/handlers/ecs/records.go +++ b/spinifex/handlers/ecs/records.go @@ -405,8 +405,12 @@ type TaskRecord struct { // ENIReleaseAttempts / ENIReleaseNextTry are the stopped-task sweep's retry // state once a release attempt has failed. Both stay zero on the common // path, so an untroubled record serialises exactly as it always has. + // ENIReleased marks that Release has already succeeded for this task's ENI, + // so a second sweep pass is a no-op rather than a repeat delete; ENIID itself + // is left in place afterwards as the forensic record DescribeTasks projects. ENIReleaseAttempts int `json:"eniReleaseAttempts,omitempty"` ENIReleaseNextTry time.Time `json:"eniReleaseNextTry,omitzero"` + ENIReleased bool `json:"eniReleased,omitempty"` // ENIPublicIP / ENIEIPAllocationID hold the auto-assigned Elastic IP for an // awsvpc task whose service has AssignPublicIp=ENABLED. Set on the RUNNING // transition and released on STOPPED. Empty otherwise. diff --git a/spinifex/handlers/ecs/tasks.go b/spinifex/handlers/ecs/tasks.go index f5faa28b0..4de4e0156 100644 --- a/spinifex/handlers/ecs/tasks.go +++ b/spinifex/handlers/ecs/tasks.go @@ -117,7 +117,7 @@ func (s *Service) provisionTaskENI(ctx context.Context, kv jetstream.KeyValue, a attachmentID, err := s.eni.Attach(ctx, accountID, rec.ContainerInstanceID, alloc.ENIID) if err != nil { s.reclaimTaskENI(ctx, accountID, rec) - if rec.ENIID == "" { + if rec.ENIReleased { // Release succeeded, so nothing is owed; RunTask's caller never // persists a failed placement, so clearing here is the end of it. rec.ENIMacAddress, rec.ENIPrivateIP, rec.ENISubnetID = "", "", "" @@ -301,10 +301,14 @@ func (s *Service) taskToAWS(accountID string, r *TaskRecord) *ecs.Task { t.StoppedReason = aws.String(r.StoppedReason) } if r.ENIID != "" { + attStatus := r.LastStatus + if r.ENIReleased { + attStatus = "DELETED" + } att := &ecs.Attachment{ Id: aws.String(r.ENIAttachmentID), Type: aws.String("ElasticNetworkInterface"), - Status: aws.String(r.LastStatus), + Status: aws.String(attStatus), Details: []*ecs.KeyValuePair{ {Name: aws.String("networkInterfaceId"), Value: aws.String(r.ENIID)}, {Name: aws.String("privateIPv4Address"), Value: aws.String(r.ENIPrivateIP)}, diff --git a/spinifex/handlers/ecs/tasks_gc.go b/spinifex/handlers/ecs/tasks_gc.go index d1e96b093..a8b53e916 100644 --- a/spinifex/handlers/ecs/tasks_gc.go +++ b/spinifex/handlers/ecs/tasks_gc.go @@ -49,8 +49,8 @@ func (sc *Scheduler) sweepStoppedTasks(ctx context.Context) (time.Duration, erro } // sweepStoppedBucket deletes STOPPED task records past retention and retries -// the ENI release for any that still owe one. A missing StoppedAt or a -// non-empty ENIID both block deletion; the latter until the release succeeds. +// the ENI release for any that still owe one. A missing StoppedAt or an owed +// (unreleased) ENI both block deletion; the latter until the release succeeds. func (s *Service) sweepStoppedBucket(ctx context.Context, kv jetstream.KeyValue, accountID string, now time.Time, retention time.Duration) (int, time.Duration, error) { keys, err := keysWithPrefix(ctx, kv, "clusters/") if err != nil { @@ -71,7 +71,7 @@ func (s *Service) sweepStoppedBucket(ctx context.Context, kv jetstream.KeyValue, if task.LastStatus != TaskStatusStopped || task.StoppedAt.IsZero() { continue } - if task.ENIID != "" { + if task.ENIID != "" && !task.ENIReleased { due := !task.ENIReleaseNextTry.After(now) switch { case due && retried < eniReleaseRetriesPerPass: @@ -79,7 +79,7 @@ func (s *Service) sweepStoppedBucket(ctx context.Context, kv jetstream.KeyValue, // itself, so only a leftover success needs no further deadline. retried++ s.retryTaskENIRelease(ctx, kv, k, accountID, &task, now) - if task.ENIID != "" && task.ENIReleaseNextTry.After(now) { + if !task.ENIReleased && task.ENIReleaseNextTry.After(now) { next = reconciler.Earliest(next, task.ENIReleaseNextTry.Sub(now)) } case due: @@ -109,7 +109,7 @@ func (s *Service) sweepStoppedBucket(ctx context.Context, kv jetstream.KeyValue, // state, failure bumps the attempt count and backs off the next try. func (s *Service) retryTaskENIRelease(ctx context.Context, kv jetstream.KeyValue, key, accountID string, task *TaskRecord, now time.Time) { s.reclaimTaskENI(ctx, accountID, task) - if task.ENIID == "" { + if task.ENIReleased { task.ENIReleaseAttempts = 0 task.ENIReleaseNextTry = time.Time{} } else { diff --git a/spinifex/handlers/ecs/tasks_gc_test.go b/spinifex/handlers/ecs/tasks_gc_test.go index 7a05d4227..3c34525e8 100644 --- a/spinifex/handlers/ecs/tasks_gc_test.go +++ b/spinifex/handlers/ecs/tasks_gc_test.go @@ -70,8 +70,8 @@ func TestSweepStoppedBucket_RetriesOwedENI_Succeeds(t *testing.T) { found, gerr := getJSON(t.Context(), kv, TaskKey("web", "t-1"), &got) require.NoError(t, gerr) require.True(t, found) - assert.Empty(t, got.ENIID, "a successful retry clears the identity") - assert.Empty(t, got.ENIAttachmentID) + assert.Equal(t, "eni-1", got.ENIID, "the identity survives release as the forensic record") + assert.True(t, got.ENIReleased, "a successful retry marks the task released") assert.Equal(t, 0, got.ENIReleaseAttempts) assert.True(t, got.ENIReleaseNextTry.IsZero()) } @@ -183,6 +183,43 @@ func TestSweepStoppedBucket_CappedOverflow_AsksForSweepInterval(t *testing.T) { "the 2 records left over from the cap must still bring the sweep back soon") } +// TestSweepStoppedBucket_PrunesReleased_KeepsOwed pins the post-fix contract: a +// released ENI is no longer "owed" and is pruned like any other stale STOPPED +// record, while an unreleased one is refused however far past retention it is. +func TestSweepStoppedBucket_PrunesReleased_KeepsOwed(t *testing.T) { + svc, _, kv := serviceTestRig(t) + now := time.Now().UTC() + + released := TaskRecord{ + TaskID: "released", Cluster: "web", ARN: TaskARN(testRegion, testAccountID, "web", "released"), + LastStatus: TaskStatusStopped, DesiredStatus: TaskStatusStopped, + NetworkMode: NetworkModeAwsvpc, ENIID: "eni-released", ENIReleased: true, + StoppedAt: now.Add(-2 * time.Hour), // well past retention + } + require.NoError(t, putJSON(t.Context(), kv, TaskKey("web", "released"), &released)) + + owed := TaskRecord{ + TaskID: "owed", Cluster: "web", ARN: TaskARN(testRegion, testAccountID, "web", "owed"), + LastStatus: TaskStatusStopped, DesiredStatus: TaskStatusStopped, + NetworkMode: NetworkModeAwsvpc, ENIID: "eni-owed", ENIReleased: false, + StoppedAt: now.Add(-2 * time.Hour), // also well past retention, but still owed + } + require.NoError(t, putJSON(t.Context(), kv, TaskKey("web", "owed"), &owed)) + + pruned, _, err := svc.sweepStoppedBucket(t.Context(), kv, testAccountID, now, time.Hour) + require.NoError(t, err) + assert.Equal(t, 1, pruned) + + exists := func(id string) bool { + var rec TaskRecord + found, gerr := getJSON(t.Context(), kv, TaskKey("web", id), &rec) + require.NoError(t, gerr) + return found + } + assert.False(t, exists("released"), "a released ENI is no longer owed, so retention prunes it") + assert.True(t, exists("owed"), "an unreleased ENI is never pruned, however stale") +} + func TestEniReleaseBackoff_DoublesThenCaps(t *testing.T) { assert.Equal(t, eniReleaseBackoffBase, eniReleaseBackoff(1)) assert.Equal(t, 2*eniReleaseBackoffBase, eniReleaseBackoff(2))