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.

Sign up for GitHub

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

Jump to bottom

Allow configurable timeout when reading security group rule #3911

Closed
wants to merge 1 commit into from
Closed

Allow configurable timeout when reading security group rule #3911

wants to merge 1 commit into from

Conversation

mildred
Copy link
Contributor

@mildred mildred commented Mar 26, 2018 •
edited
Loading

When being throttled on AWS requests, read requests are the first ones
to be throttled, and reading security group rules can take longer than
5m to complete. Transform the hard timeout of 5m with a configurable
timeout to avoid this problem.

Fixes part of #3128

@ghost ghost added the size/XS Managed by automation to categorize the size of a PR. label Mar 26, 2018
@mildred mildred force-pushed the timeout-read-security-group-rule branch from ae7c879 to ccba8ae Compare March 26, 2018 07:47
@ghost ghost added the size/XS Managed by automation to categorize the size of a PR. label Mar 26, 2018
@mildred mildred mentioned this pull request Mar 26, 2018
Closed
@mildred mildred force-pushed the timeout-read-security-group-rule branch from ccba8ae to 3c73d02 Compare March 26, 2018 08:18
@ghost ghost added size/S Managed by automation to categorize the size of a PR. and removed size/XS Managed by automation to categorize the size of a PR. labels Mar 26, 2018
@radeksimko radeksimko added service/ec2 Issues and PRs that pertain to the ec2 service. enhancement Requests to existing resources that expand the functionality or scope. thinking labels Mar 26, 2018
@mildred mildred force-pushed the timeout-read-security-group-rule branch from 3c73d02 to ad6928c Compare April 25, 2018 12:06
@ghost ghost added the size/S Managed by automation to categorize the size of a PR. label Apr 25, 2018
@mildred mildred force-pushed the timeout-read-security-group-rule branch from ad6928c to 45d212f Compare July 12, 2018 10:26
@ghost ghost added the size/S Managed by automation to categorize the size of a PR. label Jul 12, 2018
@mildred mildred mentioned this pull request Aug 13, 2018
Closed
@mildred mildred force-pushed the timeout-read-security-group-rule branch from 45d212f to e56cd54 Compare October 9, 2018 07:10
@ghost ghost added size/XS Managed by automation to categorize the size of a PR. documentation Introduces or discusses updates to documentation. and removed size/S Managed by automation to categorize the size of a PR. labels Oct 9, 2018
@mildred
Copy link
Contributor Author

mildred commented Oct 9, 2018

Updated the branch to latest master (conflicts in documentation only)

@mildred mildred force-pushed the timeout-read-security-group-rule branch from e56cd54 to 0a1b81e Compare December 19, 2018 10:31
@mildred
Copy link
Contributor Author

mildred commented Dec 19, 2018

Updated branch on top of latest v1.52.0 release

@aeschright aeschright requested a review from a team June 25, 2019 19:23
@obourdon
Copy link
Contributor

obourdon commented Aug 1, 2019

@aeschright any chance this gets reviewed and merged please

@obourdon
Copy link
Contributor

obourdon commented Aug 1, 2019

@radeksimko could this be reviewed and merged please ?

@obourdon
Copy link
Contributor

obourdon commented Aug 6, 2019 •
edited
Loading

@bflad sorry to ask you directly for this but as I saw your work in issues 9345 and 9598 do you think this one could be merged too ? Many thanks in advance

@obourdon
Copy link
Contributor

Terraform team, any chance this can get reviewed soon please ?

@obourdon obourdon force-pushed the timeout-read-security-group-rule branch from 0a1b81e to e3fe9e6 Compare August 23, 2019 08:52
@mildred @obourdon
Allow configurable timeout when reading security group rule
c76c06a 
When being throttled on AWS requests, read requests are the first ones
to be throttled, and reading security group rules can take longer than
5m to complete. Transform the hard timeout of 5m with a configurable
timeout to avoid this problem.

Fixes part of hashicorp#3128
@obourdon obourdon force-pushed the timeout-read-security-group-rule branch from e3fe9e6 to c76c06a Compare August 27, 2019 06:12
@obourdon
Copy link
Contributor

@bflad @radeksimko @aeschright Is there any specific procedure/alias/... I should follow/write to in order to get this reviewed and potentially merged quite quickly? Many thanks in advance

@aeschright
Copy link
Contributor

Hi folks, I think customizable timeouts are one of the things that require some internal discussion before we can go ahead with them. As you probably noticed, we're working through a pretty big backlog of PRs, and some team members (like me!) are still quite new. Any info you can put on the associated issue #3128 about your use cases and existing workarounds will help us proceed. Thanks for your patience! We really appreciate the contribution.

@obourdon obourdon mentioned this pull request Aug 29, 2019
Merged
@obourdon
Copy link
Contributor

@aeschright thanks for your answer and be sure I understand your point here. However there have been other cases like #51677, #3910, #3639 and #3599 (and may be others) that have made their ways into the code.
As I am taking over this PR from a former person in my company, I can not really give your more insight about why this was submitted and can not give more insight into associated issue #3128.
However, we would like to get rid of our own version of terraform-aws-provider where we have this code merged and use the standard official delivery once this gets merged
Many thanks for your understanding

