Skip to content
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

aws.ecs | deregister container instances and delete clusters #5326

Open
wants to merge 19 commits into
base: main
Choose a base branch
from

Conversation

PratMis
Copy link
Collaborator

@PratMis PratMis commented Feb 11, 2020

Closes #5039

This PR adds two actions for ecs-clusters:

  1. deregister-container-instances: lets you deregister container instances. force option can be used as per boto docs: https://boto3.amazonaws.com/v1/documentation/api/latest/reference/services/ecs.html#ECS.Client.deregister_container_instance

  2. delete: lets you delete ecs cluster. force: true, deregisters container instances followed by cluster deletion. https://boto3.amazonaws.com/v1/documentation/api/latest/reference/services/ecs.html#ECS.Client.delete_cluster
    This also cleans up any ecs managed capacity provider(asgs)

@PratMis
Copy link
Collaborator Author

PratMis commented Feb 11, 2020

I'll add one more test to delete ecs cluster with force: false to bump up codecov

@PratMis PratMis changed the title deregister container instances and delete clusters aws.ecs | deregister container instances and delete clusters Feb 11, 2020
c7n/resources/ecs.py Outdated Show resolved Hide resolved
c7n/resources/ecs.py Outdated Show resolved Hide resolved
@PratMis PratMis marked this pull request as ready for review February 14, 2020 20:30
@PratMis
Copy link
Collaborator Author

PratMis commented Feb 14, 2020

thanks @kapilt , I have addressed all findings as per your suggestion

@PratMis
Copy link
Collaborator Author

PratMis commented Mar 10, 2020

Had to add one more test to keep the codecov up!!

@kapilt
Copy link
Collaborator

kapilt commented Mar 13, 2020

thanks, looks good, one thing that came up in wrt to eks delete is actually deleting the underlying compute instances/asg. any thoughts on that? I'm not really clear on the use case here so would appreciate if thats something you feel is also of interest (also making an assumption this is something your planning on using?).

@PratMis
Copy link
Collaborator Author

PratMis commented Mar 15, 2020

Thanks for pointing it out! Yes, this is something we would be using! I'll add the deletion of underlying instances as part of cluster cleaning process in this Pr!

@PratMis
Copy link
Collaborator Author

PratMis commented Mar 23, 2020

@kapilt I was thinking of adding the deletion of underlying instance as a part of action: deregister-instance and reusing the code in delete action. Policy sample:

name: ecs-instances
resource: ecs
filters:
  - type: x
actions:
  - type: deregister-instance
     delete-instances: true

Let me know if this sounds good to you? Thanks!!

@kapilt
Copy link
Collaborator

kapilt commented Mar 24, 2020

@kapilt I was thinking of adding the deletion of underlying instance as a part of action: deregister-instance and reusing the code in delete action. Policy sample:

name: ecs-instances
resource: ecs
filters:
  - type: x
actions:
  - type: deregister-instance
     delete-instances: true

Let me know if this sounds good to you? Thanks!!

sounds good

@PratMis
Copy link
Collaborator Author

PratMis commented Mar 24, 2020

@kapilt I was thinking of adding the deletion of underlying instance as a part of action: deregister-instance and reusing the code in delete action. Policy sample:

name: ecs-instances
resource: ecs
filters:
  - type: x
actions:
  - type: deregister-instance
     delete-instances: true

Let me know if this sounds good to you? Thanks!!

sounds good

While implementing I realized that it might be better a better approach to keep the instance deletion action as part of the cluster cleanup because there might be other things like ASGs etc as part of underlying instance. Already made the code changes last night. If you feel it would be better to stick with the first approach, more than happy to make the changes. Thank you!

@PratMis PratMis mentioned this pull request Mar 24, 2020
@kapilt
Copy link
Collaborator

kapilt commented Mar 25, 2020

i'm more concerned with that these are asg instances, and this will just cause churn instead of providing value. if the ecs cluster per its capacity provider is managing the asg that seems safe to delete. if for whatever reason the asg is spanning clusters.. that would be problematic, distinction here is wholly owned asg for the cluster vs one thats shared, not that would be a good idea, but part of custodian is saving people from making accidentally making bad choices that make things worse, when its non obvious that is the outcome.

@PratMis
Copy link
Collaborator Author

PratMis commented Mar 26, 2020

Understood! I have added code to delete ecs managed asgs if any on cluster deletion with force: true. Your feedback was really helpful. Thanks so much!!

c7n/resources/ecs.py Outdated Show resolved Hide resolved
@nomecks
Copy link

nomecks commented Aug 26, 2020

Has there been any movement on this? My company has been waiting for this functionality to manage sandbox and non-production cluster sprawl.

@PratMis
Copy link
Collaborator Author

PratMis commented Aug 26, 2020

hey @nomecks , sorry for the delay. I'll try to fix this PR today as per the feedback received from @kapilt

@nomecks
Copy link

nomecks commented Sep 21, 2020

We'd still love to have this functionality!

@nomecks
Copy link

nomecks commented Nov 17, 2020

Giving this a bi-monthly bump

@vauchok
Copy link
Contributor

vauchok commented Feb 15, 2021

Hi all,

Any, updates? I think this functional would really helpful for cleaning policies.

Thanks

@PratMis
Copy link
Collaborator Author

PratMis commented Feb 19, 2021

Hey, apologies on this one. I'll get back to it this week

@kpalaparthi
Copy link

Hi,
When we expect this functionality. We are looking for ECS termination and deregister of task definitions.
can someone please let us know when this functionality can be expected

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature Request - ECS add Delete action for clusters
5 participants