mesos-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From vinodk...@apache.org
Subject mesos git commit: Fixed comparison logic for additive reconfiguration policy.
Date Fri, 12 Jan 2018 20:09:58 GMT
Repository: mesos
Updated Branches:
  refs/heads/master e39809bd6 -> 49f4b2aff


Fixed comparison logic for additive reconfiguration policy.

The case where multiple resources have the same name
was not handled correctly, and could result in false
negatives.

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


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

Branch: refs/heads/master
Commit: 49f4b2aff4d9f0603947b04071203a55d008a61c
Parents: e39809b
Author: Benno Evers <bevers@mesosphere.com>
Authored: Fri Jan 12 11:33:57 2018 -0800
Committer: Vinod Kone <vinodkone@gmail.com>
Committed: Fri Jan 12 11:35:00 2018 -0800

----------------------------------------------------------------------
 include/mesos/resources.hpp             |  5 ++
 src/common/resources.cpp                | 22 +++++++-
 src/slave/compatibility.cpp             | 73 ++++++++++++-------------
 src/slave/compatibility.hpp             |  4 +-
 src/tests/slave_compatibility_tests.cpp | 80 ++++++++++++++++++++++++++++
 5 files changed, 142 insertions(+), 42 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/mesos/blob/49f4b2af/include/mesos/resources.hpp
----------------------------------------------------------------------
diff --git a/include/mesos/resources.hpp b/include/mesos/resources.hpp
index 69bf823..7afe0d8 100644
--- a/include/mesos/resources.hpp
+++ b/include/mesos/resources.hpp
@@ -492,6 +492,11 @@ public:
   // Error if the conversion cannot be applied.
   Try<Resources> apply(const ResourceConversion& conversion) const;
 
+  // Finds a resource object with the same metadata (i.e. AllocationInfo,
+  // ReservationInfo, etc.) as the given one, ignoring the actual value.
+  // If multiple matching resources exist, the first match is returned.
+  Option<Resource> match(const Resource& resource) const;
+
   // Obtains the conversion from the given operation and applies the
   // conversion. This method serves a syntax sugar for applying a
   // resource conversion.

http://git-wip-us.apache.org/repos/asf/mesos/blob/49f4b2af/src/common/resources.cpp
----------------------------------------------------------------------
diff --git a/src/common/resources.cpp b/src/common/resources.cpp
index 69e8e34..262dcfb 100644
--- a/src/common/resources.cpp
+++ b/src/common/resources.cpp
@@ -260,7 +260,7 @@ bool operator!=(const Resource::DiskInfo& left, const Resource::DiskInfo&
right)
 }
 
 
-bool operator==(const Resource& left, const Resource& right)
+static bool compareResourceMetadata(const Resource& left, const Resource& right)
 {
   if (left.name() != right.name() || left.type() != right.type()) {
     return false;
@@ -315,6 +315,15 @@ bool operator==(const Resource& left, const Resource& right)
     return false;
   }
 
+  return true;
+}
+
+
+bool operator==(const Resource& left, const Resource& right) {
+  if (!compareResourceMetadata(left, right)) {
+    return false;
+  }
+
   if (left.type() == Value::SCALAR) {
     return left.scalar() == right.scalar();
   } else if (left.type() == Value::RANGES) {
@@ -1852,6 +1861,17 @@ Option<Value::Ranges> Resources::ephemeral_ports() const
 }
 
 
+Option<Resource> Resources::match(const Resource& resource) const
+{
+  foreach (const Resource_& resource_, resources) {
+    if (compareResourceMetadata(resource_.resource, resource)) {
+      return resource_.resource;
+    }
+  }
+
+  return None();
+}
+
 /////////////////////////////////////////////////
 // Private member functions.
 /////////////////////////////////////////////////

http://git-wip-us.apache.org/repos/asf/mesos/blob/49f4b2af/src/slave/compatibility.cpp
----------------------------------------------------------------------
diff --git a/src/slave/compatibility.cpp b/src/slave/compatibility.cpp
index 4ead4a5..5551d93 100644
--- a/src/slave/compatibility.cpp
+++ b/src/slave/compatibility.cpp
@@ -19,6 +19,8 @@
 #include <stout/strings.hpp>
 #include <stout/unreachable.hpp>
 
+#include <mesos/attributes.hpp>
+#include <mesos/resources.hpp>
 #include <mesos/values.hpp>
 
 #include "mesos/type_utils.hpp"
@@ -49,33 +51,6 @@ Try<Nothing> equal(
 }
 
 
-// T is instantiated below as either `Resource` or `Attribute`.
-template<typename T>
-Try<T> getMatchingValue(
-  const T& previous,
-  const google::protobuf::RepeatedPtrField<T>& values)
-{
-  auto match = std::find_if(
-      values.begin(),
-      values.end(),
-      [&previous](const T& value) {
-        return previous.name() == value.name();
-      });
-
-  if (match == values.end()) {
-    return Error("Couldn't find '" + previous.name() + "'");
-  }
-
-  if (match->type() != previous.type()) {
-    return Error(
-        "Type of '" + previous.name() + "' changed from " +
-        stringify(previous.type()) + " to " + stringify(match->type()));
-  }
-
-  return *match;
-}
-
-
 Try<Nothing> additive(
     const SlaveInfo& previous,
     const SlaveInfo& current)
@@ -101,16 +76,20 @@ Try<Nothing> additive(
         stringify(current.domain()));
   }
 
