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

health check: add Redis custom health checker #2993

Merged
merged 34 commits into from
Apr 17, 2018
Merged

health check: add Redis custom health checker #2993

merged 34 commits into from
Apr 17, 2018

Conversation

dio
Copy link
Member

@dio dio commented Apr 4, 2018

health check: add Redis custom health checker

This PR is part of repo-reorg effort, to let cluster's health checker to pluggable.
The first provided custom health checker is the Redis health checker. By having this,
along with the Redis filter extension (which is a hard dep for the Redis custom health
checker), health checker can be compiled out.

The implementation of custom Redis health checker is copied from the existing
implementation inside the health_checker_impl.{h, cc}.

Risk Level: Low, since this is only a code movement.

Testing:
Unit tests.

Docs Changes:
envoyproxy/data-plane-api@e0e2c40

Release Notes:
envoyproxy/data-plane-api@f70dab8

Fixes #2933.

API Changes:
envoyproxy/data-plane-api@e0e2c40

Signed-off-by: Dhi Aurrahman dio@rockybars.com

dio added 4 commits April 4, 2018 20:59
Signed-off-by: Dhi Aurrahman <dio@rockybars.com>
Signed-off-by: Dhi Aurrahman <dio@rockybars.com>
Signed-off-by: Dhi Aurrahman <dio@rockybars.com>
Signed-off-by: Dhi Aurrahman <dio@rockybars.com>
@dio dio changed the title health checker: add Redis custom health checker health check: add Redis custom health checker Apr 4, 2018
Signed-off-by: Dhi Aurrahman <dio@rockybars.com>
@mattklein123
Copy link
Member

@dio can we do the PR in a single PR? I think if you copy the files GH should track it as a move and it should hopefully be pretty easy to review. I would rather not have the code living in two places at the same time. Thank you.

dio added 2 commits April 6, 2018 08:23
Signed-off-by: Dhi Aurrahman <dio@rockybars.com>
Signed-off-by: Dhi Aurrahman <dio@rockybars.com>
@dio
Copy link
Member Author

dio commented Apr 6, 2018

OK @mattklein123, I will update here when it is ready. Especially, regarding docs.

dio added 5 commits April 6, 2018 11:37
Signed-off-by: Dhi Aurrahman <dio@rockybars.com>
Signed-off-by: Dhi Aurrahman <dio@rockybars.com>
Signed-off-by: Dhi Aurrahman <dio@rockybars.com>
Signed-off-by: Dhi Aurrahman <dio@rockybars.com>
Signed-off-by: Dhi Aurrahman <dio@rockybars.com>
@dio
Copy link
Member Author

dio commented Apr 6, 2018

Really nervous about touching 20 files. But I think it is ready for a first pass. Thank you.

cc. @mattklein123

@@ -65,27 +58,6 @@ envoy::api::v2::core::HealthCheck createGrpcHealthCheckConfig() {
return health_check;
}

