Used persistent volumes consistently in the code base. Review: https://reviews.apache.org/r/30131
Project: http://git-wip-us.apache.org/repos/asf/mesos/repo Commit: http://git-wip-us.apache.org/repos/asf/mesos/commit/820ecf4a Tree: http://git-wip-us.apache.org/repos/asf/mesos/tree/820ecf4a Diff: http://git-wip-us.apache.org/repos/asf/mesos/diff/820ecf4a Branch: refs/heads/master Commit: 820ecf4a5ab2a8651b1ebf4e3c28d21fd35becd7 Parents: 728192a Author: Jie Yu <[email protected]> Authored: Wed Jan 21 10:07:26 2015 -0800 Committer: Jie Yu <[email protected]> Committed: Mon Jan 26 11:44:33 2015 -0800 ---------------------------------------------------------------------- include/mesos/resources.hpp | 63 ++++++++++++++++++-- src/common/resources.cpp | 39 ------------ src/master/drf_sorter.cpp | 2 +- src/master/master.cpp | 2 +- src/messages/messages.proto | 18 +++--- .../containerizer/isolators/posix/disk.cpp | 2 +- src/tests/resource_offers_tests.cpp | 24 ++++---- src/tests/resources_tests.cpp | 22 +++---- src/tests/sorter_tests.cpp | 6 +- 9 files changed, 95 insertions(+), 83 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/mesos/blob/820ecf4a/include/mesos/resources.hpp ---------------------------------------------------------------------- diff --git a/include/mesos/resources.hpp b/include/mesos/resources.hpp index b5893d3..3b57568 100644 --- a/include/mesos/resources.hpp +++ b/include/mesos/resources.hpp @@ -27,6 +27,7 @@ #include <mesos/values.hpp> #include <stout/bytes.hpp> +#include <stout/check.hpp> #include <stout/error.hpp> #include <stout/foreach.hpp> #include <stout/option.hpp> @@ -126,12 +127,6 @@ public: // Returns the unreserved resources. Resources unreserved() const; - // Returns all the persistent disk resources. - // TODO(jieyu): Consider introducing a general filter mechanism for - // resources. For example: - // Resources persistentDisks = resources.filter(PersistentDiskFilter()); - Resources persistentDisks() const; - // Returns a Resources object with the same amount of each resource // type as these Resources, but with all Resource objects marked as // the specified role. @@ -225,6 +220,62 @@ public: Resources& operator -= (const Resource& that); Resources& operator -= (const Resources& that); + // The base class for all resources filters. + // TODO(jieyu): Pull resources filters out of Resources class and + // possibly put them inside a resources::filter namespace. + class Filter + { + public: + // Apply this filter to the given resources and return the + // filtered resources. + virtual Resources apply(const Resources& resources) const = 0; + }; + + class RoleFilter : public Filter + { + public: + static RoleFilter any() { return RoleFilter(); } + + RoleFilter() : type(ANY) {} + + explicit RoleFilter(const std::string& _role) + : type(SOME), role(_role) {} + + virtual Resources apply(const Resources& resources) const + { + if (type == ANY) { + return resources; + } + + CHECK_SOME(role); + + return role.get() == "*" ? + resources.unreserved() : + resources.reserved(role.get()); + } + + private: + enum { ANY, SOME } type; + Option<std::string> role; + }; + + class PersistentVolumeFilter : public Filter + { + public: + PersistentVolumeFilter() {} + + virtual Resources apply(const Resources& resources) const + { + Resources result; + foreach (const Resource& resource, resources) { + if (resource.has_disk() && resource.disk().has_persistence()) { + result += resource; + } + } + return result; + } + }; + private: // Similar to 'contains(const Resource&)' but skips the validity // check. This can be used to avoid the performance overhead of http://git-wip-us.apache.org/repos/asf/mesos/blob/820ecf4a/src/common/resources.cpp ---------------------------------------------------------------------- diff --git a/src/common/resources.cpp b/src/common/resources.cpp index 27125a8..68f6421 100644 --- a/src/common/resources.cpp +++ b/src/common/resources.cpp @@ -498,20 +498,6 @@ Resources Resources::unreserved() const } -Resources Resources::persistentDisks() const -{ - Resources result; - - foreach (const Resource& resource, resources) { - if (resource.has_disk() && resource.disk().has_persistence()) { - result += resource; - } - } - - return result; -} - - Resources Resources::flatten(const string& role) const { Resources flattened; @@ -525,31 +511,6 @@ Resources Resources::flatten(const string& role) const } -class RoleFilter -{ -public: - static RoleFilter any() { return RoleFilter(); } - - RoleFilter() : type(ANY) {} - /*implicit*/ RoleFilter(const string& _role) : type(SOME), role(_role) {} - - Resources apply(const Resources& resources) const - { - if (type == ANY) { - return resources; - } else if (role == "*") { - return resources.unreserved(); - } else { - return resources.reserved(role); - } - } - -private: - enum { ANY, SOME } type; - string role; -}; - - Option<Resources> Resources::find(const Resource& target) const { Resources found; http://git-wip-us.apache.org/repos/asf/mesos/blob/820ecf4a/src/master/drf_sorter.cpp ---------------------------------------------------------------------- diff --git a/src/master/drf_sorter.cpp b/src/master/drf_sorter.cpp index 28b16b2..584e26c 100644 --- a/src/master/drf_sorter.cpp +++ b/src/master/drf_sorter.cpp @@ -249,7 +249,7 @@ double DRFSorter::calculateShare(const string& name) // scalars. // Scalar resources may be spread across multiple 'Resource' - // objects. E.g. persistent disks. So we first collect the names + // objects. E.g. persistent volumes. So we first collect the names // of the scalar resources, before computing the totals. hashset<string> scalars; foreach (const Resource& resource, resources) { http://git-wip-us.apache.org/repos/asf/mesos/blob/820ecf4a/src/master/master.cpp ---------------------------------------------------------------------- diff --git a/src/master/master.cpp b/src/master/master.cpp index 2856cc3..ab6d1d1 100644 --- a/src/master/master.cpp +++ b/src/master/master.cpp @@ -2082,7 +2082,7 @@ struct ResourceValidator : TaskInfoValidator return Error("Persistence ID '" + id + "' contains invalid characters"); } } else if (resource.disk().has_volume()) { - return Error("Non-persistent disk volume is not supported"); + return Error("Non-persistent volume is not supported"); } else { return Error("DiskInfo is set but empty"); } http://git-wip-us.apache.org/repos/asf/mesos/blob/820ecf4a/src/messages/messages.proto ---------------------------------------------------------------------- diff --git a/src/messages/messages.proto b/src/messages/messages.proto index 5bd075e..c609f50 100644 --- a/src/messages/messages.proto +++ b/src/messages/messages.proto @@ -252,9 +252,9 @@ message RegisterSlaveMessage { required SlaveInfo slave = 1; // Resources that are persisted by the slave. It includes persistent - // disk resources currently, and may include dynamically reserved - // resources in the future. Persisted resources need to be - // explicitly released by the framework. + // volumes currently, and may include dynamically reserved resources + // in the future. Persisted resources need to be explicitly released + // by the framework. repeated Resource persisted_resources = 3; // NOTE: This is a hack for the master to detect the slave's @@ -272,9 +272,9 @@ message ReregisterSlaveMessage { required SlaveInfo slave = 2; // Resources that are persisted by the slave. It includes persistent - // disk resources currently, and may include dynamically reserved - // resources in the future. Persisted resources need to be - // explicitly released by the framework. + // volumes currently, and may include dynamically reserved resources + // in the future. Persisted resources need to be explicitly released + // by the framework. repeated Resource persisted_resources = 7; repeated ExecutorInfo executor_infos = 4; @@ -335,12 +335,12 @@ message UpdateFrameworkMessage { // This message is sent to the slave whenever there is an acquisition -// or release of a persistent resource (e.g., persistent disk or +// or release of a persistent resource (e.g., persistent volume or // dynamic reservation). message UpdateResourcesMessage { // Resources that need to be persisted on the slave. It includes - // persistent disk resources currently, and may include dynamically - // reserved resources in the future. + // persistent volumes, and may include dynamically reserved + // resources in the future. repeated Resource persisted_resources = 1; } http://git-wip-us.apache.org/repos/asf/mesos/blob/820ecf4a/src/slave/containerizer/isolators/posix/disk.cpp ---------------------------------------------------------------------- diff --git a/src/slave/containerizer/isolators/posix/disk.cpp b/src/slave/containerizer/isolators/posix/disk.cpp index bb1779f..7d37afd 100644 --- a/src/slave/containerizer/isolators/posix/disk.cpp +++ b/src/slave/containerizer/isolators/posix/disk.cpp @@ -172,7 +172,7 @@ Future<Nothing> PosixDiskIsolatorProcess::update( // Regular disk used for executor working directory. path = info->directory; } else { - // TODO(jieyu): Support persistent disk as well. + // TODO(jieyu): Support persistent volmes as well. LOG(ERROR) << "Enforcing disk quota unsupported for " << resource; continue; } http://git-wip-us.apache.org/repos/asf/mesos/blob/820ecf4a/src/tests/resource_offers_tests.cpp ---------------------------------------------------------------------- diff --git a/src/tests/resource_offers_tests.cpp b/src/tests/resource_offers_tests.cpp index 4395dd2..ffad1f8 100644 --- a/src/tests/resource_offers_tests.cpp +++ b/src/tests/resource_offers_tests.cpp @@ -580,7 +580,7 @@ TEST_F(TaskValidationTest, DISABLED_UnreservedDiskInfo) AWAIT_READY(offers); EXPECT_NE(0u, offers.get().size()); - // Create a persistent disk resource with "*" role. + // Create a persistent volume with "*" role. Resource diskResource = Resources::parse("disk", "128", "*").get(); diskResource.mutable_disk()->CopyFrom(createDiskInfo("1", "1")); @@ -642,7 +642,7 @@ TEST_F(TaskValidationTest, DISABLED_InvalidPersistenceID) AWAIT_READY(offers); EXPECT_NE(0u, offers.get().size()); - // Create a persistent disk resource with an invalid persistence id. + // Create a persistent volume with an invalid persistence id. Resource diskResource = Resources::parse("disk", "128", "role1").get(); diskResource.mutable_disk()->CopyFrom(createDiskInfo("1/", "1")); @@ -704,7 +704,7 @@ TEST_F(TaskValidationTest, DISABLED_PersistentDiskInfoWithoutVolume) AWAIT_READY(offers); EXPECT_NE(0u, offers.get().size()); - // Create a persistent disk resource without a volume. + // Create a persistent volume no volume information. Resource diskResource = Resources::parse("disk", "128", "role1").get(); diskResource.mutable_disk()->CopyFrom(createDiskInfo("1", None())); @@ -766,7 +766,7 @@ TEST_F(TaskValidationTest, DISABLED_PersistentDiskInfoWithReadOnlyVolume) AWAIT_READY(offers); EXPECT_NE(0u, offers.get().size()); - // Create a persistent disk resource with read-only volume. + // Create a read-only persistent volume. Resource diskResource = Resources::parse("disk", "128", "role1").get(); diskResource.mutable_disk()->CopyFrom(createDiskInfo("1", "1", Volume::RO)); @@ -828,7 +828,7 @@ TEST_F(TaskValidationTest, DISABLED_PersistentDiskInfoWithHostPath) AWAIT_READY(offers); EXPECT_NE(0u, offers.get().size()); - // Create a persistent disk resource with host path in the volume. + // Create a persistent volume with host path. Resource diskResource = Resources::parse("disk", "128", "role1").get(); diskResource.mutable_disk()->CopyFrom( createDiskInfo("1", "1", Volume::RW, "foo")); @@ -891,7 +891,7 @@ TEST_F(TaskValidationTest, DISABLED_NonPersistentDiskInfoWithVolume) AWAIT_READY(offers); EXPECT_NE(0u, offers.get().size()); - // Create a non-persistent disk resource with volume. + // Create a non-persistent volume. Resource diskResource = Resources::parse("disk", "128", "role1").get(); diskResource.mutable_disk()->CopyFrom(createDiskInfo(None(), "1")); @@ -919,7 +919,7 @@ TEST_F(TaskValidationTest, DISABLED_NonPersistentDiskInfoWithVolume) EXPECT_TRUE(status.get().has_message()); EXPECT_TRUE(strings::contains( status.get().message(), - "Non-persistent disk volume is not supported")); + "Non-persistent volume is not supported")); driver.stop(); driver.join(); @@ -953,7 +953,7 @@ TEST_F(TaskValidationTest, DISABLED_DuplicatedPersistenceIDWithinTask) AWAIT_READY(offers); EXPECT_NE(0u, offers.get().size()); - // Create two persistent disk resources with the same id. + // Create two persistent volumes with the same id. Resource diskResource1 = Resources::parse("disk", "128", "role1").get(); diskResource1.mutable_disk()->CopyFrom(createDiskInfo("1", "1")); @@ -993,7 +993,7 @@ TEST_F(TaskValidationTest, DISABLED_DuplicatedPersistenceIDWithinTask) } -// This test ensures that a persistent disk that is larger than the +// This test ensures that a persistent volume that is larger than the // offered disk resources results in a failed task. TEST_F(TaskValidationTest, DISABLED_AcquirePersistentDiskTooBig) { @@ -1039,8 +1039,8 @@ TEST_F(TaskValidationTest, DISABLED_AcquirePersistentDiskTooBig) AWAIT_READY(offers); EXPECT_NE(0u, offers.get().size()); - // Create a persistent disk resource with volume whose size is - // larger than the size of the offered disk. + // Create a persistent volume whose size is larger than the size of + // the offered disk. Resource diskResource = Resources::parse("disk", "2048", "role1").get(); diskResource.mutable_disk()->CopyFrom(createDiskInfo("1", "1")); @@ -1067,7 +1067,7 @@ TEST_F(TaskValidationTest, DISABLED_AcquirePersistentDiskTooBig) EXPECT_EQ(TaskStatus::REASON_TASK_INVALID, status.get().reason()); EXPECT_TRUE(status.get().has_message()); EXPECT_TRUE(strings::contains( - status.get().message(), "Failed to acquire persistent disks")); + status.get().message(), "Failed to create persistent volumes")); driver.stop(); driver.join(); http://git-wip-us.apache.org/repos/asf/mesos/blob/820ecf4a/src/tests/resources_tests.cpp ---------------------------------------------------------------------- diff --git a/src/tests/resources_tests.cpp b/src/tests/resources_tests.cpp index 351e26d..9fd2135 100644 --- a/src/tests/resources_tests.cpp +++ b/src/tests/resources_tests.cpp @@ -924,17 +924,17 @@ TEST(DiskResourcesTest, Subtraction) } -TEST(DiskResourcesTest, FilterPersistentDisks) +TEST(DiskResourcesTest, FilterPersistentVolumes) { Resources resources = Resources::parse("cpus:1;mem:512;disk:1000").get(); - Resources disk1 = createDiskResource("10", "role1", "1", "path"); - Resources disk2 = createDiskResource("20", "role2", None(), None()); + Resources r1 = createDiskResource("10", "role1", "1", "path"); + Resources r2 = createDiskResource("20", "role2", None(), None()); - resources += disk1; - resources += disk2; + resources += r1; + resources += r2; - EXPECT_EQ(resources.persistentDisks(), disk1); + EXPECT_EQ(r1, Resources::PersistentVolumeFilter().apply(resources)); } @@ -942,22 +942,22 @@ TEST(ResourcesOperationTest, CreatePersistentVolume) { Resources total = Resources::parse("cpus:1;mem:512;disk(role):1000").get(); - Resource disk1 = createDiskResource("200", "role", "1", "path"); + Resource volume1 = createDiskResource("200", "role", "1", "path"); Offer::Operation create1; create1.set_type(Offer::Operation::CREATE); - create1.mutable_create()->add_volumes()->CopyFrom(disk1); + create1.mutable_create()->add_volumes()->CopyFrom(volume1); EXPECT_SOME_EQ( - Resources::parse("cpus:1;mem:512;disk(role):800").get() + disk1, + Resources::parse("cpus:1;mem:512;disk(role):800").get() + volume1, total.apply(create1)); // Check the case of insufficient disk resources. - Resource disk2 = createDiskResource("2000", "role", "1", "path"); + Resource volume2 = createDiskResource("2000", "role", "1", "path"); Offer::Operation create2; create2.set_type(Offer::Operation::CREATE); - create2.mutable_create()->add_volumes()->CopyFrom(disk2); + create2.mutable_create()->add_volumes()->CopyFrom(volume2); EXPECT_ERROR(total.apply(create2)); } http://git-wip-us.apache.org/repos/asf/mesos/blob/820ecf4a/src/tests/sorter_tests.cpp ---------------------------------------------------------------------- diff --git a/src/tests/sorter_tests.cpp b/src/tests/sorter_tests.cpp index f085d80..520a42e 100644 --- a/src/tests/sorter_tests.cpp +++ b/src/tests/sorter_tests.cpp @@ -168,9 +168,9 @@ TEST(SorterTest, WDRFSorter) } -// Some resources are split across multiple resource objects -// (e.g. persistent disks). This test ensures that the shares -// for these are accounted correctly. +// Some resources are split across multiple resource objects (e.g. +// persistent volumes). This test ensures that the shares for these +// are accounted correctly. TEST(SorterTest, SplitResourceShares) { DRFSorter sorter;
