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
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
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.
```
124 changes: 71 additions & 53 deletions internal/runnerinstall/ecs.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,3 @@
// Copyright (c) HashiCorp, Inc.
// SPDX-License-Identifier: MPL-2.0

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we need these

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure how I managed to remove those - added back.

package runnerinstall

import (
Expand Down Expand Up @@ -359,89 +356,110 @@ 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),
req := &efs.DescribeFileSystemsInput{
Copy link
Contributor

Choose a reason for hiding this comment

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

We could possibly reduce this function by using DescribeFileSystemsPages to handle the pagination logic for us, so something like this:

var fileSystems []*efs.FileSystemDescription
err := client.DescribeFileSystemsPages(
  &efs.DescribeFileSystemsInput{}, // don't need to set marker. MaxItems also defaults to 100
  func(page *efs.DescribeFileSystemsOutput, lastPage bool) bool {
		fileSystems = append(fileSystems, page.FileSystems...)
		
    // double check that lastPage is the API telling us it's the last page though
    return lastPage 
})

MaxItems: aws.Int64(100),
})
}
if marker != "" {
req.Marker = aws.String(marker)
}
fileSystemsResp, err := efsSvc.DescribeFileSystems(req)
if err != nil {
return err
}
if marker == *fileSystemsResp.Marker {

if fileSystemsResp.NextMarker == nil {
done = true
} else {
marker = *fileSystemsResp.NextMarker
time.Sleep(5 * time.Second)
Copy link
Contributor

Choose a reason for hiding this comment

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

We probably don't need the sleep here since we are not waiting for an installation or otherwise status change

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 added this sleep because during testing, I triggered the API's rate limiting. That was before I had this loop working correctly, though so it shouldn't still happen. I will go ahead and remove this.

}
fileSystems = append(fileSystems, fileSystemsResp.FileSystems...)
}

var fileSystemId *string
for _, fileSystem := range fileSystems {
// Check if tags match ID, if so then delete things
for _, tag := range fileSystem.Tags {
if *tag.Key == "runner-id" && *tag.Value == opts.Id {
fileSystemId = fileSystem.FileSystemId
goto DeleteFileSystem
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.

} else {
var fileSystemId *string
for _, fileSystem := range fileSystems {
// Check if tags match ID, if so then delete things
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
}
}
}
}

DeleteFileSystem:
describeAccessPointsResp, err := efsSvc.DescribeAccessPoints(&efs.DescribeAccessPointsInput{
FileSystemId: fileSystemId,
})
if err != nil {
return err
}
for _, accessPoint := range describeAccessPointsResp.AccessPoints {
_, err = efsSvc.DeleteAccessPoint(&efs.DeleteAccessPointInput{AccessPointId: accessPoint.AccessPointId})
if err != nil {
return err
if *fileSystemId == "" {
s.Update("File system with tag key `runner-id` and value " + opts.Id + " not detected, skipping deletion")
s.Done()
return nil
}
}

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})
DeleteFileSystem:
describeAccessPointsResp, err := efsSvc.DescribeAccessPoints(&efs.DescribeAccessPointsInput{
FileSystemId: fileSystemId,
})
if err != nil {
return err
}
}
for _, accessPoint := range describeAccessPointsResp.AccessPoints {
_, err = efsSvc.DeleteAccessPoint(&efs.DeleteAccessPointInput{AccessPointId: accessPoint.AccessPointId})
if err != nil {
return err
}
}

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

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" +
"not be deleted, because the mount targets weren't deleted")
default:
_, err = efsSvc.DeleteFileSystem(&efs.DeleteFileSystemInput{FileSystemId: fileSystemId})
if err != nil {
if strings.Contains(err.Error(), "because it has mount targets") {
// sleep here for 5 seconds to avoid slamming the API
time.Sleep(5 * time.Second)
continue
for {
select {
case <-ctx.Done():
return errors.New("after 5 minutes, the file system could" +
"not be deleted, because the mount targets weren't deleted")
default:
_, err = efsSvc.DeleteFileSystem(&efs.DeleteFileSystemInput{FileSystemId: fileSystemId})
if err != nil {
if strings.Contains(err.Error(), "because it has mount targets") {
// sleep here for 5 seconds to avoid slamming the API
time.Sleep(5 * time.Second)
continue
}
return err
}
return err
// if we reach this point, we're done
s.Update("Runner file system deleted")
s.Done()
return nil
}
// if we reach this point, we're done
return nil
}
}
return nil
}

func (i *ECSRunnerInstaller) UninstallFlags(set *flag.Set) {
Expand Down Expand Up @@ -547,7 +565,7 @@ func launchRunner(
ExecutionRoleArn: aws.String(executionRoleArn),
Cpu: aws.String(cpu),
Memory: aws.String(memory),
Family: aws.String(defaultRunnerTagName),
Family: aws.String(installutil.DefaultRunnerName(id)),
TaskRoleArn: &taskRoleArn,
NetworkMode: aws.String("awsvpc"),
RequiresCompatibilities: []*string{aws.String(defaultTaskRuntime)},
Expand Down