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 95607e298a99cc5c5b85724dc2ca9df68b1332af Author: Chun-Hung Hsiao <[email protected]> AuthorDate: Tue Mar 26 21:02:00 2019 -0700 API changes for supporting destroying `RAW` disks. This patch enables a framework to apply `DESTROY_DISK` on a `RAW` disk backed by a preprovisioned CSI volume. This API improvement brings the following benefits: * If a `CREATE_DISK` failed because of, e.g., agent gone, but the actual CSI call was finished, `DESTROY_DISK` can be used to reclaim the "leaked" volume. * If there is an agent ID change, `DESTROY_DISK` can be used to clean up volumes created by the previous agent's SLRP. Review: https://reviews.apache.org/r/70313/ --- include/mesos/mesos.proto | 20 ++++++++------- include/mesos/v1/mesos.proto | 22 +++++++++-------- src/master/validation.cpp | 9 +++++-- src/tests/master_validation_tests.cpp | 46 +++++++++++++++++++++++++++-------- 4 files changed, 66 insertions(+), 31 deletions(-) diff --git a/include/mesos/mesos.proto b/include/mesos/mesos.proto index 35e10f1..7aa7d5b 100644 --- a/include/mesos/mesos.proto +++ b/include/mesos/mesos.proto @@ -2088,7 +2088,7 @@ message Offer { message CreateDisk { required Resource source = 1; - // NOTE: Only `MOUNT` or `BLOCK` is allowed in the `target_type` field. + // NOTE: Only `MOUNT` or `BLOCK` is allowed in this field. required Resource.DiskInfo.Source.Type target_type = 2; // Apply the specified profile to the created disk. This field must be set @@ -2099,17 +2099,19 @@ message Offer { optional string target_profile = 3; } - // Destroy a `MOUNT` or `BLOCK` disk resource backed by a CSI volume. + // Destroy a 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: + // deprovisioning and the disk resource is a `MOUNT` or `BLOCK` disk with a + // profile 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 scenarios 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. + // the disk resource is unknown to the disk profile adaptor, or the disk + // resource is a `RAW` disk with no profile but a source ID (see above + // for possible scenarios), 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 @@ -2118,7 +2120,7 @@ message Offer { // NOTE: For the time being, this API is subject to change and the related // feature is experimental. message DestroyDisk { - // NOTE: Only a `MOUNT` or `BLOCK` disk is allowed in the `source` field. + // NOTE: Only a `MOUNT`, `BLOCK` or `RAW` disk is allowed in this field. required Resource source = 1; } diff --git a/include/mesos/v1/mesos.proto b/include/mesos/v1/mesos.proto index 1d751f6..d66f29c 100644 --- a/include/mesos/v1/mesos.proto +++ b/include/mesos/v1/mesos.proto @@ -2080,7 +2080,7 @@ message Offer { message CreateDisk { required Resource source = 1; - // NOTE: Only `MOUNT` or `BLOCK` is allowed in the `target_type` field. + // NOTE: Only `MOUNT` or `BLOCK` is allowed in this field. required Resource.DiskInfo.Source.Type target_type = 2; // Apply the specified profile to the created disk. This field must be set @@ -2091,18 +2091,20 @@ message Offer { optional string target_profile = 3; } - // Destroy a `MOUNT` or `BLOCK` disk resource backed by a CSI volume. + // Destroy a 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: + // deprovisioning and the disk resource is a `MOUNT` or `BLOCK` disk with a + // profile 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 scenarios 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. - // + // the disk resource is unknown to the disk profile adaptor, or the disk + // resource is a `RAW` disk with no profile but a source ID (see above + // for possible scenarios), 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. @@ -2110,7 +2112,7 @@ message Offer { // NOTE: For the time being, this API is subject to change and the related // feature is experimental. message DestroyDisk { - // NOTE: Only a `MOUNT` or `BLOCK` disk is allowed in the `source` field. + // NOTE: Only a `MOUNT`, `BLOCK` or `RAW` disk is allowed in this field. required Resource source = 1; } diff --git a/src/master/validation.cpp b/src/master/validation.cpp index c5f0d78..f032a78 100644 --- a/src/master/validation.cpp +++ b/src/master/validation.cpp @@ -2603,8 +2603,13 @@ Option<Error> validate(const Offer::Operation::DestroyDisk& destroyDisk) } if (!Resources::isDisk(source, Resource::DiskInfo::Source::MOUNT) && - !Resources::isDisk(source, Resource::DiskInfo::Source::BLOCK)) { - return Error("'source' is neither a MOUNT or BLOCK disk resource"); + !Resources::isDisk(source, Resource::DiskInfo::Source::BLOCK) && + !Resources::isDisk(source, Resource::DiskInfo::Source::RAW)) { + return Error("'source' is neither a MOUNT, BLOCK or RAW disk resource"); + } + + if (!source.disk().source().has_id()) { + return Error("'source' is not backed by a CSI volume"); } if (Resources::isPersistentVolume(source)) { diff --git a/src/tests/master_validation_tests.cpp b/src/tests/master_validation_tests.cpp index 7f3751a..400ad68 100644 --- a/src/tests/master_validation_tests.cpp +++ b/src/tests/master_validation_tests.cpp @@ -1974,24 +1974,37 @@ TEST(OperationValidationTest, CreateDisk) TEST(OperationValidationTest, DestroyDisk) { Resource disk1 = createDiskResource( - "10", "*", None(), None(), createDiskSourceMount()); + "10", "*", None(), None(), createDiskSourceMount(None(), "volume1")); Resource disk2 = createDiskResource( - "20", "*", None(), None(), createDiskSourceBlock()); + "20", "*", None(), None(), createDiskSourceBlock("volume2")); Resource disk3 = createDiskResource( - "30", "*", None(), None(), createDiskSourcePath()); + "40", "*", None(), None(), createDiskSourceRaw("volume3")); Resource disk4 = createDiskResource( - "40", "*", None(), None(), createDiskSourceMount()); + "40", "*", None(), None(), createDiskSourcePath("volume4")); - Resource disk5 = createPersistentVolume( - Megabytes(50), "role", "id", "path", None(), createDiskSourceMount()); + Resource disk5 = createDiskResource( + "50", "*", None(), None(), createDiskSourceMount(None(), "volume5")); + + Resource disk6 = createDiskResource( + "60", "*", None(), None(), createDiskSourceRaw(None(), "profile")); + + Resource disk7 = createPersistentVolume( + Megabytes(70), + "role", + "id", + "path", + None(), + createDiskSourceMount(None(), "volume7")); disk1.mutable_provider_id()->set_value("provider1"); disk2.mutable_provider_id()->set_value("provider2"); disk3.mutable_provider_id()->set_value("provider3"); - disk5.mutable_provider_id()->set_value("provider5"); + disk4.mutable_provider_id()->set_value("provider4"); + disk6.mutable_provider_id()->set_value("provider6"); + disk7.mutable_provider_id()->set_value("provider7"); Offer::Operation::DestroyDisk destroyDisk; destroyDisk.mutable_source()->CopyFrom(disk1); @@ -2007,12 +2020,17 @@ TEST(OperationValidationTest, DestroyDisk) destroyDisk.mutable_source()->CopyFrom(disk3); error = operation::validate(destroyDisk); + EXPECT_NONE(error); + + destroyDisk.mutable_source()->CopyFrom(disk4); + + error = operation::validate(destroyDisk); ASSERT_SOME(error); EXPECT_TRUE(strings::contains( error->message, - "'source' is neither a MOUNT or BLOCK disk resource")); + "'source' is neither a MOUNT, BLOCK or RAW disk resource")); - destroyDisk.mutable_source()->CopyFrom(disk4); + destroyDisk.mutable_source()->CopyFrom(disk5); error = operation::validate(destroyDisk); ASSERT_SOME(error); @@ -2020,7 +2038,15 @@ TEST(OperationValidationTest, DestroyDisk) error->message, "'source' is not managed by a resource provider")); - destroyDisk.mutable_source()->CopyFrom(disk5); + destroyDisk.mutable_source()->CopyFrom(disk6); + + error = operation::validate(destroyDisk); + ASSERT_SOME(error); + EXPECT_TRUE(strings::contains( + error->message, + "'source' is not backed by a CSI volume")); + + destroyDisk.mutable_source()->CopyFrom(disk7); error = operation::validate(destroyDisk); ASSERT_SOME(error);
