buildkite / lifecycled

A daemon for responding to AWS AutoScaling Lifecycle Hooks
MIT License
146 stars 34 forks source link

Respect http timeouts and context with termination times #36

Closed lox closed 6 years ago

lox commented 6 years ago

As per #34, we are leaking sockets.

This is another take on refactoring the spot termination time polling that respects the context.Context that we've introduced and also uses an http client with timeouts.

Thoughts @itsdalmo?

itsdalmo commented 6 years ago

I think out of the two options you have made PR's for, I prefer #41. Primarily because it's less code to maintain in lifecycled (even if the code is tested). In the other PR you mentioned:

The downside here is that it doesn't respect timeouts and I think the default behaviour is for no automatic retries.

Retries

The ec2metadata is only used in two places:

Timeouts

It seems we can pass a non-default HTTP client in via the config.

Context

Don't think this is critical for non-long-polling requests? Could we opt to go with a shorter timeout on the client instead?

Also, if you want to go with this PR. Would it be an idea to rename the function from getTerminationTime to getMetadata and make it the generic solution for both termination time and instance id?

lox commented 6 years ago

Going with #41.