[pve-devel] [PATCH installer 0/2] fix 2 bugs related to hdsize selection

[pve-devel] [PATCH installer 0/2] fix 2 bugs related to hdsize selection

[Stoiko Ivanov]

noticed that these 2 bugs were still open in bugzilla (and assigned to me).

The issues are independent of each other - but the fixes touch the same part
of the code - hence I'm sending them in a common series.

roughly tested with a VM (scp'ing the updated proxinstall from the
second debug shell)


Stoiko Ivanov (2):
  fix #3188: update hdsize spin-button on disk-selection change
  fix #3587: make hdsize configurable for btrfs setups

 proxinstall | 95 ++++++++++++++++++++++++++++++++++++-----------------
 1 file changed, 65 insertions(+), 30 deletions(-)

-- 
2.30.2

[pve-devel] [PATCH installer 1/2] fix #3188: update hdsize spin-button on disk-selection change

[Stoiko Ivanov]

the patch splits get_hdsize_spinbtn into a sub for generating the adjustment
gtk-element and one for the button itself and moves them above
create_raid_disk_raid (so that the adjustment sub can be called)

The adjustment gets it's data from the disks selected for installation
in the GUI, if a size is not provided.

Additionally the bogus 'zfs' argument to the create_raid_advanced_grid
was dropped.

Tested with a VM with 2x 100G disks, and one with 20G

Signed-off-by: Stoiko Ivanov <s.ivanov@proxmox.com>
---
 proxinstall | 78 ++++++++++++++++++++++++++++++++++-------------------
 1 file changed, 50 insertions(+), 28 deletions(-)

diff --git a/proxinstall b/proxinstall
index 8ec7d2c..5360b08 100755
--- a/proxinstall
+++ b/proxinstall
@@ -2950,7 +2950,40 @@ my $create_label_widget_grid = sub {
     return $grid;
 };
 
+my $get_hdsize_adjustment = sub {
+    my $hdsize = shift;
+
+    if (!defined($hdsize)) {
+	#compute based on selected disks
+	my $hd_count = scalar(@$hds);
+	for (my $i = 0; $i < $hd_count; $i++) {
+	    if (defined(my $cur_hd = $config_options->{"disksel$i"})) {
+		my $disksize = int(@$cur_hd[2] / (2*1024*1024.0)); # size in GB
+		$hdsize //= $disksize;
+		$hdsize = $disksize if $disksize < $hdsize;
+	    }
+	}
+    }
+    die "could not determine hdsize for adjustment!\n" if !$hdsize;
+
+    return Gtk3::Adjustment->new($config_options->{hdsize} || $hdsize, 0, $hdsize+1, 1, 1, 1);
+};
+
+my $get_hdsize_spinbtn = sub {
+    my $hdsize = shift;
+
+    my $hdsize_entry_buffer = Gtk3::EntryBuffer->new(undef, 1);
+    my $hdsize_size_adj = $get_hdsize_adjustment->($hdsize);
+
+    my $spinbutton_hdsize = Gtk3::SpinButton->new($hdsize_size_adj, 1, 1);
+    $spinbutton_hdsize->set_buffer($hdsize_entry_buffer);
+    $spinbutton_hdsize->set_adjustment($hdsize_size_adj);
+    $spinbutton_hdsize->set_tooltip_text("only use specified size (GB) of the harddisk (rest left unpartitioned)");
+    return $spinbutton_hdsize;
+};
+
 my $create_raid_disk_grid = sub {
+    my ($hdsize_btn) = @_;
     my $hd_count = scalar(@$hds);
     my $disk_labeled_widgets = [];
     for (my $i = 0; $i < $hd_count; $i++) {
@@ -2967,6 +3000,8 @@ my $create_raid_disk_grid = sub {
 		my $diskid = $w->{pve_disk_id};
 		my $a = $w->get_active - 1;
 		$config_options->{"disksel${diskid}"} = ($a >= 0) ? $hds->[$a] : undef;
+		my $hdsize_adj = $get_hdsize_adjustment->();
+		$hdsize_btn->set_adjustment($hdsize_adj);
 	    });
 	}
 
