You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@mesos.apache.org by ji...@apache.org on 2016/02/26 20:43:17 UTC
[2/6] mesos git commit: Removed unnecessary parameter from validation
function.
Removed unnecessary parameter from validation function.
Now that Reserve operations are authorized for particular roles, it is
unnecessary to pass the framework's role into the validation function
for Reserve operations. The function previously ensured that a framework
could only reserve resources for its own role, but this check has been
removed. Since this check has been removed, the test
`ReserveOperationValidationTest.NonMatchingRole` is no longer needed and
has also been removed.
Review: https://reviews.apache.org/r/43777/
Project: http://git-wip-us.apache.org/repos/asf/mesos/repo
Commit: http://git-wip-us.apache.org/repos/asf/mesos/commit/082ab0ee
Tree: http://git-wip-us.apache.org/repos/asf/mesos/tree/082ab0ee
Diff: http://git-wip-us.apache.org/repos/asf/mesos/diff/082ab0ee
Branch: refs/heads/master
Commit: 082ab0ee9d86ad29c2d19ac103395436612ff379
Parents: f52bce7
Author: Greg Mann <gr...@mesosphere.io>
Authored: Fri Feb 26 11:42:26 2016 -0800
Committer: Jie Yu <yu...@gmail.com>
Committed: Fri Feb 26 11:42:26 2016 -0800
----------------------------------------------------------------------
src/master/http.cpp | 2 +-
src/master/master.cpp | 2 +-
src/master/validation.cpp | 1 -
src/master/validation.hpp | 1 -
src/tests/master_validation_tests.cpp | 51 ++++++------------------------
5 files changed, 12 insertions(+), 45 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/mesos/blob/082ab0ee/src/master/http.cpp
----------------------------------------------------------------------
diff --git a/src/master/http.cpp b/src/master/http.cpp
index 950206b..f3ce1aa 100644
--- a/src/master/http.cpp
+++ b/src/master/http.cpp
@@ -1204,7 +1204,7 @@ Future<Response> Master::Http::reserve(
operation.mutable_reserve()->mutable_resources()->CopyFrom(resources);
Option<Error> error = validation::operation::validate(
- operation.reserve(), None(), principal);
+ operation.reserve(), principal);
if (error.isSome()) {
return BadRequest("Invalid RESERVE operation: " + error.get().message);
http://git-wip-us.apache.org/repos/asf/mesos/blob/082ab0ee/src/master/master.cpp
----------------------------------------------------------------------
diff --git a/src/master/master.cpp b/src/master/master.cpp
index 5c81ce3..5522a44 100644
--- a/src/master/master.cpp
+++ b/src/master/master.cpp
@@ -3353,7 +3353,7 @@ void Master::_accept(
// Make sure this reserve operation is valid.
Option<Error> error = validation::operation::validate(
- operation.reserve(), framework->info.role(), principal);
+ operation.reserve(), principal);
if (error.isSome()) {
drop(framework, operation, error.get().message);
http://git-wip-us.apache.org/repos/asf/mesos/blob/082ab0ee/src/master/validation.cpp
----------------------------------------------------------------------
diff --git a/src/master/validation.cpp b/src/master/validation.cpp
index ec26981..820a9fa 100644
--- a/src/master/validation.cpp
+++ b/src/master/validation.cpp
@@ -652,7 +652,6 @@ namespace operation {
Option<Error> validate(
const Offer::Operation::Reserve& reserve,
- const Option<string>& role,
const Option<string>& principal)
{
Option<Error> error = resource::validate(reserve.resources());
http://git-wip-us.apache.org/repos/asf/mesos/blob/082ab0ee/src/master/validation.hpp
----------------------------------------------------------------------
diff --git a/src/master/validation.hpp b/src/master/validation.hpp
index 6ec883e..29dbdf1 100644
--- a/src/master/validation.hpp
+++ b/src/master/validation.hpp
@@ -105,7 +105,6 @@ namespace operation {
// Validates the RESERVE operation.
Option<Error> validate(
const Offer::Operation::Reserve& reserve,
- const Option<std::string>& role,
const Option<std::string>& principal);
http://git-wip-us.apache.org/repos/asf/mesos/blob/082ab0ee/src/tests/master_validation_tests.cpp
----------------------------------------------------------------------
diff --git a/src/tests/master_validation_tests.cpp b/src/tests/master_validation_tests.cpp
index 7db7336..c9bc38c 100644
--- a/src/tests/master_validation_tests.cpp
+++ b/src/tests/master_validation_tests.cpp
@@ -197,7 +197,7 @@ TEST_F(ReserveOperationValidationTest, MatchingRole)
Offer::Operation::Reserve reserve;
reserve.add_resources()->CopyFrom(resource);
- EXPECT_NONE(operation::validate(reserve, "role", "principal"));
+ EXPECT_NONE(operation::validate(reserve, "principal"));
}
@@ -213,7 +213,7 @@ 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"));
}
@@ -229,38 +229,7 @@ TEST_F(ReserveOperationValidationTest, DisallowReserveForStarRole)
Offer::Operation::Reserve reserve;
reserve.add_resources()->CopyFrom(resource);
- EXPECT_SOME(operation::validate(reserve, "role", "principal"));
-}
-
-
-// This test verifies that validation succeeds even if the 'role'
-// specified in the resources of the RESERVE operation does not
-// match the framework's 'role'.
-TEST_F(ReserveOperationValidationTest, NonMatchingRole)
-{
- {
- // Non-matching role, "*" reserving for "role".
- Resource resource = Resources::parse("cpus", "8", "role").get();
- resource.mutable_reservation()->CopyFrom(
- createReservationInfo("principal"));
-
- Offer::Operation::Reserve reserve;
- reserve.add_resources()->CopyFrom(resource);
-
- EXPECT_NONE(operation::validate(reserve, "*", "principal"));
- }
-
- {
- // Non-matching role, "role1" reserving for "role2".
- Resource resource = Resources::parse("cpus", "8", "role2").get();
- resource.mutable_reservation()->CopyFrom(
- createReservationInfo("principal"));
-
- Offer::Operation::Reserve reserve;
- reserve.add_resources()->CopyFrom(resource);
-
- EXPECT_NONE(operation::validate(reserve, "role1", "principal"));
- }
+ EXPECT_SOME(operation::validate(reserve, "principal"));
}
@@ -274,7 +243,7 @@ TEST_F(ReserveOperationValidationTest, MatchingPrincipal)
Offer::Operation::Reserve reserve;
reserve.add_resources()->CopyFrom(resource);
- EXPECT_NONE(operation::validate(reserve, "role", "principal"));
+ EXPECT_NONE(operation::validate(reserve, "principal"));
}
@@ -289,7 +258,7 @@ TEST_F(ReserveOperationValidationTest, NonMatchingPrincipal)
Offer::Operation::Reserve reserve;
reserve.add_resources()->CopyFrom(resource);
- EXPECT_SOME(operation::validate(reserve, "role", "principal1"));
+ EXPECT_SOME(operation::validate(reserve, "principal1"));
}
@@ -303,7 +272,7 @@ TEST_F(ReserveOperationValidationTest, FrameworkMissingPrincipal)
Offer::Operation::Reserve reserve;
reserve.add_resources()->CopyFrom(resource);
- EXPECT_SOME(operation::validate(reserve, "role", None()));
+ EXPECT_SOME(operation::validate(reserve, None()));
}
@@ -319,7 +288,7 @@ TEST_F(ReserveOperationValidationTest, ReservationInfoMissingPrincipal)
Offer::Operation::Reserve reserve;
reserve.add_resources()->CopyFrom(resource);
- EXPECT_SOME(operation::validate(reserve, "role", "principal"));
+ EXPECT_SOME(operation::validate(reserve, "principal"));
}
@@ -332,7 +301,7 @@ TEST_F(ReserveOperationValidationTest, StaticReservation)
Offer::Operation::Reserve reserve;
reserve.add_resources()->CopyFrom(staticallyReserved);
- EXPECT_SOME(operation::validate(reserve, "role", "principal"));
+ EXPECT_SOME(operation::validate(reserve, "principal"));
}
@@ -346,7 +315,7 @@ TEST_F(ReserveOperationValidationTest, NoPersistentVolumes)
Offer::Operation::Reserve reserve;
reserve.add_resources()->CopyFrom(reserved);
- EXPECT_NONE(operation::validate(reserve, "role", "principal"));
+ EXPECT_NONE(operation::validate(reserve, "principal"));
}
@@ -364,7 +333,7 @@ TEST_F(ReserveOperationValidationTest, PersistentVolumes)
reserve.add_resources()->CopyFrom(reserved);
reserve.add_resources()->CopyFrom(volume);
- EXPECT_SOME(operation::validate(reserve, "role", "principal"));
+ EXPECT_SOME(operation::validate(reserve, "principal"));
}