Chunk taskArns into groups of 100 #1209

owen · 2017-03-01T18:08:58Z

If the ECS cluster has > 100 tasks, passing them to
ecs.DescribeTasksRequest() will result in the AWS API returning
errors.

This patch breaks them into chunks of at most 100, and calls
DescribeTasks for each chunk.

timoreimann · 2017-03-02T01:26:59Z

provider/ecs.go

+			sliceEnd = len(taskArns)
+		}
+		split_taskreq = append(splitTaskReq, taskArns[i:sliceEnd])
+	}


Can we move the splitting logic into a small function, e.g.,

func chunkedTaskArns(taskArns []*string) [][]*string

?

Some tests on the newly created function would also be nice to cover the representative cases (e.g., 0, 1, 99, 100, 101, 200, 400).

good thinking! I'll catch that :)

also, my git-fu is weak and I managed to commit a broken patch :( fixes soon.

emilevauge · 2017-03-02T16:32:26Z

@owen shouldn't we rebase this against v1.2 branch to include it in the next release?

timoreimann · 2017-03-06T11:58:23Z

@owen how's your git-fu going today? ;)

also, do you want to rebase against 1.2 as @emilevauge suggested? The bugfix seems fairly important for ECS users.

owen · 2017-03-06T21:12:12Z

it's woeful! I'm king of the force push I guess.

I've:

rebased against v1.2
split to a small helper function
added test cases around splittable values

and will be force pushing again as soon as I ensure it passes a make validate

Should I keep a PR against master with this code, or do you all typically pull v1.2 fixes back in? not sure how that typically works for this project.
(and oh look, more git questions, hehe)

timoreimann · 2017-03-06T21:45:19Z

@owen we typically sync back the v1.2 branch with master regularly (usually on release of a release candidate / final version), so there should be no need to maintain a separate branch against master.

piteur · 2017-03-07T09:52:23Z

provider/ecs.go

-		Tasks:   taskArns,
-		Cluster: &provider.Cluster,
-	})
+	var splitTaskReq [][]*string


Simple comment - as an outsider interrested on this fix:

Is adding a comment to explain why we split the content & do multiple calls will add any value for future maintenance ?
Without knowing the API limitation - or not having the PR context in mind - the 100 split value can be interpreted as coming from nowhere / seems a bit magical.

+1 on that one; plus, moving the logic into a dedicated function with an informative name (as suggested previously :-) )

I think @owen is already working hard on this one. :-)

owen · 2017-03-07T15:33:40Z

A test case failure I observed last night made me realize that pointing Traefik to an empty cluster would result in bad AWS API calls being made. I've gone ahead and picked up that fix by adding an early return after the first call to ListTasks.

This now works for my empty cluster, and my ~hundred instance cluster. Tests pass, make validate looks good, and it only took one more force push to get it to go up 😬

@timoreimann

owen · 2017-03-07T20:53:29Z

@timoreimann / @emilevauge - I'm wondering if I broke your CI environment with my, er, aggressive Git usage.

Let me know if I can help :(

emilevauge · 2017-03-07T21:23:00Z

@owen yeah, it seems Travis needs a holiday... Could you push force again?

owen · 2017-03-07T21:34:45Z

Looks like the CI is building now. I added a few more tests to wake up the CI pipeline.

Apologies... I actually just learned that "rebase master -> v1.2" is as easy as a click at the top of the pull request. That would've skipped at least one force push on my end, heh.
(I have much to learn about GitHub ;-))

timoreimann

A few very small suggestions left; after that, we should be good. :-)

timoreimann · 2017-03-07T22:54:36Z

provider/ecs.go

-		Tasks:   taskArns,
-		Cluster: &provider.Cluster,
-	})
+	if len(taskArns) == 0 {


Can we add a short comment here on why we return early?

timoreimann · 2017-03-07T23:07:54Z

provider/ecs_test.go

+		}
+
+		if !reflect.DeepEqual(outCount, c.expectedLengths) {
+			t.Fatalf("Chunking %d elements, expected %#v, got %#v (case %d)", c.count, c.expectedLengths, outCount, i)


I think we should do t.Errorf here: If one case fails, we still want to see the result of the other cases.

I also think we can drop the index variable i here. c.count already suffices to identify the failing case unambiguously.

Not sure why I Fatalf'ed there rather than Errorf'ed - fixed :)

timoreimann · 2017-03-08T01:34:42Z

@owen you haven't go fmt'ed your latest commit -- please run make fmt to fix that. Apart from that, LGTM! ❤️

Could you also squash your commits? After that and another successful CI run, I think we are good to go/merge. :-)

