Skip to content
This repository has been archived by the owner on Jan 8, 2024. It is now read-only.

AWS ECS Runner Uninstall Fix #4792

Merged
merged 3 commits into from
Jun 12, 2023
Merged

AWS ECS Runner Uninstall Fix #4792

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c596458
fix: Delete the file system associated with a runner.
paladin-devops Jun 8, 2023
9329438
imp: Use pagination func for searching EFS file systems.
paladin-devops Jun 9, 2023
fc462d2
fix: Return err on DescribeMountTargets failure, during ECS runner un…
paladin-devops Jun 12, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions .changelog/4792.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
```release-note:bug
runneruninstall/aws-ecs: Fix deletion of file system for AWS ECS runner.
```
46 changes: 27 additions & 19 deletions internal/runnerinstall/ecs.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -359,28 +359,28 @@ func (i *ECSRunnerInstaller) Uninstall(ctx context.Context, opts *InstallOpts) e
return err
}

s.Update("Runner uninstalled")
s.Update("Waypoint runner AWS ECS service deleted")
}
s.Done()

// TODO: Still attempt to delete the EFS volume if the ECS service
// uninstall fails
s = sg.Add("Deleting runner file system")
efsSvc := efs.New(sess)
marker := ""
done := false
var fileSystems []*efs.FileSystemDescription
for !done {
fileSystemsResp, err := efsSvc.DescribeFileSystems(&efs.DescribeFileSystemsInput{
Marker: aws.String(marker),
MaxItems: aws.Int64(100),
err = efsSvc.DescribeFileSystemsPages(&efs.DescribeFileSystemsInput{},
func(page *efs.DescribeFileSystemsOutput, lastPage bool) bool {
fileSystems = append(fileSystems, page.FileSystems...)
return !lastPage
})
if err != nil {
return err
}
if marker == *fileSystemsResp.Marker {
done = true
}
fileSystems = append(fileSystems, fileSystemsResp.FileSystems...)
if err != nil {
return err
}

if len(fileSystems) == 0 {
s.Update("No file systems detected, skipping deletion")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because this scenario effectively skips us down to the return way down around line 462, the else is unnecessary if we simply add a return nil here, and it improves readability by reducing the mental stack people will make when reading by 1

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with you here, will update this!

To add a bit of context as to why I did it this way, initially I had both cases of this if statement hitting the same step.Done() at the end of this function, just before the return. I moved that inside the else case, so the step is completed before the return nil is hit when the file system gets deleted, but forgot to apply the same up here.

s.Done()
return nil
}

var fileSystemId *string
Expand All @@ -389,11 +389,20 @@ func (i *ECSRunnerInstaller) Uninstall(ctx context.Context, opts *InstallOpts) e
for _, tag := range fileSystem.Tags {
if *tag.Key == "runner-id" && *tag.Value == opts.Id {
fileSystemId = fileSystem.FileSystemId
// This goto skips to the logic for deleting the file system -
// we know which one we need to delete now, so there's no need
// to iterate through any additional fileSystems
goto DeleteFileSystem
}
}
}

if *fileSystemId == "" {
s.Update("File system with tag key `runner-id` and value " + opts.Id + " not detected, skipping deletion")
s.Done()
return nil
}

DeleteFileSystem:
describeAccessPointsResp, err := efsSvc.DescribeAccessPoints(&efs.DescribeAccessPointsInput{
FileSystemId: fileSystemId,
Expand All @@ -411,19 +420,16 @@ DeleteFileSystem:
describeMountTargetsResp, err := efsSvc.DescribeMountTargets(&efs.DescribeMountTargetsInput{
FileSystemId: fileSystemId,
})
if err != nil {
return err
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we not need to check this error?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We do! I lost this while moving other things around, but fixed up!

for _, mountTarget := range describeMountTargetsResp.MountTargets {
_, err = efsSvc.DeleteMountTarget(&efs.DeleteMountTargetInput{MountTargetId: mountTarget.MountTargetId})
if err != nil {
return err
}
}

ctx, cancel := context.WithTimeout(ctx, 5*time.Minute)
defer cancel()
for {
ctx, cancel := context.WithTimeout(ctx, 5*time.Minute)
defer cancel()
select {
case <-ctx.Done():
return errors.New("after 5 minutes, the file system could" +
Expand All @@ -439,6 +445,8 @@ DeleteFileSystem:
return err
}
// if we reach this point, we're done
s.Update("Runner file system deleted")
s.Done()
return nil
}
}
Expand Down