TEST(HealthCheckerFactoryTest, createRedis) {
std::string json = R"EOF(
{
Copy link
Contributor

Choose a reason for hiding this comment

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

are there any v1 tests remaining?

Copy link
Member Author

@dio dio Apr 9, 2018

Choose a reason for hiding this comment

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

I think I have moved Redis related test setups into test/extensions/health_checkers/redis/redis_test.cc and have it in V2.

Copy link
Contributor

Choose a reason for hiding this comment

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

the v1 api still supported though. so we need to test it.

cds_json.cc:54-59 is no longer covered: report

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, got it. Will have it then. Thank you for mentioning it.

Copy link
Member Author

Choose a reason for hiding this comment

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

It is covered now. cds_json.cc:54-59 of cd3398e.

dio added 3 commits April 10, 2018 05:41
Signed-off-by: Dhi Aurrahman <dio@rockybars.com>
Signed-off-by: Dhi Aurrahman <dio@rockybars.com>
Signed-off-by: Dhi Aurrahman <dio@rockybars.com>
danielhochman
danielhochman previously approved these changes Apr 10, 2018
Copy link
Contributor

@danielhochman danielhochman left a comment

Choose a reason for hiding this comment

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

looks good now. thanks for bringing back the test coverage.

Copy link
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

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

Thanks this is great. Some comments from a first pass.

@@ -64,6 +64,12 @@ envoy_cc_library(
],
)

envoy_cc_library(
name = "health_checker_interface",
Copy link
Member

Choose a reason for hiding this comment

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

nit: "health_checker_config_interface"

/**
* Creates a particular custom health checker factory implementation.
*
* @param config supplies the configuration for the health check, which should contains
Copy link
Member

Choose a reason for hiding this comment

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

"which should contains custom_health_check" is hard to follow. Rephrase?

(Edit from reading review, I know understand that the entire HC config is passed to the custom extension which can do its own config loading. If that's the case, which I think makes sense, do we really need the createEmptyConfigProto() function at all? The extension knows its config and can do its own direct conversion?) Can we clarify the docs here?

* @return HealthCheckerSharedPtr the pointer of a health checker instance.
*/
virtual Upstream::HealthCheckerSharedPtr
createCustomHealthChecker(const Protobuf::Message& config, Upstream::Cluster& cluster,
Copy link
Member

Choose a reason for hiding this comment

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

Instead of passing individual params here for cluster, runtime, random, dispatcher, etc. Can you make a FactoryContext interface similar to what we use for other filters (even if specific for HC). This will allow us to add methods there and not break filters.

Runtime::Loader& runtime, Runtime::RandomGenerator& random,
Event::Dispatcher& dispatcher) PURE;
/**
* @return ProtobufTypes::MessagePtr create empty config proto message which arrives in as an
Copy link
Member

Choose a reason for hiding this comment

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

typo "in as an"

virtual ProtobufTypes::MessagePtr createEmptyConfigProto() PURE;

/**
* @return std::string the identifying name for a particular implementation of an custom health
Copy link
Member

Choose a reason for hiding this comment

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

s/an/a

@@ -112,12 +112,10 @@ envoy_cc_library(
],
deps = [
":health_checker_base_lib",
"//include/envoy/server:health_checker_interface",
Copy link
Member

Choose a reason for hiding this comment

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

This is related to the comment I just dropped into #3045 but optimally we would not have common depend on server. It's not a big deal but can you add a TODO here and I might think about this as part of my repo reorg work.

case envoy::api::v2::core::HealthCheck::HealthCheckerCase::kGrpcHealthCheck:
if (!(cluster.info()->features() & Upstream::ClusterInfo::Features::HTTP2)) {
throw EnvoyException(fmt::format("{} cluster must support HTTP/2 for gRPC healthchecking",
cluster.info()->name()));
}
return std::make_shared<ProdGrpcHealthCheckerImpl>(cluster, hc_config, dispatcher, runtime,
random);
// Deprecates redis_health_check.
Copy link
Member

Choose a reason for hiding this comment

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

Please add to deprecated.md file. Also please update https://github.com/envoyproxy/envoy/blob/master/REPO_LAYOUT.md to discuss the new directory structure for health checkers.

namespace RedisHealthChecker {

Upstream::HealthCheckerSharedPtr RedisHealthCheckerFactory::createCustomHealthChecker(
const Protobuf::Message& config, Upstream::Cluster& cluster, Runtime::Loader& runtime,
Copy link
Member

Choose a reason for hiding this comment

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

Can't we just pass envoy::api::v2::core::HealthCheck directly to the creation function?


return std::make_shared<RedisHealthChecker>(
cluster, hc_config,
hc_config.has_redis_health_check()
Copy link
Member

Choose a reason for hiding this comment

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

can you put deprecation note here also

dio added 6 commits April 11, 2018 21:24
#2993 (comment)

Signed-off-by: Dhi Aurrahman <dio@rockybars.com>
Signed-off-by: Dhi Aurrahman <dio@rockybars.com>
#2993 (comment)

Signed-off-by: Dhi Aurrahman <dio@rockybars.com>
#2993 (comment)

Signed-off-by: Dhi Aurrahman <dio@rockybars.com>
#2993 (comment)

Signed-off-by: Dhi Aurrahman <dio@rockybars.com>
#2993 (comment)

Signed-off-by: Dhi Aurrahman <dio@rockybars.com>
dio added 2 commits April 12, 2018 22:06
Signed-off-by: Dhi Aurrahman <dio@rockybars.com>
Signed-off-by: Dhi Aurrahman <dio@rockybars.com>
Signed-off-by: Dhi Aurrahman <dio@rockybars.com>
@dio
Copy link
Member Author

dio commented Apr 12, 2018

@mattklein123 do you think we should put "warning is logged" as a default behaviour on deprecating a feature/config (unless it is stated not to)? Since in current deprecated list for 1.7.0, almost every item has it.

@mattklein123
Copy link
Member

@dio sure makes sense. Do you want to update CONTRIBUTING.md (or wherever makes sense) in a different PR?

dio added 2 commits April 13, 2018 06:30
Signed-off-by: Dhi Aurrahman <dio@rockybars.com>
Signed-off-by: Dhi Aurrahman <dio@rockybars.com>
@mattklein123
Copy link
Member

@dio thanks for your patience here. There is one more repo reorg change coming that will effect this in a small way, then we can review/get this in. I will let you know.

@@ -324,5 +324,16 @@ class TransportSocketNameValues {

typedef ConstSingleton<TransportSocketNameValues> TransportSocketNames;

/**
Copy link
Member

Choose a reason for hiding this comment

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

Please see how I broke out well_known_names in #3075. Can you replicate that structure when you merge master? Thank you!

Copy link
Member Author

Choose a reason for hiding this comment

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

@mattklein123 OK. I have it on following commits. I hope it is fine to require ext well-known names for a while from upstream health checker (until redis_health_check is removed).

dio added 3 commits April 14, 2018 06:56
Signed-off-by: Dhi Aurrahman <dio@rockybars.com>
Signed-off-by: Dhi Aurrahman <dio@rockybars.com>
Signed-off-by: Dhi Aurrahman <dio@rockybars.com>
htuch
htuch previously approved these changes Apr 16, 2018
Copy link
Member

@htuch htuch left a comment

Choose a reason for hiding this comment

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

LGTM, this is a really great; useful feature and very clean PR. I'll defer to @danielhochman on the Redis aspect, I assume it's mostly code movement.

danielhochman
danielhochman previously approved these changes Apr 16, 2018
Copy link
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

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

This is some fantastic, thanks. Just some small typo catches (maybe do another quick pass just to see if I missed any similar ones).

@@ -0,0 +1,18 @@
licenses(["notice"]) # Apache 2
# Redis custom health checker.
Copy link
Member

Choose a reason for hiding this comment

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

Delete comment (I think just copy/paste mistake)

return std::make_shared<RedisHealthChecker>(
context.cluster(), config, getRedisHealthCheckConfig(config), context.dispatcher(),
context.runtime(), context.random(),
Extensions::NetworkFilters::RedisProxy::ConnPool::ClientFactoryImpl::instance_);
Copy link
Member

Choose a reason for hiding this comment

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

nit: You can remove Extensions:: prefix as you are already in Extensions namespace.

namespace HealthCheckers {

/**
* Well-known access logger names.
Copy link
Member

Choose a reason for hiding this comment

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

s/access logger/health checker


/**
* Well-known access logger names.
* NOTE: New access loggers should use the well known name: envoy.access_loggers.name.
Copy link
Member

Choose a reason for hiding this comment

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

envoy.health_checkers.name

class HealthCheckerNameValues {
public:
// Redis health checker.
const std::string REDIS_HEALTH_CHECKER = "envoy.health_checker.redis";
Copy link
Member

Choose a reason for hiding this comment

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

"envoy.health_checkers.redis" (plural like the others)

dio added 2 commits April 17, 2018 05:28
Signed-off-by: Dhi Aurrahman <dio@rockybars.com>
Signed-off-by: Dhi Aurrahman <dio@rockybars.com>
@dio dio dismissed stale reviews from danielhochman and htuch via d11162a April 16, 2018 22:37
@dio
Copy link
Member Author

dio commented Apr 16, 2018

@mattklein123 thanks for catching the copy-paste mistakes! Fixed it in d11162a.

@mattklein123
Copy link
Member

@dio you got hit by the GH missing commit bug, can you push an empty commit? cc @htuch

Signed-off-by: Dhi Aurrahman <dio@rockybars.com>
@dio
Copy link
Member Author

dio commented Apr 17, 2018

@mattklein123 it seems OK now. Thanks!

dio added 2 commits April 17, 2018 14:52
Signed-off-by: Dhi Aurrahman <dio@rockybars.com>
Ref: envoyproxy/data-plane-api#610

Signed-off-by: Dhi Aurrahman <dio@rockybars.com>
Copy link
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

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

Awesome work. Thank you @dio!

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