@obourdon
Copy link
Contributor

Any progress on this one please @aeschright @bflad @ryndaniels ? Thanks

@obourdon
Copy link
Contributor

@aeschright @bflad @ryndaniels have you been able to discuss about this ?

@obourdon
Copy link
Contributor

@aeschright @bflad @ryndaniels any progress on this one please ?

@obourdon
Copy link
Contributor

obourdon commented Oct 5, 2019

@aeschright @bflad @ryndaniels any plans to make progress on this one soon please ?

3 similar comments
@obourdon
Copy link
Contributor

@aeschright @bflad @ryndaniels any plans to make progress on this one soon please ?

@obourdon
Copy link
Contributor

@aeschright @bflad @ryndaniels any plans to make progress on this one soon please ?

@obourdon
Copy link
Contributor

@aeschright @bflad @ryndaniels any plans to make progress on this one soon please ?

@obourdon
Copy link
Contributor

@terraformteam

@obourdon
Copy link
Contributor

@aeschright @bflad @ryndaniels any chance this one gets addressed someday please ?

@obourdon
Copy link
Contributor

obourdon commented Nov 5, 2019

@terraformteam anything I can do to have this addressed soon ???

@obourdon
Copy link
Contributor

obourdon commented Nov 8, 2019

Any news on this please ?

@obourdon
Copy link
Contributor

obourdon commented Nov 8, 2019

@ewbankkit as you helped me out with lambda & timeout issues some time ago, may be you could pass this one on to people who might be willing to have a look at this please ? Many thanks in advance

@obourdon
Copy link
Contributor

@aeschright @bflad @ryndaniels @ewbankkit any chance this one gets addressed someday please ?

@obourdon
Copy link
Contributor

Any news ?

@obourdon
Copy link
Contributor

Could this be integrated/discussed/adressed somehow ?

@obourdon
Copy link
Contributor

obourdon commented Dec 9, 2019

Nothing new ? Can someone tell me how to make progress on this please ?

@paultyng
Copy link
Contributor

paultyng commented Dec 11, 2019 •
edited
Loading

@obourdon the additional pings and comments, that do not add additional questions or relevant information to the initial description, do not change our prioritization of an issue or PR, but they do generate a lot of notification noise for us and any others subscribed or participating that slow down our response to items.

I understand your frustration with not knowing exactly when something will be delivered or a timeline, especially when its affecting your day to day work. Explaining your need like you did in one of your earlier comments is great and does help us better understand priorities. "+1", "Any news?" type comments though do not further the cause of an issue (reactions do though, as that is one signal we use in prioritization).

All that being said, I believe this issue is solved in #9812.

@paultyng paultyng closed this Dec 11, 2019
@obourdon
Copy link
Contributor

obourdon commented Dec 16, 2019 •
edited
Loading

@paultyng many thanks for pointing this out and sorry for those many updates, however I thought this was a way to prevent the Hashibot from accidentally closing/downgrading the issue/PR due to inactivity and was hoping to at least get feedback or time estimates for feedback (may be I have taken those different habits whe I worked with/for the OpenStack community ;-) )

I agree that looking at the code integrated in #9812 might solve the issue but still does not allow users to configure the timeouts from within their HCL code so may be you could reopen this and potentially integrate it in a next release of the AWS provider.

Thanks for your understanding

@obourdon
Copy link
Contributor

@paultyng is there a way to reopen this PR please ? (see comment above)

@bflad
Copy link
Contributor

bflad commented Jan 2, 2020 •
edited
Loading

Hi @obourdon 👋 If you are still having trouble with version 2.25.0 or later of the Terraform AWS Provider, which includes the fix from #9812 that should negate the need for this type of change, please submit a new GitHub bug report for further discussion about the behavior you are seeing including debug logs.

In general, customizable timeouts should not be required for Terraform AWS Provider resources except in cases where operation times are affected by some scalable dimension of the resource itself (e.g. database size affecting the length of time required for aws_db_instance resource update operations). We generally want to discourage customizable timeouts otherwise since the configuration option may not help the situation and instead be masking issues that should be fixed in the resource code for all operators, such as increasing an underlying default/hardcoded timeout.

@obourdon
Copy link
Contributor

obourdon commented Jan 3, 2020

@bflad thanks for this update. I now understand more your standpoint. We will see if we can reproduce the issue with latest versions of the provider

@ghost
Copy link

ghost commented Jan 11, 2020

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. Thanks!

@ghost ghost locked and limited conversation to collaborators Jan 11, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees
No one assigned
Labels
documentation Introduces or discusses updates to documentation. enhancement Requests to existing resources that expand the functionality or scope. service/ec2 Issues and PRs that pertain to the ec2 service. size/XS Managed by automation to categorize the size of a PR.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@mildred @obourdon @aeschright @paultyng @bflad @radeksimko