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 2018/11/29 18:31:59 UTC
[mesos] branch 1.7.x updated (155a72a -> f54285c)
This is an automated email from the ASF dual-hosted git repository.
chhsiao pushed a change to branch 1.7.x
in repository https://gitbox.apache.org/repos/asf/mesos.git.
from 155a72a Added MESOS-9419 to the 1.7.1 CHANGELOG.
new 4140b73 Changed the semantics of `CREATE_DISK` and `DESTROY_DISK` operations.
new 55ff737 Added profiles to storage pools in tests for `CREATE_DISK`.
new 37d6a22 Rewrote test `ReconcileDroppedOperation` for `CREATE_DISK`.
new b704e49 Rewrote test `ConvertPreExistingVolume` for `CREATE_DISK`.
new 1c7be01 Implemented the new `CREATE_DISK`/`DESTROY_DISK` semantics in SLRP.
new 27b400e Added validation for `Offer.Operation.CreateDisk.target_profile`.
new 0ae89dc Cleaned up `include/mesos/type_utils.hpp`.
new 9d32259 Checkpointed creation parameters for CSI volumes.
new f54285c Added MESOS-9275 to the 1.7.1 CHANGELOG.
The 9 revisions listed above as "new" are entirely new to this
repository and will be described in separate emails. The revisions
listed as "add" were already present in the repository and have only
been added to this reference.
Summary of changes:
CHANGELOG | 1 +
include/mesos/mesos.proto | 46 ++-
include/mesos/type_utils.hpp | 46 ++-
include/mesos/v1/mesos.hpp | 27 +-
include/mesos/v1/mesos.proto | 46 ++-
src/csi/state.proto | 6 +
src/csi/utils.hpp | 8 +
src/master/validation.cpp | 6 +
src/resource_provider/storage/provider.cpp | 272 ++++++++---------
.../storage/uri_disk_profile_adaptor.cpp | 26 +-
src/tests/api_tests.cpp | 12 +-
src/tests/master_tests.cpp | 6 +-
src/tests/master_validation_tests.cpp | 44 ++-
src/tests/mesos.hpp | 22 +-
src/tests/operation_reconciliation_tests.cpp | 11 +-
src/tests/resource_provider_manager_tests.cpp | 2 +-
.../storage_local_resource_provider_tests.cpp | 322 ++++++++++-----------
17 files changed, 488 insertions(+), 415 deletions(-)
[mesos] 07/09: Cleaned up `include/mesos/type_utils.hpp`.
Posted by ch...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
chhsiao pushed a commit to branch 1.7.x
in repository https://gitbox.apache.org/repos/asf/mesos.git
commit 0ae89dc43872166e9f83e5ab1cd3c63efab4bb4a
Author: Chun-Hung Hsiao <ch...@mesosphere.io>
AuthorDate: Wed Nov 14 20:13:53 2018 -0800
Cleaned up `include/mesos/type_utils.hpp`.
This patch does the following cleanups:
1. Moved `google::protobuf::Map` equality operator to `type_utils.hpp`.
2. Moved the type helper templates for the protobuf library that do not
involve mesos protobufs into the `google::protobuf` namespaces so ADL
works appropriately.
3. Removed the type helper templates for the protobuf library from
`mesos/v1/mesos.hpp` to avoid redefinition.
Review: https://reviews.apache.org/r/69363
---
include/mesos/type_utils.hpp | 46 ++++++++++++++++++++--
include/mesos/v1/mesos.hpp | 27 +------------
.../storage/uri_disk_profile_adaptor.cpp | 26 +-----------
3 files changed, 47 insertions(+), 52 deletions(-)
diff --git a/include/mesos/type_utils.hpp b/include/mesos/type_utils.hpp
index 19ea817..f276697 100644
--- a/include/mesos/type_utils.hpp
+++ b/include/mesos/type_utils.hpp
@@ -23,6 +23,7 @@
#include <boost/functional/hash.hpp>
+#include <google/protobuf/map.h>
#include <google/protobuf/repeated_field.h>
#include <mesos/mesos.hpp>
@@ -475,7 +476,7 @@ std::ostream& operator<<(
template <typename T>
inline std::ostream& operator<<(
std::ostream& stream,
- const google::protobuf::RepeatedPtrField<T>& messages)
+ const std::vector<T>& messages)
{
stream << "[ ";
for (auto it = messages.begin(); it != messages.end(); ++it) {
@@ -488,11 +489,48 @@ inline std::ostream& operator<<(
return stream;
}
+} // namespace mesos {
+
+
+/**
+ * Type utilities for the protobuf library that are not specific to particular
+ * protobuf classes. They are defined in the `google::protobuf` namespace for
+ * argument-dependent lookup.
+ */
+namespace google {
+namespace protobuf {
+
+template <typename Key, typename Value>
+inline bool operator==(
+ const Map<Key, Value>& left, const Map<Key, Value>& right)
+{
+ if (left.size() != right.size()) {
+ return false;
+ }
+
+ for (auto it = left.begin(); it != left.end(); ++it) {
+ auto found = right.find(it->first);
+ if (found == right.end() || found->second != it->second) {
+ return false;
+ }
+ }
+
+ return true;
+}
+
+
+template <typename Key, typename Value>
+inline bool operator!=(
+ const Map<Key, Value>& left, const Map<Key, Value>& right)
+{
+ return !(left == right);
+}
+
template <typename T>
inline std::ostream& operator<<(
std::ostream& stream,
- const std::vector<T>& messages)
+ const RepeatedPtrField<T>& messages)
{
stream << "[ ";
for (auto it = messages.begin(); it != messages.end(); ++it) {
@@ -505,7 +543,9 @@ inline std::ostream& operator<<(
return stream;
}
-} // namespace mesos {
+} // namespace protobuf {
+} // namespace google {
+
namespace std {
diff --git a/include/mesos/v1/mesos.hpp b/include/mesos/v1/mesos.hpp
index fda3eb4..a6a9320 100644
--- a/include/mesos/v1/mesos.hpp
+++ b/include/mesos/v1/mesos.hpp
@@ -23,8 +23,6 @@
#include <boost/functional/hash.hpp>
-#include <google/protobuf/repeated_field.h>
-
#include <mesos/v1/mesos.pb.h> // ONLY USEFUL AFTER RUNNING PROTOC.
#include <stout/strings.hpp>
@@ -32,7 +30,7 @@
// This file includes definitions for operators on public protobuf
// classes (defined in mesos.proto, module.proto, etc.) that don't
// have these operators generated by the protobuf compiler. The
-// corresponding definitions are in src/v1/type_utils.cpp.
+// corresponding definitions are in src/v1/mesos.cpp.
//
// Mesos modules need some of the protobuf classes defined in
// mesos.proto, module.proto, etc., and require some of these
@@ -469,23 +467,6 @@ std::ostream& operator<<(
template <typename T>
inline std::ostream& operator<<(
std::ostream& stream,
- const google::protobuf::RepeatedPtrField<T>& messages)
-{
- stream << "[ ";
- for (auto it = messages.begin(); it != messages.end(); ++it) {
- if (it != messages.begin()) {
- stream << ", ";
- }
- stream << *it;
- }
- stream << " ]";
- return stream;
-}
-
-
-template <typename T>
-inline std::ostream& operator<<(
- std::ostream& stream,
const std::vector<T>& messages)
{
stream << "[ ";
@@ -499,14 +480,10 @@ inline std::ostream& operator<<(
return stream;
}
-
-std::ostream& operator<<(
- std::ostream& stream,
- const hashmap<std::string, std::string>& map);
-
} // namespace v1 {
} // namespace mesos {
+
namespace std {
template <>
diff --git a/src/resource_provider/storage/uri_disk_profile_adaptor.cpp b/src/resource_provider/storage/uri_disk_profile_adaptor.cpp
index 6c998ef..cb574be 100644
--- a/src/resource_provider/storage/uri_disk_profile_adaptor.cpp
+++ b/src/resource_provider/storage/uri_disk_profile_adaptor.cpp
@@ -22,6 +22,8 @@
#include <csi/spec.hpp>
+#include <mesos/type_utils.hpp>
+
#include <mesos/module/disk_profile_adaptor.hpp>
#include <process/defer.hpp>
@@ -58,30 +60,6 @@ namespace mesos {
namespace internal {
namespace storage {
-bool operator==(
- const Map<string, string>& left,
- const Map<string, string>& right) {
- if (left.size() != right.size()) {
- return false;
- }
-
- typename Map<string, string>::const_iterator iterator = left.begin();
- while (iterator != left.end()) {
- if (right.count(iterator->first) != 1) {
- return false;
- }
-
- if (iterator->second != right.at(iterator->first)) {
- return false;
- }
-
- ++iterator;
- }
-
- return true;
-}
-
-
UriDiskProfileAdaptor::UriDiskProfileAdaptor(const Flags& _flags)
: flags(_flags),
process(new UriDiskProfileAdaptorProcess(flags))
[mesos] 03/09: Rewrote test `ReconcileDroppedOperation` for
`CREATE_DISK`.
Posted by ch...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
chhsiao pushed a commit to branch 1.7.x
in repository https://gitbox.apache.org/repos/asf/mesos.git
commit 37d6a222ab25c37cc0b5def45825583f65b11c00
Author: Chun-Hung Hsiao <ch...@mesosphere.io>
AuthorDate: Wed Nov 14 21:03:19 2018 -0800
Rewrote test `ReconcileDroppedOperation` for `CREATE_DISK`.
Previously the `ReconcileDroppedOperation` test relies on converting
preprovisioned volumes. To adapt the new semantics for `CREATE_DISK`,
this test is rewritten to create two disks from a storage pool, with one
operation dropped.
Review: https://reviews.apache.org/r/69359
---
.../storage_local_resource_provider_tests.cpp | 74 +++++++++++-----------
1 file changed, 37 insertions(+), 37 deletions(-)
diff --git a/src/tests/storage_local_resource_provider_tests.cpp b/src/tests/storage_local_resource_provider_tests.cpp
index e5751a2..6058396 100644
--- a/src/tests/storage_local_resource_provider_tests.cpp
+++ b/src/tests/storage_local_resource_provider_tests.cpp
@@ -3720,15 +3720,20 @@ TEST_F(StorageLocalResourceProviderTest, ROOT_CsiPluginRpcMetrics)
// Master reconciles operations that are missing from a reregistering slave.
-// In this case, the `ApplyOperationMessage` is dropped, so the resource
-// provider should send OPERATION_DROPPED. Operations on agent default
-// resources are also tested here; for such operations, the agent generates the
-// dropped status.
+// In this case, one of the two `ApplyOperationMessage`s is dropped, so the
+// resource provider should send only one OPERATION_DROPPED.
+//
+// TODO(greggomann): Test operations on agent default resources: for such
+// operations, the agent generates the dropped status.
TEST_F(StorageLocalResourceProviderTest, ROOT_ReconcileDroppedOperation)
{
Clock::pause();
- setupResourceProviderConfig(Bytes(0), "volume1:2GB;volume2:2GB");
+ const string profilesPath = path::join(sandbox.get(), "profiles.json");
+ ASSERT_SOME(os::write(profilesPath, createDiskProfileMapping("test")));
+ loadUriDiskProfileAdaptorModule(profilesPath);
+
+ setupResourceProviderConfig(Gigabytes(4));
master::Flags masterFlags = CreateMasterFlags();
Try<Owned<cluster::Master>> master = StartMaster(masterFlags);
@@ -3740,6 +3745,7 @@ TEST_F(StorageLocalResourceProviderTest, ROOT_ReconcileDroppedOperation)
slaveFlags.isolation = "filesystem/linux";
slaveFlags.resource_provider_config_dir = resourceProviderConfigDir;
+ slaveFlags.disk_profile_adaptor = URI_DISK_PROFILE_ADAPTOR_NAME;
Future<SlaveRegisteredMessage> slaveRegisteredMessage =
FUTURE_PROTOBUF(SlaveRegisteredMessage(), _, _);
@@ -3795,40 +3801,33 @@ TEST_F(StorageLocalResourceProviderTest, ROOT_ReconcileDroppedOperation)
EXPECT_CALL(sched, resourceOffers(&driver, _))
.WillRepeatedly(DeclineOffers(declineFilters));
- // We are only interested in pre-existing volumes, which have IDs but no
- // profile. We use pre-existing volumes to make it easy to send multiple
- // operations on multiple resources.
- auto isPreExistingVolume = [](const Resource& r) {
+ auto isRaw = [](const Resource& r) {
return r.has_disk() &&
r.disk().has_source() &&
- r.disk().source().has_id() &&
- !r.disk().source().has_profile();
+ r.disk().source().has_profile() &&
+ r.disk().source().type() == Resource::DiskInfo::Source::RAW;
};
Future<vector<Offer>> offersBeforeOperations;
- EXPECT_CALL(sched, resourceOffers(&driver, OffersHaveAnyResource(
- isPreExistingVolume)))
+ EXPECT_CALL(sched, resourceOffers(&driver, OffersHaveAnyResource(isRaw)))
.WillOnce(FutureArg<1>(&offersBeforeOperations))
.WillRepeatedly(DeclineOffers(declineFilters)); // Decline further offers.
driver.start();
AWAIT_READY(offersBeforeOperations);
- ASSERT_FALSE(offersBeforeOperations->empty());
-
- vector<Resource> sources;
+ ASSERT_EQ(1u, offersBeforeOperations->size());
- foreach (
- const Resource& resource,
- offersBeforeOperations->at(0).resources()) {
- if (isPreExistingVolume(resource) &&
- resource.disk().source().type() == Resource::DiskInfo::Source::RAW) {
- sources.push_back(resource);
- }
- }
+ Resources raw =
+ Resources(offersBeforeOperations->at(0).resources()).filter(isRaw);
- ASSERT_EQ(2u, sources.size());
+ // Create two MOUNT disks of 2GB each.
+ ASSERT_SOME_EQ(Gigabytes(4), raw.disk());
+ Resource source1 = *raw.begin();
+ source1.mutable_scalar()->set_value(
+ static_cast<double>(Gigabytes(2).bytes()) / Bytes::MEGABYTES);
+ Resource source2 = *(raw - source1).begin();
// Drop one of the operations on the way to the agent.
Future<ApplyOperationMessage> applyOperationMessage =
@@ -3846,8 +3845,8 @@ TEST_F(StorageLocalResourceProviderTest, ROOT_ReconcileDroppedOperation)
// Attempt the creation of two volumes.
driver.acceptOffers(
{offersBeforeOperations->at(0).id()},
- {CREATE_DISK(sources.at(0), Resource::DiskInfo::Source::MOUNT),
- CREATE_DISK(sources.at(1), Resource::DiskInfo::Source::MOUNT)},
+ {CREATE_DISK(source1, Resource::DiskInfo::Source::MOUNT),
+ CREATE_DISK(source2, Resource::DiskInfo::Source::MOUNT)},
acceptFilters);
// Ensure that the operations are processed.
@@ -3894,8 +3893,15 @@ TEST_F(StorageLocalResourceProviderTest, ROOT_ReconcileDroppedOperation)
Future<vector<Offer>> offersAfterOperations;
- EXPECT_CALL(sched, resourceOffers(&driver, OffersHaveAnyResource(
- isPreExistingVolume)))
+ auto isMountDisk = [](const Resource& r) {
+ return r.has_disk() &&
+ r.disk().has_source() &&
+ r.disk().source().has_profile() &&
+ r.disk().source().type() == Resource::DiskInfo::Source::MOUNT;
+ };
+
+ EXPECT_CALL(
+ sched, resourceOffers(&driver, OffersHaveAnyResource(isMountDisk)))
.WillOnce(FutureArg<1>(&offersAfterOperations));
// Advance the clock to trigger a batch allocation.
@@ -3904,14 +3910,8 @@ TEST_F(StorageLocalResourceProviderTest, ROOT_ReconcileDroppedOperation)
AWAIT_READY(offersAfterOperations);
ASSERT_FALSE(offersAfterOperations->empty());
- vector<Resource> converted;
-
- foreach (const Resource& resource, offersAfterOperations->at(0).resources()) {
- if (isPreExistingVolume(resource) &&
- resource.disk().source().type() == Resource::DiskInfo::Source::MOUNT) {
- converted.push_back(resource);
- }
- }
+ Resources converted =
+ Resources(offersAfterOperations->at(0).resources()).filter(isMountDisk);
ASSERT_EQ(1u, converted.size());
[mesos] 05/09: Implemented the new `CREATE_DISK`/`DESTROY_DISK`
semantics in SLRP.
Posted by ch...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
chhsiao pushed a commit to branch 1.7.x
in repository https://gitbox.apache.org/repos/asf/mesos.git
commit 1c7be01b2b3a5f577b12713ea319c79769e55803
Author: Chun-Hung Hsiao <ch...@mesosphere.io>
AuthorDate: Thu Nov 15 12:25:18 2018 -0800
Implemented the new `CREATE_DISK`/`DESTROY_DISK` semantics in SLRP.
The default mount/block volume capabilities is removed from SLRP.
Instead, `CREATE_DISK` will convert a preprovisioned RAW disk to a
profile disk, and `DESTROY_DISK` will always deprovision a profile disk
as long as the CSI plugin is capable of deprovisioning volumes.
Review: https://reviews.apache.org/r/69361
---
src/csi/utils.hpp | 8 +
src/resource_provider/storage/provider.cpp | 235 ++++++++++++-----------------
2 files changed, 103 insertions(+), 140 deletions(-)
diff --git a/src/csi/utils.hpp b/src/csi/utils.hpp
index 5ce318e..9145c67 100644
--- a/src/csi/utils.hpp
+++ b/src/csi/utils.hpp
@@ -45,6 +45,14 @@ bool operator==(
bool operator==(const VolumeCapability& left, const VolumeCapability& right);
+inline bool operator!=(
+ const VolumeCapability& left,
+ const VolumeCapability& right)
+{
+ return !(left == right);
+}
+
+
std::ostream& operator<<(
std::ostream& stream,
const ControllerServiceCapability::RPC::Type& type);
diff --git a/src/resource_provider/storage/provider.cpp b/src/resource_provider/storage/provider.cpp
index 5f755bf..97aa340 100644
--- a/src/resource_provider/storage/provider.cpp
+++ b/src/resource_provider/storage/provider.cpp
@@ -402,7 +402,7 @@ private:
const string& name,
const Bytes& capacity,
const DiskProfileAdaptor::ProfileInfo& profileInfo);
- Future<Nothing> deleteVolume(const string& volumeId, bool preExisting);
+ Future<bool> deleteVolume(const string& volumeId);
Future<string> validateCapability(
const string& volumeId,
const Option<Labels>& metadata,
@@ -420,7 +420,8 @@ private:
Future<vector<ResourceConversion>> applyCreateDisk(
const Resource& resource,
const id::UUID& operationUuid,
- const Resource::DiskInfo::Source::Type& type);
+ const Resource::DiskInfo::Source::Type& targetType,
+ const Option<string>& targetProfile);
Future<vector<ResourceConversion>> applyDestroyDisk(
const Resource& resource);
@@ -460,8 +461,6 @@ private:
shared_ptr<DiskProfileAdaptor> diskProfileAdaptor;
- csi::v0::VolumeCapability defaultMountCapability;
- csi::v0::VolumeCapability defaultBlockCapability;
string bootId;
process::grpc::client::Runtime runtime;
Owned<v1::resource_provider::Driver> driver;
@@ -586,14 +585,6 @@ void StorageLocalResourceProviderProcess::received(const Event& event)
void StorageLocalResourceProviderProcess::initialize()
{
- // Default mount and block capabilities for pre-existing volumes.
- defaultMountCapability.mutable_mount();
- defaultMountCapability.mutable_access_mode()
- ->set_mode(csi::v0::VolumeCapability::AccessMode::SINGLE_NODE_WRITER);
- defaultBlockCapability.mutable_block();
- defaultBlockCapability.mutable_access_mode()
- ->set_mode(csi::v0::VolumeCapability::AccessMode::SINGLE_NODE_WRITER);
-
Try<string> _bootId = os::bootId();
if (_bootId.isError()) {
LOG(ERROR) << "Failed to get boot ID: " << _bootId.error();
@@ -2646,6 +2637,7 @@ Future<Nothing> StorageLocalResourceProviderProcess::nodeUnpublish(
// Returns a CSI volume ID.
+//
// NOTE: This can only be called after `prepareControllerService`.
Future<string> StorageLocalResourceProviderProcess::createVolume(
const string& name,
@@ -2676,9 +2668,8 @@ Future<string> StorageLocalResourceProviderProcess::createVolume(
const csi::v0::Volume& volume = response.volume();
if (volumes.contains(volume.id())) {
- // The resource provider failed over after the last
- // `CreateVolume` call, but before the operation status was
- // checkpointed.
+ // The resource provider failed over after the last `createVolume`
+ // call, but before the operation status was checkpointed.
CHECK_EQ(VolumeState::CREATED,
volumes.at(volume.id()).state.state());
} else {
@@ -2698,19 +2689,13 @@ Future<string> StorageLocalResourceProviderProcess::createVolume(
}
+// Returns true if the volume has been deprovisioned.
+//
// NOTE: This can only be called after `prepareControllerService` and
// `prepareNodeService` (since it may require `NodeUnpublishVolume`).
-Future<Nothing> StorageLocalResourceProviderProcess::deleteVolume(
- const string& volumeId,
- bool preExisting)
+Future<bool> StorageLocalResourceProviderProcess::deleteVolume(
+ const string& volumeId)
{
- // We do not need the capability for pre-existing volumes since no
- // actual `DeleteVolume` call will be made.
- if (!preExisting && !controllerCapabilities.createDeleteVolume) {
- return Failure(
- "Controller capability 'CREATE_DELETE_VOLUME' is not supported");
- }
-
CHECK_SOME(controllerContainerId);
const string volumePath = csi::paths::getVolumePath(
@@ -2720,11 +2705,11 @@ Future<Nothing> StorageLocalResourceProviderProcess::deleteVolume(
volumeId);
if (!volumes.contains(volumeId)) {
- // The resource provider failed over after the last `DeleteVolume`
- // call, but before the operation status was checkpointed.
+ // The resource provider failed over after the last `deleteVolume` call, but
+ // before the operation status was checkpointed.
CHECK(!os::exists(volumePath));
- return Nothing();
+ return controllerCapabilities.createDeleteVolume;
}
const VolumeData& volume = volumes.at(volumeId);
@@ -2762,7 +2747,9 @@ Future<Nothing> StorageLocalResourceProviderProcess::deleteVolume(
// state once the above is done.
}
case VolumeState::CREATED: {
- if (!preExisting) {
+ // We only delete the volume if the `CREATE_DELETE_VOLUME` capability is
+ // supported. Otherwise, we simply leave it as a preprovisioned volume.
+ if (controllerCapabilities.createDeleteVolume) {
deleted = deleted
.then(defer(self(), &Self::getService, controllerContainerId.get()))
.then(defer(self(), [this, volumeId](csi::v0::Client client) {
@@ -2800,7 +2787,7 @@ Future<Nothing> StorageLocalResourceProviderProcess::deleteVolume(
volumes.erase(volumeId);
CHECK_SOME(os::rmdir(volumePath));
- return Nothing();
+ return controllerCapabilities.createDeleteVolume;
}));
}
@@ -2990,7 +2977,10 @@ Future<Nothing> StorageLocalResourceProviderProcess::_applyOperation(
conversions = applyCreateDisk(
operation.info().create_disk().source(),
operationUuid,
- operation.info().create_disk().target_type());
+ operation.info().create_disk().target_type(),
+ operation.info().create_disk().has_target_profile()
+ ? operation.info().create_disk().target_profile()
+ : Option<string>::none());
break;
}
@@ -3094,120 +3084,50 @@ Future<vector<ResourceConversion>>
StorageLocalResourceProviderProcess::applyCreateDisk(
const Resource& resource,
const id::UUID& operationUuid,
- const Resource::DiskInfo::Source::Type& type)
+ const Resource::DiskInfo::Source::Type& targetType,
+ const Option<string>& targetProfile)
{
CHECK_EQ(Resource::DiskInfo::Source::RAW, resource.disk().source().type());
- // NOTE: Currently we only support two type of RAW disk resources:
+ // NOTE: Currently we only support two types of RAW disk resources:
// 1. RAW disk from `GetCapacity` with a profile but no volume ID.
- // 2. RAW disk from `ListVolumes` for a pre-existing volume, which
- // has a volume ID but no profile.
+ // 2. RAW disk from `ListVolumes` for a preprovisioned volume, which has a
+ // volume ID but no profile.
//
// For 1, we check if its profile is mount or block capable, then
- // call `CreateVolume` with the operation UUID as the name (so that
+ // call `createVolume` with the operation UUID as the name (so that
// the same volume will be returned when recovering from a failover).
//
- // For 2, there are two scenarios:
+ // For 2, the target profile will be specified, so we first check if the
+ // profile is mount or block capable. Then, there are two scenarios:
// a. If the volume has a checkpointed state (because it was created
- // by a previous resource provider), we simply check if its
- // checkpointed capability supports the conversion.
+ // by a previous resource provider), we simply check if its checkpointed
+ // capability and parameters match the profile.
// b. If the volume is newly discovered, `ValidateVolumeCapabilities`
- // is called with a default mount or block capability.
+ // is called with the capability of the profile.
CHECK_NE(resource.disk().source().has_profile(),
- resource.disk().source().has_id());
+ resource.disk().source().has_id() && targetProfile.isSome());
- Future<string> created;
-
- switch (type) {
- case Resource::DiskInfo::Source::MOUNT: {
- if (resource.disk().source().has_profile()) {
- // The profile exists since any operation with a stale profile must have
- // been dropped for a mismatched resource version or a reconciliation.
- CHECK(profileInfos.contains(resource.disk().source().profile()))
- << "Profile '" << resource.disk().source().profile() << "' not found";
+ const string profile =
+ targetProfile.getOrElse(resource.disk().source().profile());
- const DiskProfileAdaptor::ProfileInfo& profileInfo =
- profileInfos.at(resource.disk().source().profile());
-
- if (!profileInfo.capability.has_mount()) {
- return Failure(
- "Profile '" + resource.disk().source().profile() +
- "' cannot be used to create a MOUNT disk");
- }
-
- // TODO(chhsiao): Call `CreateVolume` sequentially with other
- // create or delete operations, and send an `UPDATE_STATE` for
- // RAW profiled resources afterward.
- created = createVolume(
- operationUuid.toString(),
- Bytes(resource.scalar().value() * Bytes::MEGABYTES),
- profileInfo);
- } else {
- const string& volumeId = resource.disk().source().id();
-
- if (volumes.contains(volumeId)) {
- if (!volumes.at(volumeId).state.volume_capability().has_mount()) {
- return Failure(
- "Volume '" + volumeId +
- "' cannot be converted to a MOUNT disk");
- }
+ if (!profileInfos.contains(profile)) {
+ return Failure("Profile '" + profile + "' not found");
+ }
- created = volumeId;
- } else {
- // No need to call `ValidateVolumeCapabilities` sequentially
- // since the volume is not used and thus not in `volumes` yet.
- created = validateCapability(
- volumeId,
- resource.disk().source().has_metadata()
- ? resource.disk().source().metadata() : Option<Labels>::none(),
- defaultMountCapability);
- }
+ const DiskProfileAdaptor::ProfileInfo& profileInfo = profileInfos.at(profile);
+ switch (targetType) {
+ case Resource::DiskInfo::Source::MOUNT: {
+ if (!profileInfo.capability.has_mount()) {
+ return Failure(
+ "Profile '" + profile + "' cannot be used to create a MOUNT disk");
}
break;
}
case Resource::DiskInfo::Source::BLOCK: {
- if (resource.disk().source().has_profile()) {
- // The profile exists since any operation with a stale profile must have
- // been dropped for a mismatched resource version or a reconciliation.
- CHECK(profileInfos.contains(resource.disk().source().profile()))
- << "Profile '" << resource.disk().source().profile() << "' not found";
-
- const DiskProfileAdaptor::ProfileInfo& profileInfo =
- profileInfos.at(resource.disk().source().profile());
-
- if (!profileInfo.capability.has_block()) {
- return Failure(
- "Profile '" + resource.disk().source().profile() +
- "' cannot be used to create a BLOCK disk");
- }
-
- // TODO(chhsiao): Call `CreateVolume` sequentially with other
- // create or delete operations, and send an `UPDATE_STATE` for
- // RAW profiled resources afterward.
- created = createVolume(
- operationUuid.toString(),
- Bytes(resource.scalar().value() * Bytes::MEGABYTES),
- profileInfo);
- } else {
- const string& volumeId = resource.disk().source().id();
-
- if (volumes.contains(volumeId)) {
- if (!volumes.at(volumeId).state.volume_capability().has_block()) {
- return Failure(
- "Volume '" + volumeId +
- "' cannot be converted to a BLOCK disk");
- }
-
- created = volumeId;
- } else {
- // No need to call `ValidateVolumeCapabilities` sequentially
- // since the volume is not used and thus not in `volumes` yet.
- created = validateCapability(
- volumeId,
- resource.disk().source().has_metadata()
- ? resource.disk().source().metadata() : Option<Labels>::none(),
- defaultBlockCapability);
- }
+ if (!profileInfo.capability.has_block()) {
+ return Failure(
+ "Profile '" + profile + "' cannot be used to create a BLOCK disk");
}
break;
}
@@ -3218,6 +3138,40 @@ StorageLocalResourceProviderProcess::applyCreateDisk(
}
}
+ Future<string> created;
+ if (resource.disk().source().has_id()) {
+ const string& volumeId = resource.disk().source().id();
+
+ if (volumes.contains(volumeId)) {
+ const VolumeState& volumeState = volumes.at(volumeId).state;
+
+ // TODO(chhsiao): Validate the volume against the parameters of the
+ // profile once they are checkpointed.
+ if (volumeState.volume_capability() != profileInfo.capability) {
+ return Failure(
+ "Profile '" + profile + "' cannot be applied to volume '" +
+ volumeId + "'");
+ }
+
+ created = volumeId;
+ } else {
+ created = validateCapability(
+ volumeId,
+ resource.disk().source().has_metadata()
+ ? resource.disk().source().metadata()
+ : Option<Labels>::none(),
+ profileInfo.capability);
+ }
+ } else {
+ // TODO(chhsiao): Consider calling `CreateVolume` sequentially with other
+ // create or delete operations, and send an `UPDATE_STATE` for storage pools
+ // afterward. See MESOS-9254.
+ created = createVolume(
+ operationUuid.toString(),
+ Bytes(resource.scalar().value() * Bytes::MEGABYTES),
+ profileInfo);
+ }
+
return created
.then(defer(self(), [=](const string& volumeId) {
CHECK(volumes.contains(volumeId));
@@ -3225,7 +3179,8 @@ StorageLocalResourceProviderProcess::applyCreateDisk(
Resource converted = resource;
converted.mutable_disk()->mutable_source()->set_id(volumeId);
- converted.mutable_disk()->mutable_source()->set_type(type);
+ converted.mutable_disk()->mutable_source()->set_type(targetType);
+ converted.mutable_disk()->mutable_source()->set_profile(profile);
if (!volumeState.volume_attributes().empty()) {
converted.mutable_disk()->mutable_source()->mutable_metadata()
@@ -3237,7 +3192,7 @@ StorageLocalResourceProviderProcess::applyCreateDisk(
info.storage().plugin().type(),
info.storage().plugin().name());
- switch (type) {
+ switch (targetType) {
case Resource::DiskInfo::Source::MOUNT: {
// Set the root path relative to agent work dir.
converted.mutable_disk()->mutable_source()->mutable_mount()
@@ -3270,24 +3225,22 @@ StorageLocalResourceProviderProcess::applyDestroyDisk(
CHECK(resource.disk().source().type() == Resource::DiskInfo::Source::MOUNT ||
resource.disk().source().type() == Resource::DiskInfo::Source::BLOCK);
CHECK(resource.disk().source().has_id());
- CHECK(volumes.contains(resource.disk().source().id()));
+
+ const string& volumeId = resource.disk().source().id();
+ CHECK(volumes.contains(volumeId));
// Sequentialize the deletion with other operation on the same volume.
- return volumes.at(resource.disk().source().id()).sequence->add(
- std::function<Future<Nothing>()>(defer(
- self(),
- &Self::deleteVolume,
- resource.disk().source().id(),
- !resource.disk().source().has_profile())))
- .then(defer(self(), [=]() {
+ return volumes.at(volumeId).sequence->add(std::function<Future<bool>()>(
+ defer(self(), &Self::deleteVolume, volumeId)))
+ .then(defer(self(), [=](bool deprovisioned) {
Resource converted = resource;
converted.mutable_disk()->mutable_source()->set_type(
Resource::DiskInfo::Source::RAW);
converted.mutable_disk()->mutable_source()->clear_mount();
- // We only clear the volume ID and metadata if the destroyed volume is not
- // a pre-existing volume.
- if (resource.disk().source().has_profile()) {
+ // We clear the volume ID and metadata if the volume has been
+ // deprovisioned. Otherwise, we clear the profile.
+ if (deprovisioned) {
converted.mutable_disk()->mutable_source()->clear_id();
converted.mutable_disk()->mutable_source()->clear_metadata();
@@ -3317,6 +3270,8 @@ StorageLocalResourceProviderProcess::applyDestroyDisk(
defer(self(), &Self::reconcileStoragePools)));
}
}
+ } else {
+ converted.mutable_disk()->mutable_source()->clear_profile();
}
vector<ResourceConversion> conversions;
[mesos] 04/09: Rewrote test `ConvertPreExistingVolume` for
`CREATE_DISK`.
Posted by ch...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
chhsiao pushed a commit to branch 1.7.x
in repository https://gitbox.apache.org/repos/asf/mesos.git
commit b704e4946705db732fd216f516df3b0e2e36128a
Author: Chun-Hung Hsiao <ch...@mesosphere.io>
AuthorDate: Sat Nov 10 16:56:36 2018 -0800
Rewrote test `ConvertPreExistingVolume` for `CREATE_DISK`.
Due to the changes of the `CREATE_DISK` semantics, this test is
rewritten to convert a preprovisioned volume to a profile volumes, and
then to destroy it to return the space back to the storage pool.
NOTE: The updated test will fail unless r/69361 (which implements the
new `CREATE_DISK` semantics) is also applied.
Review: https://reviews.apache.org/r/69360
---
.../storage_local_resource_provider_tests.cpp | 242 +++++++++------------
1 file changed, 103 insertions(+), 139 deletions(-)
diff --git a/src/tests/storage_local_resource_provider_tests.cpp b/src/tests/storage_local_resource_provider_tests.cpp
index 6058396..db8fad9 100644
--- a/src/tests/storage_local_resource_provider_tests.cpp
+++ b/src/tests/storage_local_resource_provider_tests.cpp
@@ -2554,17 +2554,22 @@ TEST_F(
}
-// This test verifies that the storage local resource provider can
-// convert pre-existing CSI volumes into mount or block volumes.
-TEST_F(StorageLocalResourceProviderTest, ROOT_ConvertPreExistingVolume)
+// This test verifies that the storage local resource provider can import a
+// preprovisioned CSI volume as a MOUNT disk of a given profile, and return the
+// space back to the storage pool after destroying the volume.
+TEST_F(StorageLocalResourceProviderTest, ROOT_ImportPreprovisionedVolume)
{
- Clock::pause();
+ const string profilesPath = path::join(sandbox.get(), "profiles.json");
+ ASSERT_SOME(os::write(profilesPath, createDiskProfileMapping("test")));
- setupResourceProviderConfig(Bytes(0), "volume1:2GB;volume2:2GB");
+ loadUriDiskProfileAdaptorModule(profilesPath);
- master::Flags masterFlags = CreateMasterFlags();
+ // NOTE: We setup up the resource provider with an extra storage pool, so that
+ // when the storage pool is offered, we know that the corresponding profile is
+ // known to the resource provider.
+ setupResourceProviderConfig(Gigabytes(2), "volume1:2GB");
- Try<Owned<cluster::Master>> master = StartMaster(masterFlags);
+ Try<Owned<cluster::Master>> master = StartMaster();
ASSERT_SOME(master);
Owned<MasterDetector> detector = master.get()->createDetector();
@@ -2573,36 +2578,14 @@ TEST_F(StorageLocalResourceProviderTest, ROOT_ConvertPreExistingVolume)
slaveFlags.isolation = "filesystem/linux";
slaveFlags.resource_provider_config_dir = resourceProviderConfigDir;
+ slaveFlags.disk_profile_adaptor = URI_DISK_PROFILE_ADAPTOR_NAME;
- // Since the local resource provider daemon is started after the agent
- // is registered, it is guaranteed that the slave will send two
- // `UpdateSlaveMessage`s, where the latter one contains resources from
- // the storage local resource provider.
- // NOTE: The order of the two `FUTURE_PROTOBUF`s is reversed because
- // Google Mock will search the expectations in reverse order.
- Future<UpdateSlaveMessage> updateSlave2 =
- FUTURE_PROTOBUF(UpdateSlaveMessage(), _, _);
- Future<UpdateSlaveMessage> updateSlave1 =
- FUTURE_PROTOBUF(UpdateSlaveMessage(), _, _);
+ Future<SlaveRegisteredMessage> slaveRegisteredMessage =
+ FUTURE_PROTOBUF(SlaveRegisteredMessage(), _, _);
Try<Owned<cluster::Slave>> slave = StartSlave(detector.get(), slaveFlags);
ASSERT_SOME(slave);
- // Advance the clock to trigger agent registration and prevent retry.
- Clock::advance(slaveFlags.registration_backoff_factor);
-
- AWAIT_READY(updateSlave1);
-
- // NOTE: We need to resume the clock so that the resource provider can
- // periodically check if the CSI endpoint socket has been created by
- // the plugin container, which runs in another Linux process.
- Clock::resume();
-
- AWAIT_READY(updateSlave2);
- ASSERT_TRUE(updateSlave2->has_resource_providers());
-
- Clock::pause();
-
// Register a framework to exercise operations.
FrameworkInfo framework = DEFAULT_FRAMEWORK_INFO;
framework.set_roles(0, "storage");
@@ -2614,147 +2597,128 @@ TEST_F(StorageLocalResourceProviderTest, ROOT_ConvertPreExistingVolume)
EXPECT_CALL(sched, registered(&driver, _, _));
// The framework is expected to see the following offers in sequence:
- // 1. One containing two RAW pre-existing volumes before `CREATE_DISK`s.
- // 2. One containing a MOUNT and a BLOCK disk resources after
- // `CREATE_DISK`s.
- // 3. One containing two RAW pre-existing volumes after `DESTROY_DISK`s.
+ // 1. One containing a RAW preprovisioned volumes before `CREATE_DISK`.
+ // 2. One containing a MOUNT disk resources after `CREATE_DISK`.
+ // 3. One containing a RAW storage pool after `DESTROY_DISK`.
//
// We set up the expectations for these offers as the test progresses.
- Future<vector<Offer>> rawDisksOffers;
- Future<vector<Offer>> disksConvertedOffers;
- Future<vector<Offer>> disksRevertedOffers;
+ Future<vector<Offer>> rawDiskOffers;
+ Future<vector<Offer>> diskCreatedOffers;
+ Future<vector<Offer>> diskDestroyedOffers;
- // We are only interested in any pre-existing volume, which has an ID
- // but no profile.
- auto isPreExistingVolume = [](const Resource& r) {
+ // 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 offers that contain only the agent's default resources.
+ EXPECT_CALL(sched, resourceOffers(&driver, _))
+ .WillRepeatedly(DeclineOffers(declineFilters));
+
+ auto isStoragePool = [](const Resource& r, const string& profile) {
return r.has_disk() &&
r.disk().has_source() &&
- r.disk().source().has_id() &&
- !r.disk().source().has_profile();
+ r.disk().source().type() == Resource::DiskInfo::Source::RAW &&
+ !r.disk().source().has_id() &&
+ r.disk().source().has_profile() &&
+ r.disk().source().profile() == profile;
};
+ // NOTE: Instead of expecting a preprovisioned volume, we expect an offer with
+ // a 'test1' storage pool as an indication that the profile is known to the
+ // resource provider. The offer should also have the preprovisioned volume.
+ // But, an extra offer with the storage pool may be received as a side effect
+ // of this workaround, so we decline it if this happens.
EXPECT_CALL(sched, resourceOffers(&driver, OffersHaveAnyResource(
- isPreExistingVolume)))
- .WillOnce(FutureArg<1>(&rawDisksOffers));
+ std::bind(isStoragePool, lambda::_1, "test"))))
+ .WillOnce(FutureArg<1>(&rawDiskOffers))
+ .WillRepeatedly(DeclineOffers(declineFilters));
driver.start();
- AWAIT_READY(rawDisksOffers);
- ASSERT_FALSE(rawDisksOffers->empty());
-
- vector<Resource> sources;
-
- foreach (const Resource& resource, rawDisksOffers->at(0).resources()) {
- if (isPreExistingVolume(resource) &&
- resource.disk().source().type() == Resource::DiskInfo::Source::RAW) {
- sources.push_back(resource);
- }
- }
-
- ASSERT_EQ(2u, sources.size());
+ AWAIT_READY(rawDiskOffers);
+ ASSERT_EQ(1u, rawDiskOffers->size());
- // Create a volume and a block.
- EXPECT_CALL(sched, resourceOffers(&driver, OffersHaveAnyResource(
- isPreExistingVolume)))
- .WillOnce(FutureArg<1>(&disksConvertedOffers));
+ auto isPreprovisionedVolume = [](const Resource& r) {
+ return r.has_disk() &&
+ r.disk().has_source() &&
+ r.disk().source().type() == Resource::DiskInfo::Source::RAW &&
+ r.disk().source().has_id() &&
+ !r.disk().source().has_profile();
+ };
- // NOTE: The order of the two `FUTURE_PROTOBUF`s is reversed because
- // Google Mock will search the expectations in reverse order.
- Future<UpdateOperationStatusMessage> createBlockStatusUpdate =
- FUTURE_PROTOBUF(UpdateOperationStatusMessage(), _, _);
- Future<UpdateOperationStatusMessage> createVolumeStatusUpdate =
- FUTURE_PROTOBUF(UpdateOperationStatusMessage(), _, _);
+ Resources _preprovisioned = Resources(rawDiskOffers->at(0).resources())
+ .filter(isPreprovisionedVolume);
- driver.acceptOffers(
- {rawDisksOffers->at(0).id()},
- {CREATE_DISK(sources.at(0), Resource::DiskInfo::Source::MOUNT),
- CREATE_DISK(sources.at(1), Resource::DiskInfo::Source::BLOCK)});
+ ASSERT_SOME_EQ(Gigabytes(2), _preprovisioned.disk());
- AWAIT_READY(createVolumeStatusUpdate);
- AWAIT_READY(createBlockStatusUpdate);
+ Resource preprovisioned = *_preprovisioned.begin();
- // Advance the clock to trigger another allocation.
- Clock::advance(masterFlags.allocation_interval);
-
- AWAIT_READY(disksConvertedOffers);
- ASSERT_FALSE(disksConvertedOffers->empty());
+ // Get the volume path of the preprovisioned volume.
+ Option<string> volumePath;
- Option<Resource> volume;
- Option<Resource> block;
-
- foreach (const Resource& resource, disksConvertedOffers->at(0).resources()) {
- if (isPreExistingVolume(resource)) {
- if (resource.disk().source().type() ==
- Resource::DiskInfo::Source::MOUNT) {
- volume = resource;
- } else if (resource.disk().source().type() ==
- Resource::DiskInfo::Source::BLOCK) {
- block = resource;
- }
+ foreach (const Label& label,
+ preprovisioned.disk().source().metadata().labels()) {
+ if (label.key() == "path") {
+ volumePath = label.value();
+ break;
}
}
- ASSERT_SOME(volume);
- ASSERT_TRUE(volume->disk().source().has_mount());
- ASSERT_TRUE(volume->disk().source().mount().has_root());
- EXPECT_FALSE(path::absolute(volume->disk().source().mount().root()));
+ ASSERT_SOME(volumePath);
+ ASSERT_TRUE(os::exists(volumePath.get()));
- ASSERT_SOME(block);
+ auto isMountDisk = [](const Resource& r, const string& profile) {
+ return r.has_disk() &&
+ r.disk().has_source() &&
+ r.disk().source().type() == Resource::DiskInfo::Source::MOUNT &&
+ r.disk().source().has_id() &&
+ r.disk().source().has_profile() &&
+ r.disk().source().profile() == profile;
+ };
- // Destroy the created volume.
+ // Apply profile 'test' to the preprovisioned volume.
EXPECT_CALL(sched, resourceOffers(&driver, OffersHaveAnyResource(
- isPreExistingVolume)))
- .WillOnce(FutureArg<1>(&disksRevertedOffers));
+ std::bind(isMountDisk, lambda::_1, "test"))))
+ .WillOnce(FutureArg<1>(&diskCreatedOffers));
- // NOTE: The order of the two `FUTURE_PROTOBUF`s is reversed because
- // Google Mock will search the expectations in reverse order.
- Future<UpdateOperationStatusMessage> destroyBlockStatusUpdate =
- FUTURE_PROTOBUF(UpdateOperationStatusMessage(), _, _);
- Future<UpdateOperationStatusMessage> destroyVolumeStatusUpdate =
- FUTURE_PROTOBUF(UpdateOperationStatusMessage(), _, _);
+ // We use the following filter so that the resources will not be
+ // filtered for 5 seconds (the default).
+ Filters acceptFilters;
+ acceptFilters.set_refuse_seconds(0);
driver.acceptOffers(
- {disksConvertedOffers->at(0).id()},
- {DESTROY_DISK(volume.get()),
- DESTROY_DISK(block.get())});
-
- AWAIT_READY(destroyVolumeStatusUpdate);
- AWAIT_READY(destroyBlockStatusUpdate);
-
- // Advance the clock to trigger another allocation.
- Clock::advance(masterFlags.allocation_interval);
+ {rawDiskOffers->at(0).id()},
+ {CREATE_DISK(preprovisioned, Resource::DiskInfo::Source::MOUNT, "test")},
+ acceptFilters);
- AWAIT_READY(disksRevertedOffers);
- ASSERT_FALSE(disksRevertedOffers->empty());
+ AWAIT_READY(diskCreatedOffers);
+ ASSERT_EQ(1u, diskCreatedOffers->size());
- vector<Resource> destroyed;
+ Resource created = *Resources(diskCreatedOffers->at(0).resources())
+ .filter(std::bind(isMountDisk, lambda::_1, "test"))
+ .begin();
- foreach (const Resource& resource, disksRevertedOffers->at(0).resources()) {
- if (isPreExistingVolume(resource) &&
- resource.disk().source().type() == Resource::DiskInfo::Source::RAW) {
- destroyed.push_back(resource);
- }
- }
+ // Destroy the created disk.
+ EXPECT_CALL(sched, resourceOffers(&driver, OffersHaveAnyResource(
+ std::bind(isStoragePool, lambda::_1, "test"))))
+ .WillOnce(FutureArg<1>(&diskDestroyedOffers));
- ASSERT_EQ(2u, destroyed.size());
+ driver.acceptOffers(
+ {diskCreatedOffers->at(0).id()},
+ {DESTROY_DISK(created)},
+ acceptFilters);
- foreach (const Resource& resource, destroyed) {
- ASSERT_FALSE(resource.disk().source().has_mount());
- ASSERT_TRUE(resource.disk().source().has_metadata());
+ AWAIT_READY(diskDestroyedOffers);
+ ASSERT_EQ(1u, diskDestroyedOffers->size());
- // Check if the volume is not deleted by the test CSI plugin.
- Option<string> volumePath;
+ Resources raw = Resources(diskDestroyedOffers->at(0).resources())
+ .filter(std::bind(isStoragePool, lambda::_1, "test"));
- foreach (const Label& label, resource.disk().source().metadata().labels()) {
- if (label.key() == "path") {
- volumePath = label.value();
- break;
- }
- }
+ EXPECT_SOME_EQ(Gigabytes(4), raw.disk());
- ASSERT_SOME(volumePath);
- EXPECT_TRUE(os::exists(volumePath.get()));
- }
+ // Check if the volume is deleted by the test CSI plugin.
+ EXPECT_FALSE(os::exists(volumePath.get()));
}
[mesos] 06/09: Added validation for
`Offer.Operation.CreateDisk.target_profile`.
Posted by ch...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
chhsiao pushed a commit to branch 1.7.x
in repository https://gitbox.apache.org/repos/asf/mesos.git
commit 27b400e0266528b08f9da90bd41a5c19514858eb
Author: Chun-Hung Hsiao <ch...@mesosphere.io>
AuthorDate: Wed Nov 14 20:34:51 2018 -0800
Added validation for `Offer.Operation.CreateDisk.target_profile`.
Review: https://reviews.apache.org/r/69356
---
src/master/validation.cpp | 6 +++++
src/tests/master_validation_tests.cpp | 44 +++++++++++++++++++++++++++++++----
2 files changed, 46 insertions(+), 4 deletions(-)
diff --git a/src/master/validation.cpp b/src/master/validation.cpp
index c4b8d8c..249e6b2 100644
--- a/src/master/validation.cpp
+++ b/src/master/validation.cpp
@@ -2520,6 +2520,12 @@ Option<Error> validate(const Offer::Operation::CreateDisk& createDisk)
return Error("'target_type' is neither MOUNT or BLOCK");
}
+ if (source.disk().source().has_profile() == createDisk.has_target_profile()) {
+ return createDisk.has_target_profile()
+ ? Error("'target_profile' must not be set when 'source' has a profile")
+ : Error("'target_profile' must be set when 'source' has no profile");
+ }
+
return None();
}
diff --git a/src/tests/master_validation_tests.cpp b/src/tests/master_validation_tests.cpp
index ec4fd13..726d677 100644
--- a/src/tests/master_validation_tests.cpp
+++ b/src/tests/master_validation_tests.cpp
@@ -1761,32 +1761,46 @@ TEST_F(ShrinkVolumeOperationValidationTest, MissingCapability)
TEST(OperationValidationTest, CreateDisk)
{
Resource disk1 = createDiskResource(
- "10", "*", None(), None(), createDiskSourceRaw());
+ "10", "*", None(), None(), createDiskSourceRaw(None(), "profile"));
Resource disk2 = createDiskResource(
- "20", "*", None(), None(), createDiskSourceMount());
+ "20", "*", None(), None(), createDiskSourceRaw());
Resource disk3 = createDiskResource(
- "30", "*", None(), None(), createDiskSourceRaw());
+ "30", "*", None(), None(), createDiskSourceMount());
+
+ Resource disk4 = createDiskResource(
+ "40", "*", None(), None(), createDiskSourceRaw(None(), "profile"));
disk1.mutable_provider_id()->set_value("provider1");
disk2.mutable_provider_id()->set_value("provider2");
+ disk3.mutable_provider_id()->set_value("provider3");
Offer::Operation::CreateDisk createDisk;
createDisk.mutable_source()->CopyFrom(disk1);
createDisk.set_target_type(Resource::DiskInfo::Source::MOUNT);
+ createDisk.clear_target_profile();
Option<Error> error = operation::validate(createDisk);
EXPECT_NONE(error);
createDisk.mutable_source()->CopyFrom(disk1);
createDisk.set_target_type(Resource::DiskInfo::Source::BLOCK);
+ createDisk.clear_target_profile();
+
+ error = operation::validate(createDisk);
+ EXPECT_NONE(error);
+
+ createDisk.mutable_source()->CopyFrom(disk2);
+ createDisk.set_target_type(Resource::DiskInfo::Source::MOUNT);
+ createDisk.set_target_profile("profile");
error = operation::validate(createDisk);
EXPECT_NONE(error);
createDisk.mutable_source()->CopyFrom(disk1);
createDisk.set_target_type(Resource::DiskInfo::Source::PATH);
+ createDisk.clear_target_profile();
error = operation::validate(createDisk);
ASSERT_SOME(error);
@@ -1794,17 +1808,39 @@ TEST(OperationValidationTest, CreateDisk)
error->message,
"'target_type' is neither MOUNT or BLOCK"));
+ createDisk.mutable_source()->CopyFrom(disk1);
+ createDisk.set_target_type(Resource::DiskInfo::Source::MOUNT);
+ createDisk.set_target_profile("profile");
+
+ error = operation::validate(createDisk);
+ ASSERT_SOME(error);
+ EXPECT_TRUE(strings::contains(
+ error->message,
+ "'target_profile' must not be set when 'source' has a profile"));
+
createDisk.mutable_source()->CopyFrom(disk2);
createDisk.set_target_type(Resource::DiskInfo::Source::MOUNT);
+ createDisk.clear_target_profile();
error = operation::validate(createDisk);
ASSERT_SOME(error);
EXPECT_TRUE(strings::contains(
error->message,
- "'source' is not a RAW disk resource"));
+ "'target_profile' must be set when 'source' has no profile"));
createDisk.mutable_source()->CopyFrom(disk3);
createDisk.set_target_type(Resource::DiskInfo::Source::MOUNT);
+ createDisk.clear_target_profile();
+
+ error = operation::validate(createDisk);
+ ASSERT_SOME(error);
+ EXPECT_TRUE(strings::contains(
+ error->message,
+ "'source' is not a RAW disk resource"));
+
+ createDisk.mutable_source()->CopyFrom(disk4);
+ createDisk.set_target_type(Resource::DiskInfo::Source::MOUNT);
+ createDisk.clear_target_profile();
error = operation::validate(createDisk);
ASSERT_SOME(error);
[mesos] 01/09: Changed the semantics of `CREATE_DISK` and
`DESTROY_DISK` operations.
Posted by ch...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
chhsiao pushed a commit to branch 1.7.x
in repository https://gitbox.apache.org/repos/asf/mesos.git
commit 4140b73f0ac75e9700a3a9260f589d7e064bee97
Author: Chun-Hung Hsiao <ch...@mesosphere.io>
AuthorDate: Mon Oct 15 20:15:41 2018 -0700
Changed the semantics of `CREATE_DISK` and `DESTROY_DISK` operations.
The semantics of these two operations has been updated to provide
primitives to import CSI volumes and recover CSI volumes against agent
ID changes and metadata loss.
Review: https://reviews.apache.org/r/69036
---
include/mesos/mesos.proto | 46 +++++++++++++++++++++++++++++++++++++++++---
include/mesos/v1/mesos.proto | 46 +++++++++++++++++++++++++++++++++++++++++---
2 files changed, 86 insertions(+), 6 deletions(-)
diff --git a/include/mesos/mesos.proto b/include/mesos/mesos.proto
index bf46cc9..2df4dad 100644
--- a/include/mesos/mesos.proto
+++ b/include/mesos/mesos.proto
@@ -1981,7 +1981,26 @@ message Offer {
required Value.Scalar subtract = 2;
}
- // Create a `MOUNT` or `BLOCK` disk resource from a `RAW` disk resource.
+ // Create a `MOUNT` or `BLOCK` disk resource backed by a CSI volume from a
+ // `RAW` disk resource.
+ //
+ // In the typical case where the `RAW` disk resource has a profile and no
+ // source ID, a new CSI volume will be provisioned by Mesos to back the
+ // returned `MOUNT` or `BLOCK` disk resource. However, the `RAW` disk
+ // resource can instead have no profile but a source ID, indicating that
+ // it is already backed by a CSI volume in one of the following scenarios:
+ //
+ // (1) The CSI volume is preprovisioned out-of-band.
+ //
+ // (2) The CSI volume is provisioned by Mesos, but Mesos has lost the
+ // corresponding `MOUNT` or `BLOCK` resource metadata. This could
+ // happen if there has been a change in the agent ID or resource
+ // provider ID where the volume belongs.
+ //
+ // In the above cases, Mesos won't provision a new CSI volume, but instead
+ // will simply return a `MOUNT` or `BLOCK` disk resource backed by the same
+ // CSI volume, with the profile specified in this call.
+ //
// NOTE: For the time being, this API is subject to change and the related
// feature is experimental.
message CreateDisk {
@@ -1989,10 +2008,31 @@ message Offer {
// NOTE: Only `MOUNT` or `BLOCK` is allowed in the `target_type` field.
required Resource.DiskInfo.Source.Type target_type = 2;
+
+ // Apply the specified profile to the created disk. This field must be set
+ // if `source` does not have a profile, and must not be set if it has one.
+ //
+ // NOTE: The operation will fail If the specified profile is unknown to
+ // Mesos, i.e., not reported by the disk profile adaptor.
+ optional string target_profile = 3;
}
- // Destroy a `MOUNT` or `BLOCK` disk resource. This will result in a `RAW`
- // disk resource.
+ // Destroy a `MOUNT` or `BLOCK` disk resource backed by a CSI volume.
+ //
+ // In the typical case where the CSI plugin of the volume supports volume
+ // deprovisioning and the profile of the disk resource is known to Mesos,
+ // the volume will be deprovisioned and a `RAW` disk resource with the same
+ // profile but no source ID will be returned. However, the following corner
+ // cases could lead to different outcomes:
+ //
+ // (1) If the CSI plugin supports volume deprovisioning but the profile of
+ // the disk resource is no longer reported by the disk profile adaptor,
+ // the volume will be deprovisioned but no resource will be returned.
+ //
+ // (2) If the CSI plugin does not support volume deprovisioning, the volume
+ // won't be deprovisioned and a `RAW` disk resource with no profile but
+ // the same source ID will be returned.
+ //
// NOTE: For the time being, this API is subject to change and the related
// feature is experimental.
message DestroyDisk {
diff --git a/include/mesos/v1/mesos.proto b/include/mesos/v1/mesos.proto
index 8b84879..6a04193 100644
--- a/include/mesos/v1/mesos.proto
+++ b/include/mesos/v1/mesos.proto
@@ -1973,7 +1973,26 @@ message Offer {
required Value.Scalar subtract = 2;
}
- // Create a `MOUNT` or `BLOCK` disk resource from a `RAW` disk resource.
+ // Create a `MOUNT` or `BLOCK` disk resource backed by a CSI volume from a
+ // `RAW` disk resource.
+ //
+ // In the typical case where the `RAW` disk resource has a profile and no
+ // source ID, a new CSI volume will be provisioned by Mesos to back the
+ // returned `MOUNT` or `BLOCK` disk resource. However, the `RAW` disk
+ // resource can instead have no profile but a source ID, indicating that
+ // it is already backed by a CSI volume in one of the following scenarios:
+ //
+ // (1) The CSI volume is preprovisioned out-of-band.
+ //
+ // (2) The CSI volume is provisioned by Mesos, but Mesos has lost the
+ // corresponding `MOUNT` or `BLOCK` resource metadata. This could
+ // happen if there has been a change in the agent ID or resource
+ // provider ID where the volume belongs.
+ //
+ // In the above cases, Mesos won't provision a new CSI volume, but instead
+ // will simply return a `MOUNT` or `BLOCK` disk resource backed by the same
+ // CSI volume, with the profile specified in this call.
+ //
// NOTE: For the time being, this API is subject to change and the related
// feature is experimental.
message CreateDisk {
@@ -1981,10 +2000,31 @@ message Offer {
// NOTE: Only `MOUNT` or `BLOCK` is allowed in the `target_type` field.
required Resource.DiskInfo.Source.Type target_type = 2;
+
+ // Apply the specified profile to the created disk. This field must be set
+ // if `source` does not have a profile, and must not be set if it has one.
+ //
+ // NOTE: The operation will fail If the specified profile is unknown to
+ // Mesos, i.e., not reported by the disk profile adaptor.
+ optional string target_profile = 3;
}
- // Destroy a `MOUNT` or `BLOCK` disk resource. This will result in a `RAW`
- // disk resource.
+ // Destroy a `MOUNT` or `BLOCK` disk resource backed by a CSI volume.
+ //
+ // In the typical case where the CSI plugin of the volume supports volume
+ // deprovisioning and the profile of the disk resource is known to Mesos,
+ // the volume will be deprovisioned and a `RAW` disk resource with the same
+ // profile but no source ID will be returned. However, the following corner
+ // cases could lead to different outcomes:
+ //
+ // (1) If the CSI plugin supports volume deprovisioning but the profile of
+ // the disk resource is no longer reported by the disk profile adaptor,
+ // the volume will be deprovisioned but no resource will be returned.
+ //
+ // (2) If the CSI plugin does not support volume deprovisioning, the volume
+ // won't be deprovisioned and a `RAW` disk resource with no profile but
+ // the same source ID will be returned.
+ //
// NOTE: For the time being, this API is subject to change and the related
// feature is experimental.
message DestroyDisk {
[mesos] 09/09: Added MESOS-9275 to the 1.7.1 CHANGELOG.
Posted by ch...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
chhsiao pushed a commit to branch 1.7.x
in repository https://gitbox.apache.org/repos/asf/mesos.git
commit f54285c01ea741c725d77ae42d587e91fce9f72b
Author: Chun-Hung Hsiao <ch...@mesosphere.io>
AuthorDate: Tue Nov 20 19:28:47 2018 -0800
Added MESOS-9275 to the 1.7.1 CHANGELOG.
---
CHANGELOG | 1 +
1 file changed, 1 insertion(+)
diff --git a/CHANGELOG b/CHANGELOG
index 4cb3e64..99f0c57 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -18,6 +18,7 @@ Release Notes - Mesos - Version 1.7.1 (WIP)
* [MESOS-9231] - `docker inspect` may return an unexpected result to Docker executor due to a race condition.
* [MESOS-9267] - Mesos agent crashes when CNI network is not configured but used.
* [MESOS-9274] - v1 JAVA scheduler library can drop TEARDOWN upon destruction.
+ * [MESOS-9275] - Allow optional `profile` to be specified in `CREATE_DISK` offer operation.
* [MESOS-9279] - Docker Containerizer 'usage' call might be expensive if mount table is big.
* [MESOS-9281] - SLRP gets a stale checkpoint after system crash.
* [MESOS-9283] - Docker containerizer actor can get backlogged with large number of containers.
[mesos] 02/09: Added profiles to storage pools in tests for
`CREATE_DISK`.
Posted by ch...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
chhsiao pushed a commit to branch 1.7.x
in repository https://gitbox.apache.org/repos/asf/mesos.git
commit 55ff7377b2062e03c04d2fd84c2935f598877a70
Author: Chun-Hung Hsiao <ch...@mesosphere.io>
AuthorDate: Thu Nov 15 01:18:04 2018 -0800
Added profiles to storage pools in tests for `CREATE_DISK`.
This patch adds a new `targetProfile` parameter to the `CREATE_DISK`
test helper, and add profiles to all storage pools in tests, to adhere
to the new semantics of `CREATE_DISK`.
Review: https://reviews.apache.org/r/69357
---
src/tests/api_tests.cpp | 12 ++++++++++--
src/tests/master_tests.cpp | 6 +-----
src/tests/mesos.hpp | 22 ++++++++++++++++------
src/tests/operation_reconciliation_tests.cpp | 11 ++++++-----
src/tests/resource_provider_manager_tests.cpp | 2 +-
.../storage_local_resource_provider_tests.cpp | 6 +++++-
6 files changed, 39 insertions(+), 20 deletions(-)
diff --git a/src/tests/api_tests.cpp b/src/tests/api_tests.cpp
index a84109e..b42628a 100644
--- a/src/tests/api_tests.cpp
+++ b/src/tests/api_tests.cpp
@@ -1044,7 +1044,11 @@ TEST_P(MasterAPITest, GetOperations)
v1::MockResourceProvider resourceProvider(
info,
v1::createDiskResource(
- "200", "*", None(), None(), v1::createDiskSourceRaw()));
+ "200",
+ "*",
+ None(),
+ None(),
+ v1::createDiskSourceRaw(None(), "profile")));
// Start and register resource provider.
Owned<EndpointDetector> endpointDetector(
@@ -7739,7 +7743,11 @@ TEST_P(AgentAPITest, GetOperations)
v1::MockResourceProvider resourceProvider(
info,
v1::createDiskResource(
- "200", "*", None(), None(), v1::createDiskSourceRaw()));
+ "200",
+ "*",
+ None(),
+ None(),
+ v1::createDiskSourceRaw(None(), "profile")));
// Start and register a resource provider.
Owned<EndpointDetector> endpointDetector(
diff --git a/src/tests/master_tests.cpp b/src/tests/master_tests.cpp
index 1bf22e5..9d5d5a3 100644
--- a/src/tests/master_tests.cpp
+++ b/src/tests/master_tests.cpp
@@ -8869,11 +8869,7 @@ TEST_F(MasterTest, OperationUpdateDuringFailover)
resourceProviderInfo.set_name("test");
v1::Resources resourceProviderResources = v1::createDiskResource(
- "200",
- "*",
- None(),
- None(),
- v1::createDiskSourceRaw());
+ "200", "*", None(), None(), v1::createDiskSourceRaw(None(), "profile"));
v1::MockResourceProvider resourceProvider(
resourceProviderInfo,
diff --git a/src/tests/mesos.hpp b/src/tests/mesos.hpp
index 75c5fae..e4a1ab4 100644
--- a/src/tests/mesos.hpp
+++ b/src/tests/mesos.hpp
@@ -1401,13 +1401,18 @@ inline typename TOffer::Operation LAUNCH_GROUP(
template <typename TResource, typename TTargetType, typename TOffer>
inline typename TOffer::Operation CREATE_DISK(
const TResource& source,
- const TTargetType& type,
+ const TTargetType& targetType,
+ const Option<std::string>& targetProfile = None(),
const Option<std::string>& operationId = None())
{
typename TOffer::Operation operation;
operation.set_type(TOffer::Operation::CREATE_DISK);
operation.mutable_create_disk()->mutable_source()->CopyFrom(source);
- operation.mutable_create_disk()->set_target_type(type);
+ operation.mutable_create_disk()->set_target_type(targetType);
+
+ if (targetProfile.isSome()) {
+ operation.mutable_create_disk()->set_target_profile(targetProfile.get());
+ }
if (operationId.isSome()) {
operation.mutable_id()->set_value(operationId.get());
@@ -3173,18 +3178,23 @@ public:
update->mutable_status()->add_converted_resources()->CopyFrom(
operation.info().create_disk().source());
update->mutable_status()
- ->mutable_converted_resources()
- ->Mutable(0)
+ ->mutable_converted_resources(0)
->mutable_disk()
->mutable_source()
->set_type(operation.info().create_disk().target_type());
+ if (operation.info().create_disk().has_target_profile()) {
+ update->mutable_status()
+ ->mutable_converted_resources(0)
+ ->mutable_disk()
+ ->mutable_source()
+ ->set_profile(operation.info().create_disk().target_profile());
+ }
break;
case Operation::DESTROY_DISK:
update->mutable_status()->add_converted_resources()->CopyFrom(
operation.info().destroy_disk().source());
update->mutable_status()
- ->mutable_converted_resources()
- ->Mutable(0)
+ ->mutable_converted_resources(0)
->mutable_disk()
->mutable_source()
->set_type(Source::RAW);
diff --git a/src/tests/operation_reconciliation_tests.cpp b/src/tests/operation_reconciliation_tests.cpp
index aec298d..58ff18d 100644
--- a/src/tests/operation_reconciliation_tests.cpp
+++ b/src/tests/operation_reconciliation_tests.cpp
@@ -678,8 +678,8 @@ TEST_P(OperationReconciliationTest, AgentPendingOperationAfterMasterFailover)
resourceProviderInfo.set_type("org.apache.mesos.rp.test");
resourceProviderInfo.set_name("test");
- Resource disk =
- createDiskResource("200", "*", None(), None(), createDiskSourceRaw());
+ Resource disk = createDiskResource(
+ "200", "*", None(), None(), createDiskSourceRaw(None(), "profile"));
Owned<MockResourceProvider> resourceProvider(
new MockResourceProvider(
@@ -792,9 +792,10 @@ TEST_P(OperationReconciliationTest, AgentPendingOperationAfterMasterFailover)
frameworkId,
offer,
{CREATE_DISK(
- source.get(),
- Resource::DiskInfo::Source::MOUNT,
- operationId.value())}));
+ source.get(),
+ Resource::DiskInfo::Source::MOUNT,
+ None(),
+ operationId.value())}));
AWAIT_READY(applyOperation);
diff --git a/src/tests/resource_provider_manager_tests.cpp b/src/tests/resource_provider_manager_tests.cpp
index 0b9e985..82be62f 100644
--- a/src/tests/resource_provider_manager_tests.cpp
+++ b/src/tests/resource_provider_manager_tests.cpp
@@ -1024,7 +1024,7 @@ TEST_P(ResourceProviderManagerHttpApiTest, ConvertResources)
AWAIT_READY(updateSlaveMessage);
v1::Resource disk = v1::createDiskResource(
- "200", "*", None(), None(), v1::createDiskSourceRaw());
+ "200", "*", None(), None(), v1::createDiskSourceRaw(None(), "profile"));
updateSlaveMessage = FUTURE_PROTOBUF(UpdateSlaveMessage(), _, _);
diff --git a/src/tests/storage_local_resource_provider_tests.cpp b/src/tests/storage_local_resource_provider_tests.cpp
index 9b88d36..e5751a2 100644
--- a/src/tests/storage_local_resource_provider_tests.cpp
+++ b/src/tests/storage_local_resource_provider_tests.cpp
@@ -4067,7 +4067,10 @@ TEST_F(
frameworkId,
offer,
{v1::CREATE_DISK(
- source.get(), v1::Resource::DiskInfo::Source::MOUNT, operationId)}));
+ source.get(),
+ v1::Resource::DiskInfo::Source::MOUNT,
+ None(),
+ operationId)}));
AWAIT_READY(update);
@@ -4254,6 +4257,7 @@ TEST_F(
{v1::CREATE_DISK(
source.get(),
v1::Resource::DiskInfo::Source::MOUNT,
+ None(),
operationId.value())}));
AWAIT_READY(update);
[mesos] 08/09: Checkpointed creation parameters for CSI volumes.
Posted by ch...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
chhsiao pushed a commit to branch 1.7.x
in repository https://gitbox.apache.org/repos/asf/mesos.git
commit 9d32259105c4794d7d45060c9fad03954a7e39cb
Author: Chun-Hung Hsiao <ch...@mesosphere.io>
AuthorDate: Thu Nov 15 16:39:59 2018 -0800
Checkpointed creation parameters for CSI volumes.
The parameters of CSI volumes created by SLRPs are now checkpointed, and
used to validate volumes created from previous SLRP runs.
Review: https://reviews.apache.org/r/69362
---
src/csi/state.proto | 6 ++
src/resource_provider/storage/provider.cpp | 103 +++++++++++++++--------------
2 files changed, 59 insertions(+), 50 deletions(-)
diff --git a/src/csi/state.proto b/src/csi/state.proto
index 8445399..264a565 100644
--- a/src/csi/state.proto
+++ b/src/csi/state.proto
@@ -20,6 +20,9 @@ import "csi.proto";
package mesos.csi.state;
+// NOTE: The keywords 'REQUIRED' and 'OPTIONAL' are to be interpreted as
+// described in the CSI specification:
+// https://github.com/container-storage-interface/spec/blob/master/spec.md#required-vs-optional // NOLINT
// Represents the state of a provisioned volume with respect to a node.
message VolumeState {
@@ -43,6 +46,9 @@ message VolumeState {
// The capability used to publish the volume. This is a REQUIRED field.
.csi.v0.VolumeCapability volume_capability = 2;
+ // The parameters used when creating the volume. This is an OPTIONAL field.
+ map<string, string> parameters = 6;
+
// Attributes of the volume to be used on the node. This field MUST match the
// attributes of the `Volume` returned by `CreateVolume`. This is an OPTIONAL
// field.
diff --git a/src/resource_provider/storage/provider.cpp b/src/resource_provider/storage/provider.cpp
index 97aa340..2d8d08d 100644
--- a/src/resource_provider/storage/provider.cpp
+++ b/src/resource_provider/storage/provider.cpp
@@ -403,10 +403,10 @@ private:
const Bytes& capacity,
const DiskProfileAdaptor::ProfileInfo& profileInfo);
Future<bool> deleteVolume(const string& volumeId);
- Future<string> validateCapability(
+ Future<Nothing> validateVolume(
const string& volumeId,
const Option<Labels>& metadata,
- const csi::v0::VolumeCapability& capability);
+ const DiskProfileAdaptor::ProfileInfo& profileInfo);
Future<Resources> listVolumes();
Future<Resources> getCapacities();
@@ -2677,6 +2677,7 @@ Future<string> StorageLocalResourceProviderProcess::createVolume(
volumeState.set_state(VolumeState::CREATED);
volumeState.mutable_volume_capability()
->CopyFrom(profileInfo.capability);
+ *volumeState.mutable_parameters() = profileInfo.parameters;
*volumeState.mutable_volume_attributes() = volume.attributes();
volumes.put(volume.id(), std::move(volumeState));
@@ -2792,17 +2793,33 @@ Future<bool> StorageLocalResourceProviderProcess::deleteVolume(
}
-// Validates if a volume has the specified capability. This is called when
-// applying `CREATE_DISK` on a pre-existing volume, so we make it return a
-// volume ID, similar to `createVolume`.
-// NOTE: This can only be called after `prepareIdentityService` and only for
-// newly discovered volumes.
-Future<string> StorageLocalResourceProviderProcess::validateCapability(
+// Validates if a volume supports the capability of the specified profile.
+//
+// NOTE: This can only be called after `prepareIdentityService`.
+//
+// TODO(chhsiao): Validate the volume against the parameters of the profile once
+// we get CSI v1.
+Future<Nothing> StorageLocalResourceProviderProcess::validateVolume(
const string& volumeId,
const Option<Labels>& metadata,
- const csi::v0::VolumeCapability& capability)
+ const DiskProfileAdaptor::ProfileInfo& profileInfo)
{
- CHECK(!volumes.contains(volumeId));
+ // If the volume has a checkpointed state, the validation succeeds only if the
+ // capability and parameters of the specified profile are the same as those in
+ // the checkpoint.
+ if (volumes.contains(volumeId)) {
+ const VolumeState& volumeState = volumes.at(volumeId).state;
+
+ if (volumeState.volume_capability() != profileInfo.capability) {
+ return Failure("Invalid volume capability for volume '" + volumeId + "'");
+ }
+
+ if (volumeState.parameters() != profileInfo.parameters) {
+ return Failure("Invalid parameters for volume '" + volumeId + "'");
+ }
+
+ return Nothing();
+ }
if (!pluginCapabilities.controllerService) {
return Failure(
@@ -2816,19 +2833,20 @@ Future<string> StorageLocalResourceProviderProcess::validateCapability(
google::protobuf::Map<string, string> volumeAttributes;
if (metadata.isSome()) {
- volumeAttributes = convertLabelsToStringMap(metadata.get()).get();
+ volumeAttributes =
+ CHECK_NOTERROR(convertLabelsToStringMap(metadata.get()));
}
csi::v0::ValidateVolumeCapabilitiesRequest request;
request.set_volume_id(volumeId);
- request.add_volume_capabilities()->CopyFrom(capability);
+ request.add_volume_capabilities()->CopyFrom(profileInfo.capability);
*request.mutable_volume_attributes() = volumeAttributes;
return call<csi::v0::VALIDATE_VOLUME_CAPABILITIES>(
client, std::move(request))
.then(defer(self(), [=](
const csi::v0::ValidateVolumeCapabilitiesResponse& response)
- -> Future<string> {
+ -> Future<Nothing> {
if (!response.supported()) {
return Failure(
"Unsupported volume capability for volume '" + volumeId +
@@ -2837,13 +2855,15 @@ Future<string> StorageLocalResourceProviderProcess::validateCapability(
VolumeState volumeState;
volumeState.set_state(VolumeState::CREATED);
- volumeState.mutable_volume_capability()->CopyFrom(capability);
+ volumeState.mutable_volume_capability()
+ ->CopyFrom(profileInfo.capability);
+ *volumeState.mutable_parameters() = profileInfo.parameters;
*volumeState.mutable_volume_attributes() = volumeAttributes;
volumes.put(volumeId, std::move(volumeState));
checkpointVolumeState(volumeId);
- return volumeId;
+ return Nothing();
}));
}));
}
@@ -3099,12 +3119,13 @@ StorageLocalResourceProviderProcess::applyCreateDisk(
// the same volume will be returned when recovering from a failover).
//
// For 2, the target profile will be specified, so we first check if the
- // profile is mount or block capable. Then, there are two scenarios:
- // a. If the volume has a checkpointed state (because it was created
- // by a previous resource provider), we simply check if its checkpointed
+ // profile is mount or block capable. Then, we call `validateVolume` to handle
+ // the following two scenarios:
+ // a. If the volume has a checkpointed state (because it is created by a
+ // previous resource provider), we simply check if its checkpointed
// capability and parameters match the profile.
- // b. If the volume is newly discovered, `ValidateVolumeCapabilities`
- // is called with the capability of the profile.
+ // b. If the volume is newly discovered, `ValidateVolumeCapabilities` is
+ // called with the capability of the profile.
CHECK_NE(resource.disk().source().has_profile(),
resource.disk().source().has_id() && targetProfile.isSome());
@@ -3138,39 +3159,21 @@ StorageLocalResourceProviderProcess::applyCreateDisk(
}
}
- Future<string> created;
- if (resource.disk().source().has_id()) {
- const string& volumeId = resource.disk().source().id();
-
- if (volumes.contains(volumeId)) {
- const VolumeState& volumeState = volumes.at(volumeId).state;
-
- // TODO(chhsiao): Validate the volume against the parameters of the
- // profile once they are checkpointed.
- if (volumeState.volume_capability() != profileInfo.capability) {
- return Failure(
- "Profile '" + profile + "' cannot be applied to volume '" +
- volumeId + "'");
- }
-
- created = volumeId;
- } else {
- created = validateCapability(
- volumeId,
+ // TODO(chhsiao): Consider calling `createVolume` sequentially with other
+ // create or delete operations, and send an `UPDATE_STATE` for storage pools
+ // afterward. See MESOS-9254.
+ Future<string> created = resource.disk().source().has_profile()
+ ? createVolume(
+ operationUuid.toString(),
+ Bytes(resource.scalar().value() * Bytes::MEGABYTES),
+ profileInfo)
+ : validateVolume(
+ resource.disk().source().id(),
resource.disk().source().has_metadata()
? resource.disk().source().metadata()
: Option<Labels>::none(),
- profileInfo.capability);
- }
- } else {
- // TODO(chhsiao): Consider calling `CreateVolume` sequentially with other
- // create or delete operations, and send an `UPDATE_STATE` for storage pools
- // afterward. See MESOS-9254.
- created = createVolume(
- operationUuid.toString(),
- Bytes(resource.scalar().value() * Bytes::MEGABYTES),
- profileInfo);
- }
+ profileInfo)
+ .then([=] { return resource.disk().source().id(); });
return created
.then(defer(self(), [=](const string& volumeId) {