-
-
Notifications
You must be signed in to change notification settings - Fork 359
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Nuke ECS services and ECS clusters #36
Conversation
8982cb6
to
af43e9f
Compare
aws/ecs_cluster.go
Outdated
// getAllEcsClusters - Returns a string of ECS Cluster ARNs, which uniquely identifies the cluster. | ||
// NOTE: AWS api doesn't provide the necessary information to | ||
// implement `excludeAfter` filter at the cluster | ||
// level, so we will implement it at the service level. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wait, so does that mean we delete all ECS clusters, no matter how old they are?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In practice, that is not what happens because the filter applies on the ECS services and you can't delete ECS clusters that have any active services. However, if the cluster is empty, you could delete newer clusters so it is not perfect.
I amended the comments with that extra bit of clarification. I'm also going to make this check explicit ("are services still running on the cluster?") in the nuking step as an additional guard, if AWS decides to update the behavior of their API.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are other resources that don't expose a "created date." For those, we use tags to only delete resources older than a certain date. E.g., see the EIP code: https://github.com/gruntwork-io/cloud-nuke/blob/master/aws/eip.go
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was actually going to go that route originally, except in my original search I couldn't find any way to set tags in ECS. But I guess I didn't try hard enough then because now I found that they are called something else in ECS (Attribute
), which explains why I didn't find it the first time: I was looking for the wrong thing :(
... except unfortunately, it looks like it won't even let you set attributes on a cluster:
houston exec sandbox -- aws --region us-east-1 ecs put-attributes --cluster "$CLUSTER_ARN" --attributes name="cloud-nuke-test",value="1234",targetId="$CLUSTER_ARN"
An error occurred (InvalidParameterException) when calling the PutAttributes operation: Unexpected targetType 'cluster'.
I can't find anything else in the ECS API suite that lets me set custom values on a resource. The only thing we have control of is the Name
, which isn't something we should overload.
Should I just remove the cluster nuking for now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea, probably... We should be conservative in a tool like this and never delete more than expected.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make sense. Will take it out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a caveat section to the README to signal that we have considered this and made the conscious choice not to pursue this at the moment.
aws/ecs_service.go
Outdated
// 2.) Delete service object once no tasks are running. | ||
// Note that this will swallow failed deletes and continue along, logging the | ||
// service ARN so that we can find it later. | ||
func nukeAllEcsServices(awsSession *session.Session, ecsServiceClusterMap map[string]string, ecsServiceArns []*string) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a really long function with big comment blocks in the middle. Consider breaking the contents into several smaller functions: e.g., startDrainingEcsServices
, waitForServicesToFinishDraining
, etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
8ba3953
to
5487ffd
Compare
UPDATES:
|
@brikis98 Is it ok to merge this? I have taken out ECS cluster deleting as requested. This now only contains deleting ECS services, which does have a timestamp we can use. |
Yup, ship it! 👍 |
First attempt at addressing #32
This implements nuking of:
ECS clustersThis does NOT implement nuking of: