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 <bev...@mesosphere.com> Authored: Fri Jan 12 11:33:57 2018 -0800 Committer: Vinod Kone <vinodk...@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 {
