Add cache at provider level #3402

tjamet · 2023-02-13T16:54:54Z

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:

Any changes or corruption directly on the provider side will be longer to detect and to resolve, up to the cache time
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

Unit tests updated
End user documentation updated

szuecs · 2023-05-15T13:28:55Z

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

szuecs · 2023-05-15T13:29:04Z

/ok-to-test

johngmyers · 2023-05-16T04:29:35Z

provider/cached_provider.go

+		log.Info("Records cache provider: refreshing records list cache")
+		c.cache, c.err = c.Provider.Records(ctx)
+		if c.err != nil {
+			log.Errorf("Records cache provider: list records failed: %v", c.err)


Shouldn't both log and return an error.

johngmyers · 2023-05-16T04:30:26Z

provider/cached_provider.go

+		c.lastRead = time.Now()
+		cachedRecordsCallsTotal.WithLabelValues("false").Inc()
+	} else {
+		log.Info("Records cache provider: using records list from cache")


This seems a bit chatty. Perhaps debug level would be better?

johngmyers · 2023-05-16T04:37:31Z

provider/cached_provider.go

+func (c *CachedProvider) Records(ctx context.Context) ([]*endpoint.Endpoint, error) {
+	if c.needRefresh() {
+		log.Info("Records cache provider: refreshing records list cache")
+		c.cache, c.err = c.Provider.Records(ctx)


There's actually no reason to store err in the CachedProvider. If this call returns a non-nil error, then it should just set c.cache = nil.

maiconbaum · 2023-05-19T17:21:17Z

This would be a really useful feature.

pabloajz · 2023-06-01T09:22:36Z

really looking forward to have this feature.

johngmyers · 2023-06-01T15:13:49Z

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

tjamet · 2023-09-07T14:40:11Z

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

**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 Change-Id: I0bdcfa994ac1b76acedb05d458a97c080284c5aa

Change-Id: Icaf1ffe34e75c320d4efbb428f831deb8784cd11

Change-Id: I3f4646cabd66216fd028fbae3adf68129a8a2cbf

Change-Id: I13da2aa28eef3e2c8e81b502321c4dc137087b2d

k8s-ci-robot · 2023-09-08T13:43:55Z

[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 · 2023-09-08T14:10:22Z

@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 · 2023-09-15T18:29:36Z

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 · 2024-01-03T18:26:43Z

@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	`32c67df`	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 · 2024-01-16T11:05:16Z

@tjamet Do you think you can rebase this PR ?

k8s-triage-robot · 2024-04-15T11:49:14Z

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 · 2024-05-15T12:04:33Z

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 · 2024-05-23T10:49:38Z

looking forward to this feature.

k8s-triage-robot · 2024-06-22T11:15:56Z

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 · 2024-06-22T11:16:01Z

@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 · 2024-06-24T14:53:24Z

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

tjamet · 2024-07-08T07:04:16Z

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 · 2024-07-08T07:04:20Z

@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.

k8s-ci-robot added the do-not-merge/invalid-commit-message Indicates that a PR should not merge because it has an invalid commit message. label Feb 13, 2023

k8s-ci-robot requested review from njuettner and seanmalloy February 13, 2023 16:54

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Feb 13, 2023

tjamet force-pushed the add-provider-cache branch from 8f68fca to a891979 Compare February 13, 2023 16:55

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed do-not-merge/invalid-commit-message Indicates that a PR should not merge because it has an invalid commit message. labels Feb 13, 2023

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 25, 2023

k8s-ci-robot added the ok-to-test Indicates a non-member PR verified by an org member that is safe to test. label May 15, 2023

szuecs mentioned this pull request May 15, 2023

WIP Always create AAAA alias records in route53 #3605

Open

2 tasks

johngmyers reviewed May 16, 2023

View reviewed changes

Thibault Jamet and others added 5 commits September 8, 2023 13:49

Skip apply empty changes in the cache provider

acae008

Change-Id: Icaf1ffe34e75c320d4efbb428f831deb8784cd11

Add licence headers

7b74f38

Change-Id: I3f4646cabd66216fd028fbae3adf68129a8a2cbf

Add a log line when no changes on cache provider

f48d4b8

Change-Id: I13da2aa28eef3e2c8e81b502321c4dc137087b2d

Add Domain filter interface

4f50112

tjamet force-pushed the add-provider-cache branch from 8dcbacb to 4f50112 Compare September 8, 2023 13:43

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 8, 2023

Run go imports -local sigs.k8s.io/external-dns

32c67df

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 15, 2023

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Apr 15, 2024

k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels May 15, 2024

k8s-ci-robot closed this Jun 22, 2024

tjamet mentioned this pull request Jul 8, 2024

Add provider cache #4597

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add cache at provider level #3402

Add cache at provider level #3402

tjamet commented Feb 13, 2023 •

edited

Loading

szuecs commented May 15, 2023

szuecs commented May 15, 2023

johngmyers May 16, 2023

johngmyers May 16, 2023

johngmyers May 16, 2023

maiconbaum commented May 19, 2023

pabloajz commented Jun 1, 2023

johngmyers commented Jun 1, 2023

tjamet commented Sep 7, 2023

k8s-ci-robot commented Sep 8, 2023

tjamet commented Sep 8, 2023

k8s-ci-robot commented Sep 15, 2023

k8s-ci-robot commented Jan 3, 2024

mloiseleur commented Jan 16, 2024

k8s-triage-robot commented Apr 15, 2024

k8s-triage-robot commented May 15, 2024

oferz-everc commented May 23, 2024

k8s-triage-robot commented Jun 22, 2024

k8s-ci-robot commented Jun 22, 2024

oferz-everc commented Jun 24, 2024

tjamet commented Jul 8, 2024

k8s-ci-robot commented Jul 8, 2024

Add cache at provider level #3402

Add cache at provider level #3402

Conversation

tjamet commented Feb 13, 2023 • edited Loading

szuecs commented May 15, 2023

szuecs commented May 15, 2023

johngmyers May 16, 2023

Choose a reason for hiding this comment

johngmyers May 16, 2023

Choose a reason for hiding this comment

johngmyers May 16, 2023

Choose a reason for hiding this comment

maiconbaum commented May 19, 2023

pabloajz commented Jun 1, 2023

johngmyers commented Jun 1, 2023

tjamet commented Sep 7, 2023

k8s-ci-robot commented Sep 8, 2023

tjamet commented Sep 8, 2023

k8s-ci-robot commented Sep 15, 2023

k8s-ci-robot commented Jan 3, 2024

mloiseleur commented Jan 16, 2024

k8s-triage-robot commented Apr 15, 2024

k8s-triage-robot commented May 15, 2024

oferz-everc commented May 23, 2024

k8s-triage-robot commented Jun 22, 2024

k8s-ci-robot commented Jun 22, 2024

oferz-everc commented Jun 24, 2024

tjamet commented Jul 8, 2024

k8s-ci-robot commented Jul 8, 2024

tjamet commented Feb 13, 2023 •

edited

Loading