Fix error setting app password in hack create due to eventual consistency in ms graph api#4736
Fix error setting app password in hack create due to eventual consistency in ms graph api#4736
Conversation
There was a problem hiding this comment.
Pull request overview
Adds resilience to local cluster creation by retrying Microsoft Graph AddPassword calls to handle eventual consistency after creating an AAD Application.
Changes:
- Introduces retry constants for Graph API calls.
- Wraps
AddPasswordin a polling/retry loop that retries on 404 “not found” errors.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
6b4ca24 to
c3b45a6
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // check if returned error is 404 not found, if so, retry the operation | ||
| numRetries := 0 | ||
| var pwResult msgraph_models.PasswordCredentialable | ||
| err = wait.PollUntilWithContext(ctx, GraphApiRetryInterval, func(ctx context.Context) (done bool, err error) { |
There was a problem hiding this comment.
Suggestion: use wait.PollImmediateUntilWithContext (or do one direct call before polling) so retries still happen, but success path stays fast.
| // check if returned error is 404 not found, if so, retry the operation | ||
| numRetries := 0 | ||
| var pwResult msgraph_models.PasswordCredentialable | ||
| err = wait.PollUntilWithContext(ctx, GraphApiRetryInterval, func(ctx context.Context) (done bool, err error) { |
There was a problem hiding this comment.
Could we add a focused unit test around this retry loop?
Useful cases: retries on 404, no retry on non-404, and stops after max retries.
That would make this eventual-consistency handling safer to refactor later.
|
|
||
| // retry loop, due to eventual consistency, the application we just created might not be found when queried immediately, only after a retry | ||
| // check if returned error is 404 not found, if so, retry the operation | ||
| numRetries := 0 |
There was a problem hiding this comment.
Can we add the retry logic directly in the https://github.com/Azure/ARO-RP/blob/master/pkg/util/graph/graphsdk/applications/item_add_password_request_builder.go#L49 ??
What this PR does / why we need it:
Test plan for issue:
How do you know this will function as expected in production?