You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@mesos.apache.org by ch...@apache.org on 2019/06/06 19:05:35 UTC
[mesos] 04/09: Added a unit test to verify if SLRP allows changes
in volume context.
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 <ch...@mesosphere.io>
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();