chore: add canonical pcie, add regex to stop non persistent devlinks

Signed-off-by: Abhinandan Purkait <[email protected]>

Author: Abhinandan-Purkait

Cargo.lock

@@ -58,6 +58,7 @@ tracing = "0.1.40"
 nix = { version = "0.29.0", default-features = false }
 prost-types = "0.13.3"
 url = "2.5.4"
+regex = "1.11.1"
 
 grpc = { path = "../grpc" }
 shutdown = { path = "../../utils/shutdown" }

control-plane/agents/Cargo.toml

@@ -114,6 +114,8 @@ pub(crate) struct RegistryInner<S: Store> {
     ha_disabled: bool,
     /// Etcd max page size.
     etcd_max_page_size: i64,
+    /// Allow pool creation using non-persistent devlinks.
+    allow_non_persistent_devlinks: bool,
 }
 
 impl Registry {
@@ -140,6 +142,7 @@ impl Registry {
         ha_enabled: bool,
         etcd_max_page_size: i64,
         no_volume_health: bool,
+        allow_non_persistent_devlinks: bool,
     ) -> Result<Self, SvcError> {
         let store_endpoint = Self::format_store_endpoint(&store_url);
         tracing::info!("Connecting to persistent store at {}", store_endpoint);
@@ -199,6 +202,7 @@ impl Registry {
                 thin_args,
                 ha_disabled: ha_enabled,
                 etcd_max_page_size,
+                allow_non_persistent_devlinks,
             }),
         };
         registry.init().await?;
@@ -230,6 +234,11 @@ impl Registry {
         self.ha_disabled
     }
 
+    /// Check if pool creation using non-persistent devlink is allowed.
+    pub(crate) fn allow_non_persistent_devlinks(&self) -> bool {
+        self.allow_non_persistent_devlinks
+    }
+
     /// Check if the partial rebuilds are disabled.
     pub(crate) fn partial_rebuild_disabled(&self) -> bool {
         self.disable_partial_rebuild

control-plane/agents/src/bin/core/controller/registry.rs

@@ -142,6 +142,10 @@ pub(crate) struct CliArgs {
     /// Disable volume health reporting which uses pstor watchers.
     #[clap(long)]
     no_volume_health: bool,
+
+    /// Allow pools to be created using non persistent devlinks.
+    #[clap(long)]
+    allow_non_persistent_devlink: bool,
 }
 impl CliArgs {
     fn args() -> Self {
@@ -219,6 +223,7 @@ async fn server(cli_args: CliArgs) -> anyhow::Result<()> {
         cli_args.disable_ha,
         cli_args.etcd_page_limit as i64,
         cli_args.no_volume_health,
+        cli_args.allow_non_persistent_devlink,
     )
     .await?;
 

control-plane/agents/src/bin/core/main.rs

@@ -24,6 +24,7 @@ use stor_port::types::v0::{
 };
 
 use itertools::Itertools;
+use regex::Regex;
 use snafu::OptionExt;
 use std::collections::HashSet;
 use std::ops::Deref;
@@ -151,6 +152,22 @@ pub(crate) async fn devlink_preflight_checks(
         .map(|disk| utils::disk::normalize_disk(disk.as_str()))
         .collect();
 
+    if !registry.allow_non_persistent_devlinks() {
+        fn is_persistent_devlink(pattern: &str) -> Result<bool, SvcError> {
+            let re = Regex::new(utils::DEVLINK_REGEX).map_err(|_| SvcError::InvalidArguments {})?;
+            Ok(re.is_match(pattern))
+        }
+
+        if request_disks
+            .iter()
+            // Only attempt to validate if it starts with "/dev".
+            .filter(|disk| disk.starts_with("/dev"))
+            .any(|disk| !is_persistent_devlink(disk).is_ok_and(|val| val))
+        {
+            return Err(SvcError::InvalidDevlink {});
+        }
+    }
+
     let node_pools = registry
         .get_node_opt_pools(Some(request.node.clone()))
         .await?;

control-plane/agents/src/bin/core/pool/operations_helper.rs

@@ -44,7 +44,8 @@ impl ResourceLifecycle for OperationGuardArc<PoolSpec> {
             });
         }
 
-        if let Ok(pool) = registry.specs().pool(&request.id) {
+        let pool_get_result = registry.specs().pool(&request.id);
+        if let Ok(pool) = &pool_get_result {
             if pool.status.created() {
                 return Err(SvcError::AlreadyExists {
                     kind: ResourceKind::Pool,
@@ -61,8 +62,10 @@ impl ResourceLifecycle for OperationGuardArc<PoolSpec> {
             });
         }
 
-        // If the devlink for a device is used for multiple pools, reject new creation.
-        devlink_preflight_checks(request, node.clone(), registry).await?;
+        if pool_get_result.is_err() {
+            // If the devlink for a device is used for multiple pools, reject new creation.
+            devlink_preflight_checks(request, node.clone(), registry).await?
+        }
 
         let mut pool = specs
             .get_or_create_pool(request)

control-plane/agents/src/bin/core/pool/pool_operations.rs

@@ -7,6 +7,7 @@ use grpc::{
     },
 };
 use itertools::Itertools;
+use regex::Regex;
 use std::{collections::HashMap, convert::TryFrom, thread::sleep, time::Duration};
 use stor_port::types::v0::store::pool::{Encryption, EncryptionSecret};
 use stor_port::types::v0::transport::{GetBlockDevices, NodeStatus};
@@ -1347,3 +1348,34 @@ async fn reject_devlink_reuse() {
         }
     }
 }
+
+#[tokio::test]
+async fn persistent_devlink_regex() {
+    fn is_persistent_devlink(pattern: &str) -> bool {
+        let re = Regex::new(utils::DEVLINK_REGEX).expect("DEVLINK_REGEX should be valid");
+        re.is_match(pattern)
+    }
+
+    let test_suite = [
+        ("/dev/some/path", false),
+        ("/dev/mapper/some/path", false),
+        ("/dev/disk/some/path", false),
+        ("/dev/disk/by-something/path", false),
+        ("/devil/disk/by-something/path", false),
+        ("/dev/diskxyz/by-something/path", false),
+        ("/dev/disk/something-by/path", false),
+        ("/dev/disk/by-/path", false),
+        ("something", false),
+        ("/dev/somevg/lv0", false), // LVM Paths are to be used via the /dev/mapper or by /dev/disk/by-id
+        ("/dev/disk/by-id/somepath", true),
+        ("/dev/disk/by-path/somepath", true),
+        ("/dev/disk/by-label/somepath", true),
+        ("/dev/disk/by-partuuid/somepath", true),
+        ("/dev/disk/by-partlabel/somepath", true),
+        ("/dev/mapper/dm0", true),
+    ];
+
+    for test in test_suite {
+        assert_eq!(is_persistent_devlink(test.0), test.1);
+    }
+}

control-plane/agents/src/bin/core/tests/pool/mod.rs

@@ -420,6 +420,8 @@ pub enum SvcError {
         replica: String,
         source: tonic::Status,
     },
+    #[snafu(display("Invalid devlink, use a persistent devlink for pool creation"))]
+    InvalidDevlink {},
 }
 
 impl SvcError {
@@ -1111,6 +1113,12 @@ impl From<SvcError> for ReplyError {
                 source,
                 extra,
             },
+            SvcError::InvalidDevlink { .. } => ReplyError {
+                kind: ReplyErrorKind::InvalidArgument,
+                resource: ResourceKind::Block,
+                source,
+                extra,
+            },
         }
     }
 }

control-plane/agents/src/common/errors.rs

@@ -369,11 +369,13 @@ mod test {
             "uring:///dev/null",
             "uring://dev/null", // this URL is invalid
             "pcie:///0000:01:00.0",
+            "pcie:///00:01:00.0",
         ];
 
         assert_eq!(utils::disk::normalize_disk(disks[0]), "/dev/null");
         assert_eq!(utils::disk::normalize_disk(disks[1]), "/dev/null");
         assert_eq!(utils::disk::normalize_disk(disks[2]), "uring://dev/null");
         assert_eq!(utils::disk::normalize_disk(disks[3]), "/0000:01:00.0");
+        assert_eq!(utils::disk::normalize_disk(disks[4]), "/0000:01:00.0");
     }
 }

k8s/operators/src/pool/main.rs

@@ -125,7 +125,9 @@ def disks():
 
 @pytest.fixture
 def attempt_create_pool_nonexistent_disk(context, attempt_create_pool):
-    context["attempt"] = attempt_create_pool(POOL_NAME, POOL_NODE, ["/dev/no_there"])
+    context["attempt"] = attempt_create_pool(
+        POOL_NAME, POOL_NODE, ["/dev/disk/by-id/no_there"]
+    )
 
 
 @pytest.fixture