[2/2] mesos git commit: Added RevocableInfo message to Resource protobuf.

[vi...@apache.org]

Added RevocableInfo message to Resource protobuf.

Review: https://reviews.apache.org/r/33865


Project: http://git-wip-us.apache.org/repos/asf/mesos/repo
Commit: http://git-wip-us.apache.org/repos/asf/mesos/commit/c4366d8b
Tree: http://git-wip-us.apache.org/repos/asf/mesos/tree/c4366d8b
Diff: http://git-wip-us.apache.org/repos/asf/mesos/diff/c4366d8b

Branch: refs/heads/master
Commit: c4366d8b997223cb212041daa6678bc1e98271ff
Parents: bbd0582
Author: Vinod Kone <vi...@gmail.com>
Authored: Tue May 5 09:54:57 2015 -0700
Committer: Vinod Kone <vi...@gmail.com>
Committed: Fri May 15 17:13:33 2015 -0700

----------------------------------------------------------------------
 include/mesos/mesos.proto             | 10 +++
 include/mesos/resources.hpp           |  3 +
 src/common/resources.cpp              | 24 +++++++-
 src/master/validation.cpp             | 30 +++++++++
 src/tests/master_validation_tests.cpp | 20 ++++++
 src/tests/resources_tests.cpp         | 98 ++++++++++++++++++++++++++++++
 6 files changed, 183 insertions(+), 2 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/mesos/blob/c4366d8b/include/mesos/mesos.proto
----------------------------------------------------------------------
diff --git a/include/mesos/mesos.proto b/include/mesos/mesos.proto
index 15f55a3..9cc5782 100644
--- a/include/mesos/mesos.proto
+++ b/include/mesos/mesos.proto
@@ -449,6 +449,16 @@ message Resource {
   }
 
   optional DiskInfo disk = 7;
+
+  message RevocableInfo {}
+
+  // If this is set, the resources are revocable, i.e., any tasks or
+  // executors launched using these resources could get preempted or
+  // throttled at any time. This could be used by frameworks to run
+  // best effort tasks that do not need strict uptime or performance
+  // guarantees. Note that if this is set, 'disk' or 'reservation'
+  // cannot be set.
+  optional RevocableInfo revocable = 9;
 }
 
 

http://git-wip-us.apache.org/repos/asf/mesos/blob/c4366d8b/include/mesos/resources.hpp
----------------------------------------------------------------------
diff --git a/include/mesos/resources.hpp b/include/mesos/resources.hpp
index f44cb62..1e98c13 100644
--- a/include/mesos/resources.hpp
+++ b/include/mesos/resources.hpp
@@ -117,6 +117,9 @@ public:
   // Tests if the given Resource object is dynamically reserved.
   static bool isDynamicallyReserved(const Resource& resource);
 
+  // Tests if the given Resource object is revocable.
+  static bool isRevocable(const Resource& resource);
+
   // Returns the summed up Resources given a hashmap<Key, Resources>.
   //
   // NOTE: While scalar resources such as "cpus" sum correctly,

http://git-wip-us.apache.org/repos/asf/mesos/blob/c4366d8b/src/common/resources.cpp
----------------------------------------------------------------------
diff --git a/src/common/resources.cpp b/src/common/resources.cpp
index 843a06d..92b9e7f 100644
--- a/src/common/resources.cpp
+++ b/src/common/resources.cpp
@@ -112,6 +112,11 @@ bool operator == (const Resource& left, const Resource& right)
     return false;
   }
 
+  // Check RevocableInfo.
+  if (left.has_revocable() != right.has_revocable()) {
+    return false;
+  }
+
   if (left.type() == Value::SCALAR) {
     return left.scalar() == right.scalar();
   } else if (left.type() == Value::RANGES) {
@@ -167,6 +172,11 @@ static bool addable(const Resource& left, const Resource& right)
     return false;
   }
 
+  // Check RevocableInfo.
+  if (left.has_revocable() != right.has_revocable()) {
+    return false;
+  }
+
   return true;
 }
 
@@ -211,6 +221,11 @@ static bool subtractable(const Resource& left, const Resource& right)
     return false;
   }
 
+  // Check RevocableInfo.
+  if (left.has_revocable() != right.has_revocable()) {
+    return false;
+  }
+
   return true;
 }
 
@@ -219,8 +234,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, ReservationInfo
-  // and DiskInfo compatibility.
+  // 'subtractable' will verify name, role, type, ReservationInfo,
+  // DiskInfo and RevocableInfo compatibility.
   if (!subtractable(left, right)) {
     return false;
   }
@@ -508,6 +523,11 @@ bool Resources::isDynamicallyReserved(const Resource& resource)
 }
 
 
+bool Resources::isRevocable(const Resource& resource)
+{
+  return resource.has_revocable();
+}
+
 /////////////////////////////////////////////////
 // Public member functions.
 /////////////////////////////////////////////////

http://git-wip-us.apache.org/repos/asf/mesos/blob/c4366d8b/src/master/validation.cpp
----------------------------------------------------------------------
diff --git a/src/master/validation.cpp b/src/master/validation.cpp
index b53608f..20a6ac8 100644
--- a/src/master/validation.cpp
+++ b/src/master/validation.cpp
@@ -53,6 +53,27 @@ static bool invalid(char c)
 
 namespace resource {
 
+// Validates the ReservationInfos specified in the given resources (if
+// exist). Returns error if any ReservationInfo is found invalid or
+// unsupported.
+Option<Error> validateDynamicReservationInfo(
+    const RepeatedPtrField<Resource>& resources)
+{
+  foreach (const Resource& resource, resources) {
+    if (!Resources::isDynamicallyReserved(resource)) {
+      continue;
+    }
+
+    if (Resources::isRevocable(resource)) {
+      return Error(
+          "Dynamically reserved resource " + stringify(resource) +
+          " cannot be created from revocable resources");
+    }
+  }
+
+  return None();
+}
+
 
 // Validates the DiskInfos specified in the given resources (if
 // exist). Returns error if any DiskInfo is found invalid or
@@ -65,6 +86,10 @@ Option<Error> validateDiskInfo(const RepeatedPtrField<Resource>& resources)
     }
 
     if (resource.disk().has_persistence()) {
+      if (Resources::isRevocable(resource)) {
+        return Error(
+            "Persistent volumes cannot be created from revocable resources");
+      }
       if (Resources::isUnreserved(resource)) {
         return Error(
             "Persistent volumes cannot be created from unreserved resources");
@@ -149,6 +174,11 @@ Option<Error> validate(const RepeatedPtrField<Resource>& resources)
     return Error("Invalid DiskInfo: " + error.get().message);
   }
 
+  error = validateDynamicReservationInfo(resources);
+  if (error.isSome()) {
+      return Error("Invalid ReservationInfo: " + error.get().message);
+  }
+
   return None();
 }
 

http://git-wip-us.apache.org/repos/asf/mesos/blob/c4366d8b/src/tests/master_validation_tests.cpp
----------------------------------------------------------------------
diff --git a/src/tests/master_validation_tests.cpp b/src/tests/master_validation_tests.cpp
index 9a6f4fa..dc9e91e 100644
--- a/src/tests/master_validation_tests.cpp
+++ b/src/tests/master_validation_tests.cpp
@@ -93,6 +93,16 @@ TEST_F(ResourceValidationTest, DynamicReservation)
 }
 
 
+TEST_F(ResourceValidationTest, RevocableDynamicReservation)
+{
+  Resource resource = Resources::parse("cpus", "8", "role").get();
+  resource.mutable_reservation()->CopyFrom(createReservationInfo("principal"));
+  resource.mutable_revocable();
+
+  EXPECT_SOME(resource::validate(CreateResources(resource)));
+}
+
+
 TEST_F(ResourceValidationTest, InvalidRoleReservationPair)
 {
   Resource resource = Resources::parse("cpus", "8", "*").get();
@@ -166,6 +176,16 @@ TEST_F(ResourceValidationTest, NonPersistentVolume)
 }
 
 
+TEST_F(ResourceValidationTest, RevocablePersistentVolume)
+{
+  Resource volume = Resources::parse("disk", "128", "role1").get();
+  volume.mutable_disk()->CopyFrom(createDiskInfo("id1", "path1"));
+  volume.mutable_revocable();
+
+  EXPECT_SOME(resource::validate(CreateResources(volume)));
+}
+
+
 class ReserveOperationValidationTest : public MesosTest {};
 
 

