AWS ECS Runner Uninstall Fix #4792

paladin-devops · 2023-06-08T20:56:27Z

This PR fixes an error which sometimes occurred during a runner uninstall for AWS ECS runners:

❌ Uninstalling runner...
✓ Finding runner in ECS services...
! InvalidParameter: 1 validation error(s) found.

This occurred when no AWS EFS file systems were detected which had a tag waypoint-runner, with the value of the runner's ID assigned to it. This PR fixes that by properly looping through all paginated results of AWS EFS file systems to find a match, and later use that match correctly during deletion. If no matching file systems are found, runner uninstall will skip attempting to delete it.

There was also a possible resource leak with a defer inside of a for loop, which has been fixed.

Successful output looks like:

$ waypoint runner uninstall -platform=ecs -id=ABC123 -ecs-cluster=dev -ecs-region=us-east-1
✓ Runner "ABC123" uninstalled successfully
✓ Runner "ABC123" forgotten on server
✓ Waypoint runner AWS ECS service deleted
✓ Runner file system deleted

The runner uninstall for AWS will now loop correctly through all file systems' paginated results using a marker. Some other updates are made in this commit to skip deleting the file system if certain conditions are met (if there are no file systems that exist, or none with the right tag). Step groups were also updated.

catsby

Some nitpicks but nothing major that I see yet

catsby · 2023-06-08T21:31:46Z

internal/runnerinstall/ecs.go

 			done = true
+		} else {
+			marker = *fileSystemsResp.NextMarker
+			time.Sleep(5 * time.Second)


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

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.

catsby · 2023-06-08T21:41:07Z

internal/runnerinstall/ecs.go

 	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{


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 })

catsby · 2023-06-08T21:48:16Z

internal/runnerinstall/ecs.go

-				fileSystemId = fileSystem.FileSystemId
-				goto DeleteFileSystem
+	if len(fileSystems) == 0 {
+		s.Update("No file systems detected, skipping deletion")


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

https://go.dev/doc/effective_go#control-structures

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.

catsby

The license header needs to be restored

catsby · 2023-06-08T21:49:40Z

internal/runnerinstall/ecs.go

@@ -1,6 +1,3 @@
-// Copyright (c) HashiCorp, Inc.
-// SPDX-License-Identifier: MPL-2.0
-


I think we need these

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

Simplify runner uninstall func for ECS by removing an `if`. Add the copyright headers back in.

catsby

LGTM thanks for making those changes. Just a question remaining about the removed err check other than that ✅

catsby · 2023-06-09T21:55:02Z

internal/runnerinstall/ecs.go

@@ -411,19 +420,16 @@ DeleteFileSystem:
 	describeMountTargetsResp, err := efsSvc.DescribeMountTargets(&efs.DescribeMountTargetsInput{
 		FileSystemId: fileSystemId,
 	})
-	if err != nil {
-		return err
-	}


Do we not need to check this error?

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

…install.

paladin-devops · 2023-06-12T14:29:53Z

Thanks @catsby, pushed up that last err check and will merge after checks pass.

paladin-devops added plugin/ecs ecosystem Things related to waypoint interacting with external systems backport/0.11.x labels Jun 8, 2023

paladin-devops self-assigned this Jun 8, 2023

github-actions bot added the core label Jun 8, 2023

paladin-devops force-pushed the b-ecs-runner-uninstall-file-system branch from 79fe273 to c596458 Compare June 8, 2023 20:59

paladin-devops marked this pull request as ready for review June 8, 2023 20:59

paladin-devops requested a review from a team as a code owner June 8, 2023 20:59

vercel bot deployed to Preview – waypoint-ui June 8, 2023 21:00 View deployment

catsby approved these changes Jun 8, 2023

View reviewed changes

catsby suggested changes Jun 8, 2023

View reviewed changes

paladin-devops requested a review from catsby June 9, 2023 15:20

vercel bot deployed to Preview – waypoint-ui June 9, 2023 15:21 View deployment

paladin-devops force-pushed the b-ecs-runner-uninstall-file-system branch from 455f74a to 29c0687 Compare June 9, 2023 15:22

vercel bot deployed to Preview – waypoint-ui June 9, 2023 15:24 View deployment

imp: Use pagination func for searching EFS file systems.

9329438

Simplify runner uninstall func for ECS by removing an `if`. Add the copyright headers back in.

paladin-devops force-pushed the b-ecs-runner-uninstall-file-system branch from 29c0687 to 9329438 Compare June 9, 2023 16:22

vercel bot deployed to Preview – waypoint-ui June 9, 2023 16:24 View deployment

catsby approved these changes Jun 9, 2023

View reviewed changes

fix: Return err on DescribeMountTargets failure, during ECS runner un…

fc462d2

…install.

vercel bot deployed to Preview – waypoint-ui June 12, 2023 14:29 View deployment

paladin-devops merged commit a048593 into main Jun 12, 2023

paladin-devops deleted the b-ecs-runner-uninstall-file-system branch June 12, 2023 15:00

hc-github-team-waypoint mentioned this pull request Jun 12, 2023

Backport of AWS ECS Runner Uninstall Fix into release/0.11.x #4799

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

AWS ECS Runner Uninstall Fix #4792

AWS ECS Runner Uninstall Fix #4792

paladin-devops commented Jun 8, 2023 •

edited

Loading

catsby left a comment

catsby Jun 8, 2023

paladin-devops Jun 9, 2023

catsby Jun 8, 2023

catsby Jun 8, 2023

paladin-devops Jun 9, 2023

catsby left a comment

catsby Jun 8, 2023

paladin-devops Jun 9, 2023

catsby left a comment

catsby Jun 9, 2023

paladin-devops Jun 12, 2023

paladin-devops commented Jun 12, 2023

		@@ -1,6 +1,3 @@
		// Copyright (c) HashiCorp, Inc.
		// SPDX-License-Identifier: MPL-2.0

AWS ECS Runner Uninstall Fix #4792

AWS ECS Runner Uninstall Fix #4792

Conversation

paladin-devops commented Jun 8, 2023 • edited Loading

catsby left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

catsby left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

catsby left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paladin-devops commented Jun 12, 2023

paladin-devops commented Jun 8, 2023 •

edited

Loading