You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@mesos.apache.org by al...@apache.org on 2016/11/08 21:42:18 UTC
mesos git commit: Improved the validation of RESERVE operations.
Repository: mesos
Updated Branches:
refs/heads/master 8a41c7692 -> 9d0ace758
Improved the validation of RESERVE operations.
Don't allow a reservation if the framework role
doesn't match the role of all the resources.
Review: https://reviews.apache.org/r/52642/
Project: http://git-wip-us.apache.org/repos/asf/mesos/repo
Commit: http://git-wip-us.apache.org/repos/asf/mesos/commit/9d0ace75
Tree: http://git-wip-us.apache.org/repos/asf/mesos/tree/9d0ace75
Diff: http://git-wip-us.apache.org/repos/asf/mesos/diff/9d0ace75
Branch: refs/heads/master
Commit: 9d0ace758c8e2a444c15ef0c9395c9e4a15c553f
Parents: 8a41c76
Author: Gast�n Kleiman <ga...@mesosphere.com>
Authored: Tue Nov 8 21:57:29 2016 +0100
Committer: Alexander Rukletsov <al...@apache.org>
Committed: Tue Nov 8 22:30:06 2016 +0100
----------------------------------------------------------------------
src/master/http.cpp | 2 +-
src/master/master.cpp | 2 +-
src/master/validation.cpp | 24 +++++++++++++++++++-----
src/master/validation.hpp | 3 ++-
src/tests/master_validation_tests.cpp | 30 +++++++++++++++---------------
5 files changed, 38 insertions(+), 23 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/mesos/blob/9d0ace75/src/master/http.cpp
----------------------------------------------------------------------
diff --git a/src/master/http.cpp b/src/master/http.cpp
index 2f275f6..88a8df5 100644
--- a/src/master/http.cpp
+++ b/src/master/http.cpp
@@ -2182,7 +2182,7 @@ Future<Response> Master::Http::_reserve(
operation.mutable_reserve()->mutable_resources()->CopyFrom(resources);
Option<Error> error = validation::operation::validate(
- operation.reserve(), principal);
+ operation.reserve(), principal, None());
if (error.isSome()) {
return BadRequest("Invalid RESERVE operation: " + error.get().message);
http://git-wip-us.apache.org/repos/asf/mesos/blob/9d0ace75/src/master/master.cpp
----------------------------------------------------------------------
diff --git a/src/master/master.cpp b/src/master/master.cpp
index 15ad5a6..f0b6297 100644
--- a/src/master/master.cpp
+++ b/src/master/master.cpp
@@ -3862,7 +3862,7 @@ void Master::_accept(
// Make sure this reserve operation is valid.
Option<Error> error = validation::operation::validate(
- operation.reserve(), principal);
+ operation.reserve(), principal, framework->info.role());
if (error.isSome()) {
drop(framework, operation, error.get().message);
http://git-wip-us.apache.org/repos/asf/mesos/blob/9d0ace75/src/master/validation.cpp
----------------------------------------------------------------------
diff --git a/src/master/validation.cpp b/src/master/validation.cpp
index 658f0dd..42d9b4a 100644
--- a/src/master/validation.cpp
+++ b/src/master/validation.cpp
@@ -1418,8 +1418,15 @@ namespace operation {
Option<Error> validate(
const Offer::Operation::Reserve& reserve,
- const Option<string>& principal)
+ const Option<string>& principal,
+ const Option<string>& frameworkRole)
{
+ if (frameworkRole.isSome() && frameworkRole.get() == "*") {
+ return Error(
+ "A reserve operation was attempted by a framework with role"
+ " '*', but frameworks with that role cannot reserve resources");
+ }
+
Option<Error> error = resource::validate(reserve.resources());
if (error.isSome()) {
return Error("Invalid resources: " + error.get().message);
@@ -1435,19 +1442,26 @@ Option<Error> validate(
if (!resource.reservation().has_principal()) {
return Error(
"A reserve operation was attempted by principal '" +
- principal.get() + "', but there is a reserved resource in the "
- "request with no principal set in `ReservationInfo`");
+ principal.get() + "', but there is a reserved resource in the"
+ " request with no principal set in `ReservationInfo`");
}
if (resource.reservation().principal() != principal.get()) {
return Error(
"A reserve operation was attempted by principal '" +
- principal.get() + "', but there is a reserved resource in the "
- "request with principal '" + resource.reservation().principal() +
+ principal.get() + "', but there is a reserved resource in the"
+ " request with principal '" + resource.reservation().principal() +
"' set in `ReservationInfo`");
}
}
+ if (frameworkRole.isSome() && resource.role() != frameworkRole.get()) {
+ return Error(
+ "A reserve operation was attempted for a resource with role"
+ " '" + resource.role() + "', but the framework can only reserve"
+ " resources with role '" + frameworkRole.get() + "'");
+ }
+
// NOTE: This check would be covered by 'contains' since there
// shouldn't be any unreserved resources with 'disk' set.
// However, we keep this check since it will be a more useful
http://git-wip-us.apache.org/repos/asf/mesos/blob/9d0ace75/src/master/validation.hpp
----------------------------------------------------------------------
diff --git a/src/master/validation.hpp b/src/master/validation.hpp
index ed878ef..b838946 100644
--- a/src/master/validation.hpp
+++ b/src/master/validation.hpp
@@ -184,7 +184,8 @@ namespace operation {
// Validates the RESERVE operation.
Option<Error> validate(
const Offer::Operation::Reserve& reserve,
- const Option<std::string>& principal);
+ const Option<std::string>& principal,
+ const Option<std::string>& role);
// Validates the UNRESERVE operation.
http://git-wip-us.apache.org/repos/asf/mesos/blob/9d0ace75/src/tests/master_validation_tests.cpp
----------------------------------------------------------------------
diff --git a/src/tests/master_validation_tests.cpp b/src/tests/master_validation_tests.cpp
index 8697bb2..7f7bb67 100644
--- a/src/tests/master_validation_tests.cpp
+++ b/src/tests/master_validation_tests.cpp
@@ -204,22 +204,22 @@ TEST_F(ResourceValidationTest, SharedPersistentVolume)
class ReserveOperationValidationTest : public MesosTest {};
-// This test verifies that the 'role' specified in the resources of
-// the RESERVE operation needs to match the framework's 'role'.
+// This test verifies that validation fails if the reservation's role
+// doesn't match the framework's role.
TEST_F(ReserveOperationValidationTest, MatchingRole)
{
- Resource resource = Resources::parse("cpus", "8", "role").get();
+ Resource resource = Resources::parse("cpus", "8", "resourceRole").get();
resource.mutable_reservation()->CopyFrom(createReservationInfo("principal"));
Offer::Operation::Reserve reserve;
reserve.add_resources()->CopyFrom(resource);
- EXPECT_NONE(operation::validate(reserve, "principal"));
+ EXPECT_SOME(operation::validate(reserve, "principal", "frameworkRole"));
}
-// This test verifies that validation fails if the framework has a
-// "*" role even if the role matches.
+// This test verifies that validation fails if the framework role is
+// "*" even if the resource role matches.
TEST_F(ReserveOperationValidationTest, DisallowStarRoleFrameworks)
{
// The role "*" matches, but is invalid since frameworks with
@@ -230,12 +230,12 @@ TEST_F(ReserveOperationValidationTest, DisallowStarRoleFrameworks)
Offer::Operation::Reserve reserve;
reserve.add_resources()->CopyFrom(resource);
- EXPECT_SOME(operation::validate(reserve, "principal"));
+ EXPECT_SOME(operation::validate(reserve, "principal", "*"));
}
// This test verifies that validation fails if the framework attempts
-// to reserve for the "*" role.
+// to reserve a resource with the role "*".
TEST_F(ReserveOperationValidationTest, DisallowReserveForStarRole)
{
// Principal "principal" reserving for "*".
@@ -246,7 +246,7 @@ TEST_F(ReserveOperationValidationTest, DisallowReserveForStarRole)
Offer::Operation::Reserve reserve;
reserve.add_resources()->CopyFrom(resource);
- EXPECT_SOME(operation::validate(reserve, "principal"));
+ EXPECT_SOME(operation::validate(reserve, "principal", "frameworkRole"));
}
@@ -260,7 +260,7 @@ TEST_F(ReserveOperationValidationTest, MatchingPrincipal)
Offer::Operation::Reserve reserve;
reserve.add_resources()->CopyFrom(resource);
- EXPECT_NONE(operation::validate(reserve, "principal"));
+ EXPECT_NONE(operation::validate(reserve, "principal", "role"));
}
@@ -275,7 +275,7 @@ TEST_F(ReserveOperationValidationTest, NonMatchingPrincipal)
Offer::Operation::Reserve reserve;
reserve.add_resources()->CopyFrom(resource);
- EXPECT_SOME(operation::validate(reserve, "principal1"));
+ EXPECT_SOME(operation::validate(reserve, "principal1", "role"));
}
@@ -291,7 +291,7 @@ TEST_F(ReserveOperationValidationTest, ReservationInfoMissingPrincipal)
Offer::Operation::Reserve reserve;
reserve.add_resources()->CopyFrom(resource);
- EXPECT_SOME(operation::validate(reserve, "principal"));
+ EXPECT_SOME(operation::validate(reserve, "principal", "role"));
}
@@ -304,7 +304,7 @@ TEST_F(ReserveOperationValidationTest, StaticReservation)
Offer::Operation::Reserve reserve;
reserve.add_resources()->CopyFrom(staticallyReserved);
- EXPECT_SOME(operation::validate(reserve, "principal"));
+ EXPECT_SOME(operation::validate(reserve, "principal", "role"));
}
@@ -318,7 +318,7 @@ TEST_F(ReserveOperationValidationTest, NoPersistentVolumes)
Offer::Operation::Reserve reserve;
reserve.add_resources()->CopyFrom(reserved);
- EXPECT_NONE(operation::validate(reserve, "principal"));
+ EXPECT_NONE(operation::validate(reserve, "principal", "role"));
}
@@ -336,7 +336,7 @@ TEST_F(ReserveOperationValidationTest, PersistentVolumes)
reserve.add_resources()->CopyFrom(reserved);
reserve.add_resources()->CopyFrom(volume);
- EXPECT_SOME(operation::validate(reserve, "principal"));
+ EXPECT_SOME(operation::validate(reserve, "principal", "role"));
}