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

Implement long running polling #1649

Merged
merged 1 commit into from
Aug 8, 2024

Conversation

p3ck
Copy link
Collaborator

@p3ck p3ck commented Jul 24, 2024

SUMMARY

This implements long running polling the same way the GenericRestClient does with a 600 second overall timeout and 30 second polling.

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

plugins/inventory/azure_rm.py

ADDITIONAL INFORMATION

I ran the inventory integration tests but I don't have enough hosts in my azure instance to recreate the original issue. So all I can do is verify that it still works with my limited test environment.

@p3ck p3ck requested a review from Fred-sun July 24, 2024 15:26
@p3ck p3ck force-pushed the inventory_polling branch 2 times, most recently from 1f1dc43 to 93e32ea Compare July 29, 2024 15:15
@p3ck
Copy link
Collaborator Author

p3ck commented Jul 29, 2024

Possible fix for #1614

@Fred-sun
Copy link
Collaborator

@p3ck There's no problem on my end. But I don't have enough virtual machines on my side to repeat the problem. So I will push to merge #1614 users after they have no problem synchronizing updates. Thank you!

@Fred-sun Fred-sun added medium_priority Medium priority work in In trying to solve, or in working with contributors labels Jul 30, 2024
This implements long running polling the same way the GenericRestClient
does with a 600 second overall timeout and 30 second polling.

Implement retries and backoffs in the batch processing.  I looked at the
Azure SDK's pipeline which supports policies like RetryPolicy but I
don't think it will help here.  It doesn't look like we get a 429 when
sending the request to the /batch endpoint, only in the batch responses.
@p3ck p3ck force-pushed the inventory_polling branch from 93e32ea to fa90033 Compare July 31, 2024 15:58
@p3ck p3ck added the ready_for_review The PR has been modified and can be reviewed and merged label Jul 31, 2024
@p3ck p3ck requested review from nirarg and xuzhang3 July 31, 2024 16:01
# 429: Too many requests Error, Backoff and Retry
batch_retry = True
if batch_retry:
time.sleep(backoff_factor * (2 ** (retry_count)))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Regarding the backoff policy, retry time should based on the response. Other wise the retry request will be rejected too.
https://learn.microsoft.com/en-us/troubleshoot/azure/virtual-machines/windows/troubleshooting-throttling-errors#throttling-error-details

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thank you @xuzhang3,
@p3ck is on vacation (he will be back next week) so I'm checking how to add the RetryPolicy class to be used here, in order to have this response retry time to be considered

Copy link
Collaborator

Choose a reason for hiding this comment

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

The issue here is that in the current implementation the wait sleep is done once for the all batch together
Do you have any suggestion/thoughts how to change it to work for each vm separately?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@nirarg I see, misunderstand the retry policy here. The backoff policy and retry is focus on the local batch requests here so we cannot determine the retry time for all the request.

@xuzhang3 xuzhang3 merged commit 997e4de into ansible-collections:dev Aug 8, 2024
Justwmz pushed a commit to Justwmz/azure that referenced this pull request Nov 4, 2024
This implements long running polling the same way the GenericRestClient
does with a 600 second overall timeout and 30 second polling.

Implement retries and backoffs in the batch processing.  I looked at the
Azure SDK's pipeline which supports policies like RetryPolicy but I
don't think it will help here.  It doesn't look like we get a 429 when
sending the request to the /batch endpoint, only in the batch responses.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
medium_priority Medium priority ready_for_review The PR has been modified and can be reviewed and merged work in In trying to solve, or in working with contributors
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants