Merge #1476

1476: fix(lvs/bdev): don't show nil uuids r=tiagolobocastro a=tiagolobocastro Recent spdk change made all bdev have nil uuid by default. This makes list-pools a bit confusing as shows device with nil uuid, and at the same time pool also has its own uuid. So let's not add nil uuid to the URI. todo: Should we set the bdev uuid to the lvs pool uuid? Co-authored-by: Tiago Castro <[email protected]>
openebs · Jul 27, 2023 · aeb3aae · aeb3aae
2 parents e5d8339 + 5dbca87
commit aeb3aae
Show file tree

Hide file tree

Showing 10 changed files with 51 additions and 39 deletions.
diff --git a/.github/workflows/dco-bors.yml b/.github/workflows/dco-bors.yml
@@ -0,0 +1,12 @@
+name: DCO
+on:
+  push:
+    branches:
+      - staging
+
+jobs:
+  DCO:
+    runs-on: ubuntu-latest
+    if: ${{ github.actor == 'bors[bot]' }}
+    steps:
+      - run: echo "dummy DCO workflow (it won't run any check actually)"
diff --git a/io-engine/src/bdev/nexus/nexus_share.rs b/io-engine/src/bdev/nexus/nexus_share.rs
@@ -94,12 +94,12 @@ impl<'n> Share for Nexus<'n> {
     }
 
     /// TODO
