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

feat: add delete timeout to aws_autoscaling_group #1566

Closed

Conversation

filintod
Copy link

@filintod filintod commented Sep 2, 2021

PR o'clock

Description

Adding timeout to asg deletion as sometimes it can take more than the default 10m in our case.

FEATURES:

  • Added option to customize timeout delete for autoscaling groups. It can be done via
    workers_group_defaults.asg_delete_timeout as a golang duration or in each template or config
    via delete_timeout.

Checklist

@github-actions
Copy link

github-actions bot commented Sep 2, 2021

Thank you for your contribution!

The CHANGELOG.md file contents are handled by the maintainers during merge. This is to prevent pull request merge conflicts.
Please see the Contributing Guide for additional pull request review items.

Remove any changes to the CHANGELOG.md file and commit them in this pull request.

@filintod filintod force-pushed the add-timeout-asg branch 2 times, most recently from 506da5a to 2253ba0 Compare September 2, 2021 20:48
@daroga0002
Copy link
Contributor

we should address this issue in same way as in node groups:

timeouts {
create = lookup(each.value["timeouts"], "create", null)
update = lookup(each.value["timeouts"], "update", null)
delete = lookup(each.value["timeouts"], "delete", null)
}

to make a consistent user experience.

@filintod do you will be able to make this change?

@filintod
Copy link
Author

filintod commented Sep 29, 2021

@daroga0002 I can do it but next week if that works.

@daroga0002
Copy link
Contributor

@filintod do you plan to implement changes?

@github-actions
Copy link

github-actions bot commented Nov 7, 2021

This PR has been automatically marked as stale because it has been open 30 days
with no activity. Remove stale label or comment or this PR will be closed in 10 days

@filintod
Copy link
Author

filintod commented Nov 8, 2021

@daroga0002 sorry for the delay. updated the timeout to follow node groups format

@daroga0002
Copy link
Contributor

@daroga0002 sorry for the delay. updated the timeout to follow node groups format

could you paste here some example of module configuration (this will make a testing faster)?

@filintod
Copy link
Author

filintod commented Nov 10, 2021

@daroga0002 an example would be like:

module "eks" {
  source           = "terraform-aws-modules/eks/aws"
  // other configs....

  workers_group_defaults = {
    asg_force_delete = true

    # double delete timeout default value
    timeouts = {
      delete = "20m"
    }
  }
}

workers.tf Outdated
@@ -103,7 +103,9 @@ resource "aws_autoscaling_group" "workers" {
"capacity_rebalance",
local.workers_group_defaults["capacity_rebalance"]
)

timeouts {
delete = lookup(local.workers_group_defaults["timeouts"], "delete", null)
Copy link
Contributor

Choose a reason for hiding this comment

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

this should be customized via workers_group_defaults["timeouts"] but also via timeouts setting per particulal worker definition as rest of parameters which are defined here https://github.com/terraform-aws-modules/terraform-aws-eks/blob/master/locals.tf

Copy link
Author

Choose a reason for hiding this comment

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

added an extra lookup against the per particular worker definition

Copy link
Contributor

Choose a reason for hiding this comment

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

This will work, but in general this is in other way as other variables. Please review and use same pattern which we arleady using

Copy link
Author

@filintod filintod Nov 10, 2021

Choose a reason for hiding this comment

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

@daroga0002 could you explain/point out to me as I did not find it.

In node_groups, timeouts is used like:

  timeouts {
    create = lookup(each.value["timeouts"], "create", null)
    update = lookup(each.value["timeouts"], "update", null)
    delete = lookup(each.value["timeouts"], "delete", null)
  }

where each is from https://github.com/terraform-aws-modules/terraform-aws-eks/blob/master/modules/node_groups/main.tf#L2 where they have timeouts as timeouts = var.workers_group_defaults["timeouts"]

I guess I could add timeouts in locals.tf. But not sure if that is overkill as that was not added before, and from what I can see it would just change local.workers_group_defaults["timeouts"] for local.timeouts. Let me know or please point me to an example as I did not see anything similar from my point of view.

@github-actions
Copy link

github-actions bot commented Jan 1, 2022

This PR has been automatically marked as stale because it has been open 30 days
with no activity. Remove stale label or comment or this PR will be closed in 10 days

@antonbabenko
Copy link
Member

This issue has been resolved in version 18.0.0 🎉

@github-actions
Copy link

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 11, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants