Fix for #368 #374

bitvector2 · 2017-10-31T18:26:22Z

All relevent changes are in aws.go

mikkeloscar · 2017-10-31T19:02:44Z

Could you please drop all the import changes from the commits?

For go projects it's a good idea to use the original repository in your go path but just add your fork as another remote, that way you don't have to change the imports when testing it locally.

bitvector2 · 2017-10-31T19:27:56Z

Will do... On Oct 31, 2017 12:02, Mikkel Oscar Lyderik Larsen <notifications@github.com> wrote: Could you please drop all the import changes from the commits? For go projects it's a good idea to use the original repository in your go path but just add your fork as another remote, that way you don't have to change the imports when testing it locally. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub<#374 (comment)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/AJAKlkJ1A-q8DHbtacApV32ze1yqExrFks5sx27VgaJpZM4QNNql>.

bitvector2 · 2017-10-31T19:51:15Z

stashed, reset import changes, stash applied, commit'd, pushed... should be good now..

njuettner · 2017-11-01T13:33:56Z

Thanks for your PR!

Maybe you can try to rewrite your code more like this:


func batchLimitChangeSet(changes []*route53.Change) []*route53.Change {
...
	for i := 0; i < len(changes); i += batchLimit {
		batch := changes[i:min(i+batchLimit, len(changes))]
		//do something
	}
...
}
// a + b should have better names
func min(a, b int) int {
	if a <= b {
		return a
	}
	return b
}

This would be easier to read it (at least for me).

batchLimit could be somewhere else like moved to const, maybe even more configurable (but here I'm totally open).

Also please add unit tests.

bitvector2 · 2017-11-01T14:07:38Z

The reason I wrote the function in the manner I did was to preserve the atomic nature of all related records by hostname. Currently all the A records are added first to the original CS, then all the TXT records are added to the CS and the entire batch is presumed atomic. If one were to just take the head or the tail of the original CS they would be violating the integrity that they were originally expecting. This said I'm happy to rework in any way desired.

bitvector2 · 2017-11-01T14:14:56Z

I think I can combine both ideas so it is an immediately complete scheme, as you wish.

ideahitme · 2017-11-02T15:05:25Z

provider/aws.go

@@ -254,6 +257,39 @@ func (p *AWSProvider) submitChanges(changes []*route53.Change) error {
 	return nil
 }

+func limitChangeSet(cs []*route53.Change) []*route53.Change {
+	limit := 4000


this should be a constant defined on the top of the file

ideahitme · 2017-11-02T15:09:19Z

provider/aws.go

+	for name, count := range countsByName {
+		if (remaining - count) > 0 {
+			remaining -= count
+			for _, v := range cs {


as a minor optimisation countsByName, could be changesByName, then you don't have to iterate over whole set to find and append the records by name

you lost me here - are you saying there already is a changesByName or are you saying for me to move my iteration out into its own function changesByName and call it instead?

the idea was to create a map changesByName instead of countsByName:

changesByName := map[string][]*route53.Change

when u need a count, u can do len(changesByName[name]), when u need to append:

limCs = append(limCs, changesByName[name]...)

ideahitme · 2017-11-02T15:09:59Z

please address two comments above, otherwise LGTM

bitvector2 · 2017-11-02T18:09:50Z

@ideahitme please merge if you approve

bitvector2 · 2017-11-02T23:38:52Z

done

________________________________ From: Yerken <notifications@github.com> Sent: Thursday, November 2, 2017 3:50 PM To: kubernetes-incubator/external-dns Cc: Steven Logue; Author Subject: Re: [kubernetes-incubator/external-dns] Fix for #368 (#374) @ideahitme commented on this pull request.

________________________________ In provider/aws.go<#374 (comment)>:

+ }

+ + log.Warningf("Initial change batch count is %d which exceeds %d", size, limit) + + countsByName := make(map[string]int) + for _, v := range cs { + countsByName[*v.ResourceRecordSet.Name]++ + } + + var newCs = []*route53.Change{} + + remaining := limit + for name, count := range countsByName { + if (remaining - count) > 0 { + remaining -= count + for _, v := range cs { the idea was to create a map changesByName instead of countsByName: changesByName := map[string][]*route53.Change — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub<#374 (comment)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/AJAKlleFrquIIY4ACOZ5D5maTXV_Wpq4ks5sykdAgaJpZM4QNNql>.

ideahitme · 2017-11-03T09:30:56Z

thank you, looks good to me, however it would also be nice to see tests for the limitChangeSet function and AWS ApplyChanges

bitvector2 · 2017-11-03T18:16:52Z

thanks for the feedback! - tests coming

:jackiechan:

bitvector2 · 2017-11-06T20:36:08Z

@ideahitme initial tests have been added - please comment

ideahitme · 2017-11-07T16:44:24Z

provider/aws_test.go

+	limCs := limitChangeSet(cs)
+
+	limitedCount := len(limCs)
+	assert.True(t, limitedCount == maxChangeCount, "limited changeset count=%d does not equal maxChangeCount=%d", limitedCount, maxChangeCount)


what I want to be sure of here, is that we do not accidentally end up with orphaned A or TXT record. I suggest to make the test more elaborate, for example by making limitChangeSet take maxChangeCount as a parameter. Consequently in test u can use a sane value like 4 or 6 and for test itself make sure that this limit is not exceeded and correct records are picked up.

additionally having submitChanges tested as well would be nice (test case with 4000+ records for one hosted zone)

ideahitme · 2017-11-09T09:20:51Z

provider/aws.go

+}
+
+func sortChangesByActionNameType(cs []*route53.Change) []*route53.Change {
+	sort.SliceStable(cs, func(i, j int) bool {


with this change travis builds will break on go1.7, but I think we can safely drop it from travis. Would u want to do it, or u want me to update it on this PR ?

ideahitme · 2017-11-09T10:57:33Z

two minor things:

Please add this to Changelog
Travis build is failing due to g1.8 sortstable. We should drop go1.7 support from travis.

otherwise LGTM

bitvector2 · 2017-11-09T18:09:36Z

@ideahitme - rebased, updated the changelog and travis and its all ready for you now...

szuecs · 2017-11-10T14:35:23Z

👍

Raffo · 2017-11-10T14:35:29Z

👍

Fix for kubernetes-sigs#368

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 31, 2017

bitvector2 mentioned this pull request Oct 31, 2017

AWS Route53 change batches need to be limited to at most 4000 elements #368

Closed

bitvector2 force-pushed the master branch from faf8177 to 1c43dbd Compare October 31, 2017 19:48

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 31, 2017

ideahitme reviewed Nov 2, 2017

View reviewed changes

bitvector2 force-pushed the master branch from e1c5678 to ba634f0 Compare November 2, 2017 17:57

bitvector2 force-pushed the master branch from 816e182 to b8705f5 Compare November 3, 2017 01:46

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Nov 6, 2017

ideahitme reviewed Nov 7, 2017

View reviewed changes

bitvector2 force-pushed the master branch 3 times, most recently from d3627c1 to f966288 Compare November 8, 2017 19:36

ideahitme reviewed Nov 9, 2017

View reviewed changes

added AWS API limit function and unit tests

1202cb2

bitvector2 force-pushed the master branch from 11cd81b to 1202cb2 Compare November 9, 2017 18:03

Raffo merged commit cc498e1 into kubernetes-sigs:master Nov 10, 2017

ffledgling pushed a commit to ffledgling/external-dns that referenced this pull request Jan 18, 2018

Merge pull request kubernetes-sigs#374 from bitvector2/master

01f379e

Fix for kubernetes-sigs#368

This was referenced Jun 27, 2019

ExternalDNS: reduce Route53 batch size to 100 zalando-incubator/kubernetes-on-aws#2266

Merged

Single broken entity breaks the whole batch #104

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix for #368 #374

Fix for #368 #374

bitvector2 commented Oct 31, 2017

mikkeloscar commented Oct 31, 2017

bitvector2 commented Oct 31, 2017 via email

bitvector2 commented Oct 31, 2017

njuettner commented Nov 1, 2017 •

edited

bitvector2 commented Nov 1, 2017 •

edited

bitvector2 commented Nov 1, 2017 •

edited

ideahitme Nov 2, 2017

bitvector2 Nov 2, 2017

ideahitme Nov 2, 2017

bitvector2 Nov 2, 2017

ideahitme Nov 2, 2017 •

edited

ideahitme commented Nov 2, 2017

bitvector2 commented Nov 2, 2017 •

edited

bitvector2 commented Nov 2, 2017 via email

ideahitme commented Nov 3, 2017 •

edited

bitvector2 commented Nov 3, 2017 •

edited

bitvector2 commented Nov 6, 2017

ideahitme Nov 7, 2017

ideahitme Nov 7, 2017

ideahitme Nov 9, 2017 •

edited

ideahitme commented Nov 9, 2017

bitvector2 commented Nov 9, 2017

szuecs commented Nov 10, 2017

Raffo commented Nov 10, 2017

Fix for #368 #374

Fix for #368 #374

Conversation

bitvector2 commented Oct 31, 2017

mikkeloscar commented Oct 31, 2017

bitvector2 commented Oct 31, 2017 via email

bitvector2 commented Oct 31, 2017

njuettner commented Nov 1, 2017 • edited

bitvector2 commented Nov 1, 2017 • edited

bitvector2 commented Nov 1, 2017 • edited

ideahitme Nov 2, 2017

Choose a reason for hiding this comment

bitvector2 Nov 2, 2017

Choose a reason for hiding this comment

ideahitme Nov 2, 2017

Choose a reason for hiding this comment

bitvector2 Nov 2, 2017

Choose a reason for hiding this comment

ideahitme Nov 2, 2017 • edited

Choose a reason for hiding this comment

ideahitme commented Nov 2, 2017

bitvector2 commented Nov 2, 2017 • edited

bitvector2 commented Nov 2, 2017 via email

ideahitme commented Nov 3, 2017 • edited

bitvector2 commented Nov 3, 2017 • edited

bitvector2 commented Nov 6, 2017

ideahitme Nov 7, 2017

Choose a reason for hiding this comment

ideahitme Nov 7, 2017

Choose a reason for hiding this comment

ideahitme Nov 9, 2017 • edited

Choose a reason for hiding this comment

ideahitme commented Nov 9, 2017

bitvector2 commented Nov 9, 2017

szuecs commented Nov 10, 2017

Raffo commented Nov 10, 2017

njuettner commented Nov 1, 2017 •

edited

bitvector2 commented Nov 1, 2017 •

edited

bitvector2 commented Nov 1, 2017 •

edited

ideahitme Nov 2, 2017 •

edited

bitvector2 commented Nov 2, 2017 •

edited

ideahitme commented Nov 3, 2017 •

edited

bitvector2 commented Nov 3, 2017 •

edited

ideahitme Nov 9, 2017 •

edited