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

Add fail-over support to config provider. #319

Merged
merged 31 commits into from
Jun 15, 2022
Merged

Conversation

pratiksanglikar
Copy link
Contributor

This PR adds fail-over mechanism to the config provider.
With the new Connect(IEnumerable<Uri> endpoints, TokenCredential credential) API, users can now specify multiple endpoints to use when the primary endpoint fails.

Failover mechanism design decisions -

  1. When only one endpoint is provided, always use this endpoint.
  2. When multiple endpoints are provided, use the endpoints which are not in the back-off state.
  3. When all endpoints are in the back-off state, use all of the endpoints.
  4. Workflows like Load() and Refresh() work like transactions on a single endpoint. i.e. the workflow has to be executed on a single endpoint, if the workflow fails midway through, we start the workflow from the beginning with a different endpoint.

Push Notifications
Note: Current design for push notifications is for the preview version, and is different from the version we envision for GA.

  1. Upon receiving a push notification, we initiate the refresh flow without the guarantee that the latest value will be available after the refresh call. (Breaking change from existing behavior)
  2. When the push notification is received for a key-value that the user is not actively watching, we ignore the push notification (Breaking change from existing behavior)
  3. When the resource-uri of the push notification is present in the config-providers endpoint list, we update the sync token and initiate refresh workflow.
  4. When the resource-uri of the push notification is not present in the config-providers endpoint list, but we have the token-credential, we create a new configuration client, add it to our endpoint list, and update the sync token and initiate the refresh flow.
  5. When the resource-uri of the push notification is not present in the config-providers endpoint list, and we don't have the token-credential, we ignore the push notification.

Changes not in this PR but we envision for GA:

  1. Get the key-value from the server by key + label.
  2. If server key-value has same eTag as push notification eTag, server is up to date. Break;
  3. Compare last modified time of the server key-value. If it's greater than the push notification time, break;
  4. Otherwise, keep polling the server with some back-off logic until the latest value is available on the server.

@pratiksanglikar
Copy link
Contributor Author

pratiksanglikar commented May 3, 2022

Gentle reminder for review.
@jimmyca15 @avanigupta @zhenlan


In reply to: 1116548788


In reply to: 1116548788


In reply to: 1116548788


In reply to: 1116548788

@zhenlan
Copy link
Contributor

zhenlan commented Jun 8, 2022

    public static TimeSpan CalculateBackoffTime(this TimeSpan interval, TimeSpan min, TimeSpan max, int attempts)

Is this function called anywhere?


In reply to: 1150523893


In reply to: 1150523893


In reply to: 1150523893


Refers to: src/Microsoft.Extensions.Configuration.AzureAppConfiguration/Extensions/TimeSpanExtensions.cs:23 in 5985715. [](commit_id = 5985715, deletion_comment = False)

@pratiksanglikar
Copy link
Contributor Author

    public static TimeSpan CalculateBackoffTime(this TimeSpan interval, TimeSpan min, TimeSpan max, int attempts)

Yes, this method is getting used to update cache expiration time of keyvault secrets.


In reply to: 1150523893


Refers to: src/Microsoft.Extensions.Configuration.AzureAppConfiguration/Extensions/TimeSpanExtensions.cs:23 in 5985715. [](commit_id = 5985715, deletion_comment = False)

Copy link
Member

@jimmyca15 jimmyca15 left a comment

Choose a reason for hiding this comment

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

FailoverPolicy contract breach

Copy link
Contributor

@zhenlan zhenlan left a comment

Choose a reason for hiding this comment

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

:shipit:

@pratiksanglikar pratiksanglikar merged commit c8f7724 into main Jun 15, 2022
@pratiksanglikar pratiksanglikar deleted the dev/ps/failover branch June 15, 2022 19:12
avanigupta added a commit that referenced this pull request Jun 29, 2022
avanigupta added a commit that referenced this pull request Jun 29, 2022
avanigupta added a commit that referenced this pull request Jun 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants