* [pve-devel] [PATCH storage 01/10] api: disks: create: re-check disk after fork/lock
2021-09-28 11:39 [pve-devel] [PATCH-SERIES storage/widget-toolkit/manager] disk creation and wiping improvements Fabian Ebner
@ 2021-09-28 11:39 ` Fabian Ebner
2021-09-30 16:06 ` [pve-devel] applied: " Thomas Lamprecht
2021-09-28 11:39 ` [pve-devel] [PATCH storage 02/10] api: disk: work around udev bug to ensure its database is updated Fabian Ebner
` (19 subsequent siblings)
20 siblings, 1 reply; 32+ messages in thread
From: Fabian Ebner @ 2021-09-28 11:39 UTC (permalink / raw)
To: pve-devel
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
^ permalink raw reply [flat|nested] 32+ messages in thread* [pve-devel] applied: [PATCH storage 01/10] api: disks: create: re-check disk after fork/lock
2021-09-28 11:39 ` [pve-devel] [PATCH storage 01/10] api: disks: create: re-check disk after fork/lock Fabian Ebner
@ 2021-09-30 16:06 ` Thomas Lamprecht
0 siblings, 0 replies; 32+ messages in thread
From: Thomas Lamprecht @ 2021-09-30 16:06 UTC (permalink / raw)
To: Proxmox VE development discussion, Fabian Ebner
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!
^ permalink raw reply [flat|nested] 32+ messages in thread
* [pve-devel] [PATCH storage 02/10] api: disk: work around udev bug to ensure its database is updated
2021-09-28 11:39 [pve-devel] [PATCH-SERIES storage/widget-toolkit/manager] disk creation and wiping improvements Fabian Ebner
2021-09-28 11:39 ` [pve-devel] [PATCH storage 01/10] api: disks: create: re-check disk after fork/lock Fabian Ebner
@ 2021-09-28 11:39 ` Fabian Ebner
2021-09-30 16:08 ` [pve-devel] applied: " Thomas Lamprecht
2021-09-28 11:39 ` [pve-devel] [PATCH storage 03/10] diskmanage: add change_parttype helper Fabian Ebner
` (18 subsequent siblings)
20 siblings, 1 reply; 32+ messages in thread
From: Fabian Ebner @ 2021-09-28 11:39 UTC (permalink / raw)
To: pve-devel
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
^ permalink raw reply [flat|nested] 32+ messages in thread* [pve-devel] applied: [PATCH storage 02/10] api: disk: work around udev bug to ensure its database is updated
2021-09-28 11:39 ` [pve-devel] [PATCH storage 02/10] api: disk: work around udev bug to ensure its database is updated Fabian Ebner
@ 2021-09-30 16:08 ` Thomas Lamprecht
0 siblings, 0 replies; 32+ messages in thread
From: Thomas Lamprecht @ 2021-09-30 16:08 UTC (permalink / raw)
To: Proxmox VE development discussion, Fabian Ebner
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.
^ permalink raw reply [flat|nested] 32+ messages in thread
* [pve-devel] [PATCH storage 03/10] diskmanage: add change_parttype helper
2021-09-28 11:39 [pve-devel] [PATCH-SERIES storage/widget-toolkit/manager] disk creation and wiping improvements Fabian Ebner
2021-09-28 11:39 ` [pve-devel] [PATCH storage 01/10] api: disks: create: re-check disk after fork/lock Fabian Ebner
2021-09-28 11:39 ` [pve-devel] [PATCH storage 02/10] api: disk: work around udev bug to ensure its database is updated Fabian Ebner
@ 2021-09-28 11:39 ` Fabian Ebner
2021-09-28 11:39 ` [pve-devel] [PATCH storage 04/10] diskmanage: wipe blockdev: also change partition type Fabian Ebner
` (17 subsequent siblings)
20 siblings, 0 replies; 32+ messages in thread
From: Fabian Ebner @ 2021-09-28 11:39 UTC (permalink / raw)
To: pve-devel
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
^ permalink raw reply [flat|nested] 32+ messages in thread* [pve-devel] [PATCH storage 04/10] diskmanage: wipe blockdev: also change partition type
2021-09-28 11:39 [pve-devel] [PATCH-SERIES storage/widget-toolkit/manager] disk creation and wiping improvements Fabian Ebner
` (2 preceding siblings ...)
2021-09-28 11:39 ` [pve-devel] [PATCH storage 03/10] diskmanage: add change_parttype helper Fabian Ebner
@ 2021-09-28 11:39 ` Fabian Ebner
2021-09-28 11:39 ` [pve-devel] [PATCH storage 05/10] diskmanage: don't set usage for unused partitions Fabian Ebner
` (16 subsequent siblings)
20 siblings, 0 replies; 32+ messages in thread
From: Fabian Ebner @ 2021-09-28 11:39 UTC (permalink / raw)
To: pve-devel
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
^ permalink raw reply [flat|nested] 32+ messages in thread* [pve-devel] [PATCH storage 05/10] diskmanage: don't set usage for unused partitions
2021-09-28 11:39 [pve-devel] [PATCH-SERIES storage/widget-toolkit/manager] disk creation and wiping improvements Fabian Ebner
` (3 preceding siblings ...)
2021-09-28 11:39 ` [pve-devel] [PATCH storage 04/10] diskmanage: wipe blockdev: also change partition type Fabian Ebner
@ 2021-09-28 11:39 ` Fabian Ebner
2021-09-28 11:39 ` [pve-devel] [PATCH storage 06/10] api: disks: initgpt: explicitly abort for partitions Fabian Ebner
` (15 subsequent siblings)
20 siblings, 0 replies; 32+ messages in thread
From: Fabian Ebner @ 2021-09-28 11:39 UTC (permalink / raw)
To: pve-devel
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
^ permalink raw reply [flat|nested] 32+ messages in thread* [pve-devel] [PATCH storage 06/10] api: disks: initgpt: explicitly abort for partitions
2021-09-28 11:39 [pve-devel] [PATCH-SERIES storage/widget-toolkit/manager] disk creation and wiping improvements Fabian Ebner
` (4 preceding siblings ...)
2021-09-28 11:39 ` [pve-devel] [PATCH storage 05/10] diskmanage: don't set usage for unused partitions Fabian Ebner
@ 2021-09-28 11:39 ` Fabian Ebner
2021-09-30 16:02 ` Thomas Lamprecht
2021-09-28 11:39 ` [pve-devel] [PATCH storage 07/10] diskmanage: allow partitions for get_udev_info Fabian Ebner
` (14 subsequent siblings)
20 siblings, 1 reply; 32+ messages in thread
From: Fabian Ebner @ 2021-09-28 11:39 UTC (permalink / raw)
To: pve-devel
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
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [pve-devel] [PATCH storage 06/10] api: disks: initgpt: explicitly abort for partitions
2021-09-28 11:39 ` [pve-devel] [PATCH storage 06/10] api: disks: initgpt: explicitly abort for partitions Fabian Ebner
@ 2021-09-30 16:02 ` Thomas Lamprecht
2021-10-06 7:11 ` Fabian Ebner
0 siblings, 1 reply; 32+ messages in thread
From: Thomas Lamprecht @ 2021-09-30 16:02 UTC (permalink / raw)
To: Proxmox VE development discussion, Fabian Ebner
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);
>
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [pve-devel] [PATCH storage 06/10] api: disks: initgpt: explicitly abort for partitions
2021-09-30 16:02 ` Thomas Lamprecht
@ 2021-10-06 7:11 ` Fabian Ebner
0 siblings, 0 replies; 32+ messages in thread
From: Fabian Ebner @ 2021-10-06 7:11 UTC (permalink / raw)
To: Thomas Lamprecht, Proxmox VE development discussion
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);
>>
>
^ permalink raw reply [flat|nested] 32+ messages in thread
* [pve-devel] [PATCH storage 07/10] diskmanage: allow partitions for get_udev_info
2021-09-28 11:39 [pve-devel] [PATCH-SERIES storage/widget-toolkit/manager] disk creation and wiping improvements Fabian Ebner
` (5 preceding siblings ...)
2021-09-28 11:39 ` [pve-devel] [PATCH storage 06/10] api: disks: initgpt: explicitly abort for partitions Fabian Ebner
@ 2021-09-28 11:39 ` Fabian Ebner
2021-09-30 16:10 ` [pve-devel] applied: " Thomas Lamprecht
2021-09-28 11:39 ` [pve-devel] [PATCH storage 08/10] diskmanage: allow passing partitions to get_disks Fabian Ebner
` (13 subsequent siblings)
20 siblings, 1 reply; 32+ messages in thread
From: Fabian Ebner @ 2021-09-28 11:39 UTC (permalink / raw)
To: pve-devel
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
^ permalink raw reply [flat|nested] 32+ messages in thread* [pve-devel] [PATCH storage 08/10] diskmanage: allow passing partitions to get_disks
2021-09-28 11:39 [pve-devel] [PATCH-SERIES storage/widget-toolkit/manager] disk creation and wiping improvements Fabian Ebner
` (6 preceding siblings ...)
2021-09-28 11:39 ` [pve-devel] [PATCH storage 07/10] diskmanage: allow partitions for get_udev_info Fabian Ebner
@ 2021-09-28 11:39 ` Fabian Ebner
2021-09-30 16:10 ` [pve-devel] applied: " Thomas Lamprecht
2021-09-28 11:39 ` [pve-devel] [PATCH storage 09/10] partially fix #2285: api: disks: allow partitions for creation paths Fabian Ebner
` (12 subsequent siblings)
20 siblings, 1 reply; 32+ messages in thread
From: Fabian Ebner @ 2021-09-28 11:39 UTC (permalink / raw)
To: pve-devel
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
^ permalink raw reply [flat|nested] 32+ messages in thread* [pve-devel] applied: [PATCH storage 08/10] diskmanage: allow passing partitions to get_disks
2021-09-28 11:39 ` [pve-devel] [PATCH storage 08/10] diskmanage: allow passing partitions to get_disks Fabian Ebner
@ 2021-09-30 16:10 ` Thomas Lamprecht
0 siblings, 0 replies; 32+ messages in thread
From: Thomas Lamprecht @ 2021-09-30 16:10 UTC (permalink / raw)
To: Proxmox VE development discussion, Fabian Ebner
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!
^ permalink raw reply [flat|nested] 32+ messages in thread
* [pve-devel] [PATCH storage 09/10] partially fix #2285: api: disks: allow partitions for creation paths
2021-09-28 11:39 [pve-devel] [PATCH-SERIES storage/widget-toolkit/manager] disk creation and wiping improvements Fabian Ebner
` (7 preceding siblings ...)
2021-09-28 11:39 ` [pve-devel] [PATCH storage 08/10] diskmanage: allow passing partitions to get_disks Fabian Ebner
@ 2021-09-28 11:39 ` Fabian Ebner
2021-09-28 11:39 ` [pve-devel] [PATCH storage 10/10] api: disks: create: set correct partition type Fabian Ebner
` (11 subsequent siblings)
20 siblings, 0 replies; 32+ messages in thread
From: Fabian Ebner @ 2021-09-28 11:39 UTC (permalink / raw)
To: pve-devel
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
^ permalink raw reply [flat|nested] 32+ messages in thread* [pve-devel] [PATCH storage 10/10] api: disks: create: set correct partition type
2021-09-28 11:39 [pve-devel] [PATCH-SERIES storage/widget-toolkit/manager] disk creation and wiping improvements Fabian Ebner
` (8 preceding siblings ...)
2021-09-28 11:39 ` [pve-devel] [PATCH storage 09/10] partially fix #2285: api: disks: allow partitions for creation paths Fabian Ebner
@ 2021-09-28 11:39 ` Fabian Ebner
2021-09-28 11:39 ` [pve-devel] [PATCH widget-toolkit 1/2] (multi) disk selector: allow requesting partitions too Fabian Ebner
` (10 subsequent siblings)
20 siblings, 0 replies; 32+ messages in thread
From: Fabian Ebner @ 2021-09-28 11:39 UTC (permalink / raw)
To: pve-devel
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
^ permalink raw reply [flat|nested] 32+ messages in thread* [pve-devel] [PATCH widget-toolkit 1/2] (multi) disk selector: allow requesting partitions too
2021-09-28 11:39 [pve-devel] [PATCH-SERIES storage/widget-toolkit/manager] disk creation and wiping improvements Fabian Ebner
` (9 preceding siblings ...)
2021-09-28 11:39 ` [pve-devel] [PATCH storage 10/10] api: disks: create: set correct partition type Fabian Ebner
@ 2021-09-28 11:39 ` Fabian Ebner
2021-09-30 16:13 ` [pve-devel] applied: " Thomas Lamprecht
2021-09-28 11:39 ` [pve-devel] [PATCH widget-toolkit 2/2] disk list: allow wiping individual partitions Fabian Ebner
` (9 subsequent siblings)
20 siblings, 1 reply; 32+ messages in thread
From: Fabian Ebner @ 2021-09-28 11:39 UTC (permalink / raw)
To: pve-devel
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
^ permalink raw reply [flat|nested] 32+ messages in thread* [pve-devel] [PATCH widget-toolkit 2/2] disk list: allow wiping individual partitions
2021-09-28 11:39 [pve-devel] [PATCH-SERIES storage/widget-toolkit/manager] disk creation and wiping improvements Fabian Ebner
` (10 preceding siblings ...)
2021-09-28 11:39 ` [pve-devel] [PATCH widget-toolkit 1/2] (multi) disk selector: allow requesting partitions too Fabian Ebner
@ 2021-09-28 11:39 ` Fabian Ebner
2021-09-28 11:39 ` [pve-devel] [PATCH manager 1/9] api: ceph: create osd: re-check disk requirements after fork/lock Fabian Ebner
` (8 subsequent siblings)
20 siblings, 0 replies; 32+ messages in thread
From: Fabian Ebner @ 2021-09-28 11:39 UTC (permalink / raw)
To: pve-devel
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
^ permalink raw reply [flat|nested] 32+ messages in thread* [pve-devel] [PATCH manager 1/9] api: ceph: create osd: re-check disk requirements after fork/lock
2021-09-28 11:39 [pve-devel] [PATCH-SERIES storage/widget-toolkit/manager] disk creation and wiping improvements Fabian Ebner
` (11 preceding siblings ...)
2021-09-28 11:39 ` [pve-devel] [PATCH widget-toolkit 2/2] disk list: allow wiping individual partitions Fabian Ebner
@ 2021-09-28 11:39 ` Fabian Ebner
2021-09-30 16:12 ` [pve-devel] applied: " Thomas Lamprecht
2021-09-28 11:39 ` [pve-devel] [PATCH manager 2/9] api: check: create osd: use wipe_blockdev from the Diskmanage package Fabian Ebner
` (7 subsequent siblings)
20 siblings, 1 reply; 32+ messages in thread
From: Fabian Ebner @ 2021-09-28 11:39 UTC (permalink / raw)
To: pve-devel
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
^ permalink raw reply [flat|nested] 32+ messages in thread* [pve-devel] [PATCH manager 2/9] api: check: create osd: use wipe_blockdev from the Diskmanage package
2021-09-28 11:39 [pve-devel] [PATCH-SERIES storage/widget-toolkit/manager] disk creation and wiping improvements Fabian Ebner
` (12 preceding siblings ...)
2021-09-28 11:39 ` [pve-devel] [PATCH manager 1/9] api: ceph: create osd: re-check disk requirements after fork/lock Fabian Ebner
@ 2021-09-28 11:39 ` Fabian Ebner
2021-09-30 16:12 ` [pve-devel] applied: " Thomas Lamprecht
2021-09-28 11:39 ` [pve-devel] [PATCH manager 3/9] api: ceph: create osd: work around udev bug Fabian Ebner
` (6 subsequent siblings)
20 siblings, 1 reply; 32+ messages in thread
From: Fabian Ebner @ 2021-09-28 11:39 UTC (permalink / raw)
To: pve-devel
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
^ permalink raw reply [flat|nested] 32+ messages in thread* [pve-devel] [PATCH manager 3/9] api: ceph: create osd: work around udev bug
2021-09-28 11:39 [pve-devel] [PATCH-SERIES storage/widget-toolkit/manager] disk creation and wiping improvements Fabian Ebner
` (13 preceding siblings ...)
2021-09-28 11:39 ` [pve-devel] [PATCH manager 2/9] api: check: create osd: use wipe_blockdev from the Diskmanage package Fabian Ebner
@ 2021-09-28 11:39 ` Fabian Ebner
2021-09-30 16:12 ` [pve-devel] applied: " Thomas Lamprecht
2021-09-28 11:39 ` [pve-devel] [PATCH manager 4/9] api: ceph: create osd: set correct parttype for DB/WAL Fabian Ebner
` (5 subsequent siblings)
20 siblings, 1 reply; 32+ messages in thread
From: Fabian Ebner @ 2021-09-28 11:39 UTC (permalink / raw)
To: pve-devel
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
^ permalink raw reply [flat|nested] 32+ messages in thread* [pve-devel] applied: [PATCH manager 3/9] api: ceph: create osd: work around udev bug
2021-09-28 11:39 ` [pve-devel] [PATCH manager 3/9] api: ceph: create osd: work around udev bug Fabian Ebner
@ 2021-09-30 16:12 ` Thomas Lamprecht
0 siblings, 0 replies; 32+ messages in thread
From: Thomas Lamprecht @ 2021-09-30 16:12 UTC (permalink / raw)
To: Proxmox VE development discussion, Fabian Ebner
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!
^ permalink raw reply [flat|nested] 32+ messages in thread
* [pve-devel] [PATCH manager 4/9] api: ceph: create osd: set correct parttype for DB/WAL
2021-09-28 11:39 [pve-devel] [PATCH-SERIES storage/widget-toolkit/manager] disk creation and wiping improvements Fabian Ebner
` (14 preceding siblings ...)
2021-09-28 11:39 ` [pve-devel] [PATCH manager 3/9] api: ceph: create osd: work around udev bug Fabian Ebner
@ 2021-09-28 11:39 ` Fabian Ebner
2021-09-28 11:39 ` [pve-devel] [RFC manager 5/9] partially fix #2285: api: ceph: create osd: allow using partitions Fabian Ebner
` (4 subsequent siblings)
20 siblings, 0 replies; 32+ messages in thread
From: Fabian Ebner @ 2021-09-28 11:39 UTC (permalink / raw)
To: pve-devel
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
^ permalink raw reply [flat|nested] 32+ messages in thread* [pve-devel] [RFC manager 5/9] partially fix #2285: api: ceph: create osd: allow using partitions
2021-09-28 11:39 [pve-devel] [PATCH-SERIES storage/widget-toolkit/manager] disk creation and wiping improvements Fabian Ebner
` (15 preceding siblings ...)
2021-09-28 11:39 ` [pve-devel] [PATCH manager 4/9] api: ceph: create osd: set correct parttype for DB/WAL Fabian Ebner
@ 2021-09-28 11:39 ` Fabian Ebner
2021-09-28 11:39 ` [pve-devel] [RFC manager 6/9] api: ceph: create osd: set correct partition type Fabian Ebner
` (3 subsequent siblings)
20 siblings, 0 replies; 32+ messages in thread
From: Fabian Ebner @ 2021-09-28 11:39 UTC (permalink / raw)
To: pve-devel
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
^ permalink raw reply [flat|nested] 32+ messages in thread* [pve-devel] [RFC manager 6/9] api: ceph: create osd: set correct partition type
2021-09-28 11:39 [pve-devel] [PATCH-SERIES storage/widget-toolkit/manager] disk creation and wiping improvements Fabian Ebner
` (16 preceding siblings ...)
2021-09-28 11:39 ` [pve-devel] [RFC manager 5/9] partially fix #2285: api: ceph: create osd: allow using partitions Fabian Ebner
@ 2021-09-28 11:39 ` Fabian Ebner
2021-09-28 11:39 ` [pve-devel] [RFC manager 7/9] partially fix #2285: ui: ceph: allow selecting partitions Fabian Ebner
` (2 subsequent siblings)
20 siblings, 0 replies; 32+ messages in thread
From: Fabian Ebner @ 2021-09-28 11:39 UTC (permalink / raw)
To: pve-devel
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
^ permalink raw reply [flat|nested] 32+ messages in thread* [pve-devel] [RFC manager 7/9] partially fix #2285: ui: ceph: allow selecting partitions
2021-09-28 11:39 [pve-devel] [PATCH-SERIES storage/widget-toolkit/manager] disk creation and wiping improvements Fabian Ebner
` (17 preceding siblings ...)
2021-09-28 11:39 ` [pve-devel] [RFC manager 6/9] api: ceph: create osd: set correct partition type Fabian Ebner
@ 2021-09-28 11:39 ` Fabian Ebner
2021-09-28 11:40 ` [pve-devel] [PATCH manager 8/9] ui: zfs create: switch to using widget-toolkit's multiDiskSelector Fabian Ebner
2021-09-28 11:40 ` [pve-devel] [PATCH manager 9/9] partially fix #2285: ui: disk create: allow selecting partitions Fabian Ebner
20 siblings, 0 replies; 32+ messages in thread
From: Fabian Ebner @ 2021-09-28 11:39 UTC (permalink / raw)
To: pve-devel
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
^ permalink raw reply [flat|nested] 32+ messages in thread* [pve-devel] [PATCH manager 8/9] ui: zfs create: switch to using widget-toolkit's multiDiskSelector
2021-09-28 11:39 [pve-devel] [PATCH-SERIES storage/widget-toolkit/manager] disk creation and wiping improvements Fabian Ebner
` (18 preceding siblings ...)
2021-09-28 11:39 ` [pve-devel] [RFC manager 7/9] partially fix #2285: ui: ceph: allow selecting partitions Fabian Ebner
@ 2021-09-28 11:40 ` Fabian Ebner
2021-09-28 11:40 ` [pve-devel] [PATCH manager 9/9] partially fix #2285: ui: disk create: allow selecting partitions Fabian Ebner
20 siblings, 0 replies; 32+ messages in thread
From: Fabian Ebner @ 2021-09-28 11:40 UTC (permalink / raw)
To: pve-devel
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
^ permalink raw reply [flat|nested] 32+ messages in thread* [pve-devel] [PATCH manager 9/9] partially fix #2285: ui: disk create: allow selecting partitions
2021-09-28 11:39 [pve-devel] [PATCH-SERIES storage/widget-toolkit/manager] disk creation and wiping improvements Fabian Ebner
` (19 preceding siblings ...)
2021-09-28 11:40 ` [pve-devel] [PATCH manager 8/9] ui: zfs create: switch to using widget-toolkit's multiDiskSelector Fabian Ebner
@ 2021-09-28 11:40 ` Fabian Ebner
20 siblings, 0 replies; 32+ messages in thread
From: Fabian Ebner @ 2021-09-28 11:40 UTC (permalink / raw)
To: pve-devel
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
^ permalink raw reply [flat|nested] 32+ messages in thread