Merge #1505

1505: Lvs and PCIe fixes r=avishnu a=tiagolobocastro fix(pcie): spdk requires nvme path id When destroying a pcie nvme bdev we must not specify the nvme path id. Also it was noticed that when destroy fails we end up with no alias which could cause some panics. For this particular case we now add the alias back if the bdev is still present though we need a more robust fix.. Signed-off-by: Tiago Castro <[email protected]> --- fix(lvs/error): handle missing errors during import/create Some error codes not being properly mapped or handled making it impossible to create or import a pool in certain situations. This ensures we handle errors like already exists properly which allows the control-plane to ensure pool creation/import. Signed-off-by: Tiago Castro <[email protected]> Co-authored-by: Tiago Castro <[email protected]>
openebs · Aug 28, 2023 · e6e66c0 · e6e66c0
2 parents fca74e0 + 43f2d5a
commit e6e66c0
Show file tree

Hide file tree

Showing 7 changed files with 69 additions and 21 deletions.
diff --git a/io-engine/src/bdev/nexus/nexus_bdev_error.rs b/io-engine/src/bdev/nexus/nexus_bdev_error.rs
@@ -294,6 +294,9 @@ impl From<Error> for tonic::Status {
             Error::NameExists {
                 ..
             } => Status::already_exists(e.to_string()),
+            Error::InvalidArguments {
+                ..
+            } => Status::invalid_argument(e.to_string()),
             e => Status::new(Code::Internal, e.verbose()),
         }
     }

diff --git a/io-engine/src/bdev/nvme.rs b/io-engine/src/bdev/nvme.rs
@@ -11,7 +11,12 @@ use url::Url;
 
 use spdk_rs::{
     ffihelper::copy_str_with_null,
-    libspdk::{bdev_nvme_create, bdev_nvme_delete, spdk_nvme_transport_id},
+    libspdk::{
+        bdev_nvme_create,
+        bdev_nvme_delete,
+        nvme_path_id,
+        spdk_nvme_transport_id,
+    },
 };
 
 use crate::{
@@ -130,12 +135,21 @@ impl CreateDestroy for NVMe {
     async fn destroy(self: Box<Self>) -> Result<(), Self::Error> {
         if let Some(mut bdev) = UntypedBdev::lookup_by_name(&self.get_name()) {
             bdev.remove_alias(self.url.as_ref());
+
+            let mut path_id = nvme_path_id::default();
+            copy_str_with_null(&self.name, &mut path_id.trid.traddr);
+            path_id.trid.trtype = spdk_rs::libspdk::SPDK_NVME_TRANSPORT_PCIE;
+
             let errno = unsafe {
                 bdev_nvme_delete(
                     self.name.clone().into_cstring().as_ptr(),
-                    std::ptr::null(),
+                    &path_id,
                 )
             };
+            if errno != 0 {
+                UntypedBdev::lookup_by_name(&self.get_name())
+                    .map(|mut b| b.add_alias(self.url.as_ref()));
+            }
             errno_result_from_i32((), errno).context(
                 bdev_api::DestroyBdevFailed {
                     name: self.name.clone(),

diff --git a/io-engine/src/grpc/mod.rs b/io-engine/src/grpc/mod.rs
@@ -6,6 +6,7 @@ use std::{
 };
 
 use futures::channel::oneshot::Receiver;
+use nix::errno::Errno;
 pub use server::MayastorGrpcServer;
 use tonic::{Request, Response, Status};
 
@@ -26,6 +27,20 @@ impl From<BdevError> for tonic::Status {
             BdevError::InvalidUri {
                 ..
             } => Status::invalid_argument(e.to_string()),
+            BdevError::IntParamParseFailed {
+                ..
+            } => Status::invalid_argument(e.to_string()),
+            BdevError::BoolParamParseFailed {
+                ..
+            } => Status::invalid_argument(e.to_string()),
+            BdevError::CreateBdevInvalidParams {
+                source, ..
+            } => match source {
+                Errno::EINVAL => Status::invalid_argument(e.to_string()),
+                Errno::ENOENT => Status::not_found(e.to_string()),
+                Errno::EEXIST => Status::already_exists(e.to_string()),
+                _ => Status::invalid_argument(e.to_string()),
+            },
             e => Status::internal(e.to_string()),
         }
     }

diff --git a/io-engine/src/grpc/v0/mayastor_grpc.rs b/io-engine/src/grpc/v0/mayastor_grpc.rs
@@ -218,8 +218,12 @@ impl From<LvsError> for tonic::Status {
     fn from(e: LvsError) -> Self {
         match e {
             LvsError::Import {
-                ..
-            } => Status::invalid_argument(e.to_string()),
+                source, ..
+            } => match source {
+                Errno::EINVAL => Status::invalid_argument(e.to_string()),
+                Errno::EEXIST => Status::already_exists(e.to_string()),
+                _ => Status::invalid_argument(e.to_string()),
+            },
             LvsError::RepCreate {
                 source, ..
             } => {

diff --git a/io-engine/src/lvs/lvs_error.rs b/io-engine/src/lvs/lvs_error.rs
@@ -11,22 +11,22 @@ use crate::{
 #[derive(Debug, Snafu)]
 #[snafu(visibility(pub(crate)), context(suffix(false)))]
 pub enum Error {
-    #[snafu(display("failed to import pool {}", name))]
+    #[snafu(display("{source}, failed to import pool {name}"))]
     Import {
         source: Errno,
         name: String,
     },
-    #[snafu(display("errno: {} failed to create pool {}", source, name))]
+    #[snafu(display("{source}, failed to create pool {name}"))]
     PoolCreate {
         source: Errno,
         name: String,
     },
-    #[snafu(display("failed to export pool {}", name))]
+    #[snafu(display("{source}, failed to export pool {name}"))]
     Export {
         source: Errno,
         name: String,
     },
-    #[snafu(display("failed to destroy pool {}", name))]
+    #[snafu(display("{source}, failed to destroy pool {name}"))]
     Destroy {
         source: BdevError,
         name: String,

diff --git a/io-engine/src/lvs/lvs_store.rs b/io-engine/src/lvs/lvs_store.rs
@@ -290,7 +290,7 @@ impl Lvs {
                 lvs.name()
             );
             let pool_name = lvs.name().to_string();
-            lvs.export().await.unwrap();
+            lvs.export().await?;
             Err(Error::Import {
                 source: Errno::EINVAL,
                 name: pool_name,
@@ -333,10 +333,16 @@ impl Lvs {
                 BdevError::BdevExists {
                     ..
                 } => Ok(parsed.get_name()),
-                _ => Err(Error::InvalidBdev {
-                    source: e,
-                    name: args.disks[0].clone(),
-                }),
+                BdevError::CreateBdevInvalidParams {
+                    source, ..
+                } if source == Errno::EEXIST => Ok(parsed.get_name()),
+                _ => {
+                    tracing::error!("Failed to create pool bdev: {e:?}");
+                    Err(Error::InvalidBdev {
+                        source: e,
+                        name: args.disks[0].clone(),
+                    })
+                }
             },
             Ok(name) => Ok(name),
         }?;
@@ -468,10 +474,16 @@ impl Lvs {
                 BdevError::BdevExists {
                     ..
                 } => Ok(parsed.get_name()),
-                _ => Err(Error::InvalidBdev {
-                    source: e,
-                    name: args.disks[0].clone(),
-                }),
+                BdevError::CreateBdevInvalidParams {
+                    source, ..
+                } if source == Errno::EEXIST => Ok(parsed.get_name()),
+                _ => {
+                    tracing::error!("Failed to create pool bdev: {e:?}");
+                    Err(Error::InvalidBdev {
+                        source: e,
+                        name: args.disks[0].clone(),
+                    })
+                }
             },
             Ok(name) => Ok(name),
         }?;
@@ -541,7 +553,7 @@ impl Lvs {
 
         info!("{}: lvs exported successfully", self_str);
 
-        bdev_destroy(&base_bdev.bdev_uri_original_str().unwrap())
+        bdev_destroy(&base_bdev.bdev_uri_original_str().unwrap_or_default())
             .await
             .map_err(|e| Error::Destroy {
                 source: e,
@@ -635,7 +647,7 @@ impl Lvs {
 
         info!("{}: lvs destroyed successfully", self_str);
 
-        bdev_destroy(&base_bdev.bdev_uri_original_str().unwrap())
+        bdev_destroy(&base_bdev.bdev_uri_original_str().unwrap_or_default())
             .await
             .map_err(|e| Error::Destroy {
                 source: e,

diff --git a/test/grpc/test_nexus.js b/test/grpc/test_nexus.js
@@ -779,7 +779,7 @@ describe('nexus', function () {
       };
       client.createNexusV2(args, (err) => {
         if (!err) return done(new Error('Expected error'));
-        assert.equal(err.code, grpc.status.INTERNAL);
+        assert.equal(err.code, grpc.status.INVALID_ARGUMENT);
         done();
       });
     });
@@ -799,7 +799,7 @@ describe('nexus', function () {
       };
       client.createNexusV2(args, (err) => {
         if (!err) return done(new Error('Expected error'));
-        assert.equal(err.code, grpc.status.INTERNAL);
+        assert.equal(err.code, grpc.status.INVALID_ARGUMENT);
         done();
       });
     });