http://git-wip-us.apache.org/repos/asf/mesos/blob/c4366d8b/src/tests/resources_tests.cpp
----------------------------------------------------------------------
diff --git a/src/tests/resources_tests.cpp b/src/tests/resources_tests.cpp
index a7ec59e..99b2cfc 100644
--- a/src/tests/resources_tests.cpp
+++ b/src/tests/resources_tests.cpp
@@ -1147,6 +1147,104 @@ TEST(ResourcesOperationTest, CreatePersistentVolume)
   EXPECT_ERROR(total.apply(create2));
 }
 
+
+// Helper for creating a revocable resource.
+static Resource createRevocableResource(
+    const string& name,
+    const string& value,
+    const string& role,
+    bool revocable)
+{
+  Resource resource = Resources::parse(name, value, role).get();
+
+  if (revocable) {
+    resource.mutable_revocable();
+  }
+
+  return resource;
+}
+
+
+// This test verfies that revocable and non-revocable resources are
+// not considered equal.
+TEST(RevocableResourceTest, Equals)
+{
+  Resources resources1 = createRevocableResource("cpus", "1", "*", true);
+
+  Resources resources2 = resources1;
+  EXPECT_EQ(resources1, resources2);
+
+  Resources resources3 = createRevocableResource("cpus", "1", "*", false);
+  EXPECT_NE(resources1, resources3);
+}
+
+
+// This test verifies that adding revocable resources to revocable
+// resources will merge them but adding to non-revocable resources
+// will not merge.
+TEST(RevocableResourceTest, Addition)
+{
+  Resources r1 = createRevocableResource("cpus", "1", "*", true);
+  Resources r2 = createRevocableResource("cpus", "1", "*", true);
+  Resources r3 = createRevocableResource("cpus", "2", "*", true);
+
+  // Adding revocable resources will merge them.
+  EXPECT_EQ(r3, r1 + r2);
+
+  Resources r4 = createRevocableResource("cpus", "1", "*", true);
+  Resources r5 = createRevocableResource("cpus", "1", "*", false);
+  Resources r6 = createRevocableResource("cpus", "2", "*", false);
+
+  Resources sum = r4 + r5;
+
+  // Adding revocable and non-revocable resources will not merge them.
+  EXPECT_TRUE(sum.contains(r4));
+  EXPECT_TRUE(sum.contains(r5));
+  EXPECT_FALSE(sum.contains(r3));
+  EXPECT_FALSE(sum.contains(r6));
+}
+
+
+// This test verifies that subtracting revocable resources from
+// revocable resources will merge them but subtracting from
+// non-revocable resources will not merge.
+TEST(RevocableResourceTest, Subtraction)
+{
+  Resources r1 = createRevocableResource("cpus", "1", "*", true);
+  Resources r2 = createRevocableResource("cpus", "1", "*", true);
+
+  // Subtracting revocable resources will merge them.
+  EXPECT_TRUE((r1 - r2).empty());
+
+  Resources r3 = createRevocableResource("cpus", "1", "*", true);
+  Resources r4 = createRevocableResource("cpus", "1", "*", false);
+
+  // Subtracting non-revocable resources from revocable resources is
+  // a no-op.
+  EXPECT_EQ(r3, r3 - r4);
+  EXPECT_TRUE((r3 - r3).empty());
+}
+
+
+// This test verifies that adding revocable resources to revocable or
+// non-revocable resources respects the 'contains' logic.
+TEST(RevocableResourceTest, Contains)
+{
+  Resources r1 = createRevocableResource("cpus", "1", "*", true);
+  Resources r2 = createRevocableResource("cpus", "1", "*", true);
+
+  EXPECT_FALSE(r1.contains(r1 + r2));
+  EXPECT_FALSE(r2.contains(r1 + r2));
+  EXPECT_TRUE((r1 + r2).contains(r1));
+  EXPECT_TRUE((r1 + r2).contains(r2));
+  EXPECT_TRUE((r1 + r2).contains(r1 + r2));
+
+  Resources r3 = createRevocableResource("cpus", "1", "*", false);
+
+  EXPECT_TRUE((r1 + r3).contains(r1));
+  EXPECT_TRUE((r1 + r3).contains(r3));
+}
+
 } // namespace tests {
 } // namespace internal {
 } // namespace mesos {