Add cache at provider level

[tjamet]

Description

In the current implementation, DNS providers are called to list all records on every loop. This is expensive in terms of number of requests to the provider and may result in being rate limited, as reported in 1293 and 3397.

In our case, we have approximately 20,000 records in our AWS Hosted Zone. The ListResourceRecordSets API call allows a maximum of 300 items per call. That requires 67 API calls per external-dns deployment during every sync period

With this, we introduce an optional generic caching mechanism at the provider level, that re-uses the latest known list of records for a given time.

This prevents from expensive Provider calls to list all records for each object modification that does not change the actual record (annotations, statuses, ingress routing, ...)

This introduces 2 trade-offs:

1. Any changes or corruption directly on the provider side will be longer to detect and to resolve, up to the cache time

2. Any conflicting records in the DNS provider (such as a different external-dns instance) injected during the cache validity will cause the first iteration of the next reconcile loop to fail, and hence add a delay until the next retry

Checklist

• [X] Unit tests updated
• [X] End user documentation updated

[szuecs]

@tjamet do you want to rebase it? Code looks good to me

[szuecs]

/ok-to-test

[maiconbaum]

This would be a really useful feature.

[pabloajz]

really looking forward to have this feature.

[johngmyers]

The txt registry has a similar cache. It would make sense to pull this to a separate layer above the registry.

[tjamet]

Hi! Sorry, I missed your comment. I will rebase ASAP!

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: Once this PR has been reviewed and has the lgtm label, please assign seanmalloy for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment Approvers can cancel approval by writing /approve cancel in a comment

[tjamet]

@tjamet do you want to rebase it? Code looks good to me

@szuecs I have rebased the code atop the head of master.

The txt registry has a similar cache. It would make sense to pull this to a separate layer above the registry.

@johngmyers Thanks for the input. Indeed, the txt registry also has a cache mechanism. Here, the proposal is to cache at the lower level rather than at a higher one, to increase consistency. Indeed, the actual record provider is injected into the TXT registry as well as any other registry like dynamoDB.

Getting the cache at lower levels increases the cache success rate and further reduces the calls to the service provider. Indeed, if we cache at a higher level, I sense we need to either implement it in all registries (TXT, DynamoDB, AWS-SD, ...) or refactor significantly how the plan is calculated so we can have the cache management above the registry level. Indeed, currently, the main logic of filtering owned domains is executed in each registry This means that, from my trials, we would invalidate the cache any time almost every time when external-dns is used concurrently either with other domains or in multiple clusters.

This being said, I tend to indeed see that the txt cache is redundant here and we could deprecate or remove it. Happy to do so if we align on this, either in this PR or another one

[k8s-ci-robot]

PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[k8s-ci-robot]

@tjamet: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-external-dns-licensecheck	32c67df3f013070642664d200791ceee4483465d	link	true	/test pull-external-dns-licensecheck

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

[mloiseleur]

@tjamet Do you think you can rebase this PR ?

[k8s-triage-robot]

The Kubernetes project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

• Mark this PR as fresh with /remove-lifecycle stale
• Close this PR with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

[k8s-triage-robot]

The Kubernetes project currently lacks enough active contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

• Mark this PR as fresh with /remove-lifecycle rotten
• Close this PR with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle rotten

[oferz-everc]

looking forward to this feature.

[k8s-triage-robot]

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages PRs according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

• Reopen this PR with /reopen
• Mark this PR as fresh with /remove-lifecycle rotten
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/close

[k8s-ci-robot]

@k8s-triage-robot: Closed this PR.

In response to this:

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages PRs according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

• Reopen this PR with /reopen
• Mark this PR as fresh with /remove-lifecycle rotten
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

[oferz-everc]

@tjamet - Any chance to re-visit this PR? Anything I can do to assist with?

[tjamet]

Hi! I have just rebased this PR to include latest changes from master.

Let me know if there is any other thing I should take a look at

/reopen

[k8s-ci-robot]

@tjamet: Failed to re-open PR: state cannot be changed. The add-provider-cache branch was force-pushed or recreated.

In response to this:

Hi! I have just rebased this PR to include latest changes from master.

Let me know if there is any other thing I should take a look at

/reopen

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.