Enabled Resources to handle Resource::ReservationInfo. Review: https://reviews.apache.org/r/32140
Project: http://git-wip-us.apache.org/repos/asf/mesos/repo Commit: http://git-wip-us.apache.org/repos/asf/mesos/commit/2d28816f Tree: http://git-wip-us.apache.org/repos/asf/mesos/tree/2d28816f Diff: http://git-wip-us.apache.org/repos/asf/mesos/diff/2d28816f Branch: refs/heads/master Commit: 2d28816f25a24bb5bf591c0aa1fba3b4c23c631b Parents: 4d1e5b0 Author: Michael Park <[email protected]> Authored: Sun May 3 11:45:02 2015 -0700 Committer: Jie Yu <[email protected]> Committed: Sun May 3 13:08:43 2015 -0700 ---------------------------------------------------------------------- include/mesos/resources.hpp | 17 ++++- src/common/resources.cpp | 135 ++++++++++++++++++++++++++++++------- src/tests/mesos.hpp | 29 ++++++++ src/tests/resources_tests.cpp | 133 ++++++++++++++++++++++++++++++++++-- 4 files changed, 285 insertions(+), 29 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/mesos/blob/2d28816f/include/mesos/resources.hpp ---------------------------------------------------------------------- diff --git a/include/mesos/resources.hpp b/include/mesos/resources.hpp index 56affd4..fd20574 100644 --- a/include/mesos/resources.hpp +++ b/include/mesos/resources.hpp @@ -90,6 +90,13 @@ public: // NOTE: The following predicate functions assume that the given // resource is validated. + // + // Valid states of (role, reservation) pair in the Resource object. + // Unreserved : ("*", None) + // Static reservation : (R, None) + // Dynamic reservation: (R, { principal: <framework_principal> }) + // + // NOTE: ("*", { principal: <framework_principal> }) is invalid. // Tests if the given Resource object is empty. static bool isEmpty(const Resource& resource); @@ -177,8 +184,14 @@ public: // Returns a Resources object with the same amount of each resource // type as these Resources, but with all Resource objects marked as - // the specified role. - Resources flatten(const std::string& role = "*") const; + // the specified (role, reservation) pair. This is used to cross + // reservation boundaries without affecting the actual resources. + // If the optional ReservationInfo is given, the resource's + // 'reservation' field is set. Otherwise, the resource's + // 'reservation' field is cleared. + Resources flatten( + const std::string& role = "*", + const Option<Resource::ReservationInfo>& reservation = None()) const; // Finds a Resources object with the same amount of each resource // type as "targets" from these Resources. The roles specified in http://git-wip-us.apache.org/repos/asf/mesos/blob/2d28816f/src/common/resources.cpp ---------------------------------------------------------------------- diff --git a/src/common/resources.cpp b/src/common/resources.cpp index 2c99b68..b092352 100644 --- a/src/common/resources.cpp +++ b/src/common/resources.cpp @@ -24,6 +24,7 @@ #include <mesos/resources.hpp> #include <mesos/values.hpp> +#include <mesos/type_utils.hpp> #include <stout/foreach.hpp> #include <stout/lambda.hpp> @@ -41,6 +42,22 @@ namespace mesos { ///////////////////////////////////////////////// bool operator == ( + const Resource::ReservationInfo& left, + const Resource::ReservationInfo& right) +{ + return left.principal() == right.principal(); +} + + +bool operator != ( + const Resource::ReservationInfo& left, + const Resource::ReservationInfo& right) +{ + return !(left == right); +} + + +bool operator == ( const Resource::DiskInfo& left, const Resource::DiskInfo& right) { @@ -77,9 +94,21 @@ bool operator == (const Resource& left, const Resource& right) return false; } - // NOTE: Not setting the DiskInfo is the same as setting an empty - // DiskInfo, therefore we just call .disk() even if it's not set. - if (left.disk() != right.disk()) { + // Check ReservationInfo. + if (left.has_reservation() != right.has_reservation()) { + return false; + } + + if (left.has_reservation() && left.reservation() != right.reservation()) { + return false; + } + + // Check DiskInfo. + if (left.has_disk() != right.has_disk()) { + return false; + } + + if (left.has_disk() && left.disk() != right.disk()) { return false; } @@ -104,17 +133,41 @@ bool operator != (const Resource& left, const Resource& right) // Tests if we can add two Resource objects together resulting in one // valid Resource object. For example, two Resource objects with // different name, type or role are not addable. -// TODO(jieyu): Even if two Resource objects with DiskInfo have the -// same persistence ID, they cannot be added together. In fact, this -// shouldn't happen if we do not add resources from different -// namespaces (e.g., across slave). Consider adding a warning. static bool addable(const Resource& left, const Resource& right) { - return left.name() == right.name() && - left.type() == right.type() && - left.role() == right.role() && - !left.disk().has_persistence() && - !right.disk().has_persistence(); + if (left.name() != right.name() || + left.type() != right.type() || + left.role() != right.role()) { + return false; + } + + // Check ReservationInfo. + if (left.has_reservation() != right.has_reservation()) { + return false; + } + + if (left.has_reservation() && left.reservation() != right.reservation()) { + return false; + } + + // Check DiskInfo. + if (left.has_disk() != right.has_disk()) { + return false; + } + + if (left.has_disk() && left.disk() != right.disk()) { + return false; + } + + // TODO(jieyu): Even if two Resource objects with DiskInfo have the + // same persistence ID, they cannot be added together. In fact, this + // shouldn't happen if we do not add resources from different + // namespaces (e.g., across slave). Consider adding a warning. + if (left.has_disk() && left.disk().has_persistence()) { + return false; + } + + return true; } @@ -126,8 +179,6 @@ static bool addable(const Resource& left, const Resource& right) // "left = {1, 2}" and "right = {2, 3}", "left" and "right" are // subtractable because "left - right = {1}". However, "left" does not // contain "right". -// NOTE: For Resource objects that have DiskInfo, we can only do -// subtraction if they are equal. static bool subtractable(const Resource& left, const Resource& right) { if (left.name() != right.name() || @@ -136,8 +187,28 @@ static bool subtractable(const Resource& left, const Resource& right) return false; } - if (left.has_disk() || right.has_disk()) { - return left == right; + // Check ReservationInfo. + if (left.has_reservation() != right.has_reservation()) { + return false; + } + + if (left.has_reservation() && left.reservation() != right.reservation()) { + return false; + } + + // Check DiskInfo. + if (left.has_disk() != right.has_disk()) { + return false; + } + + if (left.has_disk() && left.disk() != right.disk()) { + return false; + } + + // NOTE: For Resource objects that have DiskInfo, we can only do + // subtraction if they are equal. + if (left.has_disk() && left.disk().has_persistence() && left != right) { + return false; } return true; @@ -148,8 +219,8 @@ static bool subtractable(const Resource& left, const Resource& right) static bool contains(const Resource& left, const Resource& right) { // NOTE: This is a necessary condition for 'contains'. - // 'subtractable' will verify name, role, type and DiskInfo - // compatibility. + // 'subtractable' will verify name, role, type, ReservationInfo + // and DiskInfo compatibility. if (!subtractable(left, right)) { return false; } @@ -367,6 +438,12 @@ Option<Error> Resources::validate(const Resource& resource) "DiskInfo should not be set for " + resource.name() + " resource"); } + // Checks for the invalid state of (role, reservation) pair. + if (resource.role() == "*" && resource.has_reservation()) { + return Error( + "Invalid reservation: role \"*\" cannot be dynamically reserved"); + } + return None(); } @@ -412,16 +489,16 @@ bool Resources::isReserved( const Option<std::string>& role) { if (role.isSome()) { - return resource.role() != "*" && role.get() == resource.role(); + return !isUnreserved(resource) && role.get() == resource.role(); } else { - return resource.role() != "*"; + return !isUnreserved(resource); } } bool Resources::isUnreserved(const Resource& resource) { - return resource.role() == "*"; + return resource.role() == "*" && !resource.has_reservation(); } @@ -529,12 +606,19 @@ Resources Resources::persistentVolumes() const } -Resources Resources::flatten(const string& role) const +Resources Resources::flatten( + const string& role, + const Option<Resource::ReservationInfo>& reservation) const { Resources flattened; foreach (Resource resource, resources) { resource.set_role(role); + if (reservation.isNone()) { + resource.clear_reservation(); + } else { + resource.mutable_reservation()->CopyFrom(reservation.get()); + } flattened += resource; } @@ -567,7 +651,12 @@ Option<Resources> Resources::find(const Resource& target) const if (flattened.contains(remaining)) { // Done! - return found + remaining.flatten(resource.role()); + if (!resource.has_reservation()) { + return found + remaining.flatten(resource.role()); + } else { + return found + + remaining.flatten(resource.role(), resource.reservation()); + } } else if (remaining.contains(flattened)) { found += resource; total -= resource; http://git-wip-us.apache.org/repos/asf/mesos/blob/2d28816f/src/tests/mesos.hpp ---------------------------------------------------------------------- diff --git a/src/tests/mesos.hpp b/src/tests/mesos.hpp index 19db712..563b833 100644 --- a/src/tests/mesos.hpp +++ b/src/tests/mesos.hpp @@ -376,6 +376,15 @@ inline TaskInfo createTask( } +inline Resource::ReservationInfo createReservationInfo( + const std::string& principal) +{ + Resource::ReservationInfo info; + info.set_principal(principal); + return info; +} + + // NOTE: We only set the volume in DiskInfo if 'containerPath' is set. // If volume mode is not specified, Volume::RW will be used (assuming // 'containerPath' is set). @@ -425,6 +434,26 @@ inline Resource createPersistentVolume( } +// Helpers for creating reserve operations. +inline Offer::Operation RESERVE(const Resources& resources) +{ + Offer::Operation operation; + operation.set_type(Offer::Operation::RESERVE); + operation.mutable_reserve()->mutable_resources()->CopyFrom(resources); + return operation; +} + + +// Helpers for creating unreserve operations. +inline Offer::Operation UNRESERVE(const Resources& resources) +{ + Offer::Operation operation; + operation.set_type(Offer::Operation::UNRESERVE); + operation.mutable_unreserve()->mutable_resources()->CopyFrom(resources); + return operation; +} + + // Helpers for creating offer operations. inline Offer::Operation CREATE(const Resources& volumes) { http://git-wip-us.apache.org/repos/asf/mesos/blob/2d28816f/src/tests/resources_tests.cpp ---------------------------------------------------------------------- diff --git a/src/tests/resources_tests.cpp b/src/tests/resources_tests.cpp index 7e0ad98..58d7946 100644 --- a/src/tests/resources_tests.cpp +++ b/src/tests/resources_tests.cpp @@ -831,6 +831,133 @@ TEST(ResourcesTest, Find) } +// Helper for creating a reserved resource. +static Resource createReservedResource( + const string& name, + const string& value, + const string& role, + const Option<Resource::ReservationInfo>& reservation) +{ + Resource resource = Resources::parse(name, value, role).get(); + + if (reservation.isSome()) { + resource.mutable_reservation()->CopyFrom(reservation.get()); + } + + return resource; +} + + +TEST(ReservedResourcesTest, Validation) +{ + // Unreserved. + EXPECT_NONE(Resources::validate(createReservedResource( + "cpus", "8", "*", None()))); + + // Statically role reserved. + EXPECT_NONE(Resources::validate(createReservedResource( + "cpus", "8", "role", None()))); + + // Invalid. + EXPECT_SOME(Resources::validate(createReservedResource( + "cpus", "8", "*", createReservationInfo("principal1")))); + + // Dynamically role reserved. + EXPECT_NONE(Resources::validate(createReservedResource( + "cpus", "8", "role", createReservationInfo("principal2")))); +} + + +TEST(ReservedResourcesTest, Equals) +{ + std::vector<Resources> unique = { + // Unreserved. + createReservedResource( + "cpus", "8", "*", None()), + // Statically reserved for role. + createReservedResource( + "cpus", "8", "role1", None()), + createReservedResource( + "cpus", "8", "role2", None()), + // Dynamically reserved for role. + createReservedResource( + "cpus", "8", "role1", createReservationInfo("principal1")), + createReservedResource( + "cpus", "8", "role2", createReservationInfo("principal2")) + }; + + // Test that all resources in 'unique' are considered different. + for (const Resources& left : unique) { + for (const Resources& right : unique) { + if (&left == &right) { + continue; + } + EXPECT_NE(left, right); + } + } +} + + +TEST(ReservedResourcesTest, AdditionStaticallyReserved) +{ + Resources left = createReservedResource("cpus", "8", "role", None()); + Resources right = createReservedResource("cpus", "4", "role", None()); + Resources expected = createReservedResource("cpus", "12", "role", None()); + + EXPECT_EQ(expected, left + right); +} + + +TEST(ReservedResourcesTest, AdditionDynamicallyReserved) +{ + Resource::ReservationInfo reservationInfo = + createReservationInfo("principal"); + + Resources left = + createReservedResource("cpus", "8", "role", reservationInfo); + Resources right = + createReservedResource("cpus", "4", "role", reservationInfo); + Resources expected = + createReservedResource("cpus", "12", "role", reservationInfo); + + EXPECT_EQ(expected, left + right); +} + + +TEST(ReservedResourcesTest, Subtraction) +{ + Resource::ReservationInfo reservationInfo = + createReservationInfo("principal"); + + Resources r1 = createReservedResource("cpus", "8", "role", None()); + Resources r2 = createReservedResource("cpus", "8", "role", reservationInfo); + + Resources total = r1 + r2; + + Resources r4 = createReservedResource("cpus", "6", "role", None()); + Resources r5 = createReservedResource("cpus", "4", "role", reservationInfo); + + Resources expected = r4 + r5; + + Resources r7 = createReservedResource("cpus", "2", "role", None()); + Resources r8 = createReservedResource("cpus", "4", "role", reservationInfo); + + EXPECT_EQ(expected, total - r7 - r8); +} + + +TEST(ReservedResourcesTest, Contains) +{ + Resources r1 = createReservedResource("cpus", "8", "role", None()); + Resources r2 = createReservedResource( + "cpus", "12", "role", createReservationInfo("principal")); + + EXPECT_FALSE(r1.contains(r1 + r2)); + EXPECT_FALSE(r2.contains(r1 + r2)); + EXPECT_TRUE((r1 + r2).contains(r1 + r2)); +} + + // Helper for creating a disk resource. static Resource createDiskResource( const string& value, @@ -840,10 +967,8 @@ static Resource createDiskResource( { Resource resource = Resources::parse("disk", value, role).get(); - if (persistenceID.isSome() || containerPath.isSome()) { - resource.mutable_disk()->CopyFrom( - createDiskInfo(persistenceID, containerPath)); - } + resource.mutable_disk()->CopyFrom( + createDiskInfo(persistenceID, containerPath)); return resource; }
