[pve-devel] [PATCH-SERIES storage/widget-toolkit/manager] disk creation and wiping improvements

[pve-devel] [PATCH-SERIES storage/widget-toolkit/manager] disk creation and wiping improvements

[Fabian Ebner]

which also fixes the remaining parts of #2285, i.e. extending
the diskmanage module and creation to support partitions.

The series consists of:

Tighten the checks in the disk creation paths by re-doing them after
forking/locking.

Work around a udev bug[0] leading to outdated device info from lsblk,
by explicitly calling udevadm trigger in the appropriate places.

Add support for partitions for the disk creation paths (both back-end
and UI).

Also change the partition type in the appropriate places as that
information is used when querying disk info.

[0]: https://github.com/systemd/systemd/issues/18525


Ultimately, pve-manager depends on both pve-storage and
proxmox-widget-toolkit, but many things can be applied independently.


pve-storage:

Fabian Ebner (10):
  api: disks: create: re-check disk after fork/lock
  api: disk: work around udev bug to ensure its database is updated
  diskmanage: add change_parttype helper
  diskmanage: wipe blockdev: also change partition type
  diskmanage: don't set usage for unused partitions
  api: disks: initgpt: explicitly abort for partitions
  diskmanage: allow partitions for get_udev_info
  diskmanage: allow passing partitions to get_disks
  partially fix #2285: api: disks: allow partitions for creation paths
  api: disks: create: set correct partition type

 PVE/API2/Disks.pm           | 12 +++++++-
 PVE/API2/Disks/Directory.pm | 37 ++++++++++++++++-------
 PVE/API2/Disks/LVM.pm       | 15 +++++++++-
 PVE/API2/Disks/LVMThin.pm   | 13 ++++++++
 PVE/API2/Disks/ZFS.pm       | 36 ++++++++++++++++++++--
 PVE/Diskmanage.pm           | 60 +++++++++++++++++++++++++++++--------
 6 files changed, 145 insertions(+), 28 deletions(-)


proxmox-widget-toolkit:

Fabian Ebner (2):
  (multi) disk selector: allow requesting partitions too
  disk list: allow wiping individual partitions

 src/form/DiskSelector.js      |  7 +++++++
 src/form/MultiDiskSelector.js | 22 +++++++++++++++++-----
 src/panel/DiskList.js         |  8 --------
 3 files changed, 24 insertions(+), 13 deletions(-)


pve-manager:

Fabian Ebner (9):
  api: ceph: create osd: re-check disk requirements after fork/lock
  api: check: create osd: use wipe_blockdev from the Diskmanage package
  api: ceph: create osd: work around udev bug
  api: ceph: create osd: set correct parttype for DB/WAL
  partially fix #2285: api: ceph: create osd: allow using partitions
  api: ceph: create osd: set correct partition type
  partially fix #2285: ui: ceph: allow selecting partitions
  ui: zfs create: switch to using widget-toolkit's multiDiskSelector
  partially fix #2285: ui: disk create: allow selecting partitions

 PVE/API2/Ceph/OSD.pm           | 112 +++++++++++++++++++++++++--------
 PVE/Ceph/Tools.pm              |  23 -------
 www/manager6/ceph/OSD.js       |   3 +
 www/manager6/node/Directory.js |   1 +
 www/manager6/node/LVM.js       |   1 +
 www/manager6/node/LVMThin.js   |   1 +
 www/manager6/node/ZFS.js       |  81 ++----------------------
 7 files changed, 96 insertions(+), 126 deletions(-)

-- 
2.30.2

[pve-devel] [PATCH storage 01/10] api: disks: create: re-check disk after fork/lock

[Fabian Ebner]

Because then it might not be unused anymore. If there really is a
race, this prevents e.g. sgdisk creating a partition on a device
already in use by LVM or LVM destroying a partitioned device.

For ZFS, also get the latest udev info once inside the worker.

Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---
 PVE/API2/Disks/Directory.pm |  2 ++
 PVE/API2/Disks/LVM.pm       |  2 ++
 PVE/API2/Disks/LVMThin.pm   |  2 ++
 PVE/API2/Disks/ZFS.pm       | 10 +++++++---
 4 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/PVE/API2/Disks/Directory.pm b/PVE/API2/Disks/Directory.pm
