[svc health check] allow configurable interval between checks.

[jeremymv2]

🔩 Description

Resolves #5466

This change will allow users to specify a health check interval at service load time by storing a health_check_interval_seconds in the Spec file for the service such as:

$ sudo cat /hab/sup/default/specs/elasticsearch.spec
ident = "core/elasticsearch"
group = "default"
bldr_url = "https://bldr.habitat.sh"
channel = "stable"
topology = "standalone"
update_strategy = "none"
binds = []
binding_mode = "strict"
desired_state = "up"
health_check_interval_seconds = 35

Features of the PR:

• add --health-check-interval to hab svc load ..
• validation enforces a u32 value between 1 and 86400 seconds
• Service or census changes will trigger all Services to re-run Health Check within 30s regardless if their configured interval is > 30s. The idea is to try to get an updated Health Check status within a reasonable time-frame from dependent services when an upstream pkg is updated.

Note I will follow up with test and doc updates, but first, I wanted to open this up for discussion before moving forward.

Related #5326
Related #5584

@christophermaier @mwrock @baumanj @raskchanky What are your thoughts on this implementation as a preliminary step to adding more usefulness to our Service Health Check feature?

Signed-off-by: Jeremy J. Miller [email protected]

✅ Checklist

• Necessary tests added/updated?
• Necessary docs added/updated?
• Code actually executed?
• Vetting performed (unit tests, lint, etc.)?

[thesentinels]

Thanks for the pull request! Here is what will happen next:

1. Your PR will be reviewed by the maintainers
2. If everything looks good, one of them will approve it, and your PR will be merged.

Thank you for contributing!

[baumanj]

I'm planning to look at this tomorrow and hope to have feedback for you by Monday.

[jeremymv2]

Looks like the Appveyor error was a Bldr API service unavailable.

[baumanj]

I re-started the AppVeyor job

[baumanj]

I haven't finished reading everything (will continue on Monday), but wanted to give you some early feedback. As usual, this is great work. The biggest thing so far that I'd like to see changed is all the u32s and health_check_interval_seconds to Durations and health_check_interval. There'll be some work to make Duration a type we can reference in the .proto files, but I think we can make that happen and it will be worthwhile for all the type safety that it buys.

[christophermaier]

@jeremymv2 Looking good so far, but I have some suggestions that can hopefully simplify things further. Let me know what you think.

Thanks!

[jeremymv2]

@cm @baumanj Thank you for the very helpful feedback. I believe I have addressed most of the items, but I'd still like to have some discussion around the finer details of this implementation.

HealthCheckInterval is now a custom core::Service type in habitat-sh/core#95 and therefore testing this PR will require pulling down the branch for the change in core and modifying https://github.com/habitat-sh/habitat/blob/master/Cargo.toml#L26-L32 locally to build. Additionally the core PR will need to be merged first in order for us to get a ✅ build here.

Before I move further with refining this PR and testing edge cases, I'd like to get your feedback again on this intermediary point with the new commits I've just added to ensure we're all in agreement. Thank you.

[baumanj]

Overall looks great. I'd like to simplify a few things and there may be one logic error.

[jeremymv2]

@christophermaier @baumanj some spare cycles popped up yesterday so I made some more progress. Please take a look at your convenience. As always, thank you!!

[baumanj]

The feedback changes look good; I think there's some core logic we can tighten up (or I'm failing to understand the complexity of the use case correctly)

[baumanj]

This looks great! I just had one small suggestion for nicer logging if you're so inclined, but I'm happy to approve this.

I'll plan to merge once our current release process is done.

[jeremymv2]

let's kick off a rebuild now since the PR to core merged. Could somebody with permissions help out with re-running please 😄 ?

[jeremymv2]

Once this merges, I will follow up with a doc update.

[baumanj]

Hey @jeremymv2, now that habitat-sh/core#95 is merged, I'm updating the habitat repo to include it: #6009.

Once that merges, can you rebase this so that the CI passes? Then I can merge this one.

[baumanj]

#6009 is merged now

[jeremymv2]

I've rebased; however I think we need to attempt a restart of the Appveyor tests.

[baumanj]

I've restarted the AppVeyor stuff. We should really get you access to do that yourself, @jeremymv2. You've certainly made a lot of valuable contributions.

[baumanj]

/me spins the AppVeyor wheel of fortune again

[baumanj]

AppVeyor timed out again. It was the packaging check, not the unit tests, so I'll force merge if it doesn't finish after one more try.