This is an automated email from the ASF dual-hosted git repository. mzhu pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/mesos.git
commit a7c2234d66fc557f2880894c0244785ac3e3f230 Author: Meng Zhu <[email protected]> AuthorDate: Mon May 27 15:29:40 2019 +0200 Pulled out `shrinkResources()` in the allocator as a resource utility. This reduce the clutter in the allocator code. Also added dedicated unit tests. Review: https://reviews.apache.org/r/70730 --- src/common/resources_utils.cpp | 36 ++++++++++++++++ src/common/resources_utils.hpp | 19 +++++++++ src/master/allocator/mesos/hierarchical.cpp | 39 +----------------- src/tests/resources_tests.cpp | 64 +++++++++++++++++++++++++++++ 4 files changed, 120 insertions(+), 38 deletions(-) diff --git a/src/common/resources_utils.cpp b/src/common/resources_utils.cpp index 3786bcf..1555373 100644 --- a/src/common/resources_utils.cpp +++ b/src/common/resources_utils.cpp @@ -25,6 +25,8 @@ using google::protobuf::Descriptor; using google::protobuf::Message; using google::protobuf::RepeatedPtrField; +using mesos::internal::ResourceQuantities; + namespace mesos { bool needCheckpointing(const Resource& resource) @@ -904,4 +906,38 @@ Try<Nothing> downgradeResources(Message* message) message, downgradeResource, resourcesContainment); } + +Resources shrinkResources(const Resources& resources, ResourceQuantities target) +{ + if (target.empty()) { + return Resources(); + } + + // TODO(mzhu): Add a `shuffle()` method in `Resources` to avoid this copy. + google::protobuf::RepeatedPtrField<Resource> resourceVector = resources; + + random_shuffle(resourceVector.begin(), resourceVector.end()); + + Resources result; + foreach (Resource& resource, resourceVector) { + Value::Scalar scalar = target.get(resource.name()); + + if (scalar == Value::Scalar()) { + // Resource that has zero quantity is dropped (shrunk to zero). + continue; + } + + // Target can only be explicitly specified for scalar resources. + CHECK_EQ(Value::SCALAR, resource.type()) << " Resources: " << resources; + + if (Resources::shrink(&resource, scalar)) { + target -= ResourceQuantities::fromScalarResources(resource); + result += std::move(resource); + } + } + + return result; +} + + } // namespace mesos { diff --git a/src/common/resources_utils.hpp b/src/common/resources_utils.hpp index 42b6caf..6bfe24c 100644 --- a/src/common/resources_utils.hpp +++ b/src/common/resources_utils.hpp @@ -32,6 +32,8 @@ #include <stout/option.hpp> #include <stout/try.hpp> +#include "resource_quantities.hpp" + namespace mesos { // Tests if the given Resource needs to be checkpointed on the slave. @@ -216,6 +218,23 @@ Try<Nothing> downgradeResources(std::vector<Resource>* resources); Try<Nothing> downgradeResources(google::protobuf::Message* message); +// Returns the result of shrinking resources down to the target +// scalar resource quantities. Target quantities can only be explicitly +// specified for scalar resources. Otherwise a `CHECK` error will occur. +// Note `ResourceQuantities` has absent-means-zero semantics. This means +// all non-scalar resources (if any) will be dropped post shrinking. +// +// Note that some resources are indivisible (e.g. MOUNT volume) and +// may be excluded in entirety in order to achieve the target size +// (this may lead to the result size being smaller than the target size). +// +// Note also that there may be more than one result that satisfies +// the target sizes (e.g. need to exclude 1 of 2 disks); this function +// will make a random choice in these cases. +Resources shrinkResources( + const Resources& resources, mesos::internal::ResourceQuantities target); + + } // namespace mesos { #endif // __RESOURCES_UTILS_HPP__ diff --git a/src/master/allocator/mesos/hierarchical.cpp b/src/master/allocator/mesos/hierarchical.cpp index 7cc241e..36a9268 100644 --- a/src/master/allocator/mesos/hierarchical.cpp +++ b/src/master/allocator/mesos/hierarchical.cpp @@ -43,6 +43,7 @@ #include "common/protobuf_utils.hpp" #include "common/resource_quantities.hpp" +#include "common/resources_utils.hpp" using std::set; using std::string; @@ -1611,44 +1612,6 @@ void HierarchicalAllocatorProcess::__allocate() // TODO(vinod): Implement a smarter sorting algorithm. std::random_shuffle(slaveIds.begin(), slaveIds.end()); - // Returns the result of shrinking the provided resources down to the - // target resource quantities. - // - // Note that some resources are indivisible (e.g. MOUNT volume) and - // may be excluded in entirety in order to achieve the target size - // (this may lead to the result size being smaller than the target size). - // - // Note also that there may be more than one result that satisfies - // the target sizes (e.g. need to exclude 1 of 2 disks); this function - // will make a random choice in these cases. - auto shrinkResources = [](const Resources& resources, - ResourceQuantities target) { - if (target.empty()) { - return Resources(); - } - - google::protobuf::RepeatedPtrField<Resource> resourceVector = resources; - - random_shuffle(resourceVector.begin(), resourceVector.end()); - - Resources result; - foreach (Resource& resource, resourceVector) { - Value::Scalar scalar = target.get(resource.name()); - - if (scalar == Value::Scalar()) { - // Resource that has zero quantity is dropped (shrunk to zero). - continue; - } - - if (Resources::shrink(&resource, scalar)) { - target -= ResourceQuantities::fromScalarResources(resource); - result += std::move(resource); - } - } - - return result; - }; - // To enforce quota, we keep track of consumed quota for roles with a // non-default quota. // diff --git a/src/tests/resources_tests.cpp b/src/tests/resources_tests.cpp index dbae0f6..32d103f 100644 --- a/src/tests/resources_tests.cpp +++ b/src/tests/resources_tests.cpp @@ -1875,6 +1875,70 @@ TEST(ResourcesTest, Find) } +// This test verifies the correctness of shrinking resources to the target +// resource quantities. +TEST(ResourcesTest, ShrinkToQuantities) +{ + auto shrink = [](const Resources& resources, const string& quantitiesString) { + ResourceQuantities quantities = + CHECK_NOTERROR(ResourceQuantities::fromString(quantitiesString)); + + return shrinkResources(resources, quantities); + }; + + // Emptiness. + Resources empty; + EXPECT_EQ(empty, shrink(empty, "")); + EXPECT_EQ(empty, shrink(empty, "cpus:1")); + + // Simple shrink. + Resources cpu1 = CHECK_NOTERROR(Resources::parse("cpus:1")); + EXPECT_EQ(empty, shrink(cpu1, "")); + + Resources cpu2 = CHECK_NOTERROR(Resources::parse("cpus:2")); + EXPECT_EQ(cpu1, shrink(cpu2, "cpus:1")); + + // Multiple resources. + Resources cpu2mem20 = CHECK_NOTERROR(Resources::parse("cpus:2;mem:20")); + Resources cpu1mem10 = CHECK_NOTERROR(Resources::parse("cpus:1;mem:10")); + EXPECT_EQ(cpu1mem10, shrink(cpu2mem20, "cpus:1;mem:10")); + EXPECT_EQ(cpu1mem10, shrink(cpu2mem20, "cpus:1;mem:10;disk:10")); + EXPECT_EQ(cpu1, shrink(cpu2mem20, "cpus:1")); + + // Non-choppable resources. + Resources mountDisk20 = createDiskResource( + "20", "role", None(), None(), createDiskSourceMount("mnt")); + EXPECT_EQ(Resources(), shrink(mountDisk20, "disk:10")); + + // Random chopping. + // + // We construct 20 disk resources consisted of 10 regular disk and + // 10 mount disk. A chopping of 10 disk should make a random choice + // of picking either the regular disk or the mount disk. + Resources regularDisk10 = CHECK_NOTERROR(Resources::parse("disk:10")); + Resources mountDisk10 = createDiskResource( + "10", "role", None(), None(), createDiskSourceMount("mnt")); + Resources combinedDisk20 = regularDisk10 + mountDisk10; + + // A chopping of 10 disk could return either 10 regular disk or + // 10 mount disk. + Resources shrunk10 = shrink(combinedDisk20, "disk:10"); + + // Repeat the same chopping for 1000 times, expecting the chopping + // result to be different from the first time at least once. + const size_t count = 1000u; + bool atLeastDifferentOnce; + + for (size_t i = 0; i < count; i++) { + if (shrink(combinedDisk20, "disk:10") != shrunk10) { + atLeastDifferentOnce = true; + break; + } + } + CHECK(atLeastDifferentOnce); +} + + // This test verifies that we can filter resources of a given name // from Resources. TEST(ResourcesTest, Get)
