Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add healthcheck_mode to service config #194

Merged
merged 1 commit into from
Feb 2, 2016

Conversation

fede1024
Copy link
Contributor

The healthcheck_mode option was added to nerve-tools, but it is not actually loaded from the config file since the 'healthcheck_mode' key is not in the whitelist.

@@ -495,6 +496,7 @@ def load_service_namespace_config(service, namespace, soa_dir=DEFAULT_SOA_DIR):
# and there's other things that appear in the smartstack section in
# several cases.
key_whitelist = set([
'healthcheck_mode',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm surprised this is it? Don't we need something like line 522?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't took like that's necessary because line 515 adds everything in the whitelist to the configuration.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

configure_nerve in nerve tools will use this config, and it will default to the mode if it's not present.

@solarkennedy
Copy link
Contributor

@fede1024 I'm ok with the small change, but in paasta if a service specifies this mode, seems like we should take advantage of it and adjust our healthchecks accordingly?

@Rob-Johnson
Copy link
Contributor

I think this is ok to ship.

@solarkennedy here is the corresponding commit for nerve-tools.

This is a consequence of configure_nerve using paasta as a library, particularly marathon_tools.get_services_runnning_here_for_nerve. That in turn uses this key_whitelist to filter the list of keys returned from the s_c_l, so @fede1024 needed to add healthcheck_mode to that whitelist so nerve can start using it.

in paasta if a service specifies this mode, seems like we should take advantage of it and adjust our
healthchecks accordingly?

I think we're actually okay here. We already use the healthcheck_mode param used, it was nerve that didn't have that capability. This actually brings nerve healthchecking and marathon healthchecking in line with eachother.

@solarkennedy
Copy link
Contributor

@fede1024 can you merge/rebase master and we can see how the tests look?

@fede1024 fede1024 force-pushed the SMTSTK-153_healthcheck_mode branch from 9113bc5 to 7b4d8a1 Compare February 2, 2016 19:37
@fede1024
Copy link
Contributor Author

fede1024 commented Feb 2, 2016

It's now rebased on master. Unit tests and integration tests look fine on my box.

solarkennedy added a commit that referenced this pull request Feb 2, 2016
Add healthcheck_mode to service config
@solarkennedy solarkennedy merged commit a8772e4 into master Feb 2, 2016
@solarkennedy
Copy link
Contributor

Merged.

@solarkennedy solarkennedy deleted the SMTSTK-153_healthcheck_mode branch February 2, 2016 20:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants