Add config support for health, onChange, sensor timeouts

[tgross]

For #139

This PR provides configuration options for the health, onChange, and sensor handlers to take advantage of the timeout option provided by PR #184.

TODO: Done

• verify that I haven't reduced test coverage; the timeout options themselves have been well-tested previously so I just need to make sure the extra config is covered
• figure out what's going on in Have health checks timeout #139 (comment), which I haven't been able to reproduce locally.

cc @misterbisson @justenwalker

[tgross]

I've pushed a commit for extending our test coverage, so I think we're good there. Still having trouble reproducing #139 (comment)

[tgross]

Figured out that previous build failure was because the fix in a later commit wasn't included in that build. ref #139 (comment) for details.

[tgross]

Oops, I need to add docs for this feature now.

[tgross]

Ok I think this is ready for review.

[misterbisson]

Omitting this field means that ContainerPilot will wait indefinitely for the health check.

You probably debated this privately before deciding on it, but would the service's poll or ttl value be a more sane default? Yes, this default behavior matches the previous behavior, but is that most desirable?

[misterbisson]

This looks solid, but I have a question about the default behavior.

@justenwalker: have you had a look?

[justenwalker]

@misterbisson wrote:

You probably debated this privately before deciding on it, but would the service's poll or ttl value be a more sane default? Yes, this default behavior matches the previous behavior, but is that most desirable?

Probably the poll would be a sane default, since another health check would be queued up to run.

That being said, the behavior as it exists right now is consistent across all pollables. Making an exception to the rule for health checks might mean that the results are surprising - so there's that trade-off to consider. I'm just not sure what is least surprising - but I'd lean towards using poll as a default over waiting forever since I can't really come up with a good reason why I'd ever want that.

[misterbisson]

I need to explain to @tgross that I wasn't trying to draw @justenwalker into an argument. Rather, I was pinging him to make sure he saw the change in general.

As for the default timeout, here's what this change affects:

• health in services
• onChange in backends
• check in sensors

Each of those has a poll value that might be a suitable default. I don't have a strong commitment to changing it, just asking if defaulting to forever was intentional.

[tgross]

just asking if defaulting to forever was intentional.

I left it as forever for backwards compatibility. If we default to having a timeout where there was none previously then we might break someone's application. For example, currently autopilotpattern/mysql relies on the fact that it's not going to get timed out during health checks so that it can do the snapshot to Manta (will be changed in autopilotpattern/mysql#44). The primary will be marked as unhealthy during that time but it's intentional.

That being said, I'm absolutely in agreement that using the poll value as the default (as we did for task hooks) is the right approach in a future 3.0 release.

[misterbisson]

That being said, I'm absolutely in agreement that using the poll value as the default (as we did for task hooks) is the right approach in a future 3.0 release.

Well put. It's a plan. Should we ticket that and note it in the docs?

[tgross]

Should we ticket that and note it in the docs?

Will do.

[tgross]

Opened #206 for future work and added deprecation warning to documentation.

[misterbisson]

lgtm, 🏡 🚶

[tgross]

Ok, I'm going to merge this and cut the 2.4.0 release from it.