+  // As a side effect, the Resources constructor also normalizes all addable
+  // resources, e.g. 'cpus:5;cpus:5' -> cpus:10
+  Resources previousResources(previous.resources());
+  Resources currentResources(current.resources());
+
   // TODO(bennoe): We should probably check `resources.size()` and switch to a
   // smarter algorithm for the matching when its bigger than, say, 20.
-  for (const Resource& resource : previous.resources()) {
-    Try<Resource> match =
-      getMatchingValue(resource, current.resources());
+  for (const Resource& resource : previousResources) {
+    Option<Resource> match = currentResources.match(resource);
 
-    if (match.isError()) {
+    if (match.isNone()) {
       return Error(
-          "Configuration change not permitted under 'additive' policy: " +
-          match.error());
+          "Configuration change not permitted under 'additive' policy. "
+          "Resource not found: " + stringify(resource));
     }
 
     switch (resource.type()) {
@@ -145,20 +124,34 @@ Try<Nothing> additive(
         continue;
       }
       case Value::TEXT: {
-        // Text resources are not supported.
+        // Text resources are not supported by Mesos.
         UNREACHABLE();
       }
     }
   }
 
+  const google::protobuf::RepeatedPtrField<Attribute>& currentAttributes =
+      current.attributes();
+
   for (const Attribute& attribute : previous.attributes()) {
-    Try<Attribute> match =
-      getMatchingValue(attribute, current.attributes());
+    auto match = std::find_if(
+      currentAttributes.begin(),
+      currentAttributes.end(),
+      [&attribute](const Attribute& value) {
+        return attribute.name() == value.name();
+      });
+
+    if (match == currentAttributes.end()) {
+      return Error(
+          "Configuration change not permitted under 'additive' policy: "
+          "Couldn't find attribute " +  stringify(attribute));
+    }
 
-    if (match.isError()) {
+    if (match->type() != attribute.type()) {
       return Error(
-          "Configuration change not permitted under 'additive' policy: " +
-          match.error());
+          "Configuration change not permitted under 'additive' policy: "
+          "Type of attribute '" + attribute.name() + "' changed from " +
+          stringify(attribute.type()) + " to " + stringify(match->type()));
     }
 
     switch (attribute.type()) {
@@ -192,7 +185,7 @@ Try<Nothing> additive(
         continue;
       }
       case Value::SET: {
-        // Set attributes are not supported.
+        // Set attributes are not supported by Mesos.
         UNREACHABLE();
       }
     }

http://git-wip-us.apache.org/repos/asf/mesos/blob/49f4b2af/src/slave/compatibility.hpp
----------------------------------------------------------------------
diff --git a/src/slave/compatibility.hpp b/src/slave/compatibility.hpp
index 78b421a..de21fd6 100644
--- a/src/slave/compatibility.hpp
+++ b/src/slave/compatibility.hpp
@@ -48,12 +48,14 @@ Try<Nothing> equal(
 //            | For type SCALAR: The new value must be not smaller than the old.
 //            | For type RANGE:  The new value must include the old ranges.
 //            | For type SET:    The new value must be a superset of the old.
-//            | New resources are permitted.
+//            | New resources are permitted. In the presence of reservations,
+//            | these rules are applied per role.
 // attributes | All previous attributes must be present with the same type.
 //            | For type SCALAR: The new value must be not smaller than the old.
 //            | For type RANGE:  The new value must include the old ranges.
 //            | For type TEXT:   The new value must exactly match the previous.
 //            | New attributes are permitted.
+//
 Try<Nothing> additive(
     const SlaveInfo& previous,
     const SlaveInfo& current);

http://git-wip-us.apache.org/repos/asf/mesos/blob/49f4b2af/src/tests/slave_compatibility_tests.cpp
----------------------------------------------------------------------
diff --git a/src/tests/slave_compatibility_tests.cpp b/src/tests/slave_compatibility_tests.cpp
index ab5ed29..12bfdc3 100644
--- a/src/tests/slave_compatibility_tests.cpp
+++ b/src/tests/slave_compatibility_tests.cpp
@@ -66,6 +66,11 @@ TEST_F(SlaveCompatibilityTest, Equal)
 
   *(changedResources.mutable_resources()) = Resources::parse("cpus:600").get();
   ASSERT_ERROR(slave::compatibility::equal(original, changedResources));
+
+  *(changedResources.mutable_resources()) =
+      Resources::parse("cpus:250;cpus:250").get();
+
+  ASSERT_SOME(slave::compatibility::equal(original, changedResources));
 }
 
 
@@ -116,6 +121,11 @@ TEST_F(SlaveCompatibilityTest, Additive)
   ASSERT_ERROR(slave::compatibility::additive(
       changedScalarResource, originalScalarResource));
 
+  // Going from 50 to 30+30 is still an increase
+  SlaveInfo changedScalarResource2 = createSlaveInfo("cpus:30;cpus:30", "");
+  ASSERT_SOME(slave::compatibility::additive(
+      originalScalarResource, changedScalarResource2));
+
   // Range attributes can be extended but not shrinked.
   SlaveInfo originalRangeResource = createSlaveInfo("range:[100-200]", "");
   SlaveInfo changedRangeResource = createSlaveInfo("range:[100-300]", "");
@@ -170,6 +180,76 @@ TEST_F(SlaveCompatibilityTest, Additive)
       changedRangeAttribute, originalRangeAttribute));
 }
 