index 2decb89..0068db6 100644
--- a/PVE/API2/Disks/Directory.pm
+++ b/PVE/API2/Disks/Directory.pm
@@ -212,6 +212,8 @@ __PACKAGE__->register_method ({
 	    my $mountunitpath = "/etc/systemd/system/$mountunitname";
 
 	    PVE::Diskmanage::locked_disk_action(sub {
+		PVE::Diskmanage::assert_disk_unused($dev);
+
 		# create partition
 		my $cmd = [$SGDISK, '-n1', '-t1:8300', $dev];
 		print "# ", join(' ', @$cmd), "\n";
diff --git a/PVE/API2/Disks/LVM.pm b/PVE/API2/Disks/LVM.pm
index 569e9d1..2c216c0 100644
--- a/PVE/API2/Disks/LVM.pm
+++ b/PVE/API2/Disks/LVM.pm
@@ -154,6 +154,8 @@ __PACKAGE__->register_method ({
 
 	my $worker = sub {
 	    PVE::Diskmanage::locked_disk_action(sub {
+		PVE::Diskmanage::assert_disk_unused($dev);
+
 		PVE::Storage::LVMPlugin::lvm_create_volume_group($dev, $name);
 
 		if ($param->{add_storage}) {
diff --git a/PVE/API2/Disks/LVMThin.pm b/PVE/API2/Disks/LVMThin.pm
index 4d303f8..81d91a6 100644
--- a/PVE/API2/Disks/LVMThin.pm
+++ b/PVE/API2/Disks/LVMThin.pm
@@ -108,6 +108,8 @@ __PACKAGE__->register_method ({
 
 	my $worker = sub {
 	    PVE::Diskmanage::locked_disk_action(sub {
+		PVE::Diskmanage::assert_disk_unused($dev);
+
 		PVE::Storage::LVMPlugin::lvm_create_volume_group($dev, $name);
 		my $pv = PVE::Storage::LVMPlugin::lvm_pv_info($dev);
 		# keep some free space just in case
diff --git a/PVE/API2/Disks/ZFS.pm b/PVE/API2/Disks/ZFS.pm
index 60077c4..885b93c 100644
--- a/PVE/API2/Disks/ZFS.pm
+++ b/PVE/API2/Disks/ZFS.pm
@@ -345,9 +345,6 @@ __PACKAGE__->register_method ({
 	foreach my $dev (@$devs) {
 	    $dev = PVE::Diskmanage::verify_blockdev_path($dev);
 	    PVE::Diskmanage::assert_disk_unused($dev);
-	    my $sysfsdev = $dev =~ s!^/dev/!/sys/block/!r;
-	    my $udevinfo = PVE::Diskmanage::get_udev_info($sysfsdev);
-	    $dev = $udevinfo->{by_id_link} if defined($udevinfo->{by_id_link});
 	}
 
 	PVE::Storage::assert_sid_unused($name) if $param->{add_storage};
@@ -374,6 +371,13 @@ __PACKAGE__->register_method ({
 
 	my $worker = sub {
 	    PVE::Diskmanage::locked_disk_action(sub {
+		for my $dev (@$devs) {
+		    PVE::Diskmanage::assert_disk_unused($dev);
+		    my $sysfsdev = $dev =~ s!^/dev/!/sys/block/!r;
+		    my $udevinfo = PVE::Diskmanage::get_udev_info($sysfsdev);
+		    $dev = $udevinfo->{by_id_link} if defined($udevinfo->{by_id_link});
+		}
+
 		# create zpool with desired raidlevel
 
 		my $cmd = [$ZPOOL, 'create', '-o', "ashift=$ashift", $name];
-- 
2.30.2

[pve-devel] [PATCH storage 02/10] api: disk: work around udev bug to ensure its database is updated

[Fabian Ebner]

There is a udev bug [0] which can ultimately lead to the udev database
for certain devices not being actively updated. Determining whether a
disk is used or not in get_disks() (in part) relies upon lsblk, which
queries the udev database. Ensure the information is updated by
manually calling 'udevadm trigger' for the changed devices.

It's most important for the 'directory' API path, as mounting depends
on the '/dev/disk/by-uuid'-symlink to be generated.

[0]: https://github.com/systemd/systemd/issues/18525

Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---
 PVE/API2/Disks.pm           | 11 ++++++++++-
 PVE/API2/Disks/Directory.pm |  6 ++++++
 PVE/API2/Disks/LVM.pm       |  8 +++++++-
 PVE/API2/Disks/LVMThin.pm   |  6 ++++++
 PVE/API2/Disks/ZFS.pm       |  6 ++++++
 5 files changed, 35 insertions(+), 2 deletions(-)

diff --git a/PVE/API2/Disks.pm b/PVE/API2/Disks.pm
index 6c20931..96c19fd 100644
--- a/PVE/API2/Disks.pm
+++ b/PVE/API2/Disks.pm
@@ -9,6 +9,7 @@ use HTTP::Status qw(:constants);
 use PVE::Diskmanage;
 use PVE::JSONSchema qw(get_standard_option);
 use PVE::SafeSyslog;
+use PVE::Tools qw(run_command);
 
 use PVE::API2::Disks::Directory;
 use PVE::API2::Disks::LVM;
@@ -302,7 +303,15 @@ __PACKAGE__->register_method ({
 	my $rpcenv = PVE::RPCEnvironment::get();
 	my $authuser = $rpcenv->get_user();
 
-	my $worker = sub { PVE::Diskmanage::wipe_blockdev($disk); };
+	my $worker = sub {
+	    PVE::Diskmanage::wipe_blockdev($disk);
+
+	    # FIXME: Remove once we depend on systemd >= v249.
+	    # Work around udev bug https://github.com/systemd/systemd/issues/18525 to ensure the
+	    # udev database is updated.
+	    eval { run_command(['udevadm', 'trigger', $disk]); };
+	    warn $@ if $@;
+	};
 
 	my $basename = basename($disk); # avoid '/' in the ID
 
diff --git a/PVE/API2/Disks/Directory.pm b/PVE/API2/Disks/Directory.pm
index 0068db6..1285274 100644
--- a/PVE/API2/Disks/Directory.pm
+++ b/PVE/API2/Disks/Directory.pm
@@ -266,6 +266,12 @@ __PACKAGE__->register_method ({
 
 		$write_ini->($ini, $mountunitpath);
 
+		# FIXME: Remove once we depend on systemd >= v249.
+		# Work around udev bug https://github.com/systemd/systemd/issues/18525 to ensure the
+		# udev database is updated and the $uuid_path symlink is actually created!
+		eval { run_command(['udevadm', 'trigger', $part]); };
+		warn $@ if $@;
+
 		run_command(['systemctl', 'daemon-reload']);
 		run_command(['systemctl', 'enable', $mountunitname]);
 		run_command(['systemctl', 'start', $mountunitname]);
diff --git a/PVE/API2/Disks/LVM.pm b/PVE/API2/Disks/LVM.pm
index 2c216c0..eb8f5c0 100644
--- a/PVE/API2/Disks/LVM.pm
+++ b/PVE/API2/Disks/LVM.pm
@@ -7,7 +7,7 @@ use PVE::Storage::LVMPlugin;
 use PVE::Diskmanage;
 use PVE::JSONSchema qw(get_standard_option);
 use PVE::API2::Storage::Config;
-use PVE::Tools qw(lock_file);
+use PVE::Tools qw(lock_file run_command);
 
 use PVE::RPCEnvironment;
 use PVE::RESTHandler;
@@ -158,6 +158,12 @@ __PACKAGE__->register_method ({
 
 		PVE::Storage::LVMPlugin::lvm_create_volume_group($dev, $name);
 
+		# FIXME: Remove once we depend on systemd >= v249.
+		# Work around udev bug https://github.com/systemd/systemd/issues/18525 to ensure the
+		# udev database is updated.
+		eval { run_command(['udevadm', 'trigger', $dev]); };
+		warn $@ if $@;
+
 		if ($param->{add_storage}) {
 		    my $storage_params = {
 			type => 'lvm',
diff --git a/PVE/API2/Disks/LVMThin.pm b/PVE/API2/Disks/LVMThin.pm
index 81d91a6..2fd8484 100644
--- a/PVE/API2/Disks/LVMThin.pm
+++ b/PVE/API2/Disks/LVMThin.pm
@@ -132,6 +132,12 @@ __PACKAGE__->register_method ({
 		    $name
 		]);
 
+		# FIXME: Remove once we depend on systemd >= v249.
+		# Work around udev bug https://github.com/systemd/systemd/issues/18525 to ensure the
+		# udev database is updated.
+		eval { run_command(['udevadm', 'trigger', $dev]); };
+		warn $@ if $@;
+
 		if ($param->{add_storage}) {
 		    my $storage_params = {
 			type => 'lvmthin',
diff --git a/PVE/API2/Disks/ZFS.pm b/PVE/API2/Disks/ZFS.pm
index 885b93c..1534631 100644
--- a/PVE/API2/Disks/ZFS.pm
+++ b/PVE/API2/Disks/ZFS.pm
@@ -406,6 +406,12 @@ __PACKAGE__->register_method ({
 		    run_command($cmd);
 		}
 
+		# FIXME: Remove once we depend on systemd >= v249.
+		# Work around udev bug https://github.com/systemd/systemd/issues/18525 to ensure the
+		# udev database is updated.
+		eval { run_command(['udevadm', 'trigger', $devs->@*]); };
+		warn $@ if $@;
+
 		if ($param->{add_storage}) {
 		    my $storage_params = {
 			type => 'zfspool',
-- 
2.30.2

[pve-devel] [PATCH storage 03/10] diskmanage: add change_parttype helper

[Fabian Ebner]

Only supports GPT-partitioned disks as I didn't see an option for
sgdisk to make it also work with MBR-partitioned disks. And while
sfdisk could be used instead (or additionally) it would be a new
dependency, and AFAICS require some conversion of partition type GUIDs
to MBR types on our part.

Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---
 PVE/Diskmanage.pm | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/PVE/Diskmanage.pm b/PVE/Diskmanage.pm
index 10e1218..3fea649 100644
--- a/PVE/Diskmanage.pm
+++ b/PVE/Diskmanage.pm
@@ -895,6 +895,23 @@ sub is_mounted {
     return $found;
 }
 
+# Currently only supports GPT-partitioned disks.
+sub change_parttype {
+    my ($partpath, $parttype) = @_;
+
+    my $err = "unable to change partition type for $partpath";
+
+    my $partnum = get_partnum($partpath);
+    my $blockdev = get_blockdev($partpath);
+    my $dev = strip_dev($blockdev);
+
+    my $info = get_disks($dev, 1);
+    die "$err - unable to get disk info for '$blockdev'\n" if !defined($info->{$dev});
+    die "$err - disk '$blockdev' is not GPT partitioned\n" if !$info->{$dev}->{gpt};
+
+    run_command(['sgdisk', "-t${partnum}:${parttype}", $blockdev], errmsg => $err);
+}
+
 # Wipes all labels and the first 200 MiB of a disk/partition (or the whole if it is smaller).
 # Expected to be called with a result of verify_blockdev_path().
 sub wipe_blockdev {
-- 
2.30.2

[pve-devel] [PATCH storage 04/10] diskmanage: wipe blockdev: also change partition type

[Fabian Ebner]

when called with a partition. Since get_disks uses the partition type
(among other things) to detect LVM and ZFS volumes, such volumes would
still be seen as in-use after wiping. Thus, also change the partition
type and simply use 0x83 "Linux filesystem".

Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---
 PVE/Diskmanage.pm | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/PVE/Diskmanage.pm b/PVE/Diskmanage.pm
index 3fea649..5614e40 100644
--- a/PVE/Diskmanage.pm
+++ b/PVE/Diskmanage.pm
@@ -913,6 +913,7 @@ sub change_parttype {
 }
 
 # Wipes all labels and the first 200 MiB of a disk/partition (or the whole if it is smaller).
+# If called with a partition, also sets the partition type to 0x83 'Linux filesystem'.
 # Expected to be called with a result of verify_blockdev_path().
 sub wipe_blockdev {
     my ($devpath) = @_;
@@ -945,6 +946,11 @@ sub wipe_blockdev {
 	['dd', 'if=/dev/zero', "of=${devpath}", 'bs=1M', 'conv=fdatasync', "count=${count}"],
 	errmsg => "error wiping '${devpath}'",
     );
+
+    if (is_partition($devpath)) {
+	eval { change_parttype($devpath, '8300'); };
+	warn $@ if $@;
+    }
 }
 
 1;
-- 
2.30.2

[pve-devel] [PATCH storage 05/10] diskmanage: don't set usage for unused partitions

[Fabian Ebner]

The disk type is already 'partition' so there's no additional
information here. And it would need to serve as a code-word for
unused partitions. The cleaner approach is to not set the usage.

Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---

Was not a smart decision back then...If this is considered a breaking
change for the disk list API call, I'll go for a different approach of
course, but AFAICS it would be one of:
1. Keep usage 'partition', but also return the unused partitions when
'include-partitions=1,type=unused', and have disk_is_used not complain
about usage 'partition'.
2. Add a new type filter/extend it so it can handle multiple types,
i.e. type=unused,partition.
3. Request all disks in the frontend and filter there.

 PVE/Diskmanage.pm | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/PVE/Diskmanage.pm b/PVE/Diskmanage.pm
index 5614e40..7aad707 100644
--- a/PVE/Diskmanage.pm
+++ b/PVE/Diskmanage.pm
@@ -647,7 +647,7 @@ sub get_disks {
 	    # for devices, this check is done explicitly later
 	    return 'Device Mapper' if !dir_is_empty("$sysdir/holders");
 
-	    return 'partition';
+	    return; # unused partition
 	};
 
 	my $collect_ceph_info = sub {
@@ -716,7 +716,6 @@ sub get_disks {
 	my $used = $determine_usage->($devpath, $sysdir, 0);
 	if (!$include_partitions) {
 	    foreach my $part (sort keys %{$partitions}) {
-		next if $partitions->{$part}->{used} eq 'partition';
 		$used //= $partitions->{$part}->{used};
 	    }
 	} else {
-- 
2.30.2

[pve-devel] [PATCH storage 06/10] api: disks: initgpt: explicitly abort for partitions

[Fabian Ebner]

In preparation to extend disk_is_used to support partitions. Without
this new check, initgpt would also allow partitions once disk_is_used
supports partitions, which is not desirable.

Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---
 PVE/API2/Disks.pm |  1 +
 PVE/Diskmanage.pm | 10 ++++++++--
 2 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/PVE/API2/Disks.pm b/PVE/API2/Disks.pm
index 96c19fd..25c9ded 100644
--- a/PVE/API2/Disks.pm
+++ b/PVE/API2/Disks.pm
@@ -260,6 +260,7 @@ __PACKAGE__->register_method ({
 
 	my $authuser = $rpcenv->get_user();
 
+	die "$disk is a partition\n" if PVE::Diskmanage::is_partition($disk);
 	die "disk $disk already in use\n" if PVE::Diskmanage::disk_is_used($disk);
 	my $worker = sub {
 	    PVE::Diskmanage::init_disk($disk, $param->{uuid});
diff --git a/PVE/Diskmanage.pm b/PVE/Diskmanage.pm
index 7aad707..73cbb8b 100644
--- a/PVE/Diskmanage.pm
+++ b/PVE/Diskmanage.pm
@@ -57,8 +57,8 @@ sub init_disk {
 
     assert_blockdev($disk);
 
-    # we should already have checked if it is in use in the api call
-    # but we check again for safety
+    # we should already have checked these in the api call, but we check again for safety
+    die "$disk is a partition\n" if is_partition($disk);
     die "disk $disk is already in use\n" if disk_is_used($disk);
 
     my $id = $uuid || 'R';
@@ -798,6 +798,12 @@ sub get_blockdev {
     return $block_dev;
 }
 
+sub is_partition {
+    my ($dev_path) = @_;
+
+    return defined(eval { get_partnum($dev_path) });
+}
+
 sub locked_disk_action {
     my ($sub) = @_;
     my $res = PVE::Tools::lock_file('/run/lock/pve-diskmanage.lck', undef, $sub);
-- 
2.30.2

[pve-devel] [PATCH storage 07/10] diskmanage: allow partitions for get_udev_info

[Fabian Ebner]

both existing callers only call this with non-partitions currently, so
the change should be backwards compatible.

In preparation to enable ZFS creation on top of partitions (where the
udev info is used to get the stable by-id path of a device).

Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---
 PVE/Diskmanage.pm | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/PVE/Diskmanage.pm b/PVE/Diskmanage.pm
index 73cbb8b..d4acc93 100644
--- a/PVE/Diskmanage.pm
+++ b/PVE/Diskmanage.pm
@@ -327,7 +327,7 @@ sub get_udev_info {
     warn $@ if $@;
     return undef if !$info;
 
-    return undef if $info !~ m/^E: DEVTYPE=disk$/m;
+    return undef if $info !~ m/^E: DEVTYPE=(disk|partition)$/m;
     return undef if $info =~ m/^E: ID_CDROM/m;
 
     # we use this, because some disks are not simply in /dev
-- 
2.30.2

[pve-devel] [PATCH storage 08/10] diskmanage: allow passing partitions to get_disks

[Fabian Ebner]

Requires that the $include_partitions parameter is set too, which:
1. Makes sense, because the partition won't be included in the result
   otherwise.
2. Ensures backwards compatibility for existing callers that don't
   use $include_partitions. No existing callers use both $disks and
   $include_partitions at the same time, so nothing learns to
   "support" partitions by accident.

Moving the strip_dev helper to the top, so it can be used everywhere.

Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---
 PVE/Diskmanage.pm | 20 ++++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)

diff --git a/PVE/Diskmanage.pm b/PVE/Diskmanage.pm
index d4acc93..f0e14dc 100644
--- a/PVE/Diskmanage.pm
+++ b/PVE/Diskmanage.pm
@@ -20,6 +20,12 @@ my $PVS = "/sbin/pvs";
 my $LVS = "/sbin/lvs";
 my $LSBLK = "/bin/lsblk";
 
+my sub strip_dev :prototype($) {
+    my ($devpath) = @_;
+    $devpath =~ s|^/dev/||;
+    return $devpath;
+}
+
 sub check_bin {
     my ($path) = @_;
     return -x $path;
@@ -518,6 +524,14 @@ sub get_disks {
 	# we get cciss/c0d0 but need cciss!c0d0
 	$_ =~ s|cciss/|cciss!| for @$disks;
 
+	if ($include_partitions) {
+	    # Proper blockdevice is needed for the regex, use parent for partitions.
+	    for my $disk ($disks->@*) {
+		next if !is_partition("/dev/$disk");
+		$disk = strip_dev(get_blockdev("/dev/$disk"));
+	    }
+	}
+
 	$disk_regex = "(?:" . join('|', @$disks) . ")";
     }
 
@@ -852,12 +866,6 @@ sub append_partition {
     return $partition;
 }
 
-my sub strip_dev :prototype($) {
-    my ($devpath) = @_;
-    $devpath =~ s|^/dev/||;
-    return $devpath;
-}
-
 # Check if a disk or any of its partitions has a holder.
 # Can also be called with a partition.
 # Expected to be called with a result of verify_blockdev_path().
-- 
2.30.2

[pve-devel] [PATCH storage 09/10] partially fix #2285: api: disks: allow partitions for creation paths

[Fabian Ebner]

The calls for directory and ZFS need slight adaptations. Except for
those, the only thing that needs to be done is support partitions in
the disk_is_used helper.

Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---
 PVE/API2/Disks/Directory.pm | 28 ++++++++++++++++------------
 PVE/API2/Disks/ZFS.pm       | 11 ++++++++++-
 PVE/Diskmanage.pm           |  2 +-
 3 files changed, 27 insertions(+), 14 deletions(-)

diff --git a/PVE/API2/Disks/Directory.pm b/PVE/API2/Disks/Directory.pm
index 1285274..52f0c86 100644
--- a/PVE/API2/Disks/Directory.pm
+++ b/PVE/API2/Disks/Directory.pm
@@ -214,20 +214,24 @@ __PACKAGE__->register_method ({
 	    PVE::Diskmanage::locked_disk_action(sub {
 		PVE::Diskmanage::assert_disk_unused($dev);
 
-		# create partition
-		my $cmd = [$SGDISK, '-n1', '-t1:8300', $dev];
-		print "# ", join(' ', @$cmd), "\n";
-		run_command($cmd);
-
-		my ($devname) = $dev =~ m|^/dev/(.*)$|;
-		my $part = "/dev/";
-		dir_glob_foreach("/sys/block/$devname", qr/\Q$devname\E.+/, sub {
-		    my ($partition) = @_;
-		    $part .= $partition;
-		});
+		my $part = $dev;
+
+		if (!PVE::Diskmanage::is_partition($dev)) {
+		    # create partition
+		    my $cmd = [$SGDISK, '-n1', '-t1:8300', $dev];
+		    print "# ", join(' ', @$cmd), "\n";
+		    run_command($cmd);
+
+		    my ($devname) = $dev =~ m|^/dev/(.*)$|;
+		    $part = "/dev/";
+		    dir_glob_foreach("/sys/block/$devname", qr/\Q$devname\E.+/, sub {
+			my ($partition) = @_;
+			$part .= $partition;
+		    });
+		}
 
 		# create filesystem
-		$cmd = [$MKFS, '-t', $type, $part];
+		my $cmd = [$MKFS, '-t', $type, $part];
 		print "# ", join(' ', @$cmd), "\n";
 		run_command($cmd);
 
diff --git a/PVE/API2/Disks/ZFS.pm b/PVE/API2/Disks/ZFS.pm
index 1534631..6486404 100644
--- a/PVE/API2/Disks/ZFS.pm
+++ b/PVE/API2/Disks/ZFS.pm
@@ -373,7 +373,16 @@ __PACKAGE__->register_method ({
 	    PVE::Diskmanage::locked_disk_action(sub {
 		for my $dev (@$devs) {
 		    PVE::Diskmanage::assert_disk_unused($dev);
-		    my $sysfsdev = $dev =~ s!^/dev/!/sys/block/!r;
+
+		    my $is_partition = PVE::Diskmanage::is_partition($dev);
+		    my $sysfsdev = $is_partition ? PVE::Diskmanage::get_blockdev($dev) : $dev;
+
+		    $sysfsdev =~ s!^/dev/!/sys/block/!;
+		    if ($is_partition) {
+			my $part = $dev =~ s!^/dev/!!r;
+			$sysfsdev .= "/${part}";
+		    }
+
 		    my $udevinfo = PVE::Diskmanage::get_udev_info($sysfsdev);
 		    $dev = $udevinfo->{by_id_link} if defined($udevinfo->{by_id_link});
 		}
diff --git a/PVE/Diskmanage.pm b/PVE/Diskmanage.pm
index f0e14dc..18459f9 100644
--- a/PVE/Diskmanage.pm
+++ b/PVE/Diskmanage.pm
@@ -78,7 +78,7 @@ sub disk_is_used {
     my $dev = $disk;
     $dev =~ s|^/dev/||;
 
-    my $disklist = get_disks($dev, 1);
+    my $disklist = get_disks($dev, 1, 1);
 
     die "'$disk' is not a valid local disk\n" if !defined($disklist->{$dev});
     return 1 if $disklist->{$dev}->{used};
-- 
2.30.2

[pve-devel] [PATCH storage 10/10] api: disks: create: set correct partition type

[Fabian Ebner]

Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---
 PVE/API2/Disks/Directory.pm |  5 ++++-
 PVE/API2/Disks/LVM.pm       |  5 +++++
 PVE/API2/Disks/LVMThin.pm   |  5 +++++
 PVE/API2/Disks/ZFS.pm       | 11 +++++++++++
 4 files changed, 25 insertions(+), 1 deletion(-)

diff --git a/PVE/API2/Disks/Directory.pm b/PVE/API2/Disks/Directory.pm
index 52f0c86..3a90a2e 100644
--- a/PVE/API2/Disks/Directory.pm
+++ b/PVE/API2/Disks/Directory.pm
@@ -216,7 +216,10 @@ __PACKAGE__->register_method ({
 
 		my $part = $dev;
 
-		if (!PVE::Diskmanage::is_partition($dev)) {
+		if (PVE::Diskmanage::is_partition($dev)) {
+		    eval { PVE::Diskmanage::change_parttype($dev, '8300'); };
+		    warn $@ if $@;
+		} else {
 		    # create partition
 		    my $cmd = [$SGDISK, '-n1', '-t1:8300', $dev];
 		    print "# ", join(' ', @$cmd), "\n";
diff --git a/PVE/API2/Disks/LVM.pm b/PVE/API2/Disks/LVM.pm
index eb8f5c0..885e02b 100644
--- a/PVE/API2/Disks/LVM.pm
+++ b/PVE/API2/Disks/LVM.pm
@@ -156,6 +156,11 @@ __PACKAGE__->register_method ({
 	    PVE::Diskmanage::locked_disk_action(sub {
 		PVE::Diskmanage::assert_disk_unused($dev);
 
+		if (PVE::Diskmanage::is_partition($dev)) {
+		    eval { PVE::Diskmanage::change_parttype($dev, '8E00'); };
+		    warn $@ if $@;
+		}
+
 		PVE::Storage::LVMPlugin::lvm_create_volume_group($dev, $name);
 
 		# FIXME: Remove once we depend on systemd >= v249.
diff --git a/PVE/API2/Disks/LVMThin.pm b/PVE/API2/Disks/LVMThin.pm
index 2fd8484..83ebc46 100644
--- a/PVE/API2/Disks/LVMThin.pm
+++ b/PVE/API2/Disks/LVMThin.pm
@@ -110,6 +110,11 @@ __PACKAGE__->register_method ({
 	    PVE::Diskmanage::locked_disk_action(sub {
 		PVE::Diskmanage::assert_disk_unused($dev);
 
+		if (PVE::Diskmanage::is_partition($dev)) {
+		    eval { PVE::Diskmanage::change_parttype($dev, '8E00'); };
+		    warn $@ if $@;
+		}
+
 		PVE::Storage::LVMPlugin::lvm_create_volume_group($dev, $name);
 		my $pv = PVE::Storage::LVMPlugin::lvm_pv_info($dev);
 		# keep some free space just in case
diff --git a/PVE/API2/Disks/ZFS.pm b/PVE/API2/Disks/ZFS.pm
index 6486404..7f96bb7 100644
--- a/PVE/API2/Disks/ZFS.pm
+++ b/PVE/API2/Disks/ZFS.pm
@@ -375,6 +375,17 @@ __PACKAGE__->register_method ({
 		    PVE::Diskmanage::assert_disk_unused($dev);
 
 		    my $is_partition = PVE::Diskmanage::is_partition($dev);
+
+		    if ($is_partition) {
+			eval {
+			    PVE::Diskmanage::change_parttype(
+				$dev,
+				'6a898cc3-1dd2-11b2-99a6-080020736631',
+			    );
+			};
+			warn $@ if $@;
+		    }
+
 		    my $sysfsdev = $is_partition ? PVE::Diskmanage::get_blockdev($dev) : $dev;
 
 		    $sysfsdev =~ s!^/dev/!/sys/block/!;
-- 
2.30.2

[pve-devel] [PATCH widget-toolkit 1/2] (multi) disk selector: allow requesting partitions too

[Fabian Ebner]

No functional change for existing users is intended.

Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---
 src/form/DiskSelector.js      |  7 +++++++
 src/form/MultiDiskSelector.js | 22 +++++++++++++++++-----
 2 files changed, 24 insertions(+), 5 deletions(-)

diff --git a/src/form/DiskSelector.js b/src/form/DiskSelector.js
index 4552beb..3292661 100644
--- a/src/form/DiskSelector.js
+++ b/src/form/DiskSelector.js
@@ -8,6 +8,9 @@ Ext.define('Proxmox.form.DiskSelector', {
     // journal_disk: all disks with gpt
     diskType: undefined,
 
+    // use include-partitions=1 as a parameter
+    includePartitions: false,
+
     // the property the backend wnats for the type ('type' by default)
     typeProperty: 'type',
 
@@ -53,6 +56,10 @@ Ext.define('Proxmox.form.DiskSelector', {
 	    extraParams[me.typeProperty] = me.diskType;
 	}
 
+	if (me.includePartitions) {
+	    extraParams['include-partitions'] = 1;
+	}
+
 	var store = Ext.create('Ext.data.Store', {
 	    filterOnLoad: true,
 	    model: 'pmx-disk-list',
diff --git a/src/form/MultiDiskSelector.js b/src/form/MultiDiskSelector.js
index 9e989a4..88cdc80 100644
--- a/src/form/MultiDiskSelector.js
+++ b/src/form/MultiDiskSelector.js
@@ -24,6 +24,9 @@ Ext.define('Proxmox.form.MultiDiskSelector', {
     // the type of disks to show
     diskType: 'unused',
 
+    // add include-partitions=1 as a request parameter
+    includePartitions: false,
+
     disks: [],
 
     allowBlank: false,
@@ -141,22 +144,31 @@ Ext.define('Proxmox.form.MultiDiskSelector', {
     initComponent: function() {
 	let me = this;
 
+	let extraParams = {};
+
 	if (!me.url) {
 	    if (!me.nodename) {
 		throw "no url or nodename given";
 	    }
 
-	    let node = me.nodename;
-	    let param = me.typeParameter;
-	    let type = me.diskType;
-	    me.url = `/api2/json/nodes/${node}/disks/list?${param}=${type}`;
+	    me.url = `/api2/json/nodes/${me.nodename}/disks/list`;
+
+	    extraParams[me.typeParameter] = me.diskType;
+
+	    if (me.includePartitions) {
+		extraParams['include-partitions'] = 1;
+	    }
 	}
 
 	me.disks = [];
 
 	me.callParent();
 	let store = me.getStore();
-	store.getProxy().setUrl(me.url);
+	store.setProxy({
+	    type: 'proxmox',
+	    url: me.url,
+	    extraParams,
+	});
 	store.load();
 	store.sort({ property: me.valueField });
     },
-- 
2.30.2

[pve-devel] [PATCH widget-toolkit 2/2] disk list: allow wiping individual partitions

[Fabian Ebner]

Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---
 src/panel/DiskList.js | 8 --------
 1 file changed, 8 deletions(-)

diff --git a/src/panel/DiskList.js b/src/panel/DiskList.js
index 90a6553..eb8b1a8 100644
--- a/src/panel/DiskList.js
+++ b/src/panel/DiskList.js
@@ -399,14 +399,6 @@ Ext.define('Proxmox.DiskList', {
 		    return `${mainMessage}<br><br>${additionalInfo}`;
 		},
 		disabled: true,
-		enableFn: function(rec) {
-		    // TODO enable for partitions once they can be selected for ZFS,LVM,etc. creation
-		    if (!rec || rec.data.parent) {
-			return false;
-		    } else {
-			return true;
-		    }
-		},
 		handler: 'wipeDisk',
 	    });
 	}
-- 
2.30.2

[pve-devel] [PATCH manager 1/9] api: ceph: create osd: re-check disk requirements after fork/lock

[Fabian Ebner]

Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---
 PVE/API2/Ceph/OSD.pm | 54 +++++++++++++++++++++++++-------------------
 1 file changed, 31 insertions(+), 23 deletions(-)

diff --git a/PVE/API2/Ceph/OSD.pm b/PVE/API2/Ceph/OSD.pm
index 79548b2f..83a9c932 100644
--- a/PVE/API2/Ceph/OSD.pm
+++ b/PVE/API2/Ceph/OSD.pm
@@ -319,29 +319,35 @@ __PACKAGE__->register_method ({
 	    }
 	}
 
-	# test osd requirements early
-	my $devlist = [ map { $_->{name} } values %$devs ];
-	my $disklist = PVE::Diskmanage::get_disks($devlist, 1);
-	my $dev = $devs->{dev}->{dev};
-	my $devname = $devs->{dev}->{name};
-	die "unable to get device info for '$dev'\n" if !$disklist->{$devname};
-	die "device '$dev' is already in use\n" if $disklist->{$devname}->{used};
-
-	# test db/wal requirements early
-	for my $type ( qw(db_dev wal_dev) ) {
-	    my $d = $devs->{$type};
-	    next if !$d;
-	    my $name = $d->{name};
-	    my $info = $disklist->{$name};
-	    die "unable to get device info for '$d->{dev}' for type $type\n" if !$disklist->{$name};
-	    if (my $usage = $info->{used}) {
-		if ($usage eq 'partitions') {
-		    die "device '$d->{dev}' is not GPT partitioned\n" if !$info->{gpt};
-		} elsif ($usage ne 'LVM') {
-		    die "device '$d->{dev}' is already in use and has no LVM on it\n";
+	my $test_disk_requirements = sub {
+	    my ($disklist) = @_;
+
+	    my $dev = $devs->{dev}->{dev};
+	    my $devname = $devs->{dev}->{name};
+	    die "unable to get device info for '$dev'\n" if !$disklist->{$devname};
+	    die "device '$dev' is already in use\n" if $disklist->{$devname}->{used};
+
+	    for my $type ( qw(db_dev wal_dev) ) {
+		my $d = $devs->{$type};
+		next if !$d;
+		my $name = $d->{name};
+		my $info = $disklist->{$name};
+		die "unable to get device info for '$d->{dev}' for type $type\n" if !$disklist->{$name};
+		if (my $usage = $info->{used}) {
+		    if ($usage eq 'partitions') {
+			die "device '$d->{dev}' is not GPT partitioned\n" if !$info->{gpt};
+		    } elsif ($usage ne 'LVM') {
+			die "device '$d->{dev}' is already in use and has no LVM on it\n";
+		    }
 		}
 	    }
-	}
+	};
+
+
+	# test disk requirements early
+	my $devlist = [ map { $_->{name} } values %$devs ];
+	my $disklist = PVE::Diskmanage::get_disks($devlist, 1);
+	$test_disk_requirements->($disklist);
 
 	# get necessary ceph infos
 	my $rados = PVE::RADOS->new();
@@ -427,13 +433,15 @@ __PACKAGE__->register_method ({
 	    my $upid = shift;
 
 	    PVE::Diskmanage::locked_disk_action(sub {
-		# update disklist
+		# update disklist and re-test requirements
 		$disklist = PVE::Diskmanage::get_disks($devlist, 1);
+		$test_disk_requirements->($disklist);
 
 		my $dev_class = $param->{'crush-device-class'};
 		my $cmd = ['ceph-volume', 'lvm', 'create', '--cluster-fsid', $fsid ];
 		push @$cmd, '--crush-device-class', $dev_class if $dev_class;
 
+		my $devname = $devs->{dev}->{name};
 		my $devpath = $disklist->{$devname}->{devpath};
 		print "create OSD on $devpath (bluestore)\n";
 
@@ -471,7 +479,7 @@ __PACKAGE__->register_method ({
 	    });
 	};
 
-	return $rpcenv->fork_worker('cephcreateosd', $devname,  $authuser, $worker);
+	return $rpcenv->fork_worker('cephcreateosd', $devs->{dev}->{name},  $authuser, $worker);
     }});
 
 # Check if $osdid belongs to $nodename
-- 
2.30.2

[pve-devel] [PATCH manager 2/9] api: check: create osd: use wipe_blockdev from the Diskmanage package

[Fabian Ebner]

which is mostly a copy of the wipe_disks helper with the difference
that it also uses wipefs on the device and its partitions.

Remove the wipe_disks helper as no users remain.

Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---

For this one, the dependency on libpve-storage-perl is already new
enough.

 PVE/API2/Ceph/OSD.pm |  4 ++--
 PVE/Ceph/Tools.pm    | 23 -----------------------
 2 files changed, 2 insertions(+), 25 deletions(-)

diff --git a/PVE/API2/Ceph/OSD.pm b/PVE/API2/Ceph/OSD.pm
index 83a9c932..97393912 100644
--- a/PVE/API2/Ceph/OSD.pm
+++ b/PVE/API2/Ceph/OSD.pm
@@ -473,7 +473,7 @@ __PACKAGE__->register_method ({
 		push @$cmd, '--data', $devpath;
 		push @$cmd, '--dmcrypt' if $param->{encrypted};
 
-		PVE::Ceph::Tools::wipe_disks($devpath);
+		PVE::Diskmanage::wipe_blockdev($devpath);
 
 		run_command($cmd);
 	    });
@@ -590,7 +590,7 @@ __PACKAGE__->register_method ({
 		my $partnum = PVE::Diskmanage::get_partnum($part);
 		my $devpath = PVE::Diskmanage::get_blockdev($part);
 
-		PVE::Ceph::Tools::wipe_disks($part);
+		PVE::Diskmanage::wipe_blockdev($part);
 		print "remove partition $part (disk '${devpath}', partnum $partnum)\n";
 		eval { run_command(['/sbin/sgdisk', '-d', $partnum, "${devpath}"]); };
 		warn $@ if $@;
diff --git a/PVE/Ceph/Tools.pm b/PVE/Ceph/Tools.pm
index e7124074..f54d837a 100644
--- a/PVE/Ceph/Tools.pm
+++ b/PVE/Ceph/Tools.pm
@@ -333,29 +333,6 @@ sub get_or_create_admin_keyring {
     return $pve_ckeyring_path;
 }
 
-# wipe the first 200 MB to clear off leftovers from previous use, otherwise a
-# create OSD fails.
-sub wipe_disks {
-    my (@devs) = @_;
-
-    my @wipe_cmd = qw(/bin/dd if=/dev/zero bs=1M conv=fdatasync);
-
-    foreach my $devpath (@devs) {
-	my $devname = basename($devpath);
-	my $dev_size = PVE::Tools::file_get_contents("/sys/class/block/$devname/size");
-
-	($dev_size) = $dev_size =~ m|(\d+)|; # untaint $dev_size
-	die "Coulnd't get the size of the device $devname\n" if (!defined($dev_size));
-
-	my $size = ($dev_size * 512 / 1024 / 1024);
-	my $count = ($size < 200) ? $size : 200;
-
-	print "wipe disk/partition: $devpath\n";
-	eval { run_command([@wipe_cmd, "count=$count", "of=${devpath}"]) };
-	warn $@ if $@;
-    }
-};
-
 # get ceph-volume managed osds
 sub ceph_volume_list {
     my $result = {};
-- 
2.30.2

[pve-devel] [PATCH manager 3/9] api: ceph: create osd: work around udev bug

[Fabian Ebner]

There is a udev bug [0] which can ultimately lead to the udev database
for certain devices not being actively updated. The Diskmanage package
relies upon lsblk for certain info, and lsblk queries the udev
database. Ensure the information is updated by manually calling
'udevadm trigger' for the changed devices.

Without the fix, and a bit of bad luck, a cleaned up disk could still
show up as an 'LVM2_member' for example.

[0]: https://github.com/systemd/systemd/issues/18525

Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---
 PVE/API2/Ceph/OSD.pm | 32 +++++++++++++++++++++++++++++++-
 1 file changed, 31 insertions(+), 1 deletion(-)

diff --git a/PVE/API2/Ceph/OSD.pm b/PVE/API2/Ceph/OSD.pm
index 97393912..fbbc2139 100644
--- a/PVE/API2/Ceph/OSD.pm
+++ b/PVE/API2/Ceph/OSD.pm
@@ -371,6 +371,9 @@ __PACKAGE__->register_method ({
 	    file_set_contents($ceph_bootstrap_osd_keyring, $bindata);
 	};
 
+	# See FIXME below
+	my @udev_trigger_devs = ();
+
 	my $create_part_or_lv = sub {
 	    my ($dev, $size, $type) = @_;
 
@@ -393,6 +396,8 @@ __PACKAGE__->register_method ({
 		PVE::Storage::LVMPlugin::lvm_create_volume_group($dev->{devpath}, $vg);
 		PVE::Storage::LVMPlugin::lvcreate($vg, $lv, "${size}k");
 
+		push @udev_trigger_devs, $dev->{devpath};
+
 		return "$vg/$lv";
 
 	    } elsif ($dev->{used} eq 'LVM') {
@@ -423,7 +428,9 @@ __PACKAGE__->register_method ({
 	    } elsif ($dev->{used} eq 'partitions' && $dev->{gpt}) {
 		# create new partition at the end
 
-		return PVE::Diskmanage::append_partition($dev->{devpath}, $size * 1024);
+		my $part = PVE::Diskmanage::append_partition($dev->{devpath}, $size * 1024);
+		push @udev_trigger_devs, $part;
+		return $part;
 	    }
 
 	    die "cannot use '$dev->{devpath}' for '$type'\n";
@@ -445,6 +452,8 @@ __PACKAGE__->register_method ({
 		my $devpath = $disklist->{$devname}->{devpath};
 		print "create OSD on $devpath (bluestore)\n";
 
+		push @udev_trigger_devs, $devpath;
+
 		my $osd_size = $disklist->{$devname}->{size};
 		my $size_map = {
 		    db => int($osd_size / 10), # 10% of OSD
@@ -476,6 +485,12 @@ __PACKAGE__->register_method ({
 		PVE::Diskmanage::wipe_blockdev($devpath);
 
 		run_command($cmd);
+
+		# FIXME: Remove once we depend on systemd >= v249.
+		# Work around udev bug https://github.com/systemd/systemd/issues/18525 to ensure the
+		# udev database is updated.
+		eval { run_command(['udevadm', 'trigger', @udev_trigger_devs]); };
+		warn $@ if $@;
 	    });
 	};
 
@@ -583,6 +598,9 @@ __PACKAGE__->register_method ({
 	    # try to unmount from standard mount point
 	    my $mountpoint = "/var/lib/ceph/osd/ceph-$osdid";
 
+	    # See FIXME below
+	    my $udev_trigger_devs = {};
+
 	    my $remove_partition = sub {
 		my ($part) = @_;
 
@@ -590,6 +608,8 @@ __PACKAGE__->register_method ({
 		my $partnum = PVE::Diskmanage::get_partnum($part);
 		my $devpath = PVE::Diskmanage::get_blockdev($part);
 
+		$udev_trigger_devs->{$devpath} = 1;
+
 		PVE::Diskmanage::wipe_blockdev($part);
 		print "remove partition $part (disk '${devpath}', partnum $partnum)\n";
 		eval { run_command(['/sbin/sgdisk', '-d', $partnum, "${devpath}"]); };
@@ -611,6 +631,8 @@ __PACKAGE__->register_method ({
 
 			    eval { run_command(['/sbin/pvremove', $dev], errfunc => sub {}) };
 			    warn $@ if $@;
+
+			    $udev_trigger_devs->{$dev} = 1;
 			}
 		    }
 		}
@@ -648,6 +670,14 @@ __PACKAGE__->register_method ({
 		    }
 		}
 	    }
+
+	    # FIXME: Remove once we depend on systemd >= v249.
+	    # Work around udev bug https://github.com/systemd/systemd/issues/18525 to ensure the
+	    # udev database is updated.
+	    if ($cleanup) {
+		eval { run_command(['udevadm', 'trigger', keys $udev_trigger_devs->%*]); };
+		warn $@ if $@;
+	    }
 	};
 
 	return $rpcenv->fork_worker('cephdestroyosd', $osdsection,  $authuser, $worker);
-- 
2.30.2

[pve-devel] [PATCH manager 4/9] api: ceph: create osd: set correct parttype for DB/WAL

[Fabian Ebner]

The get_ceph_journals function in pve-storage uses this information.

Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---

Dependency bump for pve-storage for the new helper needed.

 PVE/API2/Ceph/OSD.pm | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/PVE/API2/Ceph/OSD.pm b/PVE/API2/Ceph/OSD.pm
index fbbc2139..028b37d0 100644
--- a/PVE/API2/Ceph/OSD.pm
+++ b/PVE/API2/Ceph/OSD.pm
@@ -427,8 +427,18 @@ __PACKAGE__->register_method ({
 
 	    } elsif ($dev->{used} eq 'partitions' && $dev->{gpt}) {
 		# create new partition at the end
+		my $parttypes = {
+		    'osd-db' => '30CD0809-C2B2-499C-8879-2D6B78529876',
+		    'osd-wal' => '5CE17FCE-4087-4169-B7FF-056CC58473F9',
+		};
 
 		my $part = PVE::Diskmanage::append_partition($dev->{devpath}, $size * 1024);
+
+		if (my $parttype = $parttypes->{$type}) {
+		    eval { PVE::Diskmanage::change_parttype($part, $parttype); };
+		    warn $@ if $@;
+		}
+
 		push @udev_trigger_devs, $part;
 		return $part;
 	    }
-- 
2.30.2

[pve-devel] [RFC manager 5/9] partially fix #2285: api: ceph: create osd: allow using partitions

[Fabian Ebner]

Note that this does not only allow partitions to be used, but for DB
and WAL disks, one more type of disk, that wasn't allowed before.
Namely, GPT-partitioned disks with any partitions detected as used.
The reason is get_disks' behavior:
  * Without $include_partitions=1, the disk will have the same usage
    as it's first used partition, and thus wasn't allowed. (Except in
    the case that usage was LVM, where the check was bypassed, but
    luckily OSD creation just failed later because no Ceph volume
    group would be detected).
  * With $include_partitions=1, the disk will have usage 'partitions'
    and thus be allowed.

Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---

RFC, because of the weird situation with the DB/WAL disks...

Dependency bump for pve-storage is needed to actually make it work,
because previously it wasn't supported to pass along partitions as
elements of the first argument.

 PVE/API2/Ceph/OSD.pm | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/PVE/API2/Ceph/OSD.pm b/PVE/API2/Ceph/OSD.pm
index 028b37d0..c511f315 100644
--- a/PVE/API2/Ceph/OSD.pm
+++ b/PVE/API2/Ceph/OSD.pm
@@ -346,7 +346,7 @@ __PACKAGE__->register_method ({
 
 	# test disk requirements early
 	my $devlist = [ map { $_->{name} } values %$devs ];
-	my $disklist = PVE::Diskmanage::get_disks($devlist, 1);
+	my $disklist = PVE::Diskmanage::get_disks($devlist, 1, 1);
 	$test_disk_requirements->($disklist);
 
 	# get necessary ceph infos
@@ -451,7 +451,7 @@ __PACKAGE__->register_method ({
 
 	    PVE::Diskmanage::locked_disk_action(sub {
 		# update disklist and re-test requirements
-		$disklist = PVE::Diskmanage::get_disks($devlist, 1);
+		$disklist = PVE::Diskmanage::get_disks($devlist, 1, 1);
 		$test_disk_requirements->($disklist);
 
 		my $dev_class = $param->{'crush-device-class'};
-- 
2.30.2

[pve-devel] [RFC manager 6/9] api: ceph: create osd: set correct partition type

[Fabian Ebner]

Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---

RFC, because the previous one is and this only makes sense with it.

Dependency bump for pve-storage is needed for the new helpers.

 PVE/API2/Ceph/OSD.pm | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/PVE/API2/Ceph/OSD.pm b/PVE/API2/Ceph/OSD.pm
index c511f315..18329eeb 100644
--- a/PVE/API2/Ceph/OSD.pm
+++ b/PVE/API2/Ceph/OSD.pm
@@ -396,6 +396,11 @@ __PACKAGE__->register_method ({
 		PVE::Storage::LVMPlugin::lvm_create_volume_group($dev->{devpath}, $vg);
 		PVE::Storage::LVMPlugin::lvcreate($vg, $lv, "${size}k");
 
+		if (PVE::Diskmanage::is_partition($dev->{devpath})) {
+		    eval { PVE::Diskmanage::change_parttype($dev->{devpath}, '8E00'); };
+		    warn $@ if $@;
+		}
+
 		push @udev_trigger_devs, $dev->{devpath};
 
 		return "$vg/$lv";
@@ -494,6 +499,11 @@ __PACKAGE__->register_method ({
 
 		PVE::Diskmanage::wipe_blockdev($devpath);
 
+		if (PVE::Diskmanage::is_partition($devpath)) {
+		    eval { PVE::Diskmanage::change_parttype($devpath, '8E00'); };
+		    warn $@ if $@;
+		}
+
 		run_command($cmd);
 
 		# FIXME: Remove once we depend on systemd >= v249.
-- 
2.30.2

[pve-devel] [RFC manager 7/9] partially fix #2285: ui: ceph: allow selecting partitions

[Fabian Ebner]

For DB and WAL disks, not only partitions will show up now, but one
more type of disk, that didn't show up before: Namely, GPT-partitioned
disks with any partitions detected as used.

It's confusing as the size shown is of the full disk, with no
indication that a new partition will be appended at the end. This
problem was already present before, but only affected GPT-partitioned
disks where no usage on a partition was detected.

Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---

RFC, because of the weird situation with the DB/WAL disks...Even if we
choose to extend the support in the back-end, it might make sense to
limit what we show in the front-end.

Depends on new widget-toolkit to have an effect.

 www/manager6/ceph/OSD.js | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/www/manager6/ceph/OSD.js b/www/manager6/ceph/OSD.js
index aea38d6c..3e775606 100644
--- a/www/manager6/ceph/OSD.js
+++ b/www/manager6/ceph/OSD.js
@@ -38,6 +38,7 @@ Ext.define('PVE.CephCreateOsd', {
 			    name: 'dev',
 			    nodename: me.nodename,
 			    diskType: 'unused',
+			    includePartitions: true,
 			    fieldLabel: gettext('Disk'),
 			    allowBlank: false,
 			},
@@ -48,6 +49,7 @@ Ext.define('PVE.CephCreateOsd', {
 			    name: 'db_dev',
 			    nodename: me.nodename,
 			    diskType: 'journal_disks',
+			    includePartitions: true,
 			    fieldLabel: gettext('DB Disk'),
 			    value: '',
 			    autoSelect: false,
@@ -101,6 +103,7 @@ Ext.define('PVE.CephCreateOsd', {
 			    name: 'wal_dev',
 			    nodename: me.nodename,
 			    diskType: 'journal_disks',
+			    includePartitions: true,
 			    fieldLabel: gettext('WAL Disk'),
 			    value: '',
 			    autoSelect: false,
-- 
2.30.2

[pve-devel] [PATCH manager 8/9] ui: zfs create: switch to using widget-toolkit's multiDiskSelector

[Fabian Ebner]

Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---

For this one, the dependency requirement is already new enough.

 www/manager6/node/ZFS.js | 80 ++--------------------------------------
 1 file changed, 4 insertions(+), 76 deletions(-)

diff --git a/www/manager6/node/ZFS.js b/www/manager6/node/ZFS.js
index 8ea364bf..2537ee5a 100644
--- a/www/manager6/node/ZFS.js
+++ b/www/manager6/node/ZFS.js
@@ -16,35 +16,13 @@ Ext.define('PVE.node.CreateZFS', {
 	    throw "no node name specified";
 	}
 
-	let update_disklist = function() {
-	    let grid = me.down('#disklist');
-	    let disks = grid.getSelection();
-
-	    disks.sort(function(a, b) {
-		let aOrder = a.get('order') || 0;
-		let bOrder = b.get('order') || 0;
-		return aOrder - bOrder;
-	    });
-
-	    let selectedDevices = disks.map(disk => disk.get('devpath')).join(';');
-
-	    me.down('field[name=devices]').setValue(selectedDevices);
-	};
-
 	Ext.apply(me, {
 	    url: `/nodes/${me.nodename}/disks/zfs`,
 	    method: 'POST',
 	    items: [
 		{
 		    xtype: 'inputpanel',
-		    onGetValues: values => values, // FIXME leftover?
 		    column1: [
-			{
-			    xtype: 'textfield',
-			    hidden: true,
-			    name: 'devices',
-			    allowBlank: false,
-			},
 			{
 			    xtype: 'proxmoxtextfield',
 			    name: 'name',
@@ -99,62 +77,13 @@ Ext.define('PVE.node.CreateZFS', {
 		    ],
 		    columnB: [
 			{
-			    xtype: 'grid',
+			    xtype: 'pmxMultiDiskSelector',
+			    name: 'devices',
+			    nodename: me.nodename,
+			    diskType: 'unused',
 			    height: 200,
 			    emptyText: gettext('No Disks unused'),
 			    itemId: 'disklist',
-			    selModel: 'checkboxmodel',
-			    listeners: {
-				selectionchange: update_disklist,
-			    },
-			    store: {
-				proxy: {
-				    type: 'proxmox',
-				    url: `/api2/json/nodes/${me.nodename}/disks/list?type=unused`,
-				},
-			    },
-			    columns: [
-				{
-				    text: gettext('Device'),
-				    dataIndex: 'devpath',
-				    flex: 2,
-				},
-				{
-				    text: gettext('Model'),
-				    dataIndex: 'model',
-				    flex: 2,
-				},
-				{
-				    text: gettext('Serial'),
-				    dataIndex: 'serial',
-				    flex: 2,
-				},
-				{
-				    text: gettext('Size'),
-				    dataIndex: 'size',
-				    renderer: Proxmox.Utils.render_size,
-				    flex: 1,
-				},
-				{
-				    header: gettext('Order'),
-				    xtype: 'widgetcolumn',
-				    dataIndex: 'order',
-				    sortable: true,
-				    flex: 1,
-				    widget: {
-					xtype: 'proxmoxintegerfield',
-					minValue: 1,
-					isFormField: false,
-					listeners: {
-					    change: function(numberfield, value, old_value) {
-						let record = numberfield.getWidgetRecord();
-						record.set('order', value);
-						update_disklist(record);
-					    },
-					},
-				    },
-				},
-			    ],
 			},
 		    ],
 		},
@@ -170,7 +99,6 @@ Ext.define('PVE.node.CreateZFS', {
 	});
 
         me.callParent();
-	me.down('#disklist').getStore().load();
     },
 });
 
-- 
2.30.2

[pve-devel] [PATCH manager 9/9] partially fix #2285: ui: disk create: allow selecting partitions

[Fabian Ebner]

Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---

Depends on new widget-toolkit to have an effect.

 www/manager6/node/Directory.js | 1 +
 www/manager6/node/LVM.js       | 1 +
 www/manager6/node/LVMThin.js   | 1 +
 www/manager6/node/ZFS.js       | 1 +
 4 files changed, 4 insertions(+)

diff --git a/www/manager6/node/Directory.js b/www/manager6/node/Directory.js
index 50c8d677..abb562f5 100644
--- a/www/manager6/node/Directory.js
+++ b/www/manager6/node/Directory.js
@@ -26,6 +26,7 @@ Ext.define('PVE.node.CreateDirectory', {
 		    name: 'device',
 		    nodename: me.nodename,
 		    diskType: 'unused',
+		    includePartitions: true,
 		    fieldLabel: gettext('Disk'),
 		    allowBlank: false,
 		},
diff --git a/www/manager6/node/LVM.js b/www/manager6/node/LVM.js
index 4b5225d8..dc09bc8b 100644
--- a/www/manager6/node/LVM.js
+++ b/www/manager6/node/LVM.js
@@ -26,6 +26,7 @@ Ext.define('PVE.node.CreateLVM', {
 		    name: 'device',
 		    nodename: me.nodename,
 		    diskType: 'unused',
+		    includePartitions: true,
 		    fieldLabel: gettext('Disk'),
 		    allowBlank: false,
 		},
diff --git a/www/manager6/node/LVMThin.js b/www/manager6/node/LVMThin.js
index db9ea249..2af510ef 100644
--- a/www/manager6/node/LVMThin.js
+++ b/www/manager6/node/LVMThin.js
@@ -24,6 +24,7 @@ Ext.define('PVE.node.CreateLVMThin', {
 		    name: 'device',
 		    nodename: me.nodename,
 		    diskType: 'unused',
+		    includePartitions: true,
 		    fieldLabel: gettext('Disk'),
 		    allowBlank: false,
 		},
diff --git a/www/manager6/node/ZFS.js b/www/manager6/node/ZFS.js
index 2537ee5a..af475a21 100644
--- a/www/manager6/node/ZFS.js
+++ b/www/manager6/node/ZFS.js
@@ -81,6 +81,7 @@ Ext.define('PVE.node.CreateZFS', {
 			    name: 'devices',
 			    nodename: me.nodename,
 			    diskType: 'unused',
+			    includePartitions: true,
 			    height: 200,
 			    emptyText: gettext('No Disks unused'),
 			    itemId: 'disklist',
-- 
2.30.2

Re: [pve-devel] [PATCH storage 06/10] api: disks: initgpt: explicitly abort for partitions

[Thomas Lamprecht]

On 28.09.21 13:39, Fabian Ebner wrote:
> In preparation to extend disk_is_used to support partitions. Without
> this new check, initgpt would also allow partitions once disk_is_used
> supports partitions, which is not desirable.
> 
> Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
> ---
>  PVE/API2/Disks.pm |  1 +
>  PVE/Diskmanage.pm | 10 ++++++++--
>  2 files changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/PVE/API2/Disks.pm b/PVE/API2/Disks.pm
> index 96c19fd..25c9ded 100644
> --- a/PVE/API2/Disks.pm
> +++ b/PVE/API2/Disks.pm
> @@ -260,6 +260,7 @@ __PACKAGE__->register_method ({
>  
>  	my $authuser = $rpcenv->get_user();
>  
> +	die "$disk is a partition\n" if PVE::Diskmanage::is_partition($disk);
>  	die "disk $disk already in use\n" if PVE::Diskmanage::disk_is_used($disk);
>  	my $worker = sub {
>  	    PVE::Diskmanage::init_disk($disk, $param->{uuid});
> diff --git a/PVE/Diskmanage.pm b/PVE/Diskmanage.pm
> index 7aad707..73cbb8b 100644
> --- a/PVE/Diskmanage.pm
> +++ b/PVE/Diskmanage.pm
> @@ -57,8 +57,8 @@ sub init_disk {
>  
>      assert_blockdev($disk);
>  
> -    # we should already have checked if it is in use in the api call
> -    # but we check again for safety
> +    # we should already have checked these in the api call, but we check again for safety
> +    die "$disk is a partition\n" if is_partition($disk);
>      die "disk $disk is already in use\n" if disk_is_used($disk);
>  
>      my $id = $uuid || 'R';
> @@ -798,6 +798,12 @@ sub get_blockdev {
>      return $block_dev;
>  }
>  
> +sub is_partition {
> +    my ($dev_path) = @_;
> +
> +    return defined(eval { get_partnum($dev_path) });
> +}
> +

you add `is_partition` here but use it already in patch 04/10, can we reorder that?
Or maybe squash in the addition into 03/10?

>  sub locked_disk_action {
>      my ($sub) = @_;
>      my $res = PVE::Tools::lock_file('/run/lock/pve-diskmanage.lck', undef, $sub);
> 

[pve-devel] applied: [PATCH storage 01/10] api: disks: create: re-check disk after fork/lock

[Thomas Lamprecht]

On 28.09.21 13:39, Fabian Ebner wrote:
> Because then it might not be unused anymore. If there really is a
> race, this prevents e.g. sgdisk creating a partition on a device
> already in use by LVM or LVM destroying a partitioned device.
> 
> For ZFS, also get the latest udev info once inside the worker.
> 
> Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
> ---
>  PVE/API2/Disks/Directory.pm |  2 ++
>  PVE/API2/Disks/LVM.pm       |  2 ++
>  PVE/API2/Disks/LVMThin.pm   |  2 ++
>  PVE/API2/Disks/ZFS.pm       | 10 +++++++---
>  4 files changed, 13 insertions(+), 3 deletions(-)
> 
>

applied, thanks!

[pve-devel] applied: [PATCH storage 02/10] api: disk: work around udev bug to ensure its database is updated

[Thomas Lamprecht]

On 28.09.21 13:39, Fabian Ebner wrote:
> There is a udev bug [0] which can ultimately lead to the udev database
> for certain devices not being actively updated. Determining whether a
> disk is used or not in get_disks() (in part) relies upon lsblk, which
> queries the udev database. Ensure the information is updated by
> manually calling 'udevadm trigger' for the changed devices.
> 
> It's most important for the 'directory' API path, as mounting depends
> on the '/dev/disk/by-uuid'-symlink to be generated.
> 
> [0]: https://github.com/systemd/systemd/issues/18525
> 
> Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
> ---
>  PVE/API2/Disks.pm           | 11 ++++++++++-
>  PVE/API2/Disks/Directory.pm |  6 ++++++
>  PVE/API2/Disks/LVM.pm       |  8 +++++++-
>  PVE/API2/Disks/LVMThin.pm   |  6 ++++++
>  PVE/API2/Disks/ZFS.pm       |  6 ++++++
>  5 files changed, 35 insertions(+), 2 deletions(-)
> 
>

applied, thanks! albeit putting that into a method and only calling it like:

PVE::Diskmanage::workaround_stale_udev_db_bug(); # FIXME: remove in PVE 8.x

would have been slightly nicer, or at least more concise.
But anyhow, no biggie -> applied.

[pve-devel] applied: [PATCH storage 07/10] diskmanage: allow partitions for get_udev_info

[Thomas Lamprecht]

On 28.09.21 13:39, Fabian Ebner wrote:
> both existing callers only call this with non-partitions currently, so
> the change should be backwards compatible.
> 
> In preparation to enable ZFS creation on top of partitions (where the
> udev info is used to get the stable by-id path of a device).
> 
> Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
> ---
>  PVE/Diskmanage.pm | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
>

applied, thanks!

[pve-devel] applied: [PATCH storage 08/10] diskmanage: allow passing partitions to get_disks

[Thomas Lamprecht]

On 28.09.21 13:39, Fabian Ebner wrote:
> Requires that the $include_partitions parameter is set too, which:
> 1. Makes sense, because the partition won't be included in the result
>    otherwise.
> 2. Ensures backwards compatibility for existing callers that don't
>    use $include_partitions. No existing callers use both $disks and
>    $include_partitions at the same time, so nothing learns to
>    "support" partitions by accident.
> 
> Moving the strip_dev helper to the top, so it can be used everywhere.
> 
> Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
> ---
>  PVE/Diskmanage.pm | 20 ++++++++++++++------
>  1 file changed, 14 insertions(+), 6 deletions(-)
> 
>

applied, thanks!

[pve-devel] applied: [PATCH manager 3/9] api: ceph: create osd: work around udev bug

[Thomas Lamprecht]

On 28.09.21 13:39, Fabian Ebner wrote:
> There is a udev bug [0] which can ultimately lead to the udev database
> for certain devices not being actively updated. The Diskmanage package
> relies upon lsblk for certain info, and lsblk queries the udev
> database. Ensure the information is updated by manually calling
> 'udevadm trigger' for the changed devices.
> 
> Without the fix, and a bit of bad luck, a cleaned up disk could still
> show up as an 'LVM2_member' for example.
> 
> [0]: https://github.com/systemd/systemd/issues/18525
> 
> Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
> ---
>  PVE/API2/Ceph/OSD.pm | 32 +++++++++++++++++++++++++++++++-
>  1 file changed, 31 insertions(+), 1 deletion(-)
> 
>

applied, thanks!

[pve-devel] applied: [PATCH manager 2/9] api: check: create osd: use wipe_blockdev from the Diskmanage package

[Thomas Lamprecht]

On 28.09.21 13:39, Fabian Ebner wrote:
> which is mostly a copy of the wipe_disks helper with the difference
> that it also uses wipefs on the device and its partitions.
> 
> Remove the wipe_disks helper as no users remain.
> 
> Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
> ---
> 
> For this one, the dependency on libpve-storage-perl is already new
> enough.
> 
>  PVE/API2/Ceph/OSD.pm |  4 ++--
>  PVE/Ceph/Tools.pm    | 23 -----------------------
>  2 files changed, 2 insertions(+), 25 deletions(-)
> 
>

applied, thanks!

[pve-devel] applied: [PATCH manager 1/9] api: ceph: create osd: re-check disk requirements after fork/lock

[Thomas Lamprecht]

On 28.09.21 13:39, Fabian Ebner wrote:
> Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
> ---
>  PVE/API2/Ceph/OSD.pm | 54 +++++++++++++++++++++++++-------------------
>  1 file changed, 31 insertions(+), 23 deletions(-)
> 
>

applied, thanks!

[pve-devel] applied: [PATCH widget-toolkit 1/2] (multi) disk selector: allow requesting partitions too

[Thomas Lamprecht]

On 28.09.21 13:39, Fabian Ebner wrote:
> No functional change for existing users is intended.
> 
> Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
> ---
>  src/form/DiskSelector.js      |  7 +++++++
>  src/form/MultiDiskSelector.js | 22 +++++++++++++++++-----
>  2 files changed, 24 insertions(+), 5 deletions(-)
> 
>

applied, thanks!

Re: [pve-devel] [PATCH storage 06/10] api: disks: initgpt: explicitly abort for partitions

[Fabian Ebner]

Am 30.09.21 um 18:02 schrieb Thomas Lamprecht:
> On 28.09.21 13:39, Fabian Ebner wrote:
>> In preparation to extend disk_is_used to support partitions. Without
>> this new check, initgpt would also allow partitions once disk_is_used
>> supports partitions, which is not desirable.
>>
>> Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
>> ---
>>   PVE/API2/Disks.pm |  1 +
>>   PVE/Diskmanage.pm | 10 ++++++++--
>>   2 files changed, 9 insertions(+), 2 deletions(-)
>>
>> diff --git a/PVE/API2/Disks.pm b/PVE/API2/Disks.pm
>> index 96c19fd..25c9ded 100644
>> --- a/PVE/API2/Disks.pm
>> +++ b/PVE/API2/Disks.pm
>> @@ -260,6 +260,7 @@ __PACKAGE__->register_method ({
>>   
>>   	my $authuser = $rpcenv->get_user();
>>   
>> +	die "$disk is a partition\n" if PVE::Diskmanage::is_partition($disk);
>>   	die "disk $disk already in use\n" if PVE::Diskmanage::disk_is_used($disk);
>>   	my $worker = sub {
>>   	    PVE::Diskmanage::init_disk($disk, $param->{uuid});
>> diff --git a/PVE/Diskmanage.pm b/PVE/Diskmanage.pm
>> index 7aad707..73cbb8b 100644
>> --- a/PVE/Diskmanage.pm
>> +++ b/PVE/Diskmanage.pm
>> @@ -57,8 +57,8 @@ sub init_disk {
>>   
>>       assert_blockdev($disk);
>>   
>> -    # we should already have checked if it is in use in the api call
>> -    # but we check again for safety
>> +    # we should already have checked these in the api call, but we check again for safety
>> +    die "$disk is a partition\n" if is_partition($disk);
>>       die "disk $disk is already in use\n" if disk_is_used($disk);
>>   
>>       my $id = $uuid || 'R';
>> @@ -798,6 +798,12 @@ sub get_blockdev {
>>       return $block_dev;
>>   }
>>   
>> +sub is_partition {
>> +    my ($dev_path) = @_;
>> +
>> +    return defined(eval { get_partnum($dev_path) });
>> +}
>> +
> 
> you add `is_partition` here but use it already in patch 04/10, can we reorder that?
> Or maybe squash in the addition into 03/10?
> 

Sorry about that. I think I had the condition inlined in 04/10 at first 
and forgot to re-order after I introduced and switched to the helper.

I'll send a v2 and squash the addition into 03/10.

>>   sub locked_disk_action {
>>       my ($sub) = @_;
>>       my $res = PVE::Tools::lock_file('/run/lock/pve-diskmanage.lck', undef, $sub);
>>
>