-    fn bdev_uri(&self) -> Option<String> {
+    fn bdev_uri(&self) -> Option<url::Url> {
         unsafe { self.bdev().bdev_uri() }
     }
 
     /// TODO
-    fn bdev_uri_original(&self) -> Option<String> {
+    fn bdev_uri_original(&self) -> Option<url::Url> {
         unsafe { self.bdev().bdev_uri_original() }
     }
 }

diff --git a/io-engine/src/bdev_api.rs b/io-engine/src/bdev_api.rs
@@ -4,7 +4,10 @@ use snafu::Snafu;
 use std::{convert::TryFrom, num::ParseIntError, str::ParseBoolError};
 use url::ParseError;
 
-use crate::{bdev::uri, core::Bdev};
+use crate::{
+    bdev::uri,
+    core::{Bdev, Share},
+};
 
 // parse URI and bdev create/destroy errors common for all types of bdevs
 #[derive(Debug, Snafu, Clone)]
@@ -152,19 +155,7 @@ where
     type Error = BdevError;
 
     fn try_from(bdev: Bdev<T>) -> Result<Self, Self::Error> {
-        for alias in bdev.aliases().iter() {
-            if let Ok(mut uri) = url::Url::parse(alias) {
-                if bdev_uri_eq(&bdev, &uri) {
-                    if !uri.query_pairs().any(|e| e.0 == "uuid") {
-                        uri.query_pairs_mut()
-                            .append_pair("uuid", &bdev.uuid_as_string());
-                    }
-                    return Ok(uri);
-                }
-            }
-        }
-
-        Err(BdevError::BdevNoMatchingUri {
+        bdev.bdev_uri().ok_or(BdevError::BdevNoMatchingUri {
             name: bdev.name().to_string(),
             aliases: bdev.aliases(),
         })

diff --git a/io-engine/src/core/bdev.rs b/io-engine/src/core/bdev.rs
@@ -279,27 +279,24 @@ where
     }
 
     /// return the URI that was used to construct the bdev
-    fn bdev_uri(&self) -> Option<String> {
-        for alias in self.aliases().iter() {
-            if let Ok(mut uri) = url::Url::parse(alias) {
-                if bdev_uri_eq(self, &uri) {
-                    if !uri.query_pairs().any(|e| e.0 == "uuid") {
-                        uri.query_pairs_mut()
-                            .append_pair("uuid", &self.uuid_as_string());
-                    }
-                    return Some(uri.to_string());
-                }
+    fn bdev_uri(&self) -> Option<url::Url> {
+        self.bdev_uri_original().map(|mut uri| {
+            if !uri.query_pairs().any(|e| e.0 == "uuid")
+                && !self.uuid().is_nil()
+            {
+                uri.query_pairs_mut()
+                    .append_pair("uuid", &self.uuid_as_string());
             }
-        }
-        None
+            uri
+        })
     }
 
     /// return the URI that was used to construct the bdev, without uuid
-    fn bdev_uri_original(&self) -> Option<String> {
+    fn bdev_uri_original(&self) -> Option<url::Url> {
         for alias in self.aliases().iter() {
             if let Ok(uri) = url::Url::parse(alias) {
                 if bdev_uri_eq(self, &uri) {
-                    return Some(uri.to_string());
+                    return Some(uri);
                 }
             }
         }

diff --git a/io-engine/src/core/share.rs b/io-engine/src/core/share.rs
@@ -192,8 +192,14 @@ pub trait Share: std::fmt::Debug {
     fn allowed_hosts(&self) -> Vec<String>;
 
     /// TODO
-    fn bdev_uri(&self) -> Option<String>;
+    fn bdev_uri(&self) -> Option<url::Url>;
+    fn bdev_uri_str(&self) -> Option<String> {
+        self.bdev_uri().map(|uri| uri.to_string())
+    }
 
     /// TODO
-    fn bdev_uri_original(&self) -> Option<String>;
+    fn bdev_uri_original(&self) -> Option<url::Url>;
+    fn bdev_uri_original_str(&self) -> Option<String> {
+        self.bdev_uri_original().map(|uri| uri.to_string())
+    }
 }
diff --git a/io-engine/src/grpc/v0/mayastor_grpc.rs b/io-engine/src/grpc/v0/mayastor_grpc.rs
@@ -297,7 +297,10 @@ impl From<Lvs> for Pool {
     fn from(l: Lvs) -> Self {
         Self {
             name: l.name().into(),
-            disks: vec![l.base_bdev().bdev_uri().unwrap_or_else(|| "".into())],
+            disks: vec![l
+                .base_bdev()
+                .bdev_uri_str()
+                .unwrap_or_else(|| "".into())],
             state: PoolState::PoolOnline.into(),
             capacity: l.capacity(),
             used: l.used(),

diff --git a/io-engine/src/grpc/v1/pool.rs b/io-engine/src/grpc/v1/pool.rs
@@ -140,7 +140,10 @@ impl From<Lvs> for Pool {
         Self {
             uuid: l.uuid(),
             name: l.name().into(),
-            disks: vec![l.base_bdev().bdev_uri().unwrap_or_else(|| "".into())],
+            disks: vec![l
+                .base_bdev()
+                .bdev_uri_str()
+                .unwrap_or_else(|| "".into())],
             state: PoolState::PoolOnline.into(),
             capacity: l.capacity(),
             used: l.used(),

diff --git a/io-engine/src/lvs/lvs_lvol.rs b/io-engine/src/lvs/lvs_lvol.rs
@@ -248,11 +248,11 @@ impl Share for Lvol {
     /// returns the URI that is used to construct the bdev. This is always None
     /// as lvols can not be created by URIs directly, but only through the
     /// ['Lvs'] interface.
-    fn bdev_uri(&self) -> Option<String> {
+    fn bdev_uri(&self) -> Option<url::Url> {
         None
     }
 
-    fn bdev_uri_original(&self) -> Option<String> {
+    fn bdev_uri_original(&self) -> Option<url::Url> {
         None
     }
 }

diff --git a/io-engine/src/lvs/lvs_store.rs b/io-engine/src/lvs/lvs_store.rs
@@ -539,7 +539,7 @@ impl Lvs {
 
         info!("{}: lvs exported successfully", self_str);
 
-        bdev_destroy(&base_bdev.bdev_uri_original().unwrap())
+        bdev_destroy(&base_bdev.bdev_uri_original_str().unwrap())
             .await
             .map_err(|e| Error::Destroy {
                 source: e,
@@ -633,7 +633,7 @@ impl Lvs {
 
         info!("{}: lvs destroyed successfully", self_str);
 
-        bdev_destroy(&base_bdev.bdev_uri_original().unwrap())
+        bdev_destroy(&base_bdev.bdev_uri_original_str().unwrap())
             .await
             .map_err(|e| Error::Destroy {
                 source: e,

diff --git a/io-engine/src/subsys/config/pool.rs b/io-engine/src/subsys/config/pool.rs
@@ -185,7 +185,7 @@ impl From<LvsBdev> for Pool {
         Self {
             name: lvs_bdev.name(),
             disks: vec![base
-                .bdev_uri()
+                .bdev_uri_str()
                 .unwrap_or_else(|| base.name().to_string())],
             replicas: None,
         }