Repository: mesos Updated Branches: refs/heads/1.2.x 12890ba05 -> e88229982
Fixed crash with tasks that use very small resource values. When parsing resources, inputs such as "cpus:0" are considered "empty" and are omitted from the resulting `Resources` object. However, a very small resource value like "cpus:0.00001" was considered non-empty, despite the fact that the numerical behavior for resources means that these two values compare as equal. This inconsistency resulted in a `CHECK` failure in the sorter when a task that used a tiny amount of resources reached a terminal state. This commit fixes the resource parsing logic to treat such very small resource values as empty resources, which resolves the inconsistency above. The resulting behavior might be a bit surprising to users (since the task will launch with zero resources instead of a very small resource value), but improving this is left as future work (MESOS-1807). Review: https://reviews.apache.org/r/57752 Project: http://git-wip-us.apache.org/repos/asf/mesos/repo Commit: http://git-wip-us.apache.org/repos/asf/mesos/commit/d6b34409 Tree: http://git-wip-us.apache.org/repos/asf/mesos/tree/d6b34409 Diff: http://git-wip-us.apache.org/repos/asf/mesos/diff/d6b34409 Branch: refs/heads/1.2.x Commit: d6b3440998097ea24437c6fe61dba19c70e4eac7 Parents: 12890ba Author: Neil Conway <[email protected]> Authored: Thu Mar 9 15:46:53 2017 -0500 Committer: Neil Conway <[email protected]> Committed: Thu Mar 23 15:47:54 2017 -0700 ---------------------------------------------------------------------- src/common/resources.cpp | 4 +++- src/tests/master_tests.cpp | 47 ++++++++++++++++++++++++++++++++++++++ src/tests/resources_tests.cpp | 44 +++++++++++++++++++++++++++++++++++ src/v1/resources.cpp | 4 +++- 4 files changed, 97 insertions(+), 2 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/mesos/blob/d6b34409/src/common/resources.cpp ---------------------------------------------------------------------- diff --git a/src/common/resources.cpp b/src/common/resources.cpp index 388e3ef..db29cee 100644 --- a/src/common/resources.cpp +++ b/src/common/resources.cpp @@ -841,7 +841,9 @@ Option<Error> Resources::validate(const RepeatedPtrField<Resource>& resources) bool Resources::isEmpty(const Resource& resource) { if (resource.type() == Value::SCALAR) { - return resource.scalar().value() == 0; + Value::Scalar zero; + zero.set_value(0); + return resource.scalar() == zero; } else if (resource.type() == Value::RANGES) { return resource.ranges().range_size() == 0; } else if (resource.type() == Value::SET) { http://git-wip-us.apache.org/repos/asf/mesos/blob/d6b34409/src/tests/master_tests.cpp ---------------------------------------------------------------------- diff --git a/src/tests/master_tests.cpp b/src/tests/master_tests.cpp index 3b4123b..23d81e6 100644 --- a/src/tests/master_tests.cpp +++ b/src/tests/master_tests.cpp @@ -6325,6 +6325,53 @@ TEST_F(MasterTest, AgentRestartNoReregisterRateLimit) driver.join(); } + +TEST_F(MasterTest, TaskWithTinyResources) +{ + Try<Owned<cluster::Master>> master = StartMaster(); + ASSERT_SOME(master); + + Owned<MasterDetector> detector = master.get()->createDetector(); + Try<Owned<cluster::Slave>> slave = StartSlave(detector.get()); + ASSERT_SOME(slave); + + MockScheduler sched; + MesosSchedulerDriver driver( + &sched, DEFAULT_FRAMEWORK_INFO, master.get()->pid, DEFAULT_CREDENTIAL); + + EXPECT_CALL(sched, registered(&driver, _, _)); + + Future<vector<Offer>> offers; + EXPECT_CALL(sched, resourceOffers(&driver, _)) + .WillOnce(FutureArg<1>(&offers)) + .WillRepeatedly(Return()); // Ignore subsequent offers. + + driver.start(); + + AWAIT_READY(offers); + ASSERT_FALSE(offers->empty()); + + Offer offer = offers.get()[0]; + + TaskInfo task = createTask( + offer.slave_id(), + Resources::parse("cpus:0.00001;mem:1").get(), + SLEEP_COMMAND(1000)); + + Future<TaskStatus> runningStatus; + EXPECT_CALL(sched, statusUpdate(&driver, _)) + .WillOnce(FutureArg<1>(&runningStatus)); + + driver.launchTasks(offer.id(), {task}); + + AWAIT_READY(runningStatus); + EXPECT_EQ(TASK_RUNNING, runningStatus->state()); + EXPECT_EQ(task.task_id(), runningStatus->task_id()); + + driver.stop(); + driver.join(); +} + } // namespace tests { } // namespace internal { } // namespace mesos { http://git-wip-us.apache.org/repos/asf/mesos/blob/d6b34409/src/tests/resources_tests.cpp ---------------------------------------------------------------------- diff --git a/src/tests/resources_tests.cpp b/src/tests/resources_tests.cpp index 2bdce3c..3896bd8 100644 --- a/src/tests/resources_tests.cpp +++ b/src/tests/resources_tests.cpp @@ -1701,6 +1701,19 @@ TEST(ResourcesTest, PrecisionRounding) } +TEST(ResourcesTest, PrecisionVerySmallValue) +{ + Resources r1 = Resources::parse("cpus:2;mem:1024").get(); + Resources r2 = Resources::parse("cpus:0.00001;mem:1").get(); + + Resources r3 = r1 - (r1 - r2); + EXPECT_TRUE(r3.contains(r2)); + + Resources r4 = Resources::parse("cpus:0;mem:1").get(); + EXPECT_EQ(r2, r4); +} + + TEST(ReservedResourcesTest, Validation) { // Unreserved. @@ -3310,6 +3323,37 @@ TEST_P(Resources_Contains_BENCHMARK_Test, Contains) << endl; } + +class Resources_Parse_BENCHMARK_Test + : public MesosTest, + public ::testing::WithParamInterface<size_t> {}; + + +INSTANTIATE_TEST_CASE_P( + Resources_Parse, + Resources_Parse_BENCHMARK_Test, + ::testing::Values(1000U, 10000U, 50000U)); + + +TEST_P(Resources_Parse_BENCHMARK_Test, Parse) +{ + const size_t iterationCount = GetParam(); + const size_t resourcesCount = 100; + + vector<string> rawResources; + + for (size_t i = 0; i < resourcesCount; i++) { + rawResources.push_back("res" + stringify(i) + ":" + stringify(i)); + } + + string inputString = strings::join(";", rawResources); + + for (size_t i = 0; i < iterationCount; i++) { + Try<Resources> resource = Resources::parse(inputString); + EXPECT_SOME(resource); + } +} + } // namespace tests { } // namespace internal { } // namespace mesos { http://git-wip-us.apache.org/repos/asf/mesos/blob/d6b34409/src/v1/resources.cpp ---------------------------------------------------------------------- diff --git a/src/v1/resources.cpp b/src/v1/resources.cpp index e47c4d4..e6197a6 100644 --- a/src/v1/resources.cpp +++ b/src/v1/resources.cpp @@ -830,7 +830,9 @@ Option<Error> Resources::validate(const RepeatedPtrField<Resource>& resources) bool Resources::isEmpty(const Resource& resource) { if (resource.type() == Value::SCALAR) { - return resource.scalar().value() == 0; + Value::Scalar zero; + zero.set_value(0); + return resource.scalar() == zero; } else if (resource.type() == Value::RANGES) { return resource.ranges().range_size() == 0; } else if (resource.type() == Value::SET) {
