cloud-provider-azure
cloud-provider-azure copied to clipboard
Add timeout setting for default transport
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Does this PR introduce a user-facing change?
NONE
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:
[APPROVALNOTIFIER] This PR is APPROVED
This pull-request has been approved by: MartinForReal
The full list of commands accepted by this bot can be found here.
The pull request process is described here
- ~~OWNERS~~ [MartinForReal]
Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment
I believe this PR may be related to #5642 right? I wonder if rather then configuring the default http client, we should be assessing appropriate cancellation timeouts for contexts we pass through.
See for example https://github.com/openshift/cloud-provider-azure/blob/7c0a8afb84df58905abe03583929f0f70ad48236/pkg/provider/azure_loadbalancer_repo.go#L135-L136, we create a context and pass it into the create or update call, would it not be more idiomatic to add a context specific timeout. (context.WithTimeout).
I think using specific timeouts throughout the codebase would be appropriate as it would allow you to asses what timeout you need for each specific use case, rather than setting a blanket rule, WDYT?
I think a better approach would be adding timeout in in reconcile loop or retry loop.
I think using specific timeouts throughout the codebase would be appropriate as it would allow you to asses what timeout you need for each specific use case, rather than setting a blanket rule, WDYT?
Thanks for the reviewing and yes we should add those context timeouts. Let's add those in a following PR.
/lgtm