Add an ECS provider #1088
Add an ECS provider #1088
Conversation
|
+1 |
There was a problem hiding this comment.
Wow @lpetre thanks a lot 😍 !
Few comments here and there. Coud you complete the sample toml file?
My main concern is on goroutines management in provider/ecs.go. Not sure it will behave correctly on the long term (possible memory leak).
Other than that, looks good :)
/cc @containous/traefik
provider/ecs.go
Outdated
| sess := session.New() | ||
| ec2meta := ec2metadata.New(sess) | ||
| if provider.Region == "" { | ||
| log.Printf("No EC2 region provided, querying instance metadata endpoint...") |
There was a problem hiding this comment.
Can you use a log level instead ?
provider/ecs.go
Outdated
| }() | ||
|
|
||
| operation := func() error { | ||
| select { |
There was a problem hiding this comment.
Why did you add a select clause here?
There was a problem hiding this comment.
In the case where the stop channel has been sent a value, I cancel the context, my operation will return context.Canceled, and backoff.RetryNotify will call my operation again. So I handle that here.
There was a problem hiding this comment.
Isn't this use case already handled by https://github.com/containous/traefik/pull/1088/files#diff-20ed89f06f1c428e51205096690cb934R149? Besides, You are not waiting anything here, you will just pass one time.
There was a problem hiding this comment.
There was a problem hiding this comment.
I agree with your comment that this https://github.com/containous/traefik/pull/1088/files#diff-20ed89f06f1c428e51205096690cb934R149 also handles the case, but only if we're waiting for the refresh timer. If we had been in the middle of an AWS http request, that is when the select was useful.
provider/ecs.go
Outdated
| notify := func(err error, time time.Duration) { | ||
| log.Errorf("ECS connection error %+v, retrying in %s", err, time) | ||
| } | ||
| err := backoff.RetryNotify(operation, job.NewBackOff(backoff.NewExponentialBackOff()), notify) |
There was a problem hiding this comment.
Could you wrap it with safe.OperationWithRecover(operation), in case of a panic in the goroutine?
|
Thanks for the feedback @emilevauge. Regarding the goroutines in Otherwise there is a goroutine owned by the pool that is responsible for calling my I don't think there is a way for goroutines to leak here. |
|
Wow @lpetre. Thanks a lot! Beside the little thing of @emilevauge , I'm good :) LGTM 👼 |
|
Hey @lpetre , thanks for all your help! I'll review your latest commits soon. One thing upfront: There are conflicts in your glide files. Could you resolve them and rebase your branch to only have 1 commit? :) |
|
@SantoDE no problem, I'll do that as soon as I'm back at my computer. |
|
@SantoDE this is rebased and up to date with latest master |
|
@lpetre thanks a lot for you reactivity 👏 |
I need to do a couple things, but I wanted to open this PR for discussion.
TODO: