* [pbs-devel] [SERIES proxmox-backup proxmox-widget-toolkit 0/3] add partitions to disks/list endpoint
@ 2022-04-04 9:50 Hannes Laimer
2022-04-04 9:50 ` [pbs-devel] [PATCH proxmox-backup 1/3] api2: disks endpoint return partitions Hannes Laimer
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Hannes Laimer @ 2022-04-04 9:50 UTC (permalink / raw)
To: pbs-devel
In order to work with the existing frontend component for displying the
disklist, either
* the partition data has to be return with the same struct a disk is
returned. Which leads to the same struct being used for different
things -> quite a few fields are always empty for partitions and a new
'type' field would be needed. Also the code structure in PBS has to be
changed quite a bit.
* the existing frontend has to be adjusted to handle data from PVE and
PBS properly.
I went with the second option because the adjustments nedded in the UI
compenent were minimal and, IMHO, adjusting the API to fit the UI is the
wrong direction.
Not sure if the small changes in proxmox-widget-toolkit justify sending
this series also to pve-devel.
NOTE: The partition data will be needed in later patches for removable
datastores.
proxmox-backup:
Hannes Laimer (2):
api2: disks endpoint return partitions
ui: disks: show partitions by default
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 ++++++++++++++++++++++++++++++-
www/panel/StorageAndDisks.js | 1 +
5 files changed, 131 insertions(+), 6 deletions(-)
proxmox-widget-toolkit:
Hannes Laimer (1):
DiskList: handle partition data from PBS backend
src/panel/DiskList.js | 13 ++++++++++++-
1 file changed, 12 insertions(+), 1 deletion(-)
--
2.30.2
^ permalink raw reply [flat|nested] 6+ messages in thread
* [pbs-devel] [PATCH proxmox-backup 1/3] api2: disks endpoint return partitions
2022-04-04 9:50 [pbs-devel] [SERIES proxmox-backup proxmox-widget-toolkit 0/3] add partitions to disks/list endpoint Hannes Laimer
@ 2022-04-04 9:50 ` Hannes Laimer
2022-04-06 9:12 ` Wolfgang Bumiller
2022-04-04 9:50 ` [pbs-devel] [PATCH proxmox-widget-toolkit 2/2] DiskList: handle partition data from PBS backend Hannes Laimer
2022-04-04 9:50 ` [pbs-devel] [PATCH proxmox-backup 3/3] ui: disks: show partitions by default Hannes Laimer
2 siblings, 1 reply; 6+ messages in thread
From: Hannes Laimer @ 2022-04-04 9:50 UTC (permalink / raw)
To: pbs-devel
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)?;
if info.used != DiskUsageType::Unused {
bail!("disk '{}' is already in use.", disk);
diff --git a/src/api2/node/disks/mod.rs b/src/api2/node/disks/mod.rs
index c44ccfee..478829fb 100644
--- a/src/api2/node/disks/mod.rs
+++ b/src/api2/node/disks/mod.rs
@@ -33,6 +33,12 @@ pub mod zfs;
optional: true,
default: false,
},
+ "include-partitions": {
+ description: "Include partitions.",
+ type: bool,
+ optional: true,
+ default: false,
+ },
"usage-type": {
type: DiskUsageType,
optional: true,
@@ -53,11 +59,12 @@ pub mod zfs;
/// List local disks
pub fn list_disks(
skipsmart: bool,
+ include_partitions: bool,
usage_type: Option<DiskUsageType>,
) -> Result<Vec<DiskUsageInfo>, Error> {
let mut list = Vec::new();
- for (_, info) in get_disks(None, skipsmart)? {
+ for (_, info) in get_disks(None, skipsmart, include_partitions)? {
if let Some(ref usage_type) = usage_type {
if info.used == *usage_type {
list.push(info);
@@ -140,7 +147,7 @@ pub fn initialize_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)?;
if info.used != DiskUsageType::Unused {
bail!("disk '{}' is already in use.", disk);
diff --git a/src/api2/node/disks/zfs.rs b/src/api2/node/disks/zfs.rs
index a542f9e0..5329f44f 100644
--- a/src/api2/node/disks/zfs.rs
+++ b/src/api2/node/disks/zfs.rs
@@ -177,7 +177,7 @@ pub fn create_zpool(
let devices: Vec<String> = devices.as_array().unwrap().iter()
.map(|v| v.as_str().unwrap().to_string()).collect();
- let disk_map = crate::tools::disks::get_disks(None, true)?;
+ let disk_map = crate::tools::disks::get_disks(None, true, false)?;
for disk in devices.iter() {
match disk_map.get(disk) {
Some(info) => {
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)]
+#[serde(rename_all = "lowercase")]
+pub enum PartitionUsageType {
+ /// Partition is not used (as far we can tell)
+ Unused,
+ /// Partition is mounted
+ Mounted,
+ /// 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,
+}
+
#[api()]
#[derive(Debug, Serialize, Deserialize, PartialEq)]
#[serde(rename_all = "lowercase")]
@@ -622,6 +642,23 @@ pub enum DiskUsageType {
FileSystem,
}
+#[api()]
+#[derive(Debug, Serialize, Deserialize)]
+#[serde(rename_all = "kebab-case")]
+/// Baic information about a partition
+pub struct PartitionInfo {
+ /// The partition name
+ pub name: String,
+ /// What the partition is used for
+ pub used: PartitionUsageType,
+ /// The partition devpath
+ pub devpath: Option<String>,
+ /// Size in bytes
+ pub size: Option<u64>,
+ /// Size in bytes
+ pub gpt: bool,
+}
+
#[api(
properties: {
used: {
@@ -632,6 +669,12 @@ pub enum DiskUsageType {
},
status: {
type: SmartStatus,
+ },
+ partitions: {
+ optional: true,
+ items: {
+ type: PartitionInfo
+ }
}
}
)]
@@ -656,6 +699,8 @@ pub struct DiskUsageInfo {
pub size: u64,
/// Serisal number
pub serial: Option<String>,
+ /// Partitions on the device
+ pub partitions: Option<Vec<PartitionInfo>>,
/// Linux device path (/dev/xxx)
pub devpath: Option<String>,
/// Set if disk contains a GPT partition table
@@ -733,10 +778,14 @@ fn scan_partitions(
}
/// Get disk usage information for a single disk
-pub fn get_disk_usage_info(disk: &str, no_smart: bool) -> Result<DiskUsageInfo, Error> {
+pub fn get_disk_usage_info(
+ disk: &str,
+ no_smart: bool,
+ include_partitions: bool,
+) -> Result<DiskUsageInfo, Error> {
let mut filter = Vec::new();
filter.push(disk.to_string());
- let mut map = get_disks(Some(filter), no_smart)?;
+ let mut map = get_disks(Some(filter), no_smart, include_partitions)?;
if let Some(info) = map.remove(disk) {
Ok(info)
} else {
@@ -750,6 +799,8 @@ pub fn get_disks(
disks: Option<Vec<String>>,
// do no include data from smartctl
no_smart: bool,
+ // include partitions
+ include_partitions: bool,
) -> Result<HashMap<String, DiskUsageInfo>, Error> {
let disk_manager = DiskManage::new();
@@ -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();
+ disk.partitions().map_or(None, |parts| {
+ Some(
+ parts
+ .iter()
+ .map(|(nr, disk)| {
+ 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() {
+ 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
+ }
+ _ => 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
^ permalink raw reply [flat|nested] 6+ messages in thread
* [pbs-devel] [PATCH proxmox-widget-toolkit 2/2] DiskList: handle partition data from PBS backend
2022-04-04 9:50 [pbs-devel] [SERIES proxmox-backup proxmox-widget-toolkit 0/3] add partitions to disks/list endpoint Hannes Laimer
2022-04-04 9:50 ` [pbs-devel] [PATCH proxmox-backup 1/3] api2: disks endpoint return partitions Hannes Laimer
@ 2022-04-04 9:50 ` Hannes Laimer
2022-04-07 8:37 ` Dominik Csapak
2022-04-04 9:50 ` [pbs-devel] [PATCH proxmox-backup 3/3] ui: disks: show partitions by default Hannes Laimer
2 siblings, 1 reply; 6+ messages in thread
From: Hannes Laimer @ 2022-04-04 9:50 UTC (permalink / raw)
To: pbs-devel
Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
---
src/panel/DiskList.js | 13 ++++++++++++-
1 file changed, 12 insertions(+), 1 deletion(-)
diff --git a/src/panel/DiskList.js b/src/panel/DiskList.js
index eb8b1a8..c1877ef 100644
--- a/src/panel/DiskList.js
+++ b/src/panel/DiskList.js
@@ -169,8 +169,19 @@ Ext.define('Proxmox.DiskList', {
let data = item.data;
data.leaf = true;
data.expanded = true;
- data.children = [];
+ data.children = data.partitions ? data.partitions : [];
+ for (let p of data.children) {
+ p['disk-type'] = 'partition';
+ p.iconCls = 'fa fa-fw fa-hdd-o x-fa-tree';
+ p.parent = data.devpath;
+ p.children = [];
+ p.leaf = true;
+ }
data.iconCls = 'fa fa-fw fa-hdd-o x-fa-tree';
+ if (data.children.length > 0) {
+ data.leaf = false;
+ }
+
if (!data.parent) {
disks[data.devpath] = data;
}
--
2.30.2
^ permalink raw reply [flat|nested] 6+ messages in thread
* [pbs-devel] [PATCH proxmox-backup 3/3] ui: disks: show partitions by default
2022-04-04 9:50 [pbs-devel] [SERIES proxmox-backup proxmox-widget-toolkit 0/3] add partitions to disks/list endpoint Hannes Laimer
2022-04-04 9:50 ` [pbs-devel] [PATCH proxmox-backup 1/3] api2: disks endpoint return partitions Hannes Laimer
2022-04-04 9:50 ` [pbs-devel] [PATCH proxmox-widget-toolkit 2/2] DiskList: handle partition data from PBS backend Hannes Laimer
@ 2022-04-04 9:50 ` Hannes Laimer
2 siblings, 0 replies; 6+ messages in thread
From: Hannes Laimer @ 2022-04-04 9:50 UTC (permalink / raw)
To: pbs-devel
Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
---
www/panel/StorageAndDisks.js | 1 +
1 file changed, 1 insertion(+)
diff --git a/www/panel/StorageAndDisks.js b/www/panel/StorageAndDisks.js
index 5fd6faf4..7bd7042c 100644
--- a/www/panel/StorageAndDisks.js
+++ b/www/panel/StorageAndDisks.js
@@ -16,6 +16,7 @@ Ext.define('PBS.StorageAndDiskPanel', {
{
xtype: 'pmxDiskList',
title: gettext('Disks'),
+ includePartitions: true,
itemId: 'disks',
iconCls: 'fa fa-hdd-o',
},
--
2.30.2
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [pbs-devel] [PATCH proxmox-backup 1/3] api2: disks endpoint return partitions
2022-04-04 9:50 ` [pbs-devel] [PATCH proxmox-backup 1/3] api2: disks endpoint return partitions Hannes Laimer
@ 2022-04-06 9:12 ` Wolfgang Bumiller
0 siblings, 0 replies; 6+ messages in thread
From: Wolfgang Bumiller @ 2022-04-06 9:12 UTC (permalink / raw)
To: Hannes Laimer; +Cc: pbs-devel
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [pbs-devel] [PATCH proxmox-widget-toolkit 2/2] DiskList: handle partition data from PBS backend
2022-04-04 9:50 ` [pbs-devel] [PATCH proxmox-widget-toolkit 2/2] DiskList: handle partition data from PBS backend Hannes Laimer
@ 2022-04-07 8:37 ` Dominik Csapak
0 siblings, 0 replies; 6+ messages in thread
From: Dominik Csapak @ 2022-04-07 8:37 UTC (permalink / raw)
To: pbs-devel
some nits inline:
On 4/4/22 11:50, Hannes Laimer wrote:
> Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
> ---
> src/panel/DiskList.js | 13 ++++++++++++-
> 1 file changed, 12 insertions(+), 1 deletion(-)
>
> diff --git a/src/panel/DiskList.js b/src/panel/DiskList.js
> index eb8b1a8..c1877ef 100644
> --- a/src/panel/DiskList.js
> +++ b/src/panel/DiskList.js
> @@ -169,8 +169,19 @@ Ext.define('Proxmox.DiskList', {
> let data = item.data;
> data.leaf = true;
> data.expanded = true;
> - data.children = [];
> + data.children = data.partitions ? data.partitions : [];
could be written as 'data.children = data.partitions ?? [];'
> + for (let p of data.children) {
> + p['disk-type'] = 'partition';
> + p.iconCls = 'fa fa-fw fa-hdd-o x-fa-tree';
> + p.parent = data.devpath;
> + p.children = [];
> + p.leaf = true;
> + }
> data.iconCls = 'fa fa-fw fa-hdd-o x-fa-tree';
> + if (data.children.length > 0) {
> + data.leaf = false;
> + }
we already set it above to true and here potentially to false.
better imho would be to not set it at the beginning and here doing a
data.leaf = data.children.length === 0;
so that we only set it once
> +
> if (!data.parent) {
> disks[data.devpath] = data;
> }
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-04-07 8:38 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-04-04 9:50 [pbs-devel] [SERIES proxmox-backup proxmox-widget-toolkit 0/3] add partitions to disks/list endpoint Hannes Laimer
2022-04-04 9:50 ` [pbs-devel] [PATCH proxmox-backup 1/3] api2: disks endpoint return partitions Hannes Laimer
2022-04-06 9:12 ` Wolfgang Bumiller
2022-04-04 9:50 ` [pbs-devel] [PATCH proxmox-widget-toolkit 2/2] DiskList: handle partition data from PBS backend Hannes Laimer
2022-04-07 8:37 ` Dominik Csapak
2022-04-04 9:50 ` [pbs-devel] [PATCH proxmox-backup 3/3] ui: disks: show partitions by default Hannes Laimer
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox