chore(bors): merge pull request #647

647: Snapshot/timeout: add missing volume timeouts and refactor naming r=tiagolobocastro a=tiagolobocastro fix(snapshot/timeout): add missing volume snapshot timeout Signed-off-by: Tiago Castro <[email protected]> --- refactor(snapshot/destroy): rename delete to destroy as per convention Signed-off-by: Tiago Castro <[email protected]> Co-authored-by: Tiago Castro <[email protected]>
openebs · Jul 20, 2023 · 89e8393 · 89e8393
2 parents 6b23df9 + caf1364
commit 89e8393
Show file tree

Hide file tree

Showing 11 changed files with 56 additions and 49 deletions.
diff --git a/control-plane/agents/src/bin/core/tests/volume/snapshot.rs b/control-plane/agents/src/bin/core/tests/volume/snapshot.rs
@@ -3,7 +3,7 @@ use deployer_cluster::{Cluster, ClusterBuilder};
 use grpc::operations::{
     pool::traits::PoolOperations,
     replica::traits::ReplicaOperations,
-    volume::traits::{CreateVolumeSnapshot, DeleteVolumeSnapshot, VolumeOperations},
+    volume::traits::{CreateVolumeSnapshot, DestroyVolumeSnapshot, VolumeOperations},
 };
 use std::time::Duration;
 use stor_port::{
@@ -96,7 +96,7 @@ async fn snapshot() {
     assert!(!snaps.entries().is_empty());
 
     vol_cli
-        .delete_snapshot(&DeleteVolumeSnapshot::from(&replica_snapshot), None)
+        .destroy_snapshot(&DestroyVolumeSnapshot::from(&replica_snapshot), None)
         .await
         .unwrap();
 
@@ -143,7 +143,7 @@ async fn snapshot() {
 
     tracing::info!("Nexus Snapshot: {nexus_snapshot:?}");
     vol_cli
-        .delete_snapshot(&DeleteVolumeSnapshot::from(&nexus_snapshot), None)
+        .destroy_snapshot(&DestroyVolumeSnapshot::from(&nexus_snapshot), None)
         .await
         .unwrap();
 
@@ -207,7 +207,7 @@ async fn thin_provisioning(cluster: &Cluster, volume: Volume) {
         .await
         .unwrap();
     vol_cli
-        .delete_snapshot(&DeleteVolumeSnapshot::from(&snapshot), None)
+        .destroy_snapshot(&DestroyVolumeSnapshot::from(&snapshot), None)
         .await
         .unwrap();
 
@@ -276,7 +276,7 @@ async fn pool_destroy_validation(cluster: &Cluster) {
     assert_eq!(del_error.kind, ReplyErrorKind::InUse);
 
     vol_cli
-        .delete_snapshot(&DeleteVolumeSnapshot::from(&replica_snapshot), None)
+        .destroy_snapshot(&DestroyVolumeSnapshot::from(&replica_snapshot), None)
         .await
         .unwrap();
 }

diff --git a/control-plane/agents/src/bin/core/volume/service.rs b/control-plane/agents/src/bin/core/volume/service.rs
@@ -18,11 +18,11 @@ use grpc::{
     context::Context,
     operations::{
         volume::traits::{
-            CreateVolumeInfo, CreateVolumeSnapshot, CreateVolumeSnapshotInfo, DeleteVolumeSnapshot,
-            DeleteVolumeSnapshotInfo, DestroyShutdownTargetsInfo, DestroyVolumeInfo,
-            PublishVolumeInfo, RepublishVolumeInfo, SetVolumeReplicaInfo, ShareVolumeInfo,
-            UnpublishVolumeInfo, UnshareVolumeInfo, VolumeOperations, VolumeSnapshot,
-            VolumeSnapshots,
+            CreateVolumeInfo, CreateVolumeSnapshot, CreateVolumeSnapshotInfo,
+            DestroyShutdownTargetsInfo, DestroyVolumeInfo, DestroyVolumeSnapshot,
+            DestroyVolumeSnapshotInfo, PublishVolumeInfo, RepublishVolumeInfo,
+            SetVolumeReplicaInfo, ShareVolumeInfo, UnpublishVolumeInfo, UnshareVolumeInfo,
+            VolumeOperations, VolumeSnapshot, VolumeSnapshots,
         },
         Pagination,
     },
@@ -184,14 +184,14 @@ impl VolumeOperations for Service {
         Ok(snapshot)
     }
 
-    async fn delete_snapshot(
+    async fn destroy_snapshot(
         &self,
-        request: &dyn DeleteVolumeSnapshotInfo,
+        request: &dyn DestroyVolumeSnapshotInfo,
         _ctx: Option<Context>,
     ) -> Result<(), ReplyError> {
         let service = self.clone();
         let request = request.info();
-        Context::spawn(async move { service.delete_snapshot(request).await }).await??;
+        Context::spawn(async move { service.destroy_snapshot(request).await }).await??;
         Ok(())
     }
 
@@ -388,7 +388,7 @@ impl Service {
 
     /// Delete a volume snapshot.
     #[tracing::instrument(level = "info", skip(self), err, fields(volume.uuid = ?request.source_id, snapshot.source_uuid = ?request.source_id, snapshot.uuid = %request.snap_id))]
-    async fn delete_snapshot(&self, request: DeleteVolumeSnapshot) -> Result<(), SvcError> {
+    async fn destroy_snapshot(&self, request: DestroyVolumeSnapshot) -> Result<(), SvcError> {
         // Fetch the snapshot spec.
         let snapshot = self.specs().volume_snapshot_rsc(request.snap_id()).ok_or(
             SvcError::VolSnapshotNotFound {

diff --git a/control-plane/grpc/proto/v1/volume/volume.proto b/control-plane/grpc/proto/v1/volume/volume.proto
@@ -409,14 +409,14 @@ message CreateSnapshotReply {
 }
 
 // Delete a snapshot of the volume
-message DeleteSnapshotRequest {
+message DestroySnapshotRequest {
   // uuid of the volume
   optional string   volume_id = 1;
   // uuid of the snapshot
   string          snapshot_id = 2;
 }
-// Reply type for a DeleteSnapshotRequest request
-message DeleteSnapshotReply {
+// Reply type for a DestroySnapshotRequest request
+message DestroySnapshotReply {
   optional common.ReplyError error = 1;
 }
 
@@ -516,6 +516,6 @@ service VolumeGrpc {
 
   // Snapshots
   rpc CreateSnapshot (CreateSnapshotRequest) returns (CreateSnapshotReply) {}
-  rpc DeleteSnapshot (DeleteSnapshotRequest) returns (DeleteSnapshotReply) {}
+  rpc DestroySnapshot (DestroySnapshotRequest) returns (DestroySnapshotReply) {}
   rpc GetSnapshots (GetSnapshotsRequest) returns (GetSnapshotsReply) {}
 }
diff --git a/control-plane/grpc/src/context.rs b/control-plane/grpc/src/context.rs
@@ -53,6 +53,8 @@ pub fn timeout_grpc(op_id: MessageId, timeout_opts: TimeoutOptions) -> Duration
                 MessageIdVs::PublishVolume => min_timeouts.nexus(),
                 MessageIdVs::UnpublishVolume => min_timeouts.nexus(),
                 MessageIdVs::RepublishVolume => min_timeouts.nexus() * 2,
+                MessageIdVs::CreateVolumeSnapshot => min_timeouts.volume_snapshot(),
+                MessageIdVs::DestroyVolumeSnapshot => min_timeouts.volume_snapshot(),
 
                 MessageIdVs::CreateNexus => min_timeouts.nexus(),
                 MessageIdVs::CreateNexusSnapshot => min_timeouts.nexus_snapshot(),

diff --git a/control-plane/grpc/src/operations/volume/client.rs b/control-plane/grpc/src/operations/volume/client.rs
@@ -9,7 +9,7 @@ use crate::{
                 ShareVolumeInfo, UnpublishVolumeInfo, UnshareVolumeInfo, VolumeOperations,
                 VolumeSnapshot, VolumeSnapshots,
             },
-            traits_snapshots::DeleteVolumeSnapshotInfo,
+            traits_snapshots::DestroyVolumeSnapshotInfo,
         },
         Pagination,
     },
@@ -260,13 +260,13 @@ impl VolumeOperations for VolumeClient {
     }
 
     #[tracing::instrument(name = "VolumeClient::delete_snapshot", level = "debug", skip(self))]
-    async fn delete_snapshot(
+    async fn destroy_snapshot(
         &self,
-        request: &dyn DeleteVolumeSnapshotInfo,
+        request: &dyn DestroyVolumeSnapshotInfo,
         ctx: Option<Context>,
     ) -> Result<(), ReplyError> {
-        let req = self.request(request, ctx, MessageIdVs::DeleteVolumeSnapshot);
-        let response = self.client().delete_snapshot(req).await?.into_inner();
+        let req = self.request(request, ctx, MessageIdVs::DestroyVolumeSnapshot);
+        let response = self.client().destroy_snapshot(req).await?.into_inner();
         match response.error {
             None => Ok(()),
             Some(err) => Err(err.into()),

diff --git a/control-plane/grpc/src/operations/volume/server.rs b/control-plane/grpc/src/operations/volume/server.rs
@@ -7,8 +7,8 @@ use crate::{
         unpublish_volume_reply,
         volume_grpc_server::{VolumeGrpc, VolumeGrpcServer},
         CreateSnapshotReply, CreateSnapshotRequest, CreateVolumeReply, CreateVolumeRequest,
-        DeleteSnapshotReply, DeleteSnapshotRequest, DestroyShutdownTargetReply,
-        DestroyShutdownTargetRequest, DestroyVolumeReply, DestroyVolumeRequest, GetSnapshotsReply,
+        DestroyShutdownTargetReply, DestroyShutdownTargetRequest, DestroySnapshotReply,
+        DestroySnapshotRequest, DestroyVolumeReply, DestroyVolumeRequest, GetSnapshotsReply,
         GetSnapshotsRequest, GetVolumesReply, GetVolumesRequest, ProbeRequest, ProbeResponse,
         PublishVolumeReply, PublishVolumeRequest, RepublishVolumeReply, RepublishVolumeRequest,
         SetVolumeReplicaReply, SetVolumeReplicaRequest, ShareVolumeReply, ShareVolumeRequest,
@@ -218,14 +218,14 @@ impl VolumeGrpc for VolumeServer {
         }
     }
 
-    async fn delete_snapshot(
+    async fn destroy_snapshot(
         &self,
-        request: tonic::Request<DeleteSnapshotRequest>,
-    ) -> Result<tonic::Response<DeleteSnapshotReply>, tonic::Status> {
+        request: tonic::Request<DestroySnapshotRequest>,
+    ) -> Result<tonic::Response<DestroySnapshotReply>, tonic::Status> {
         let req = request.into_inner().validated()?;
-        match self.service.delete_snapshot(&req, None).await {
-            Ok(()) => Ok(Response::new(DeleteSnapshotReply { error: None })),
-            Err(e) => Ok(Response::new(DeleteSnapshotReply {
+        match self.service.destroy_snapshot(&req, None).await {
+            Ok(()) => Ok(Response::new(DestroySnapshotReply { error: None })),
+            Err(e) => Ok(Response::new(DestroySnapshotReply {
                 error: Some(e.into()),
             })),
         }

diff --git a/control-plane/grpc/src/operations/volume/traits.rs b/control-plane/grpc/src/operations/volume/traits.rs
@@ -107,9 +107,9 @@ pub trait VolumeOperations: Send + Sync {
         ctx: Option<Context>,
     ) -> Result<VolumeSnapshot, ReplyError>;
     /// Delete a volume snapshot.
-    async fn delete_snapshot(
+    async fn destroy_snapshot(
         &self,
-        request: &dyn DeleteVolumeSnapshotInfo,
+        request: &dyn DestroyVolumeSnapshotInfo,
         ctx: Option<Context>,
     ) -> Result<(), ReplyError>;
     /// List volume snapshots.

diff --git a/control-plane/grpc/src/operations/volume/traits_snapshots.rs b/control-plane/grpc/src/operations/volume/traits_snapshots.rs
@@ -30,7 +30,7 @@ pub trait CreateVolumeSnapshotInfo: Send + Sync + std::fmt::Debug {
 }
 
 /// Volume snapshot deletion information.
-pub trait DeleteVolumeSnapshotInfo: Send + Sync + std::fmt::Debug {
+pub trait DestroyVolumeSnapshotInfo: Send + Sync + std::fmt::Debug {
     /// Snapshot creation information.
     fn info(&self) -> SnapshotInfo<Option<VolumeId>>;
 }
@@ -66,7 +66,7 @@ impl VolumeSnapshot {
     }
 }
 
-impl From<&VolumeSnapshot> for DeleteVolumeSnapshot {
+impl From<&VolumeSnapshot> for DestroyVolumeSnapshot {
     fn from(snapshot: &VolumeSnapshot) -> Self {
         Self::new(
             &Some(snapshot.spec().source_id.clone()),
@@ -343,7 +343,7 @@ impl VolumeSnapshots {
 pub type CreateVolumeSnapshot = SnapshotInfo<VolumeId>;
 
 /// Validated delete volume snapshot parameters.
-pub type DeleteVolumeSnapshot = SnapshotInfo<Option<VolumeId>>;
+pub type DestroyVolumeSnapshot = SnapshotInfo<Option<VolumeId>>;
 
 impl ValidateRequestTypes for volume::CreateSnapshotRequest {
     type Validated = CreateVolumeSnapshot;
@@ -358,10 +358,10 @@ impl ValidateRequestTypes for volume::CreateSnapshotRequest {
         })
     }
 }
-impl ValidateRequestTypes for volume::DeleteSnapshotRequest {
-    type Validated = DeleteVolumeSnapshot;
+impl ValidateRequestTypes for volume::DestroySnapshotRequest {
+    type Validated = DestroyVolumeSnapshot;
     fn validated(self) -> Result<Self::Validated, ReplyError> {
-        Ok(DeleteVolumeSnapshot {
+        Ok(DestroyVolumeSnapshot {
             source_id: match self.volume_id {
                 None => None,
                 Some(id) => Some(id.try_into_id(ResourceKind::VolumeSnapshot, "volume_id")?),
@@ -378,8 +378,8 @@ impl CreateVolumeSnapshotInfo for CreateVolumeSnapshot {
         self.clone()
     }
 }
-impl DeleteVolumeSnapshotInfo for DeleteVolumeSnapshot {
-    fn info(&self) -> DeleteVolumeSnapshot {
+impl DestroyVolumeSnapshotInfo for DestroyVolumeSnapshot {
+    fn info(&self) -> DestroyVolumeSnapshot {
         self.clone()
     }
 }
@@ -393,8 +393,8 @@ impl From<&dyn CreateVolumeSnapshotInfo> for volume::CreateSnapshotRequest {
         }
     }
 }
-impl From<&dyn DeleteVolumeSnapshotInfo> for volume::DeleteSnapshotRequest {
-    fn from(value: &dyn DeleteVolumeSnapshotInfo) -> Self {
+impl From<&dyn DestroyVolumeSnapshotInfo> for volume::DestroySnapshotRequest {
+    fn from(value: &dyn DestroyVolumeSnapshotInfo) -> Self {
         let info = value.info();
         Self {
             volume_id: info.source_id.map(|id| id.to_string()),

diff --git a/control-plane/rest/service/src/v0/snapshots.rs b/control-plane/rest/service/src/v0/snapshots.rs
@@ -1,7 +1,7 @@
 use super::*;
 use grpc::operations::{
     volume::traits::{
-        CreateVolumeSnapshot, DeleteVolumeSnapshot, ReplicaSnapshot, VolumeOperations,
+        CreateVolumeSnapshot, DestroyVolumeSnapshot, ReplicaSnapshot, VolumeOperations,
         VolumeReplicaSnapshotState, VolumeSnapshot,
     },
     MaxEntries, Pagination, StartingToken,
@@ -18,8 +18,8 @@ fn client() -> impl VolumeOperations {
 impl apis::actix_server::Snapshots for RestApi {
     async fn del_snapshot(Path(snapshot_id): Path<Uuid>) -> Result<(), RestError<RestJsonError>> {
         client()
-            .delete_snapshot(
-                &DeleteVolumeSnapshot {
+            .destroy_snapshot(
+                &DestroyVolumeSnapshot {
                     source_id: None,
                     snap_id: snapshot_id.into(),
                 },
@@ -33,8 +33,8 @@ impl apis::actix_server::Snapshots for RestApi {
         Path((volume_id, snapshot_id)): Path<(Uuid, Uuid)>,
     ) -> Result<(), RestError<RestJsonError>> {
         client()
-            .delete_snapshot(
-                &DeleteVolumeSnapshot {
+            .destroy_snapshot(
+                &DestroyVolumeSnapshot {
                     source_id: Some(volume_id.into()),
                     snap_id: snapshot_id.into(),
                 },

diff --git a/control-plane/stor-port/src/transport_api/mod.rs b/control-plane/stor-port/src/transport_api/mod.rs
@@ -528,6 +528,11 @@ impl RequestMinTimeout {
     pub fn nexus_snapshot(&self) -> Duration {
         self.nexus_snapshot
     }
+    /// Minimum timeout for a volume snapshot operation.
+    pub fn volume_snapshot(&self) -> Duration {
+        // not quite sure how much slack to give here, maybe this is enough?
+        self.nexus_snapshot + self.replica_snapshot
+    }
     /// Minimum timeout for a pool operation.
     pub fn pool(&self) -> Duration {
         self.pool

diff --git a/control-plane/stor-port/src/types/v0/transport/mod.rs b/control-plane/stor-port/src/types/v0/transport/mod.rs
@@ -136,7 +136,7 @@ pub enum MessageIdVs {
     /// Create volume snapshot.
     CreateVolumeSnapshot,
     /// Delete volume snapshot.
-    DeleteVolumeSnapshot,
+    DestroyVolumeSnapshot,
     /// Get volume snapshots.
     GetVolumeSnapshots,
     /// Generic JSON gRPC message.