@@ -3022,30 +3057,8 @@ my $create_raid_disk_grid = sub {
     return $vbox;
 };
 
-# shared between different ui parts (e.g., ZFS and "normal" single disk FS)
-my $hdsize_size_adj;
-my $hdsize_entry_buffer;
-
-my $get_hdsize_spinbtn = sub {
-    my $hdsize = shift;
-
-    $hdsize_entry_buffer //= Gtk3::EntryBuffer->new(undef, 1);
-
-    if (defined($hdsize)) {
-	$hdsize_size_adj = Gtk3::Adjustment->new($config_options->{hdsize} || $hdsize, 0, $hdsize+1, 1, 1, 1);
-    } else {
-	die "called get_hdsize_spinbtn with \$hdsize_size_adj not defined but did not pass hdsize!\n"
-	    if !defined($hdsize_size_adj);
-    }
-
-    my $spinbutton_hdsize = Gtk3::SpinButton->new($hdsize_size_adj, 1, 1);
-    $spinbutton_hdsize->set_buffer($hdsize_entry_buffer);
-    $spinbutton_hdsize->set_adjustment($hdsize_size_adj);
-    $spinbutton_hdsize->set_tooltip_text("only use specified size (GB) of the harddisk (rest left unpartitioned)");
-    return $spinbutton_hdsize;
-};
-
 my $create_raid_advanced_grid = sub {
+    my ($hdsize_btn) = @_;
     my $labeled_widgets = [];
     my $spinbutton_ashift = Gtk3::SpinButton->new_with_range(9, 13, 1);
     $spinbutton_ashift->set_tooltip_text("zpool ashift property (pool sector size, default 2^12)");
@@ -3098,7 +3111,7 @@ my $create_raid_advanced_grid = sub {
     $spinbutton_copies->set_value($config_options->{copies});
     push @$labeled_widgets, "copies", $spinbutton_copies;
 
-    push @$labeled_widgets, "hdsize", $get_hdsize_spinbtn->();
+    push @$labeled_widgets, "hdsize", $hdsize_btn;
     return $create_label_widget_grid->($labeled_widgets);;
 };
 
@@ -3176,8 +3189,9 @@ sub create_hdoption_view {
 	$hdsize = int((-s $target_hd) / (1024*1024*1024.0));
     }
 
-    my $spinbutton_hdsize = $get_hdsize_spinbtn->($hdsize);
-    push @$hdsize_labeled_widgets, "hdsize", $spinbutton_hdsize;
+    my $spinbutton_hdsize_nonraid = $get_hdsize_spinbtn->($hdsize);
+    push @$hdsize_labeled_widgets, "hdsize", $spinbutton_hdsize_nonraid;
+    my $spinbutton_hdsize = $spinbutton_hdsize_nonraid;
 
     my $entry_swapsize = Gtk3::Entry->new();
     $entry_swapsize->set_tooltip_text("maximum SWAP size (GB)");
@@ -3208,13 +3222,14 @@ sub create_hdoption_view {
 	push @$hdsize_labeled_widgets, "maxvz", $entry_maxvz;
     }
 
+    my $spinbutton_hdsize_zfs = $get_hdsize_spinbtn->($hdsize);
     my $options_stack = Gtk3::Stack->new();
     $options_stack->set_visible(1);
     $options_stack->set_hexpand(1);
     $options_stack->set_vexpand(1);
-    $options_stack->add_titled(&$create_raid_disk_grid(), "raiddisk", "Disk Setup");
+    $options_stack->add_titled(&$create_raid_disk_grid($spinbutton_hdsize_zfs), "raiddisk", "Disk Setup");
     $options_stack->add_titled(&$create_label_widget_grid($hdsize_labeled_widgets), "hdsize", "Size Options");
-    $options_stack->add_titled(&$create_raid_advanced_grid("zfs"), "raidzfsadvanced", "Advanced Options");
+    $options_stack->add_titled(&$create_raid_advanced_grid($spinbutton_hdsize_zfs), "raidzfsadvanced", "Advanced Options");
     $options_stack->set_visible_child_name("raiddisk");
     my $options_stack_switcher = Gtk3::StackSwitcher->new();
     $options_stack_switcher->set_halign('center');
@@ -3250,6 +3265,13 @@ sub create_hdoption_view {
 	} else {
 	    $target_hd_label->set_text("Target Harddisk: ");
 	}
+
+	if ($raid) {
+	    $spinbutton_hdsize = $spinbutton_hdsize_zfs;
+	} else {
+	    $spinbutton_hdsize = $spinbutton_hdsize_nonraid;
+	}
+
 	my (undef, $pref_width) = $dialog->get_preferred_width();
 	my (undef, $pref_height) = $dialog->get_preferred_height();
 	$pref_height = 750 if $pref_height > 750;
-- 
2.30.2

[pve-devel] [PATCH installer 2/2] fix #3587: make hdsize configurable for btrfs setups

[Stoiko Ivanov]

as described in the bug-entry it is still not possible to have a
swapfile on general btrfs setups (only on single disk (single data
profile - documented in [0,1], and my quick tests confirmed it).

Users who still need/want swap can now set a hdsize smaller than their
disk-size to keep a part unpartitioned for adding a swap-partition
after installation (like with ZFS).

I quickly considered sticking with a single 'advanced raid' tab and
adapting the visibility of the individual lines, but did not see an
elegant way (as far as this is possible with GUI code) of doing that.

Signed-off-by: Stoiko Ivanov <s.ivanov@proxmox.com>
---
 proxinstall | 25 +++++++++++++++++++------
 1 file changed, 19 insertions(+), 6 deletions(-)

diff --git a/proxinstall b/proxinstall
index 5360b08..93f2443 100755
--- a/proxinstall
+++ b/proxinstall
@@ -1341,7 +1341,7 @@ sub extract_data {
 		my $logical_bsize = @$hd[4];
 
 		my ($size, $osdev, $efidev) =
-		    partition_bootable_disk($devname, undef, '8300');
+		    partition_bootable_disk($devname, $config_options->{hdsize}, '8300');
 		$rootdev = $osdev if !defined($rootdev); # simply point to first disk
 		my $by_id = find_stable_path("/dev/disk/by-id", $devname);
 		push @$bootdevinfo, {
@@ -2983,7 +2983,7 @@ my $get_hdsize_spinbtn = sub {
 };
 
 my $create_raid_disk_grid = sub {
-    my ($hdsize_btn) = @_;
+    my ($hdsize_buttons) = @_;
     my $hd_count = scalar(@$hds);
     my $disk_labeled_widgets = [];
     for (my $i = 0; $i < $hd_count; $i++) {
@@ -3001,7 +3001,9 @@ my $create_raid_disk_grid = sub {
 		my $a = $w->get_active - 1;
 		$config_options->{"disksel${diskid}"} = ($a >= 0) ? $hds->[$a] : undef;
 		my $hdsize_adj = $get_hdsize_adjustment->();
-		$hdsize_btn->set_adjustment($hdsize_adj);
+		for my $btn (@$hdsize_buttons) {
+		    $btn->set_adjustment($hdsize_adj);
+		}
 	    });
 	}
 
@@ -3115,6 +3117,13 @@ my $create_raid_advanced_grid = sub {
     return $create_label_widget_grid->($labeled_widgets);;
 };
 
+my $create_btrfs_raid_advanced_grid = sub {
+    my ($hdsize_btn) = @_;
+    my $labeled_widgets = [];
+    push @$labeled_widgets, "hdsize", $hdsize_btn;
+    return $create_label_widget_grid->($labeled_widgets);;
+};
+
 sub create_hdoption_view {
 
     my $dialog = Gtk3::Dialog->new();
@@ -3223,13 +3232,16 @@ sub create_hdoption_view {
     }
 
     my $spinbutton_hdsize_zfs = $get_hdsize_spinbtn->($hdsize);
+    my $spinbutton_hdsize_btrfs = $get_hdsize_spinbtn->($hdsize);
+    my $hdsize_buttons = [ $spinbutton_hdsize_zfs, $spinbutton_hdsize_btrfs ];
     my $options_stack = Gtk3::Stack->new();
     $options_stack->set_visible(1);
     $options_stack->set_hexpand(1);
     $options_stack->set_vexpand(1);
-    $options_stack->add_titled(&$create_raid_disk_grid($spinbutton_hdsize_zfs), "raiddisk", "Disk Setup");
+    $options_stack->add_titled(&$create_raid_disk_grid($hdsize_buttons), "raiddisk", "Disk Setup");
     $options_stack->add_titled(&$create_label_widget_grid($hdsize_labeled_widgets), "hdsize", "Size Options");
     $options_stack->add_titled(&$create_raid_advanced_grid($spinbutton_hdsize_zfs), "raidzfsadvanced", "Advanced Options");
+    $options_stack->add_titled(&$create_btrfs_raid_advanced_grid($spinbutton_hdsize_btrfs), "raidbtrfsadvanced", "Advanced Options");
     $options_stack->set_visible_child_name("raiddisk");
     my $options_stack_switcher = Gtk3::StackSwitcher->new();
     $options_stack_switcher->set_halign('center');
@@ -3257,8 +3269,9 @@ sub create_hdoption_view {
 	    $hw_raid_note->set_markup($msg);
 	}
 	$hw_raid_note->set_visible($raid);
-	$options_stack_switcher->set_visible($is_zfs);
+	$options_stack_switcher->set_visible($raid);
 	$options_stack->get_child_by_name("raidzfsadvanced")->set_visible($is_zfs);
+	$options_stack->get_child_by_name("raidbtrfsadvanced")->set_visible(!$is_zfs);
 	if ($raid) {
 	    $target_hd_label->set_text("Target: $config_options->{filesys} ");
 	    $options_stack->set_visible_child_name("raiddisk");
@@ -3267,7 +3280,7 @@ sub create_hdoption_view {
 	}
 
 	if ($raid) {
-	    $spinbutton_hdsize = $spinbutton_hdsize_zfs;
+	    $spinbutton_hdsize = $is_zfs ? $spinbutton_hdsize_zfs : $spinbutton_hdsize_btrfs;
 	} else {
 	    $spinbutton_hdsize = $spinbutton_hdsize_nonraid;
 	}
-- 
2.30.2

[pve-devel] applied-series: [PATCH installer 0/2] fix 2 bugs related to hdsize selection

[Thomas Lamprecht]

On 19.04.22 14:02, Stoiko Ivanov wrote:
> noticed that these 2 bugs were still open in bugzilla (and assigned to me).
> 
> The issues are independent of each other - but the fixes touch the same part
> of the code - hence I'm sending them in a common series.
> 
> roughly tested with a VM (scp'ing the updated proxinstall from the
> second debug shell)
> 
> 
> Stoiko Ivanov (2):
>   fix #3188: update hdsize spin-button on disk-selection change
>   fix #3587: make hdsize configurable for btrfs setups
> 
>  proxinstall | 95 ++++++++++++++++++++++++++++++++++++-----------------
>  1 file changed, 65 insertions(+), 30 deletions(-)
> 


applied, thanks! Made some followups on top to avoid the relatively expensive recreation
of the GTK::Adjustment element for each view every time a disk selection change and some
improvements/clean-ups in the pre-existing code too.