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:32:07 UTC
[mesos] 08/09: Checkpointed creation parameters for CSI volumes.
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) {