gsadmin/spinifex
1
0
Fork 0
mirror of https://github.com/mulgadc/spinifex.git synced 2026-10-15 17:11:30 +00:00
Code Issues Packages Projects Releases Wiki Activity

fix(ecs): keep a stopped task's ENI attachment after its release (#1059)

Browse Source 
DescribeTasks returned an empty attachments array for every STOPPED awsvpc task. On env19, 134 of 135 tasks in a cluster reported attachments: [], and only the one PENDING task carried an ElasticNetworkInterface block. On AWS a stopped task keeps its attachment for the retention window, with status DELETED and the networkInterfaceId still readable, because it is part of the forensic record of a task that has stopped.

taskToAWS gates the attachment on ENIID being set, and the ENI release retry had made a successful release clear that field so a non-empty ENIID would mean "still owed" and the sweep could retry from it. The signal was right; carrying it in the emptiness of ENIID was not, because the same field is the projection's only source.

The state moves to an ENIReleased flag of its own. ENIID stays put, so the attachment survives as long as the record does, and the sweep's owed test, its refusal to prune, and the placement rollback all read the flag instead. A released record ages out on retention exactly as it did before the retry landed. The attachment reports DELETED once released rather than echoing the task's own status.

This also restores the evidence path the ENI-leak probe depends on: it follows a stopped task to its ENI, and with no attachment to follow it had been reporting skipped on every run.

Closes mulga-5bnx3.
This commit is contained in:
Josh Siviour
2026-09-17 07:56:45 +10:00
committed by GitHub
parent 0fd6848fbd
commit ead2f2fe6c
6 changed files with 95 additions and 17 deletions
Download Patch File Download Diff File Expand all files Collapse all files
spinifex/handlers/ecs/eni.go
+5 -5
View File
@@ -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
spinifex/handlers/ecs/eni_test.go
+36 -3
View File
@@ -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"}))
spinifex/handlers/ecs/records.go
+4
View File
@@ -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.
spinifex/handlers/ecs/tasks.go
+6 -2
View File
@@ -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)},
spinifex/handlers/ecs/tasks_gc.go
+5 -5
View File
@@ -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 {
spinifex/handlers/ecs/tasks_gc_test.go
+39 -2
View File
@@ -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))