+
+TEST_F(SlaveCompatibilityTest, AdditiveWithReservations)
+{
+  SlaveInfo originalReservations = createSlaveInfo("foo(A):10;foo(B):20", "");
+
+  // Ok: Both roles have increased amounts of `foo`.
+  SlaveInfo increasedReservations = createSlaveInfo("foo(A):20;foo(B):30", "");
+  ASSERT_SOME(slave::compatibility::additive(
+      originalReservations, increasedReservations));
+
+  // Not ok: The total increases, but the amount of `foo` reserved for role A
+  // decreased.
+  SlaveInfo modifiedReservations = createSlaveInfo("foo(A):5;foo(B):50", "");
+  ASSERT_ERROR(slave::compatibility::additive(
+      originalReservations, modifiedReservations));
+}
+
+
+TEST_F(SlaveCompatibilityTest, Disks)
+{
+  const char* diskAsJson = R"_(
+  [
+    {
+      "name": "disk",
+      "type": "SCALAR",
+      "scalar": {
+        "value": 868000
+      }
+    }
+  ]
+  )_";
+
+  const  char* diskAndMountAsJson = R"_(
+   [
+    {
+      "name": "disk",
+      "type": "SCALAR",
+      "scalar": {
+        "value": 868000
+      }
+    },
+    {
+      "name": "disk",
+      "type": "SCALAR",
+      "scalar": {
+        "value": 1830000
+      },
+      "disk": {
+        "source": {
+          "type": "MOUNT",
+          "mount": {
+            "root" : "/srv/mesos/volumes/a"
+          }
+        }
+      }
+    }
+  ]
+  )_";
+
+
+
+  SlaveInfo info1 = createSlaveInfo(diskAsJson, "");
+  SlaveInfo info2 = createSlaveInfo(diskAndMountAsJson, "");
+
+  ASSERT_SOME(slave::compatibility::additive(info1, info2));
+
+  // MESOS-8410.
+  ASSERT_SOME(slave::compatibility::additive(info2, info2));
+}
+
 } // namespace tests {
 } // namespace internal {
 } // namespace mesos {


Mime
View raw message