If the ECS cluster has > 100 tasks, passing them to ecs.DescribeTasksRequest() will result in the AWS API returning errors. This patch breaks them into chunks of at most 100, and calls DescribeTasks for each chunk. We also return early in case ListTasks returns no values; this prevents DescribeTasks from throwing HTTP errors.

owen · 2017-03-08T14:43:02Z

I fear Travis may not be happy with me again:

[ERROR]	Update failed for gopkg.in/fsnotify.v1: Cannot detect VCS
[ERROR]	Failed to install: Cannot detect VCS
The command '/bin/sh -c cd integration && glide install' returned a non-zero code: 1
make: *** [build] Error 1

timoreimann · 2017-03-08T14:55:53Z

gopkg.in is flaking again. Retriggered the build.

timoreimann · 2017-03-08T21:36:18Z

Grrr, and now the integration tests are flaking. :(

Retrying HARDER...

emilevauge · 2017-03-08T21:48:19Z

emilevauge

LGTM
Thanks @owen 👍

owen mentioned this pull request Mar 1, 2017

An ECS cluster with > 100 tasks cannot be enumerated #1210

Closed

timoreimann suggested changes Mar 2, 2017

View reviewed changes

owen force-pushed the ecs-chunk-taskarns branch from 605c59c to b9536db Compare March 2, 2017 01:41

emilevauge added the status/2-needs-review label Mar 3, 2017

piteur reviewed Mar 7, 2017

View reviewed changes

owen force-pushed the ecs-chunk-taskarns branch from b9536db to b796eb9 Compare March 7, 2017 15:32

owen changed the base branch from master to v1.2 March 7, 2017 15:33

timoreimann suggested changes Mar 7, 2017

View reviewed changes

owen force-pushed the ecs-chunk-taskarns branch from 17e7775 to 11a68ce Compare March 8, 2017 01:54

timoreimann approved these changes Mar 8, 2017

View reviewed changes

emilevauge approved these changes Mar 9, 2017

View reviewed changes

emilevauge added this to the 1.2 milestone Mar 9, 2017

emilevauge added status/3-needs-merge and removed status/2-needs-review labels Mar 9, 2017

emilevauge merged commit ee9032f into traefik:v1.2 Mar 9, 2017

emilevauge mentioned this pull request Mar 9, 2017

Make the CI great again! #1266

Closed

5 tasks

ldez added the kind/enhancement a new or improved feature. label Apr 29, 2017

ldez removed the status/3-needs-merge label May 19, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Chunk taskArns into groups of 100 #1209

Chunk taskArns into groups of 100 #1209

owen commented Mar 1, 2017

timoreimann Mar 2, 2017

timoreimann Mar 2, 2017

owen Mar 2, 2017

owen Mar 2, 2017

emilevauge commented Mar 2, 2017 •

edited

timoreimann commented Mar 6, 2017

owen commented Mar 6, 2017

timoreimann commented Mar 6, 2017

piteur Mar 7, 2017

timoreimann Mar 7, 2017 •

edited

owen commented Mar 7, 2017

owen commented Mar 7, 2017

emilevauge commented Mar 7, 2017

owen commented Mar 7, 2017

timoreimann left a comment

timoreimann Mar 7, 2017

owen Mar 8, 2017

timoreimann Mar 7, 2017

timoreimann Mar 7, 2017

owen Mar 8, 2017

timoreimann commented Mar 8, 2017

owen commented Mar 8, 2017

timoreimann commented Mar 8, 2017 •

edited

timoreimann commented Mar 8, 2017

emilevauge commented Mar 8, 2017

emilevauge left a comment

Chunk taskArns into groups of 100 #1209

Chunk taskArns into groups of 100 #1209

Conversation

owen commented Mar 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emilevauge commented Mar 2, 2017 • edited

timoreimann commented Mar 6, 2017

owen commented Mar 6, 2017

timoreimann commented Mar 6, 2017

Choose a reason for hiding this comment

timoreimann Mar 7, 2017 • edited

Choose a reason for hiding this comment

owen commented Mar 7, 2017

owen commented Mar 7, 2017

emilevauge commented Mar 7, 2017

owen commented Mar 7, 2017

timoreimann left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timoreimann commented Mar 8, 2017

owen commented Mar 8, 2017

timoreimann commented Mar 8, 2017 • edited

timoreimann commented Mar 8, 2017

emilevauge commented Mar 8, 2017

emilevauge left a comment

Choose a reason for hiding this comment

emilevauge commented Mar 2, 2017 •

edited

timoreimann Mar 7, 2017 •

edited

timoreimann commented Mar 8, 2017 •

edited