Re: [pbs-devel] [PATCH proxmox-backup 1/3] api2: disks endpoint return partitions

[Wolfgang Bumiller <w.bumiller@proxmox.com>]

On Mon, Apr 04, 2022 at 09:50:46AM +0000, Hannes Laimer wrote:
> Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
> ---
>  src/api2/node/disks/directory.rs |   2 +-
>  src/api2/node/disks/mod.rs       |  11 ++-
>  src/api2/node/disks/zfs.rs       |   2 +-
>  src/tools/disks/mod.rs           | 121 ++++++++++++++++++++++++++++++-
>  4 files changed, 130 insertions(+), 6 deletions(-)
> 
> diff --git a/src/api2/node/disks/directory.rs b/src/api2/node/disks/directory.rs
> index bf1a1be6..79fe2624 100644
> --- a/src/api2/node/disks/directory.rs
> +++ b/src/api2/node/disks/directory.rs
> @@ -149,7 +149,7 @@ pub fn create_datastore_disk(
>  
>      let auth_id = rpcenv.get_auth_id().unwrap();
>  
> -    let info = get_disk_usage_info(&disk, true)?;
> +    let info = get_disk_usage_info(&disk, true, false)?;

I already feel like 2 bool parameters in a row warrants a query builder
type sane with defaults.

    DiskUsageQuery::new(&disk)
        .smart(false) // optional
        .partitions(true) // optional
        .query()

but this can be added later (the implementation itself can still stay as
a regular function anyway )

>  
>      if info.used != DiskUsageType::Unused {
>          bail!("disk '{}' is already in use.", disk);
(...)
> diff --git a/src/tools/disks/mod.rs b/src/tools/disks/mod.rs
> index 94da7b3a..fb9c78a9 100644
> --- a/src/tools/disks/mod.rs
> +++ b/src/tools/disks/mod.rs
> @@ -602,6 +602,26 @@ fn get_file_system_devices(lsblk_info: &[LsblkInfo]) -> Result<HashSet<u64>, Err
>      Ok(device_set)
>  }
>  
> +#[api()]
> +#[derive(Debug, Serialize, Deserialize, PartialEq)]

+Clone

And if it's just a simple list like this +Copy, however, this should
probably contain the file system string in the last member, so not Copy.

> +#[serde(rename_all = "lowercase")]
> +pub enum PartitionUsageType {
> +    /// Partition is not used (as far we can tell)
> +    Unused,
> +    /// Partition is mounted
> +    Mounted,

I don't think `Mounted` should be in the same enum here.

In PVE::Diskmanage we can have a file system type with " (mounted)"
attached to it as a string, as well as "just" mounted with no extra
info.

> +    /// Partition is used by LVM
> +    LVM,
> +    /// Partition is used by ZFS
> +    ZFS,
> +    /// Partition is an EFI partition
> +    EFI,
> +    /// Partition is a BIOS partition
> +    BIOS,
> +    /// Partition contains a file system label
> +    FileSystem,

This should contain the file system string.

> +}
> +
>  #[api()]
>  #[derive(Debug, Serialize, Deserialize, PartialEq)]
>  #[serde(rename_all = "lowercase")]
(...)
> @@ -837,6 +888,71 @@ pub fn get_disks(
>  
>          let wwn = disk.wwn().map(|s| s.to_string_lossy().into_owned());
>  
> +        let partitions: Option<Vec<PartitionInfo>> = if include_partitions {
> +            let lsblk_infos = get_lsblk_info();

Please explicitly discard the error with a `.ok()` here and match for
`Some` instead of `Ok` later on.

> +            disk.partitions().map_or(None, |parts| {
> +                Some(
> +                    parts
> +                        .iter()
> +                        .map(|(nr, disk)| {

Too much indentation, please factorize.

This looks like $determine_usage in PVE::Diskmanage.
If factorized into its own function it's easy to just use `return` (and
keep it in the same order as the perl code, particularly the mounted
part is a bit different atm.)

Makes it easier to check for equivalent API behavior (if we want that).

(On a side node: Ideally I *would* like to move the perl & rust code
into a direction where we can soon replace `PVE::Diskmanage` itself with
this rust code via pve-rs)

> +                            let devpath = disk
> +                                .device_path()
> +                                .map(|p| p.to_owned())
> +                                .map(|p| p.to_string_lossy().to_string());
> +
> +                            let mut used = PartitionUsageType::Unused;
> +
> +                            if let Some(devnum) = disk.devnum().ok() {
> +                                if lvm_devices.contains(&devnum) {
> +                                    used = PartitionUsageType::LVM;
> +                                }
> +                                if zfs_devices.contains(&devnum) {
> +                                    used = PartitionUsageType::ZFS;
> +                                }
> +                            }
> +                            match disk.is_mounted() {
> +                                Ok(true) => used = PartitionUsageType::Mounted,
> +                                Ok(false) => {}
> +                                Err(_) => {} // skip devices with undetectable mount status
> +                            }
> +
> +                            if let Some(devpath) = devpath.as_ref() {
> +                                if let Ok(infos) = lsblk_infos.as_ref() {

Could drop a level of indentation here via a tuple match:

    if let (Some(devpath), Some(infos)) = (devpath.as_ref(), lsblk_info.as_ref()) {

> +                                    for info in infos.iter().filter(|i| i.path.eq(devpath)) {
> +                                        used = match info.partition_type.as_deref() {
> +                                            Some("21686148-6449-6e6f-744e-656564454649") => {
> +                                                PartitionUsageType::BIOS
> +                                            }
> +                                            Some("c12a7328-f81f-11d2-ba4b-00a0c93ec93b") => {
> +                                                PartitionUsageType::EFI
> +                                            }

PVE::Diskmanage also has a 'ZFS reserved' case, should we not add this
here as well?

> +                                            _ => used,
> +                                        }
> +                                    }
> +                                }
> +                            }
> +
> +                            if used == PartitionUsageType::Unused
> +                                && file_system_devices.contains(&devnum)
> +                            {
> +                                used = PartitionUsageType::FileSystem;
> +                            }
> +
> +                            PartitionInfo {
> +                                name: format!("{}{}", name, nr),
> +                                devpath,
> +                                used,
> +                                size: disk.size().ok(),
> +                                gpt: disk.has_gpt(),
> +                            }
> +                        })
> +                        .collect(),
> +                )
> +            })
> +        } else {
> +            None
> +        };
> +
>          if usage != DiskUsageType::Mounted {
>              match scan_partitions(disk_manager.clone(), &lvm_devices, &zfs_devices, &name) {
>                  Ok(part_usage) => {
> @@ -870,6 +986,7 @@ pub fn get_disks(
>              name: name.clone(),
>              vendor,
>              model,
> +            partitions,
>              serial,
>              devpath,
>              size,
> -- 
> 2.30.2