This is an automated email from the ASF dual-hosted git repository. chhsiao pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/mesos.git
commit a7b98f702ddf4e8c9cfd9a7e14e050d802f80084 Author: Chun-Hung Hsiao <[email protected]> AuthorDate: Mon May 6 11:44:40 2019 -0700 Added a unit test to verify if SLRP allows changes in volume context. Review: https://reviews.apache.org/r/70622 --- src/examples/test_csi_plugin.cpp | 43 ++++-- .../storage_local_resource_provider_tests.cpp | 155 +++++++++++++++++++-- 2 files changed, 172 insertions(+), 26 deletions(-) diff --git a/src/examples/test_csi_plugin.cpp b/src/examples/test_csi_plugin.cpp index 7ff08b8..6202173 100644 --- a/src/examples/test_csi_plugin.cpp +++ b/src/examples/test_csi_plugin.cpp @@ -146,6 +146,12 @@ public: "specified as a semicolon-delimited list of param=value pairs.\n" "(Example: 'param1=value1;param2=value2')"); + add(&Flags::volume_metadata, + "volume_metadata", + "The static properties to add to the contextual information of each\n" + "volume. The metadata are specified as a semicolon-delimited list of\n" + "prop=value pairs. (Example: 'prop1=value1;prop2=value2')"); + add(&Flags::volumes, "volumes", "Creates preprovisioned volumes upon start-up. The volumes are\n" @@ -164,6 +170,7 @@ public: string work_dir; Bytes available_capacity; Option<string> create_parameters; + Option<string> volume_metadata; Option<string> volumes; Option<string> forward; }; @@ -184,12 +191,14 @@ public: const string& _workDir, const Bytes& _availableCapacity, const hashmap<string, string>& _createParameters, + const hashmap<string, string>& _volumeMetadata, const hashmap<string, Bytes>& _volumes) : apiVersion(_apiVersion), endpoint(_endpoint), workDir(_workDir), availableCapacity(_availableCapacity), - createParameters(_createParameters.begin(), _createParameters.end()) + createParameters(_createParameters.begin(), _createParameters.end()), + volumeMetadata(_volumeMetadata.begin(), _volumeMetadata.end()) { // Construct the default mount volume capability. defaultVolumeCapability.mutable_mount(); @@ -221,7 +230,7 @@ public: } VolumeInfo volumeInfo{ - capacity, getVolumePath(capacity, name), Map<string, string>()}; + capacity, getVolumePath(capacity, name), volumeMetadata}; Try<Nothing> mkdir = os::mkdir(volumeInfo.id); CHECK_SOME(mkdir) @@ -483,6 +492,7 @@ private: Bytes availableCapacity; VolumeCapability defaultVolumeCapability; Map<string, string> createParameters; + Map<string, string> volumeMetadata; hashmap<string, VolumeInfo> volumes; }; @@ -1289,7 +1299,7 @@ Try<VolumeInfo> TestCSIPlugin::parseVolumePath(const string& dir) << "Cannot reconstruct volume path '" << dir << "' from volume name '" << name.get() << "' and capacity " << capacity.get(); - return VolumeInfo{capacity.get(), dir, Map<string, string>()}; + return VolumeInfo{capacity.get(), dir, volumeMetadata}; } @@ -1352,7 +1362,7 @@ Try<VolumeInfo, StatusError> TestCSIPlugin::createVolume( VolumeInfo volumeInfo{min(max(defaultSize, requiredBytes), limitBytes), getVolumePath(volumeInfo.capacity, name), - Map<string, string>()}; + volumeMetadata}; Try<Nothing> mkdir = os::mkdir(volumeInfo.id); if (mkdir.isError()) { @@ -1995,19 +2005,27 @@ int main(int argc, char** argv) foreachpair (const string& param, const vector<string>& values, strings::pairs(flags.create_parameters.get(), ";", "=")) { - Option<Error> error; - if (values.size() != 1) { - error = "Parameter keys must be unique"; - } else { - createParameters.put(param, values[0]); + cerr << "Parameter key '" << param << "' is not unique" << endl; + return EXIT_FAILURE; } - if (error.isSome()) { - cerr << "Failed to parse the '--create_parameters' flags: " - << error->message << endl; + createParameters.put(param, values[0]); + } + } + + hashmap<string, string> volumeMetadata; + + if (flags.volume_metadata.isSome()) { + foreachpair (const string& prop, + const vector<string>& values, + strings::pairs(flags.volume_metadata.get(), ";", "=")) { + if (values.size() != 1) { + cerr << "Metadata key '" << prop << "' is not unique" << endl; return EXIT_FAILURE; } + + volumeMetadata.put(prop, values[0]); } } @@ -2060,6 +2078,7 @@ int main(int argc, char** argv) flags.work_dir, flags.available_capacity, createParameters, + volumeMetadata, volumes); plugin.run(); diff --git a/src/tests/storage_local_resource_provider_tests.cpp b/src/tests/storage_local_resource_provider_tests.cpp index 7dd08be..3823305 100644 --- a/src/tests/storage_local_resource_provider_tests.cpp +++ b/src/tests/storage_local_resource_provider_tests.cpp @@ -22,6 +22,8 @@ #include <tuple> #include <vector> +#include <google/protobuf/repeated_field.h> + #include <mesos/csi/v0.hpp> #include <mesos/csi/v1.hpp> @@ -79,6 +81,8 @@ using std::shared_ptr; using std::string; using std::vector; +using google::protobuf::RepeatedPtrField; + using mesos::internal::slave::ContainerDaemonProcess; using mesos::master::detector::MasterDetector; @@ -234,8 +238,9 @@ public: void setupResourceProviderConfig( const Bytes& capacity, const Option<string> volumes = None(), + const Option<string> forward = None(), const Option<string> createParameters = None(), - const Option<string> forward = None()) + const Option<string> volumeMetadata = None()) { const string testCsiPluginPath = path::join(tests::flags.build_dir, "src", "test-csi-plugin"); @@ -269,9 +274,10 @@ public: "--api_version=%s", "--work_dir=%s", "--available_capacity=%s", + "--volumes=%s", "%s", - "%s", - "%s" + "--create_parameters=%s", + "--volume_metadata=%s" ] }, "resources": [ @@ -305,10 +311,10 @@ public: GetParam(), testCsiPluginWorkDir.get(), stringify(capacity), - createParameters.isSome() - ? "--create_parameters=" + createParameters.get() : "", - volumes.isSome() ? "--volumes=" + volumes.get() : "", - forward.isSome() ? "--forward=" + forward.get() : ""); + volumes.getOrElse(""), + forward.isSome() ? "--forward=" + forward.get() : "", + createParameters.getOrElse(""), + volumeMetadata.getOrElse("")); ASSERT_SOME(resourceProviderConfig); @@ -1236,6 +1242,127 @@ TEST_P(StorageLocalResourceProviderTest, CreateDestroyDiskWithRecovery) } +// This test verifies that the storage local resource provider can properly +// handle changes in volume metadata. This is a regression test for MESOS-9395. +TEST_P(StorageLocalResourceProviderTest, RecoverDiskWithChangedMetadata) +{ + const string profilesPath = path::join(sandbox.get(), "profiles.json"); + + ASSERT_SOME( + os::write(profilesPath, createDiskProfileMapping({{"test", None()}}))); + + loadUriDiskProfileAdaptorModule(profilesPath); + + // Add metadata "label=foo" to each created volume. + setupResourceProviderConfig( + Gigabytes(4), None(), None(), None(), "label=foo"); + + Try<Owned<cluster::Master>> master = StartMaster(); + ASSERT_SOME(master); + + Owned<MasterDetector> detector = master.get()->createDetector(); + + slave::Flags slaveFlags = CreateSlaveFlags(); + slaveFlags.disk_profile_adaptor = URI_DISK_PROFILE_ADAPTOR_NAME; + + Try<Owned<cluster::Slave>> slave = StartSlave(detector.get(), slaveFlags); + ASSERT_SOME(slave); + + // Register a framework to exercise operations. + FrameworkInfo framework = DEFAULT_FRAMEWORK_INFO; + framework.set_roles(0, "storage/role"); + + MockScheduler sched; + MesosSchedulerDriver driver( + &sched, framework, master.get()->pid, DEFAULT_CREDENTIAL); + + EXPECT_CALL(sched, registered(&driver, _, _)); + + // We use the following filter to filter offers that do not have wanted + // resources for 365 days (the maximum). + Filters declineFilters; + declineFilters.set_refuse_seconds(Days(365).secs()); + + // Decline unwanted offers. The master can send such offers before the + // resource provider receives profile updates. + EXPECT_CALL(sched, resourceOffers(&driver, _)) + .WillRepeatedly(DeclineOffers(declineFilters)); + + Future<vector<Offer>> offers; + EXPECT_CALL(sched, resourceOffers(&driver, OffersHaveAnyResource( + &Resources::hasResourceProvider))) + .WillOnce(FutureArg<1>(&offers)); + + driver.start(); + + AWAIT_READY(offers); + ASSERT_EQ(1u, offers->size()); + + Offer offer = offers->at(0); + + // Create a MOUNT disk. + RepeatedPtrField<Resource> raw = + Resources(offer.resources()).filter(&Resources::hasResourceProvider); + + ASSERT_EQ(1, raw.size()); + ASSERT_TRUE(isStoragePool(raw[0], "test")); + + EXPECT_CALL(sched, resourceOffers(&driver, OffersHaveAnyResource( + &Resources::hasResourceProvider))) + .WillOnce(FutureArg<1>(&offers)); + + driver.acceptOffers( + {offer.id()}, + {CREATE_DISK(raw[0], Resource::DiskInfo::Source::MOUNT)}); + + AWAIT_READY(offers); + ASSERT_EQ(1u, offers->size()); + + offer = offers->at(0); + + RepeatedPtrField<Resource> created = + Resources(offer.resources()).filter(&Resources::hasResourceProvider); + + ASSERT_EQ(1, created.size()); + ASSERT_TRUE(isMountDisk(created[0], "test")); + EXPECT_TRUE(created[0].disk().source().has_metadata()); + + // Restart the agent. + EXPECT_CALL(sched, offerRescinded(_, _)); + + slave.get()->terminate(); + + // Add metadata "label=bar" to each created volume. This dose not conform to + // the CSI specification in terms of backward compatibility, but Mesos should + // be robust to handle changes in the metadata. + setupResourceProviderConfig( + Gigabytes(4), None(), None(), None(), "label=bar"); + + EXPECT_CALL(sched, resourceOffers(&driver, OffersHaveAnyResource( + Resources::hasResourceProvider))) + .WillOnce(FutureArg<1>(&offers)); + + slave = StartSlave(detector.get(), slaveFlags); + ASSERT_SOME(slave); + + AWAIT_READY(offers); + ASSERT_EQ(1u, offers->size()); + + offer = offers->at(0); + + RepeatedPtrField<Resource> recovered = + Resources(offer.resources()).filter(&Resources::hasResourceProvider); + + ASSERT_EQ(1, recovered.size()) << "To many resources: " << recovered; + ASSERT_TRUE(isMountDisk(recovered[0], "test")); + ASSERT_TRUE(recovered[0].disk().source().has_metadata()); + EXPECT_NE(created[0].disk().source().metadata(), + recovered[0].disk().source().metadata()) + << "'" << JSON::protobuf(created[0].disk().source().metadata()) << "' vs " + "'" << JSON::protobuf(recovered[0].disk().source().metadata()) << "'"; +} + + // This test verifies that a framework cannot create a volume during and after // the profile disappears, and destroying a volume with a stale profile will // recover the freed disk with another appeared profile. @@ -1644,7 +1771,7 @@ TEST_P(StorageLocalResourceProviderTest, ROOT_AgentRegisteredWithNewId) loadUriDiskProfileAdaptorModule(profilesPath); - setupResourceProviderConfig(Gigabytes(5), None(), "label=foo"); + setupResourceProviderConfig(Gigabytes(5), None(), None(), "label=foo"); Try<Owned<cluster::Master>> master = StartMaster(); ASSERT_SOME(master); @@ -1809,7 +1936,7 @@ TEST_P(StorageLocalResourceProviderTest, ROOT_AgentRegisteredWithNewId) // allocation for the persistent volume. // // TODO(chhsiao): Remove this workaround once MESOS-9553 is done. - setupResourceProviderConfig(Gigabytes(6), None(), "label=foo"); + setupResourceProviderConfig(Gigabytes(6), None(), None(), "label=foo"); // NOTE: The order of these expectations is reversed because Google Mock will // search the expectations in reverse order. @@ -2992,7 +3119,7 @@ TEST_P(StorageLocalResourceProviderTest, CreatePersistentBlockVolume) MockCSIPlugin plugin; ASSERT_SOME(plugin.startup(mockCsiEndpoint)); - setupResourceProviderConfig(Bytes(0), None(), None(), mockCsiEndpoint); + setupResourceProviderConfig(Bytes(0), None(), mockCsiEndpoint); Try<Owned<cluster::Master>> master = StartMaster(); ASSERT_SOME(master); @@ -3115,7 +3242,7 @@ TEST_P(StorageLocalResourceProviderTest, DestroyUnpublishedPersistentVolume) MockCSIPlugin plugin; ASSERT_SOME(plugin.startup(mockCsiEndpoint)); - setupResourceProviderConfig(Bytes(0), None(), None(), mockCsiEndpoint); + setupResourceProviderConfig(Bytes(0), None(), mockCsiEndpoint); Try<Owned<cluster::Master>> master = StartMaster(); ASSERT_SOME(master); @@ -3279,7 +3406,7 @@ TEST_P( MockCSIPlugin plugin; ASSERT_SOME(plugin.startup(mockCsiEndpoint)); - setupResourceProviderConfig(Bytes(0), None(), None(), mockCsiEndpoint); + setupResourceProviderConfig(Bytes(0), None(), mockCsiEndpoint); Try<Owned<cluster::Master>> master = StartMaster(); ASSERT_SOME(master); @@ -3495,7 +3622,7 @@ TEST_P( MockCSIPlugin plugin; ASSERT_SOME(plugin.startup(mockCsiEndpoint)); - setupResourceProviderConfig(Bytes(0), None(), None(), mockCsiEndpoint); + setupResourceProviderConfig(Bytes(0), None(), mockCsiEndpoint); Try<Owned<cluster::Master>> master = StartMaster(); ASSERT_SOME(master); @@ -5623,7 +5750,7 @@ TEST_P(StorageLocalResourceProviderTest, RetryRpcWithExponentialBackoff) MockCSIPlugin plugin; ASSERT_SOME(plugin.startup(mockCsiEndpoint)); - setupResourceProviderConfig(Bytes(0), None(), None(), mockCsiEndpoint); + setupResourceProviderConfig(Bytes(0), None(), mockCsiEndpoint); master::Flags masterFlags = CreateMasterFlags();
