Modified `createStrippedScalarQuantity()` to clear all metadata fields. Currently `createStrippedScalarQuantity()` strips resource meta-data and transforms dynamic reservations into a static reservation. However, no current code depends on the reservations in resources returned by this helper function. This leads to boilerplate code around call sites and performance overhead.
This patch updates the function to clear all reservation information. Review: https://reviews.apache.org/r/67615/ Project: http://git-wip-us.apache.org/repos/asf/mesos/repo Commit: http://git-wip-us.apache.org/repos/asf/mesos/commit/8814f143 Tree: http://git-wip-us.apache.org/repos/asf/mesos/tree/8814f143 Diff: http://git-wip-us.apache.org/repos/asf/mesos/diff/8814f143 Branch: refs/heads/1.4.x Commit: 8814f143b095addc3dffcc29dab275ba0b8f9d5d Parents: d582754 Author: Meng Zhu <[email protected]> Authored: Thu Jun 21 09:09:39 2018 -0700 Committer: Greg Mann <[email protected]> Committed: Tue Jul 3 07:59:34 2018 -0700 ---------------------------------------------------------------------- include/mesos/resources.hpp | 8 ++--- include/mesos/v1/resources.hpp | 8 ++--- src/common/resources.cpp | 22 +++---------- src/master/allocator/mesos/hierarchical.cpp | 40 ++++++------------------ src/tests/resources_tests.cpp | 15 +++------ src/tests/sorter_tests.cpp | 2 +- src/v1/resources.cpp | 22 +++---------- 7 files changed, 30 insertions(+), 87 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/mesos/blob/8814f143/include/mesos/resources.hpp ---------------------------------------------------------------------- diff --git a/include/mesos/resources.hpp b/include/mesos/resources.hpp index a9144f6..6cc36a3 100644 --- a/include/mesos/resources.hpp +++ b/include/mesos/resources.hpp @@ -443,11 +443,9 @@ public: Resources toUnreserved() const; // Returns a Resources object that contains all the scalar resources - // in this object, but with their AllocationInfo, ReservationInfo, - // DiskInfo, and SharedInfo omitted. The `role` and RevocableInfo, - // if any, are preserved. Because we clear ReservationInfo but - // preserve `role`, this means that stripping a dynamically reserved - // resource makes it effectively statically reserved. + // but with all the meta-data fields, such as AllocationInfo, + // ReservationInfo and etc. cleared. Only scalar resources' name, + // type (SCALAR) and value are preserved. // // This is intended for code that would like to aggregate together // Resource values without regard for metadata like whether the http://git-wip-us.apache.org/repos/asf/mesos/blob/8814f143/include/mesos/v1/resources.hpp ---------------------------------------------------------------------- diff --git a/include/mesos/v1/resources.hpp b/include/mesos/v1/resources.hpp index 6ec8d15..03db6ba 100644 --- a/include/mesos/v1/resources.hpp +++ b/include/mesos/v1/resources.hpp @@ -443,11 +443,9 @@ public: Resources toUnreserved() const; // Returns a Resources object that contains all the scalar resources - // in this object, but with their AllocationInfo, ReservationInfo, - // DiskInfo, and SharedInfo omitted. The `role` and RevocableInfo, - // if any, are preserved. Because we clear ReservationInfo but - // preserve `role`, this means that stripping a dynamically reserved - // resource makes it effectively statically reserved. + // but with all the meta-data fields, such as AllocationInfo, + // ReservationInfo and etc. cleared. Only scalar resources' name, + // type (SCALAR) and value are preserved. // // This is intended for code that would like to aggregate together // Resource values without regard for metadata like whether the http://git-wip-us.apache.org/repos/asf/mesos/blob/8814f143/src/common/resources.cpp ---------------------------------------------------------------------- diff --git a/src/common/resources.cpp b/src/common/resources.cpp index 158ca92..c859131 100644 --- a/src/common/resources.cpp +++ b/src/common/resources.cpp @@ -1519,24 +1519,12 @@ Resources Resources::createStrippedScalarQuantity() const foreach (const Resource& resource, resources) { if (resource.type() == Value::SCALAR) { - Resource scalar = resource; - scalar.clear_provider_id(); - scalar.clear_allocation_info(); - - // We collapse the stack of reservations here to a single `STATIC` - // reservation in order to maintain existing behavior of ignoring - // the reservation type, and keeping the reservation role. - if (Resources::isReserved(scalar)) { - Resource::ReservationInfo collapsedReservation; - collapsedReservation.set_type(Resource::ReservationInfo::STATIC); - collapsedReservation.set_role(Resources::reservationRole(scalar)); - scalar.clear_reservations(); - scalar.add_reservations()->CopyFrom(collapsedReservation); - } + Resource scalar; + + scalar.set_name(resource.name()); + scalar.set_type(resource.type()); + scalar.mutable_scalar()->CopyFrom(resource.scalar()); - scalar.clear_disk(); - scalar.clear_shared(); - scalar.clear_revocable(); stripped.add(scalar); } } http://git-wip-us.apache.org/repos/asf/mesos/blob/8814f143/src/master/allocator/mesos/hierarchical.cpp ---------------------------------------------------------------------- diff --git a/src/master/allocator/mesos/hierarchical.cpp b/src/master/allocator/mesos/hierarchical.cpp index cd80791..3817c2f 100644 --- a/src/master/allocator/mesos/hierarchical.cpp +++ b/src/master/allocator/mesos/hierarchical.cpp @@ -1491,10 +1491,7 @@ void HierarchicalAllocatorProcess::__allocate() auto getQuotaRoleAllocatedResources = [this](const string& role) { CHECK(quotas.contains(role)); - // NOTE: `allocationScalarQuantities` omits dynamic reservation, - // persistent volume info, and allocation info. We additionally - // remove the resource's `role` here via `toUnreserved()`. - return quotaRoleSorter->allocationScalarQuantities(role).toUnreserved(); + return quotaRoleSorter->allocationScalarQuantities(role); }; // We need to keep track of allocated reserved resourecs for roles @@ -1532,10 +1529,8 @@ void HierarchicalAllocatorProcess::__allocate() quotaRoleSorter->allocation(role); foreachvalue (const Resources& resources, allocations) { - // We need to remove the static reservation metadata here via - // `toUnreserved()`. allocatedReservationScalarQuantities[role] += - resources.reserved().createStrippedScalarQuantity().toUnreserved(); + resources.reserved().createStrippedScalarQuantity(); } } @@ -1581,20 +1576,11 @@ void HierarchicalAllocatorProcess::__allocate() // allocated resources - // unallocated reservations - // unallocated revocable resources - - // NOTE: `totalScalarQuantities` omits dynamic reservation, - // persistent volume info, and allocation info. We additionally - // remove the static reservations here via `toUnreserved()`. - Resources availableHeadroom = - roleSorter->totalScalarQuantities().toUnreserved(); + Resources availableHeadroom = roleSorter->totalScalarQuantities(); // Subtract allocated resources from the total. foreachkey (const string& role, roles) { - // NOTE: `totalScalarQuantities` omits dynamic reservation, - // persistent volume info, and allocation info. We additionally - // remove the static reservations here via `toUnreserved()`. - availableHeadroom -= - roleSorter->allocationScalarQuantities(role).toUnreserved(); + availableHeadroom -= roleSorter->allocationScalarQuantities(role); } // Calculate total allocated reservations. Note that we need to ensure @@ -1612,11 +1598,8 @@ void HierarchicalAllocatorProcess::__allocate() } foreachvalue (const Resources& resources, allocations) { - // NOTE: `totalScalarQuantities` omits dynamic reservation, - // persistent volume info, and allocation info. We additionally - // remove the static reservations here via `toUnreserved()`. totalAllocatedReservationScalarQuantities += - resources.reserved().createStrippedScalarQuantity().toUnreserved(); + resources.reserved().createStrippedScalarQuantity(); } } @@ -1627,11 +1610,8 @@ void HierarchicalAllocatorProcess::__allocate() // Subtract revocable resources. foreachvalue (const Slave& slave, slaves) { - // NOTE: `totalScalarQuantities` omits dynamic reservation, - // persistent volume info, and allocation info. We additionally - // remove the static reservations here via `toUnreserved()`. - availableHeadroom -= slave.getAvailable().revocable() - .createStrippedScalarQuantity().toUnreserved(); + availableHeadroom -= + slave.getAvailable().revocable().createStrippedScalarQuantity(); } // Due to the two stages in the allocation algorithm and the nature of @@ -2550,9 +2530,8 @@ void HierarchicalAllocatorProcess::trackReservations( { foreachpair (const string& role, const Resources& resources, reservations) { - // We remove the static reservation metadata here via `toUnreserved()`. const Resources scalarQuantitesToTrack = - resources.createStrippedScalarQuantity().toUnreserved(); + resources.createStrippedScalarQuantity(); reservationScalarQuantities[role] += scalarQuantitesToTrack; } @@ -2568,9 +2547,8 @@ void HierarchicalAllocatorProcess::untrackReservations( Resources& currentReservationQuantity = reservationScalarQuantities.at(role); - // We remove the static reservation metadata here via `toUnreserved()`. const Resources scalarQuantitesToUntrack = - resources.createStrippedScalarQuantity().toUnreserved(); + resources.createStrippedScalarQuantity(); CHECK(currentReservationQuantity.contains(scalarQuantitesToUntrack)); currentReservationQuantity -= scalarQuantitesToUntrack; http://git-wip-us.apache.org/repos/asf/mesos/blob/8814f143/src/tests/resources_tests.cpp ---------------------------------------------------------------------- diff --git a/src/tests/resources_tests.cpp b/src/tests/resources_tests.cpp index f1aa17c..21ad1ff 100644 --- a/src/tests/resources_tests.cpp +++ b/src/tests/resources_tests.cpp @@ -2536,19 +2536,14 @@ TEST(ResourcesOperationTest, StrippedResourcesVolume) Resources stripped = volume.createStrippedScalarQuantity(); EXPECT_TRUE(stripped.persistentVolumes().empty()); + EXPECT_TRUE(stripped.reserved().empty()); EXPECT_EQ(Megabytes(200), stripped.disk().get()); - // `createStrippedScalarQuantity` doesn't remove the `role` from a - // reserved resource. - EXPECT_FALSE(stripped.reserved("role").empty()); - Resource strippedVolume = *(stripped.begin()); ASSERT_EQ(Value::SCALAR, strippedVolume.type()); - EXPECT_FLOAT_EQ(200, strippedVolume.scalar().value()); - EXPECT_EQ("role", Resources::reservationRole(strippedVolume)); + EXPECT_DOUBLE_EQ(200, strippedVolume.scalar().value()); EXPECT_EQ("disk", strippedVolume.name()); - EXPECT_EQ(1, strippedVolume.reservations_size()); EXPECT_FALSE(strippedVolume.has_disk()); EXPECT_FALSE(Resources::isPersistentVolume(strippedVolume)); } @@ -2577,13 +2572,11 @@ TEST(ResourcesOperationTest, StrippedResourcesReserved) Resources stripped = dynamicallyReserved.createStrippedScalarQuantity(); - EXPECT_FALSE(stripped.reserved("role").empty()); + EXPECT_TRUE(stripped.reserved("role").empty()); foreach (const Resource& resource, stripped) { - EXPECT_EQ("role", Resources::reservationRole(resource)); - EXPECT_FALSE(resource.reservations().empty()); EXPECT_FALSE(Resources::isDynamicallyReserved(resource)); - EXPECT_FALSE(Resources::isUnreserved(resource)); + EXPECT_TRUE(Resources::isUnreserved(resource)); } } http://git-wip-us.apache.org/repos/asf/mesos/blob/8814f143/src/tests/sorter_tests.cpp ---------------------------------------------------------------------- diff --git a/src/tests/sorter_tests.cpp b/src/tests/sorter_tests.cpp index 6d048ec..8c2549a 100644 --- a/src/tests/sorter_tests.cpp +++ b/src/tests/sorter_tests.cpp @@ -1594,7 +1594,7 @@ TYPED_TEST(CommonSorterTest, RemoveSharedResources) sorter.add(slaveId, sharedDisk); Resources quantity2 = sorter.totalScalarQuantities(); - EXPECT_EQ(Resources::parse("disk(role1):100").get(), quantity2 - quantity1); + EXPECT_EQ(Resources::parse("disk:100").get(), quantity2 - quantity1); sorter.add(slaveId, sharedDisk); Resources quantity3 = sorter.totalScalarQuantities(); http://git-wip-us.apache.org/repos/asf/mesos/blob/8814f143/src/v1/resources.cpp ---------------------------------------------------------------------- diff --git a/src/v1/resources.cpp b/src/v1/resources.cpp index 530c460..aa1da43 100644 --- a/src/v1/resources.cpp +++ b/src/v1/resources.cpp @@ -1547,24 +1547,12 @@ Resources Resources::createStrippedScalarQuantity() const foreach (const Resource& resource, resources) { if (resource.type() == Value::SCALAR) { - Resource scalar = resource; - scalar.clear_provider_id(); - scalar.clear_allocation_info(); - - // We collapse the stack of reservations here to a single `STATIC` - // reservation in order to maintain existing behavior of ignoring - // the reservation type, and keeping the reservation role. - if (Resources::isReserved(scalar)) { - Resource::ReservationInfo collapsedReservation; - collapsedReservation.set_type(Resource::ReservationInfo::STATIC); - collapsedReservation.set_role(Resources::reservationRole(scalar)); - scalar.clear_reservations(); - scalar.add_reservations()->CopyFrom(collapsedReservation); - } + Resource scalar; + + scalar.set_name(resource.name()); + scalar.set_type(resource.type()); + scalar.mutable_scalar()->CopyFrom(resource.scalar()); - scalar.clear_disk(); - scalar.clear_shared(); - scalar.clear_revocable(); stripped.add(scalar); } }
