mesos git commit: Added validation for operations applied on resource provider resources.

[ji...@apache.org]

Repository: mesos
Updated Branches:
  refs/heads/master dfa813ecb -> c88c2e241


Added validation for operations applied on resource provider resources.

Some operations can be provided with multiple resources. As we currently
don't support operations that use resources of multiple resource
providers, validation has been added to make sure that these resources
are from a single resource provider.

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


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

Branch: refs/heads/master
Commit: c88c2e241c32e917e58df3f458d5905f1688c1f7
Parents: dfa813e
Author: Jan Schlicht <ja...@mesosphere.io>
Authored: Wed Jan 3 15:45:53 2018 -0800
Committer: Jie Yu <yu...@gmail.com>
Committed: Wed Jan 3 15:45:53 2018 -0800

----------------------------------------------------------------------
 src/master/validation.cpp             |  22 +++
 src/tests/master_validation_tests.cpp | 241 +++++++++++++++++++++++++++++
 2 files changed, 263 insertions(+)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/mesos/blob/c88c2e24/src/master/validation.cpp
----------------------------------------------------------------------
diff --git a/src/master/validation.cpp b/src/master/validation.cpp
index 44d7a57..f0b8677 100644
--- a/src/master/validation.cpp
+++ b/src/master/validation.cpp
@@ -1942,6 +1942,12 @@ Option<Error> validate(
     return Error("Invalid resources: " + error.get().message);
   }
 
+  error =
+    resource::internal::validateSingleResourceProvider(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());
@@ -2080,6 +2086,12 @@ Option<Error> validate(
     return Error("Invalid resources: " + error.get().message);
   }
 
+  error =
+    resource::internal::validateSingleResourceProvider(unreserve.resources());
+  if (error.isSome()) {
+    return Error("Invalid resources: " + error.get().message);
+  }
+
   // NOTE: We don't check that 'FrameworkInfo.principal' matches
   // 'Resource.ReservationInfo.principal' here because the authorization
   // depends on the "unreserve" ACL which specifies which 'principal' can
@@ -2117,6 +2129,11 @@ Option<Error> validate(
     return Error("Invalid resources: " + error.get().message);
   }
 
+  error = resource::internal::validateSingleResourceProvider(create.volumes());
+  if (error.isSome()) {
+    return Error("Invalid resources: " + error.get().message);
+  }
+
   error = resource::validatePersistentVolume(create.volumes());
   if (error.isSome()) {
     return Error("Not a persistent volume: " + error.get().message);
@@ -2232,6 +2249,11 @@ Option<Error> validate(
     return Error("Invalid resources: " + error.get().message);
   }
 
+  error = resource::internal::validateSingleResourceProvider(volumes);
+  if (error.isSome()) {
+    return Error("Invalid resources: " + error.get().message);
+  }
+
   error = resource::validatePersistentVolume(volumes);
   if (error.isSome()) {
     return Error("Not a persistent volume: " + error.get().message);

http://git-wip-us.apache.org/repos/asf/mesos/blob/c88c2e24/src/tests/master_validation_tests.cpp
----------------------------------------------------------------------
diff --git a/src/tests/master_validation_tests.cpp b/src/tests/master_validation_tests.cpp
index d6216cc..ed8f9bd 100644
--- a/src/tests/master_validation_tests.cpp
+++ b/src/tests/master_validation_tests.cpp
@@ -708,6 +708,74 @@ TEST_F(ReserveOperationValidationTest, AgentHierarchicalRoleCapability)
 }
 
 
+// This test verifies that validation fails if resources from multiple
+// resource providers are used.
+TEST_F(ReserveOperationValidationTest, MultipleResourceProviders)
+{
+  protobuf::slave::Capabilities capabilities;
+
+  FrameworkInfo frameworkInfo;
+  frameworkInfo.set_role("role");
+  frameworkInfo.add_capabilities()->set_type(
+      FrameworkInfo::Capability::RESERVATION_REFINEMENT);
+
+  Resource resource1 = Resources::parse("cpus", "8", "*").get();
+  resource1.add_reservations()->CopyFrom(
+      createDynamicReservationInfo("role", "principal"));
+
+  Resource resource2 = Resources::parse("disk", "10", "*").get();
+  resource2.mutable_provider_id()->set_value("provider1");
+  resource2.add_reservations()->CopyFrom(
+      createDynamicReservationInfo("role", "principal"));
+
+  Resource resource3 = Resources::parse("disk", "10", "*").get();
+  resource3.mutable_provider_id()->set_value("provider2");
+  resource3.add_reservations()->CopyFrom(
+      createDynamicReservationInfo("role", "principal"));
+
+  {
+    Offer::Operation::Reserve reserve;
+    reserve.mutable_resources()->CopyFrom(
+        allocatedResources(Resources(resource1) + resource2, "role"));
+
+    Option<Error> error =
+      operation::validate(reserve, "principal", capabilities, frameworkInfo);
+
+    ASSERT_SOME(error);
+    EXPECT_TRUE(strings::contains(
+        error->message,
+        "Invalid resources: Some resources have a resource provider "
+        "and some do not"));
+  }
+
+  {
+    Offer::Operation::Reserve reserve;
+    reserve.mutable_resources()->CopyFrom(
+        allocatedResources(Resources(resource2) + resource3, "role"));
+
+    Option<Error> error =
+      operation::validate(reserve, "principal", capabilities, frameworkInfo);
+
+    ASSERT_SOME(error);
+    EXPECT_TRUE(strings::contains(
+        error->message,
+        "Invalid resources: The resources have multiple resource providers: "
+        "provider2, provider1"));
+  }
+
+  {
+    Offer::Operation::Reserve reserve;
+    reserve.mutable_resources()->CopyFrom(
+        allocatedResources(resource2, "role"));
+
+    Option<Error> error =
+      operation::validate(reserve, "principal", capabilities, frameworkInfo);
+
+    EXPECT_NONE(error) << error->message;
+  }
+}
+
+
 class UnreserveOperationValidationTest : public MesosTest {};
 
 
@@ -781,6 +849,63 @@ TEST_F(UnreserveOperationValidationTest, NoPersistentVolumes)
 }
 
 
+// This test verifies that validation fails if resources from multiple
+// resource providers are used.
+TEST_F(UnreserveOperationValidationTest, MultipleResourceProviders)
+{
+  Resource resource1 = Resources::parse("cpus", "8", "*").get();
+  resource1.add_reservations()->CopyFrom(
+      createDynamicReservationInfo("role", "principal"));
+
+  Resource resource2 = Resources::parse("disk", "10", "*").get();
+  resource2.mutable_provider_id()->set_value("provider1");
+  resource2.add_reservations()->CopyFrom(
+      createDynamicReservationInfo("role", "principal"));
+
+  Resource resource3 = Resources::parse("disk", "10", "*").get();
+  resource3.mutable_provider_id()->set_value("provider2");
+  resource3.add_reservations()->CopyFrom(
+      createDynamicReservationInfo("role", "principal"));
+
+  {
+    Offer::Operation::Unreserve unreserve;
+    unreserve.add_resources()->CopyFrom(resource1);
+    unreserve.add_resources()->CopyFrom(resource2);
+
+    Option<Error> error = operation::validate(unreserve);
+
+    ASSERT_SOME(error);
+    EXPECT_TRUE(strings::contains(
+        error->message,
+        "Invalid resources: Some resources have a resource provider "
+        "and some do not"));
+  }
+
+  {
+    Offer::Operation::Unreserve unreserve;
+    unreserve.add_resources()->CopyFrom(resource2);
+    unreserve.add_resources()->CopyFrom(resource3);
+
+    Option<Error> error = operation::validate(unreserve);
+
+    ASSERT_SOME(error);
+    EXPECT_TRUE(strings::contains(
+        error->message,
+        "Invalid resources: The resources have multiple resource providers: "
+        "provider2, provider1"));
+  }
+
+  {
+    Offer::Operation::Unreserve unreserve;
+    unreserve.add_resources()->CopyFrom(resource2);
+
+    Option<Error> error = operation::validate(unreserve);
+
+    EXPECT_NONE(error) << error->message;
+  }
+}
+
+
 class CreateOperationValidationTest : public MesosTest {};
 
 
@@ -1093,6 +1218,66 @@ TEST_F(CreateOperationValidationTest, AgentHierarchicalRoleCapability)
 }
 
 
+// This test verifies that validation fails if resources from multiple
+// resource providers are used.
+TEST_F(CreateOperationValidationTest, MultipleResourceProviders)
+{
+  protobuf::slave::Capabilities capabilities;
+
+  Resource resource1 = Resources::parse("disk", "10", "role").get();
+  resource1.mutable_disk()->CopyFrom(createDiskInfo("id1", "path1"));
+
+
+  Resource resource2 = Resources::parse("disk", "10", "role").get();
+  resource2.mutable_provider_id()->set_value("provider1");
+  resource2.mutable_disk()->CopyFrom(createDiskInfo("id2", "path2"));
+
+  Resource resource3 = Resources::parse("disk", "10", "role").get();
+  resource3.mutable_provider_id()->set_value("provider2");
+  resource3.mutable_disk()->CopyFrom(createDiskInfo("id3", "path3"));
+
+  {
+    Offer::Operation::Create create;
+    create.add_volumes()->CopyFrom(resource1);
+    create.add_volumes()->CopyFrom(resource2);
+
+    Option<Error> error =
+      operation::validate(create, Resources(), None(), capabilities);
+
+    ASSERT_SOME(error);
+    EXPECT_TRUE(strings::contains(
+        error->message,
+        "Invalid resources: Some resources have a resource provider "
+        "and some do not"));
+  }
+
+  {
+    Offer::Operation::Create create;
+    create.add_volumes()->CopyFrom(resource2);
+    create.add_volumes()->CopyFrom(resource3);
+
+    Option<Error> error =
+      operation::validate(create, Resources(), None(), capabilities);
+
+    ASSERT_SOME(error);
+    EXPECT_TRUE(strings::contains(
+        error->message,
+        "Invalid resources: The resources have multiple resource providers: "
+        "provider2, provider1"));
+  }
+
+  {
+    Offer::Operation::Create create;
+    create.add_volumes()->CopyFrom(resource2);
+
+    Option<Error> error =
+      operation::validate(create, Resources(), None(), capabilities);
+
+    EXPECT_NONE(error) << error->message;
+  }
+}
+
+
 class DestroyOperationValidationTest : public ::testing::Test {};
 
 
@@ -1221,6 +1406,62 @@ TEST_F(DestroyOperationValidationTest, UnknownPersistentVolume)
 }
 
 
+// This test verifies that validation fails if resources from multiple
+// resource providers are used.
+TEST_F(DestroyOperationValidationTest, MultipleResourceProviders)
+{
+  Resource resource1 = Resources::parse("disk", "10", "role").get();
+  resource1.mutable_disk()->CopyFrom(createDiskInfo("id1", "path1"));
+
+  Resource resource2 = Resources::parse("disk", "10", "role").get();
+  resource2.mutable_provider_id()->set_value("provider1");
+  resource2.mutable_disk()->CopyFrom(createDiskInfo("id2", "path2"));
+
+  Resource resource3 = Resources::parse("disk", "10", "role").get();
+  resource3.mutable_provider_id()->set_value("provider2");
+  resource3.mutable_disk()->CopyFrom(createDiskInfo("id3", "path3"));
+
+  Resources volumes = Resources(resource1) + resource2 + resource3;
+
+  {
+    Offer::Operation::Destroy destroy;
+    destroy.add_volumes()->CopyFrom(resource1);
+    destroy.add_volumes()->CopyFrom(resource2);
+
+    Option<Error> error = operation::validate(destroy, volumes, {}, {});
+
+    ASSERT_SOME(error);
+    EXPECT_TRUE(strings::contains(
+        error->message,
+        "Invalid resources: Some resources have a resource provider "
+        "and some do not"));
+  }
+
+  {
+    Offer::Operation::Destroy destroy;
+    destroy.add_volumes()->CopyFrom(resource2);
+    destroy.add_volumes()->CopyFrom(resource3);
+
+    Option<Error> error = operation::validate(destroy, volumes, {}, {});
+
+    ASSERT_SOME(error);
+    EXPECT_TRUE(strings::contains(
+        error->message,
+        "Invalid resources: The resources have multiple resource providers: "
+        "provider2, provider1"));
+  }
+
+  {
+    Offer::Operation::Destroy destroy;
+    destroy.add_volumes()->CopyFrom(resource2);
+
+    Option<Error> error = operation::validate(destroy, volumes, {}, {});
+
+    EXPECT_NONE(error) << error->message;
+  }
+}
+
+
 TEST(OperationValidationTest, CreateVolume)
 {
   Resource disk1 = createDiskResource(