You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@mesos.apache.org by ji...@apache.org on 2015/05/03 22:09:21 UTC
[2/4] mesos git commit: Enabled Resources to handle
Resource::ReservationInfo.
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 <mc...@gmail.com>
Authored: Sun May 3 11:45:02 2015 -0700
Committer: Jie Yu <yu...@gmail.com>
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;
}