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

Ensure maximum run interval #1543

Closed
wants to merge 4 commits into from

Conversation

sheerun
Copy link
Contributor

@sheerun sheerun commented Apr 22, 2020

This pull request adds additional mechanism by which we can be sure synchronization won't run:

  1. More than once per Interval
  2. More than one at a time

This is done by keeping track of whether RunOnce is already running, and what is next possible run time (i.e. one Interval since last time RunOnce has been called).

This can fix various issue like current one that causes RunOnce to be called twice when external-dns configured with --events runs, or multiple overlapping updates that can cause issues like #1463, especially if update time is for some reason longer than Interval (many records, network timeouts etc.)

I've attached tests that test this behavior. I can also attest this works on real cluster (docker image at sheerun/external-dns:v0.7.1-54-g98ac8302).

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Apr 22, 2020
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: sheerun
To complete the pull request process, please assign raffo
You can assign the PR to them by writing /assign @raffo in a comment when ready.

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

Needs approval from an approver in each of these files:

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

@sheerun
Copy link
Contributor Author

sheerun commented Apr 22, 2020

/assign @Raffo

Copy link
Contributor

@Raffo Raffo left a comment

Choose a reason for hiding this comment

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

Thank you for your PR @sheerun ! I added some comments.

controller/controller.go Outdated Show resolved Hide resolved
controller/controller.go Outdated Show resolved Hide resolved
controller/controller.go Outdated Show resolved Hide resolved
testing := false
if v, ok := ctx.Value(nowValue).(time.Time); ok {
now = v
testing = true
Copy link
Contributor

Choose a reason for hiding this comment

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

What about having an early return here by testing the condition on line 131 instead of having multiple if statements like in line 144?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Both ifs are on purpose the one on line 134 represents case when function is throttled, the one on line 144 represents case when function executes normally. We need both to properly test this function in tests. Please note that in one case error is returned and in other nil (error returned in tests means that function is trottled, nil returned means no throttling).

Co-Authored-By: Raffaele Di Fazio <raffo@github.com>
@Raffo
Copy link
Contributor

Raffo commented May 8, 2020

@sheerun do you want to have another look at this one?

@sheerun
Copy link
Contributor Author

sheerun commented May 8, 2020

I think one more case needs to be handled: when RunOnceThottled is called during another executing of RunOnceThrottled is running, then we should call it one more time after first one finishes (e.g. if ingress is changed during updating of records, we should run reconciliation once again). Additional logic is also needed when ingress is updated multiple times (or multiple ingresses are updated) during updating of dns records.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants