This is an automated email from the ASF dual-hosted git repository. bennoe pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/mesos.git
commit 799a1e7415ebad9c010398fb4fd407e1066fdd31 Author: Joseph Wu <[email protected]> AuthorDate: Fri Apr 26 13:51:21 2019 +0200 Relaxed protobuf union validation strictness. As part of MESOS-6874, the master validates protobuf unions passed as part of an ExecutorInfo::ContainerInfo. This prevents a task from specifying, for example, a ContainerInfo::MESOS, but filling out the docker field (which is then ignored by the agent). This validation change is actually an API change, because previously runnable ExecutorInfo's and TaskInfo's will now fail validation. This has two visible effects on clusters: * Agents running containers with invalid protobuf unions will not be able to reregister with the master. * Existing frameworks will not be able to re-launch the same tasks that were working before a Mesos master upgrade. This changes the validation to print a warning instead. Where possible, the warning will provide some information to indicate which task or executor is sending the invalid protobuf. Review: https://reviews.apache.org/r/70546/ --- src/common/validation.cpp | 9 ++++++++- src/master/validation.cpp | 8 +++++++- src/tests/master_validation_tests.cpp | 19 +++---------------- 3 files changed, 18 insertions(+), 18 deletions(-) diff --git a/src/common/validation.cpp b/src/common/validation.cpp index 458f225..14a8c7b 100644 --- a/src/common/validation.cpp +++ b/src/common/validation.cpp @@ -268,7 +268,14 @@ Option<Error> validateContainerInfo(const ContainerInfo& containerInfo) { Option<Error> unionError = protobuf::validateProtobufUnion(containerInfo); if (unionError.isSome()) { - return unionError; + // TODO(josephw): There is not enough information in this function to + // print an actionable warning. Readers of this warning will not know + // which container (task or executor) has this problem. Printing + // the entire ContainerInfo is the best we can do. + LOG(WARNING) + << "Invalid protobuf union detected in the given ContainerInfo (" + << containerInfo.DebugString() + << "): " << unionError.get(); } foreach (const Volume& volume, containerInfo.volumes()) { diff --git a/src/master/validation.cpp b/src/master/validation.cpp index d7f210f..9fb0850 100644 --- a/src/master/validation.cpp +++ b/src/master/validation.cpp @@ -1001,7 +1001,13 @@ Option<Error> validateType(const ExecutorInfo& executor) Option<Error> unionError = protobuf::validateProtobufUnion(executor.container()); if (unionError.isSome()) { - return unionError; + LOG(WARNING) + << "Executor " << executor.executor_id() + // NOTE: Validation of FrameworkID is done after this validation. + << " of framework '" + << (executor.has_framework_id() ? executor.framework_id().value() : "") + << "' has an invalid protobuf union: " + << unionError.get(); } } switch (executor.type()) { diff --git a/src/tests/master_validation_tests.cpp b/src/tests/master_validation_tests.cpp index c98f751..1b7a827 100644 --- a/src/tests/master_validation_tests.cpp +++ b/src/tests/master_validation_tests.cpp @@ -3341,7 +3341,9 @@ TEST_F(TaskValidationTest, TaskMissingDockerInfo) // This test verifies that a task that has `ContainerInfo` set as MESOS // but has a `DockerInfo` is rejected during `TaskInfo` validation. -TEST_F(TaskValidationTest, TaskMesosTypeWithDockerInfo) +// TODO(josephw): Enable this regression test when we officially deprecate +// this invalid protobuf. See MESOS-6874 and MESOS-9740. +TEST_F(TaskValidationTest, DISABLED_TaskMesosTypeWithDockerInfo) { Try<Owned<cluster::Master>> master = StartMaster(); ASSERT_SOME(master); @@ -3548,21 +3550,6 @@ TEST_F(ExecutorValidationTest, ExecutorType) "'ExecutorInfo.container.mesos.image' must not be set for " "'DEFAULT' executor")); } - { - // Invalid protobuf union in ContainerInfo. - executorInfo.set_type(ExecutorInfo::CUSTOM); - executorInfo.mutable_command(); - executorInfo.mutable_container()->set_type(ContainerInfo::DOCKER); - executorInfo.mutable_container()->mutable_mesos(); - - Option<Error> error = ::executor::internal::validateType(executorInfo); - - EXPECT_SOME(error); - EXPECT_TRUE(strings::contains( - error->message, - "Protobuf union `mesos.ContainerInfo` with `Type == DOCKER` " - "should not have the field `mesos` set.")); - } }
