Re: [pbs-devel] [PATCH proxmox-backup v12 09/26] api: disks list: add exclude-used flag

["Hannes Laimer" <h.laimer@proxmox.com>]

I think we could even drop this completely, given we allow multiple
datastores on the same device, there isn't really any other reason for
this patch.

On Mon Oct 14, 2024 at 3:42 PM CEST, Fabian Grünbichler wrote:
> On September 4, 2024 4:11 pm, Hannes Laimer wrote:
> > ... used by the partition selector for removable datastore creation.
> > 
> > Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
> > ---
> >  src/api2/node/disks/mod.rs |  8 +++++
> >  src/tools/disks/mod.rs     | 60 ++++++++++++++++++++++++++++++++------
> >  2 files changed, 59 insertions(+), 9 deletions(-)
> > 
> > diff --git a/src/api2/node/disks/mod.rs b/src/api2/node/disks/mod.rs
> > index 4ef4ee2b..5bb7eb94 100644
> > --- a/src/api2/node/disks/mod.rs
> > +++ b/src/api2/node/disks/mod.rs
> > @@ -41,6 +41,12 @@ pub mod zfs;
> >                  optional: true,
> >                  default: false,
> >              },
> > +            "exclude-used": {
> > +                description: "Exclude partitions already used for removable datastores or mounted directories.",
>
> see comment below..
>
> > +                type: bool,
> > +                optional: true,
> > +                default: false,
> > +            },
> >              "usage-type": {
> >                  type: DiskUsageType,
> >                  optional: true,
> > @@ -62,6 +68,7 @@ pub mod zfs;
> >  pub fn list_disks(
> >      skipsmart: bool,
> >      include_partitions: bool,
> > +    exclude_used: bool,
> >      usage_type: Option<DiskUsageType>,
> >  ) -> Result<Vec<DiskUsageInfo>, Error> {
> >      let mut list = Vec::new();
> > @@ -69,6 +76,7 @@ pub fn list_disks(
> >      for (_, info) in DiskUsageQuery::new()
> >          .smart(!skipsmart)
> >          .partitions(include_partitions)
> > +        .exclude_used(exclude_used)
>
> see comment below..
>
> >          .query()?
> >      {
> >          if let Some(ref usage_type) = usage_type {
> > diff --git a/src/tools/disks/mod.rs b/src/tools/disks/mod.rs
> > index 93c1a882..8d84e312 100644
> > --- a/src/tools/disks/mod.rs
> > +++ b/src/tools/disks/mod.rs
> > @@ -30,6 +30,7 @@ pub use zpool_list::*;
> >  mod lvm;
> >  pub use lvm::*;
> >  mod smart;
> > +use crate::api2::node::disks::directory::list_datastore_mounts;
>
> nit: wrong sorting of use statements, but also wrong direction of usage?
> tools shouldn't use api code, but the other way round..
>
> it would probably be cleaner to instead pass in a list of excluded
> devices/mountpoints/..?
>
> >  pub use smart::*;
> >  
> >  static ISCSI_PATH_REGEX: LazyLock<regex::Regex> =
> > @@ -824,6 +825,7 @@ fn scan_partitions(
> >  pub struct DiskUsageQuery {
> >      smart: bool,
> >      partitions: bool,
> > +    exclude_used: bool,
> >  }
> >  
> >  impl DiskUsageQuery {
> > @@ -831,6 +833,7 @@ impl DiskUsageQuery {
> >          Self {
> >              smart: true,
> >              partitions: false,
> > +            exclude_used: false,
> >          }
> >      }
> >  
> > @@ -844,12 +847,22 @@ impl DiskUsageQuery {
> >          self
> >      }
> >  
> > +    pub fn exclude_used(&mut self, exclude_used: bool) -> &mut Self {
> > +        self.exclude_used = exclude_used;
> > +        self
> > +    }
> > +
> >      pub fn query(&self) -> Result<HashMap<String, DiskUsageInfo>, Error> {
> > -        get_disks(None, !self.smart, self.partitions)
> > +        get_disks(None, !self.smart, self.partitions, self.exclude_used)
> >      }
> >  
> >      pub fn find(&self, disk: &str) -> Result<DiskUsageInfo, Error> {
> > -        let mut map = get_disks(Some(vec![disk.to_string()]), !self.smart, self.partitions)?;
> > +        let mut map = get_disks(
> > +            Some(vec![disk.to_string()]),
> > +            !self.smart,
> > +            self.partitions,
> > +            self.exclude_used,
> > +        )?;
> >          if let Some(info) = map.remove(disk) {
> >              Ok(info)
> >          } else {
> > @@ -858,7 +871,7 @@ impl DiskUsageQuery {
> >      }
> >  
> >      pub fn find_all(&self, disks: Vec<String>) -> Result<HashMap<String, DiskUsageInfo>, Error> {
> > -        get_disks(Some(disks), !self.smart, self.partitions)
> > +        get_disks(Some(disks), !self.smart, self.partitions, self.exclude_used)
> >      }
> >  }
> >  
> > @@ -931,6 +944,8 @@ fn get_disks(
> >      no_smart: bool,
> >      // include partitions
> >      include_partitions: bool,
> > +    // skip partitions which are in use
> > +    exclude_used: bool,
> >  ) -> Result<HashMap<String, DiskUsageInfo>, Error> {
> >      let disk_manager = DiskManage::new();
> >  
> > @@ -948,6 +963,31 @@ fn get_disks(
> >  
> >      // fixme: ceph journals/volumes
> >  
> > +    let uuids_in_use = if exclude_used && include_partitions {
>
> this here is tricky, if exclusion only matters for partitions that
> should be mentioned somewhere.. but then again, why is that the case?
> couldn't a removable datastore be directly on a disk without partitions?
>
> > +        let (config, _digest) = pbs_config::datastore::config()?;
> > +
> > +        let uuids_from_datastores: Vec<String> = config
> > +            .sections
> > +            .iter()
> > +            .filter_map(|(_, (_, data))| {
> > +                data.as_object()
> > +                    .and_then(|cfg| cfg.get("backing-device"))
> > +                    .and_then(serde_json::Value::as_str)
> > +                    .map(String::from)
> > +            })
> > +            .collect();
> > +
> > +        let uuids_from_mounts: Vec<String> = list_datastore_mounts()?
> > +            .into_iter()
> > +            // FIXME: include UUID in DatastoreMountInfo
> > +            .filter_map(|mount| mount.device.split('/').last().map(String::from))
> > +            .collect();
> > +
> > +        [&uuids_from_datastores[..], &uuids_from_mounts[..]].concat()
> > +    } else {
> > +        Vec::new()
> > +    };
>
> i.e., pass in this Vec/slice..
>
> > +
> >      let mut result = HashMap::new();
> >  
> >      for item in proxmox_sys::fs::scan_subdir(libc::AT_FDCWD, "/sys/block", &BLOCKDEVICE_NAME_REGEX)?
> > @@ -1020,12 +1060,14 @@ fn get_disks(
> >  
> >          let partitions: Option<Vec<PartitionInfo>> = if include_partitions {
> >              disk.partitions().map_or(None, |parts| {
> > -                Some(get_partitions_info(
> > -                    parts,
> > -                    &lvm_devices,
> > -                    &zfs_devices,
> > -                    &file_system_devices,
> > -                ))
> > +                Some(
> > +                    get_partitions_info(parts, &lvm_devices, &zfs_devices, &file_system_devices)
> > +                        .into_iter()
> > +                        .filter(|part| {
> > +                            !part.uuid.as_ref().is_some_and(|u| uuids_in_use.contains(u))
> > +                        })
> > +                        .collect(),
> > +                )
> >              })
> >          } else {
> >              None
> > -- 
> > 2.39.2
> > 
> > 
> > 
> > _______________________________________________
> > pbs-devel mailing list
> > pbs-devel@lists.proxmox.com
> > https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
> > 
> > 
> > 
>
>
> _______________________________________________
> pbs-devel mailing list
> pbs-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel



_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel