Repository: mesos Updated Branches: refs/heads/master 47cdd5a7b -> b50f6c8af
Moved `validation::healthCheck` to common code. The master validation uses `validation::healthCheck`, so move it to common validation library so that the master doesn't have a dependency on the checker library. Review: https://reviews.apache.org/r/67794/ Project: http://git-wip-us.apache.org/repos/asf/mesos/repo Commit: http://git-wip-us.apache.org/repos/asf/mesos/commit/0fa39352 Tree: http://git-wip-us.apache.org/repos/asf/mesos/tree/0fa39352 Diff: http://git-wip-us.apache.org/repos/asf/mesos/diff/0fa39352 Branch: refs/heads/master Commit: 0fa39352efe8e8df8fa50f76441cdb4c6b7a74ad Parents: 47cdd5a Author: James Peach <[email protected]> Authored: Tue Jul 3 20:54:32 2018 +1000 Committer: James Peach <[email protected]> Committed: Tue Jul 3 20:54:32 2018 +1000 ---------------------------------------------------------------------- src/checks/health_checker.cpp | 97 +---------------------------------- src/checks/health_checker.hpp | 10 ---- src/common/validation.cpp | 91 ++++++++++++++++++++++++++++++++ src/common/validation.hpp | 2 + src/master/validation.cpp | 3 +- src/tests/health_check_tests.cpp | 34 ++++++------ 6 files changed, 115 insertions(+), 122 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/mesos/blob/0fa39352/src/checks/health_checker.cpp ---------------------------------------------------------------------- diff --git a/src/checks/health_checker.cpp b/src/checks/health_checker.cpp index 4975495..833f3a1 100644 --- a/src/checks/health_checker.cpp +++ b/src/checks/health_checker.cpp @@ -182,7 +182,7 @@ Try<Owned<HealthChecker>> HealthChecker::create( Variant<runtime::Plain, runtime::Docker, runtime::Nested> runtime) { // Validate the 'HealthCheck' protobuf. - Option<Error> error = validation::healthCheck(healthCheck); + Option<Error> error = common::validation::validateHealthCheck(healthCheck); if (error.isSome()) { return error.get(); } @@ -337,101 +337,6 @@ void HealthChecker::success() consecutiveFailures = 0; } - -namespace validation { - -Option<Error> healthCheck(const HealthCheck& healthCheck) -{ - if (!healthCheck.has_type()) { - return Error("HealthCheck must specify 'type'"); - } - - switch (healthCheck.type()) { - case HealthCheck::COMMAND: { - if (!healthCheck.has_command()) { - return Error("Expecting 'command' to be set for COMMAND health check"); - } - - const CommandInfo& command = healthCheck.command(); - - if (!command.has_value()) { - string commandType = - (command.shell() ? "'shell command'" : "'executable path'"); - - return Error("Command health check must contain " + commandType); - } - - Option<Error> error = - common::validation::validateCommandInfo(command); - if (error.isSome()) { - return Error( - "Health check's `CommandInfo` is invalid: " + error->message); - } - - // TODO(alexr): Make sure irrelevant fields, e.g., `uris` are not set. - - break; - } - case HealthCheck::HTTP: { - if (!healthCheck.has_http()) { - return Error("Expecting 'http' to be set for HTTP health check"); - } - - const HealthCheck::HTTPCheckInfo& http = healthCheck.http(); - - if (http.has_scheme() && - http.scheme() != "http" && - http.scheme() != "https") { - return Error( - "Unsupported HTTP health check scheme: '" + http.scheme() + "'"); - } - - if (http.has_path() && !strings::startsWith(http.path(), '/')) { - return Error( - "The path '" + http.path() + - "' of HTTP health check must start with '/'"); - } - - break; - } - case HealthCheck::TCP: { - if (!healthCheck.has_tcp()) { - return Error("Expecting 'tcp' to be set for TCP health check"); - } - - break; - } - case HealthCheck::UNKNOWN: { - return Error( - "'" + HealthCheck::Type_Name(healthCheck.type()) + "'" - " is not a valid health check type"); - } - } - - if (healthCheck.has_delay_seconds() && healthCheck.delay_seconds() < 0.0) { - return Error("Expecting 'delay_seconds' to be non-negative"); - } - - if (healthCheck.has_grace_period_seconds() && - healthCheck.grace_period_seconds() < 0.0) { - return Error("Expecting 'grace_period_seconds' to be non-negative"); - } - - if (healthCheck.has_interval_seconds() && - healthCheck.interval_seconds() < 0.0) { - return Error("Expecting 'interval_seconds' to be non-negative"); - } - - if (healthCheck.has_timeout_seconds() && - healthCheck.timeout_seconds() < 0.0) { - return Error("Expecting 'timeout_seconds' to be non-negative"); - } - - return None(); -} - -} // namespace validation { - } // namespace checks { } // namespace internal { } // namespace mesos { http://git-wip-us.apache.org/repos/asf/mesos/blob/0fa39352/src/checks/health_checker.hpp ---------------------------------------------------------------------- diff --git a/src/checks/health_checker.hpp b/src/checks/health_checker.hpp index b3f508e..d6cecc0 100644 --- a/src/checks/health_checker.hpp +++ b/src/checks/health_checker.hpp @@ -104,16 +104,6 @@ private: process::Owned<CheckerProcess> process; }; - -namespace validation { - -// TODO(alexr): A better place for this function would be something like -// "mesos_validation.cpp", since it validates API protobuf which is not -// solely related to the health checking library. -Option<Error> healthCheck(const HealthCheck& check); - -} // namespace validation { - } // namespace checks { } // namespace internal { } // namespace mesos { http://git-wip-us.apache.org/repos/asf/mesos/blob/0fa39352/src/common/validation.cpp ---------------------------------------------------------------------- diff --git a/src/common/validation.cpp b/src/common/validation.cpp index 74450df..e7988ce 100644 --- a/src/common/validation.cpp +++ b/src/common/validation.cpp @@ -302,6 +302,97 @@ Option<Error> validateGpus(const RepeatedPtrField<Resource>& resources) return None(); } + +Option<Error> validateHealthCheck(const HealthCheck& healthCheck) +{ + if (!healthCheck.has_type()) { + return Error("HealthCheck must specify 'type'"); + } + + switch (healthCheck.type()) { + case HealthCheck::COMMAND: { + if (!healthCheck.has_command()) { + return Error("Expecting 'command' to be set for COMMAND health check"); + } + + const CommandInfo& command = healthCheck.command(); + + if (!command.has_value()) { + string commandType = + (command.shell() ? "'shell command'" : "'executable path'"); + + return Error("Command health check must contain " + commandType); + } + + Option<Error> error = + common::validation::validateCommandInfo(command); + if (error.isSome()) { + return Error( + "Health check's `CommandInfo` is invalid: " + error->message); + } + + // TODO(alexr): Make sure irrelevant fields, e.g., `uris` are not set. + + break; + } + case HealthCheck::HTTP: { + if (!healthCheck.has_http()) { + return Error("Expecting 'http' to be set for HTTP health check"); + } + + const HealthCheck::HTTPCheckInfo& http = healthCheck.http(); + + if (http.has_scheme() && + http.scheme() != "http" && + http.scheme() != "https") { + return Error( + "Unsupported HTTP health check scheme: '" + http.scheme() + "'"); + } + + if (http.has_path() && !strings::startsWith(http.path(), '/')) { + return Error( + "The path '" + http.path() + + "' of HTTP health check must start with '/'"); + } + + break; + } + case HealthCheck::TCP: { + if (!healthCheck.has_tcp()) { + return Error("Expecting 'tcp' to be set for TCP health check"); + } + + break; + } + case HealthCheck::UNKNOWN: { + return Error( + "'" + HealthCheck::Type_Name(healthCheck.type()) + "'" + " is not a valid health check type"); + } + } + + if (healthCheck.has_delay_seconds() && healthCheck.delay_seconds() < 0.0) { + return Error("Expecting 'delay_seconds' to be non-negative"); + } + + if (healthCheck.has_grace_period_seconds() && + healthCheck.grace_period_seconds() < 0.0) { + return Error("Expecting 'grace_period_seconds' to be non-negative"); + } + + if (healthCheck.has_interval_seconds() && + healthCheck.interval_seconds() < 0.0) { + return Error("Expecting 'interval_seconds' to be non-negative"); + } + + if (healthCheck.has_timeout_seconds() && + healthCheck.timeout_seconds() < 0.0) { + return Error("Expecting 'timeout_seconds' to be non-negative"); + } + + return None(); +} + } // namespace validation { } // namespace common { } // namespace internal { http://git-wip-us.apache.org/repos/asf/mesos/blob/0fa39352/src/common/validation.hpp ---------------------------------------------------------------------- diff --git a/src/common/validation.hpp b/src/common/validation.hpp index 3f60d7a..44259d6 100644 --- a/src/common/validation.hpp +++ b/src/common/validation.hpp @@ -54,6 +54,8 @@ Option<Error> validateContainerInfo(const ContainerInfo& containerInfo); Option<Error> validateGpus( const google::protobuf::RepeatedPtrField<Resource>& resources); +Option<Error> validateHealthCheck(const HealthCheck& healthCheck); + } // namespace validation { } // namespace common { } // namespace internal { http://git-wip-us.apache.org/repos/asf/mesos/blob/0fa39352/src/master/validation.cpp ---------------------------------------------------------------------- diff --git a/src/master/validation.cpp b/src/master/validation.cpp index 798fc79..51cfb74 100644 --- a/src/master/validation.cpp +++ b/src/master/validation.cpp @@ -1241,7 +1241,8 @@ Option<Error> validateCheck(const TaskInfo& task) Option<Error> validateHealthCheck(const TaskInfo& task) { if (task.has_health_check()) { - Option<Error> error = checks::validation::healthCheck(task.health_check()); + Option<Error> error = + common::validation::validateHealthCheck(task.health_check()); if (error.isSome()) { return Error("Task uses invalid health check: " + error->message); } http://git-wip-us.apache.org/repos/asf/mesos/blob/0fa39352/src/tests/health_check_tests.cpp ---------------------------------------------------------------------- diff --git a/src/tests/health_check_tests.cpp b/src/tests/health_check_tests.cpp index 8c7100f..34102e2 100644 --- a/src/tests/health_check_tests.cpp +++ b/src/tests/health_check_tests.cpp @@ -24,6 +24,8 @@ #include "checks/health_checker.hpp" +#include "common/validation.hpp" + #include "docker/docker.hpp" #include "slave/slave.hpp" @@ -47,6 +49,8 @@ namespace http = process::http; +using mesos::internal::common::validation::validateHealthCheck; + using mesos::internal::master::Master; using mesos::internal::slave::Containerizer; @@ -199,11 +203,11 @@ TEST_F(HealthCheckTest, HealthCheckProtobufValidation) { HealthCheck healthCheckProto; - Option<Error> validate = validation::healthCheck(healthCheckProto); + Option<Error> validate = validateHealthCheck(healthCheckProto); EXPECT_SOME(validate); healthCheckProto.set_type(HealthCheck::UNKNOWN); - validate = validation::healthCheck(healthCheckProto); + validate = validateHealthCheck(healthCheckProto); EXPECT_SOME(validate); } @@ -212,15 +216,15 @@ TEST_F(HealthCheckTest, HealthCheckProtobufValidation) HealthCheck healthCheckProto; healthCheckProto.set_type(HealthCheck::COMMAND); - Option<Error> validate = validation::healthCheck(healthCheckProto); + Option<Error> validate = validateHealthCheck(healthCheckProto); EXPECT_SOME(validate); healthCheckProto.set_type(HealthCheck::HTTP); - validate = validation::healthCheck(healthCheckProto); + validate = validateHealthCheck(healthCheckProto); EXPECT_SOME(validate); healthCheckProto.set_type(HealthCheck::TCP); - validate = validation::healthCheck(healthCheckProto); + validate = validateHealthCheck(healthCheckProto); EXPECT_SOME(validate); } @@ -232,21 +236,21 @@ TEST_F(HealthCheckTest, HealthCheckProtobufValidation) healthCheckProto.mutable_http()->set_port(8080); healthCheckProto.set_delay_seconds(-1.0); - Option<Error> validate = validation::healthCheck(healthCheckProto); + Option<Error> validate = validateHealthCheck(healthCheckProto); EXPECT_SOME(validate); healthCheckProto.set_delay_seconds(0.0); healthCheckProto.set_interval_seconds(-1.0); - validate = validation::healthCheck(healthCheckProto); + validate = validateHealthCheck(healthCheckProto); EXPECT_SOME(validate); healthCheckProto.set_interval_seconds(0.0); healthCheckProto.set_timeout_seconds(-1.0); - validate = validation::healthCheck(healthCheckProto); + validate = validateHealthCheck(healthCheckProto); EXPECT_SOME(validate); healthCheckProto.set_timeout_seconds(0.0); - validate = validation::healthCheck(healthCheckProto); + validate = validateHealthCheck(healthCheckProto); EXPECT_NONE(validate); } @@ -256,7 +260,7 @@ TEST_F(HealthCheckTest, HealthCheckProtobufValidation) healthCheckProto.set_type(HealthCheck::COMMAND); healthCheckProto.mutable_command()->CopyFrom(CommandInfo()); - Option<Error> validate = validation::healthCheck(healthCheckProto); + Option<Error> validate = validateHealthCheck(healthCheckProto); EXPECT_SOME(validate); } @@ -267,7 +271,7 @@ TEST_F(HealthCheckTest, HealthCheckProtobufValidation) healthCheckProto.set_type(HealthCheck::COMMAND); healthCheckProto.mutable_command()->CopyFrom(createCommandInfo("exit 0")); - Option<Error> validate = validation::healthCheck(healthCheckProto); + Option<Error> validate = validateHealthCheck(healthCheckProto); EXPECT_NONE(validate); Environment::Variable* variable = @@ -275,7 +279,7 @@ TEST_F(HealthCheckTest, HealthCheckProtobufValidation) ->mutable_variables()->Add(); variable->set_name("ENV_VAR_KEY"); - validate = validation::healthCheck(healthCheckProto); + validate = validateHealthCheck(healthCheckProto); EXPECT_SOME(validate); } @@ -286,16 +290,16 @@ TEST_F(HealthCheckTest, HealthCheckProtobufValidation) healthCheckProto.set_type(HealthCheck::HTTP); healthCheckProto.mutable_http()->set_port(8080); - Option<Error> validate = validation::healthCheck(healthCheckProto); + Option<Error> validate = validateHealthCheck(healthCheckProto); EXPECT_NONE(validate); healthCheckProto.mutable_http()->set_scheme("ftp"); - validate = validation::healthCheck(healthCheckProto); + validate = validateHealthCheck(healthCheckProto); EXPECT_SOME(validate); healthCheckProto.mutable_http()->set_scheme("https"); healthCheckProto.mutable_http()->set_path("healthz"); - validate = validation::healthCheck(healthCheckProto); + validate = validateHealthCheck(healthCheckProto); EXPECT_SOME(validate); } }
