Skip to content

sdk: add customize initial poll delay via ctx - #1325

Open
wuxu92 wants to merge 1 commit into
hashicorp:mainfrom
wuxu92:custom-initial-poll
Open

sdk: add customize initial poll delay via ctx#1325
wuxu92 wants to merge 1 commit into
hashicorp:mainfrom
wuxu92:custom-initial-poll

Conversation

@wuxu92

@wuxu92 wuxu92 commented May 1, 2026

Copy link
Copy Markdown
Contributor

Community Note

  • Please vote on this PR by adding a 👍 reaction to the original PR to help the community and maintainers prioritize for review
  • Please do not leave comments along the lines of "+1", "me too" or "any updates", they generate extra noise for PR followers and do not help prioritize for review

Description

InitialPollingDelay in ctx

The DefaultPollingInterval is 10 seconds and there is no way to customize it when the LRO response header doesn't contain the Retry-After header. this may cause long waiting time for resources like azurerm_netowork_interface: it has a/many lock[s] and may create many instances at one apply, but the LRO has no retry-after header and 10 seconds initial delay is too long, so this PR add a WithInitialPollingDelay method to customize the delay value.

related issue: hashicorp/terraform-provider-azurerm#30365 the create of network interface LRO actually finished within 1 second, but because of the 10 second default initial delay and the locks cause sequential creation, the customer has to wait for a long time to complete. there was also an internal IcM ticket in MS about creating 100+ NIC in one apply.

delete poller honor the Retry-After header

The current delete poller always using deletePoller with DefaultPollingInterval but actually the DELETE response may contain the Retry-After header, this PR also add the logic to honor it, for example the delete of azurerm_network_interfaces Retry-After value is 4.

DELETE https://management.azure.com/subscriptions/<>/resourceGroups/acctestRG-xuwu1/providers/Microsoft.Network/networkInterfaces/acctestni2-subnet2?api-version=2025-01-01 HTTP/2.0
user-agent: HashiCorp/go-azure-sdk (Go-http-Client/1.1 networkinterfaces/2025-01-01) HashiCorp Terraform/1.14.3
authorization: Bearer -<>
content-type: application/json; charset=utf-8
accept-encoding: gzip

HTTP/2.0 200 
cache-control: no-cache
pragma: no-cache
expires: -1
location: https://management.azure.com/subscriptions/<>/providers/Microsoft.Network/locations/eastus2/operationResults/<>?api-version=2025-01-01&t=<>
retry-after: 4
date: Fri, 01 May 2026 01:16:09 GMT
content-length: 0

This is a (please select all that apply):

  • Bug Fix
  • New Feature
  • Enhancement
  • Breaking Change

Related Issue(s)

Fixes #0000

Rollback Plan

If a change needs to be reverted, we will publish an updated version of the provider.

Changes to Security Controls

Are there any changes to security controls (access controls, encryption, logging) in this pull request? If so, explain.

Note

If this PR changes meaningfully during the course of review please update the title and description as required.

@wuxu92
wuxu92 requested a review from a team as a code owner May 1, 2026 04:37
@github-actions github-actions Bot added the release-once-merged The SDK should be released once this PR is merged label May 1, 2026
@magodo

magodo commented May 4, 2026

Copy link
Copy Markdown
Contributor

Instead of mutating the initial poll interval, the original issue is better to be addressed by refactoring the code of resources like NIC from:

  • Lock
  • CreateOrUpdateThenPoll

to:

  • Lock
  • CreateOrUpdate
  • Unlock
  • Poll

@wuxu92

wuxu92 commented May 4, 2026

Copy link
Copy Markdown
Contributor Author

It's still possible to encounter operation conflict during polling stage, right? because polling means the operation is in progress

Instead of mutating the initial poll interval, the original issue is better to be addressed by refactoring the code of resources like NIC from:

  • Lock
  • CreateOrUpdateThenPoll

to:

  • Lock
  • CreateOrUpdate
  • Unlock
  • Poll

@magodo

magodo commented May 4, 2026

Copy link
Copy Markdown
Contributor

I don't think so, the polling (in most cases) is just a GET operation to an endpoint for querying the status of the in-progress operation.

@wuxu92

wuxu92 commented May 4, 2026

Copy link
Copy Markdown
Contributor Author

The creation of the resource is still in progress if poll is not done, so we can directly unlock just after the CreateOrUpdate call when it's a LRO, because it's still possible to encounter operation in conflict error out of the lock.

@magodo magodo left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

You might be right. I thought the lock was introduced to avoid race condition of the initial DELETE request which is apparently not the case...

I've taken a look at the actual implementation and put down some comments.

Comment on lines +60 to +64
interval := DefaultPollingInterval
if retryAfter := retryAfterFromResponse(response); retryAfter != nil {
interval = *retryAfter
}
deletePoller, deletePollerErr := deletePollerFromResponse(response, client, interval)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should this be part of the deletePollerFromResponse, just similar to longRunningOperationPollerFromResponse?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

because the interval is a input arg of deletePollerFromResponse, it would cause confusion if we change interval value inside the function. it is different from longRunningOperationPollerFromResponse which only has the response as the input parameter.

return v
}
return 0
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

With the change of hornoring the retry-after in deletePollerFromResponse, is this change still necessary for the issue reported?

Also, I don't know if using the context to change the initial delay (actually it might better to be called default retry duration as it can be used not only in the initial delay), another easier way might be to simply add a public method to mutate it similar to AllowRetryOnError().

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

WithInitialPollingDelay is primarily used during the creation of the network interface resource to customize the initial delay. It is designed to modify only the initial polling delay, not the polling interval. The retry duration after the first poll is determined by the most recent poll result, which seems appropriate to me.

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

Labels

release-once-merged The SDK should be released once this PR is merged

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants