provider: request_timeout is ignored

[rjeczalik]

Currently request_timeout (configured here [0]) is overwritten with deployment_timeout [1].

Two questions:

• should the effective timeout be min(request_timeout, deployment_timeout)?
• if request_timeout < deployment_timeout (for my use-case deploying takes 2m30s) then apply is going to fail, while the actual deployment is in progress; moreover if request_timeout > 60s then apply is also going to fail, due to hard-coded timeouts for event listener; I did not check whether go-marathon is using streaming api vs rest api, assuming the later shouldn't the request_timeout affect only plain http request/response and not span over deployment operation? if the former, shouldn't the (*http.Client).Timeout be unset (0) and we implement the timeout by polling the deployment status? Currently it's not possible to distinct temporary networking errors from marathon api timeouts.

[0] https://github.com/Banno/terraform-provider-marathon/blob/1ccf8237686780c3e77fefe173293de31199b7ea/marathon/provider.go#L66-L68
[1] https://github.com/Banno/terraform-provider-marathon/blob/1ccf8237686780c3e77fefe173293de31199b7ea/marathon/provider.go#L76-L78

[adamdecaf]

@rjeczalik I've got a feeling that this was accidently overridden for the case when we want to watch the deployment. Rather, we should probably have a different timeout when watching the deploy.

[adamdecaf]

It looks like go-marathon is pulling in github.com/donovanhide/eventsource which is using server push to wait on events. I'd have to check, but is http.Client looking at Timeout to close the connection if no events are gathered in that time?

[rjeczalik]

I'd have to check, but is http.Client looking at Timeout to close the connection if no events are gathered in that time?

Looks like eventsource uses some XHR for polling on events. Yes, http.Client cancels http request after Timeout is reached (if Timeout != 0). In case of streaming API Timeout should not be used if the library does not automatically reconnect (in case of temporary network errors).

[adamdecaf]

We likely should create a different http client for reading off the event stream then.

https://github.com/Banno/terraform-provider-marathon/blob/master/marathon/resource_marathon_app.go#L413

The provider's client could be used for single get/post/put/delete events.