You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@mesos.apache.org by bm...@apache.org on 2017/02/16 02:03:21 UTC
mesos git commit: Updated RESERVE operation validation for MULTI_ROLE
frameworks.
Repository: mesos
Updated Branches:
refs/heads/master 7ae60706a -> 07b016f05
Updated RESERVE operation validation for MULTI_ROLE frameworks.
This change introduces validation of the 'AllocationInfo' of
resources used in reservations.
Review: https://reviews.apache.org/r/55462/
Project: http://git-wip-us.apache.org/repos/asf/mesos/repo
Commit: http://git-wip-us.apache.org/repos/asf/mesos/commit/07b016f0
Tree: http://git-wip-us.apache.org/repos/asf/mesos/tree/07b016f0
Diff: http://git-wip-us.apache.org/repos/asf/mesos/diff/07b016f0
Branch: refs/heads/master
Commit: 07b016f053c1c82aa177922293a9701da1783de7
Parents: 7ae6070
Author: Benjamin Bannier <be...@mesosphere.io>
Authored: Wed Feb 15 17:15:55 2017 -0800
Committer: Benjamin Mahler <bm...@apache.org>
Committed: Wed Feb 15 18:02:47 2017 -0800
----------------------------------------------------------------------
src/master/validation.cpp | 54 +++++++++++++++--
src/tests/master_validation_tests.cpp | 93 +++++++++++++++++++++++-------
2 files changed, 120 insertions(+), 27 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/mesos/blob/07b016f0/src/master/validation.cpp
----------------------------------------------------------------------
diff --git a/src/master/validation.cpp b/src/master/validation.cpp
index 2bfca95..b8c6b49 100644
--- a/src/master/validation.cpp
+++ b/src/master/validation.cpp
@@ -1609,11 +1609,17 @@ Option<Error> validate(
const Option<string>& principal,
const Option<FrameworkInfo>& frameworkInfo)
{
+ // NOTE: this ensures the reservation is not being made to the "*" role.
Option<Error> error = resource::validate(reserve.resources());
if (error.isSome()) {
return Error("Invalid resources: " + error.get().message);
}
+ Option<hashset<string>> frameworkRoles;
+ if (frameworkInfo.isSome()) {
+ frameworkRoles = protobuf::framework::getRoles(frameworkInfo.get());
+ }
+
foreach (const Resource& resource, reserve.resources()) {
if (!Resources::isDynamicallyReserved(resource)) {
return Error(
@@ -1637,15 +1643,51 @@ Option<Error> validate(
}
}
- if (frameworkInfo.isSome()) {
- set<string> frameworkRoles =
- protobuf::framework::getRoles(frameworkInfo.get());
+ if (frameworkRoles.isSome()) {
+ // If information on the framework was passed we are dealing
+ // with a request over the framework API. In this case we expect
+ // that the reserved resources where allocated to the role, and
+ // that the allocation role and reservation role match the role
+ // of the framework.
+ if (!resource.allocation_info().has_role()) {
+ return Error(
+ "A reserve operation was attempted on unallocated resource"
+ " " + stringify(resource) + ", but frameworks can only"
+ " perform reservations on allocated resources");
+ }
- if (frameworkRoles.count(resource.role()) == 0) {
+ if (resource.allocation_info().role() != resource.role()) {
return Error(
"A reserve operation was attempted for a resource with role"
- " '" + resource.role() + "', but the framework can only reserve"
- " resources with roles '" + stringify(frameworkRoles) + "'");
+ " '" + resource.role() + "', but the resource was allocated"
+ " to role '" + resource.allocation_info().role() + "'");
+ }
+
+ if (!frameworkRoles->contains(resource.allocation_info().role())) {
+ return Error(
+ "A reserve operation was attempted for a resource allocated"
+ " to role '" + resource.role() + "', but the framework only"
+ " has roles '" + stringify(frameworkRoles.get()) + "'");
+ }
+
+ if (!frameworkRoles->contains(resource.role())) {
+ return Error(
+ "A reserve operation was attempted for a resource with role"
+ " '" + resource.role() + "', but the framework can only"
+ " reserve resources with roles"
+ " '" + stringify(frameworkRoles.get()) + "'");
+ }
+ } else {
+ // If no `FrameworkInfo` was passed we are dealing with a
+ // request via the operator HTTP API. In this case we expect
+ // that the reserved resources have no `AllocationInfo` set
+ // because operators can only reserve from the unallocated
+ // resources.
+ if (resource.has_allocation_info()) {
+ return Error(
+ "A reserve operation was attempted with an allocated resource,"
+ " but the operator API only allows reservations to be made to"
+ " unallocated resources");
}
}
http://git-wip-us.apache.org/repos/asf/mesos/blob/07b016f0/src/tests/master_validation_tests.cpp
----------------------------------------------------------------------
diff --git a/src/tests/master_validation_tests.cpp b/src/tests/master_validation_tests.cpp
index fd1f4a6..83c88ae 100644
--- a/src/tests/master_validation_tests.cpp
+++ b/src/tests/master_validation_tests.cpp
@@ -259,12 +259,22 @@ TEST_F(ReserveOperationValidationTest, MatchingRole)
operation::validate(reserve, "principal", frameworkInfo);
EXPECT_SOME(error);
+
+ // Now verify with a MULTI_ROLE framework.
+ frameworkInfo.clear_role();
+ frameworkInfo.add_roles("role1");
+ frameworkInfo.add_roles("role2");
+ frameworkInfo.add_capabilities()->set_type(
+ FrameworkInfo::Capability::MULTI_ROLE);
+
+ error = operation::validate(reserve, "principal", frameworkInfo);
+
+ EXPECT_SOME(error);
}
-// This test verifies that validation fails if the framework role is
-// "*" even if the resource role matches.
-TEST_F(ReserveOperationValidationTest, DisallowStarRoleFrameworks)
+// This test verifies that validation fails if reserving to the "*" role.
+TEST_F(ReserveOperationValidationTest, DisallowReservingToStar)
{
// The role "*" matches, but is invalid since frameworks with
// "*" role cannot reserve resources.
@@ -281,26 +291,15 @@ TEST_F(ReserveOperationValidationTest, DisallowStarRoleFrameworks)
operation::validate(reserve, "principal", frameworkInfo);
EXPECT_SOME(error);
-}
+ // Now verify with a MULTI_ROLE framework.
+ frameworkInfo.clear_role();
+ frameworkInfo.add_roles("role");
+ frameworkInfo.add_roles("*");
+ frameworkInfo.add_capabilities()->set_type(
+ FrameworkInfo::Capability::MULTI_ROLE);
-// This test verifies that validation fails if the framework attempts
-// to reserve a resource with the role "*".
-TEST_F(ReserveOperationValidationTest, DisallowReserveForStarRole)
-{
- // Principal "principal" reserving for "*".
- Resource resource = Resources::parse("cpus", "8", "*").get();
- resource.mutable_reservation()->CopyFrom(
- createReservationInfo("principal"));
-
- Offer::Operation::Reserve reserve;
- reserve.add_resources()->CopyFrom(resource);
-
- FrameworkInfo frameworkInfo;
- frameworkInfo.set_role("frameworkRole");
-
- Option<Error> error =
- operation::validate(reserve, "principal", frameworkInfo);
+ error = operation::validate(reserve, "principal", frameworkInfo);
EXPECT_SOME(error);
}
@@ -417,6 +416,58 @@ TEST_F(ReserveOperationValidationTest, NoPersistentVolumes)
}
+// This test verifies that validation fails if a resource is reserved
+// for a role different from the one it was allocated to.
+TEST_F(ReserveOperationValidationTest, MismatchedAllocation)
+{
+ Resource resource = Resources::parse("cpus", "8", "role1").get();
+ resource.mutable_reservation()->CopyFrom(createReservationInfo("principal"));
+
+ Offer::Operation::Reserve reserve;
+ reserve.mutable_resources()->CopyFrom(
+ allocatedResources(resource, "role2"));
+
+ FrameworkInfo frameworkInfo;
+ frameworkInfo.add_roles("role1");
+ frameworkInfo.add_roles("role2");
+ frameworkInfo.add_capabilities()->set_type(
+ FrameworkInfo::Capability::MULTI_ROLE);
+
+ Option<Error> error =
+ operation::validate(reserve, "principal", frameworkInfo);
+
+ ASSERT_SOME(error);
+ EXPECT_TRUE(
+ strings::contains(
+ error->message,
+ "A reserve operation was attempted for a resource with role "
+ "'role1', but the resource was allocated to role 'role2'"));
+}
+
+
+// This test verifies that validation fails if an allocated resource
+// is used in the operator HTTP API.
+TEST_F(ReserveOperationValidationTest, UnexpectedAllocatedResource)
+{
+ Resource resource = Resources::parse("cpus", "8", "role").get();
+ resource.mutable_reservation()->CopyFrom(createReservationInfo("principal"));
+
+ Offer::Operation::Reserve reserve;
+ reserve.mutable_resources()->CopyFrom(allocatedResources(resource, "role"));
+
+ // HTTP-API style invocations do not pass a `FrameworkInfo`.
+ Option<Error> error = operation::validate(reserve, "principal", None());
+
+ ASSERT_SOME(error);
+ EXPECT_TRUE(
+ strings::contains(
+ error->message,
+ "A reserve operation was attempted with an allocated resource,"
+ " but the operator API only allows reservations to be made"
+ " to unallocated resources"));
+}
+
+
class UnreserveOperationValidationTest : public MesosTest {};