[pve-devel] [PATCH installer v5 0/3] expose zfs arc size setting for all products

[pve-devel] [PATCH installer v5 0/3] expose zfs arc size setting for all products

[Christoph Heiss]

As suggested by Thomas, leaves the ZFS default if the user never touches
the setting in the installer (i.e. not writing a modprobe file).
See also the discussion in v1 [0].

[0] https://lists.proxmox.com/pipermail/pve-devel/2024-February/061659.html

Testing
=======

Tested the installation of PVE, PBS and PMG, with each once letting the
arc size setting untouched and once setting it to some specific value.
Also checked for each whether the correct default value was displayed.

Afterwards, checked that for PVE the module parameter was always written
to /etc/modprobe.d/, for PBS that it was only written in case it was
explicitly set.

History
=======

v4: https://lists.proxmox.com/pipermail/pve-devel/2024-May/063957.html
v3: https://lists.proxmox.com/pipermail/pve-devel/2024-April/062976.html
v2: https://lists.proxmox.com/pipermail/pve-devel/2024-February/061667.html
v1: https://lists.proxmox.com/pipermail/pve-devel/2023-November/060898.html

Changes v4 -> v5:
  * rebased on latest master
  * tui: fixed value not persisting across dialog open/close for non-PVE

Changes v3 -> v4:
  * rebased on latest master

Changes v2 -> v3:
  * tui: when empty & focused, do not show the placeholder value at all
  * gui: rework using Gtk3::Adjustment based on Maximilanos suggestion

Changes v1 -> v2:
  * rebased on latest master
  * add placeholder functionality for arc max size in TUI
  * "emulate" placeholder functionality in GTK on best-effort basis

Diffstat
========

Christoph Heiss (3):
  tui: NumericEditView: add optional placeholder value
  tui: expose arc size setting for zfs bootdisks for all products
  proxinstall: expose arc size setting for zfs bootdisks for all
    products

 Proxmox/Install/RunEnv.pm                   |  3 +-
 proxinstall                                 | 26 ++++-----
 proxmox-installer-common/src/options.rs     |  3 +-
 proxmox-tui-installer/src/views/bootdisk.rs | 48 +++++++++------
 proxmox-tui-installer/src/views/mod.rs      | 65 +++++++++++++++++++--
 5 files changed, 108 insertions(+), 37 deletions(-)

--
2.44.0



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [PATCH installer v5 1/3] tui: NumericEditView: add optional placeholder value

[Christoph Heiss]

Enables to add an optional placeholder value to `NumericEditView`, which
will be displayed in a different (darker) color and not returned by
`.get_content*()`.

Can be used for having default values in the TUI, but with different
handling in the back.

Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
---
Changes v4 -> v5:
  * no changes

Changes v3 -> v4:
  * no changes

Changes v2 -> v3:
  * when empty & focused, do not show the placeholder value at all

Changes v1 -> v2:
  * new patch

 proxmox-tui-installer/src/views/mod.rs | 66 ++++++++++++++++++++++++--
 1 file changed, 62 insertions(+), 4 deletions(-)

diff --git a/proxmox-tui-installer/src/views/mod.rs b/proxmox-tui-installer/src/views/mod.rs
index a098d1f..c7491fc 100644
--- a/proxmox-tui-installer/src/views/mod.rs
+++ b/proxmox-tui-installer/src/views/mod.rs
@@ -2,9 +2,10 @@ use std::{net::IpAddr, rc::Rc, str::FromStr};
 
 use cursive::{
     event::{Event, EventResult},
+    theme::BaseColor,
     view::{Resizable, ViewWrapper},
     views::{EditView, LinearLayout, NamedView, ResizedView, SelectView, TextView},
-    Rect, Vec2, View,
+    Printer, Rect, Vec2, View,
 };
 
 use proxmox_installer_common::utils::CidrAddress;
@@ -27,6 +28,7 @@ pub use timezone::*;
 pub struct NumericEditView<T> {
     view: LinearLayout,
     max_value: Option<T>,
+    placeholder: Option<T>,
     max_content_width: Option<usize>,
     allow_empty: bool,
 }
@@ -39,6 +41,7 @@ impl<T: Copy + ToString + FromStr + PartialOrd> NumericEditView<T> {
         Self {
             view,
             max_value: None,
+            placeholder: None,
             max_content_width: None,
             allow_empty: false,
         }
@@ -57,6 +60,7 @@ impl<T: Copy + ToString + FromStr + PartialOrd> NumericEditView<T> {
         Self {
             view,
             max_value: None,
+            placeholder: None,
             max_content_width: None,
             allow_empty: false,
         }
@@ -87,15 +91,42 @@ impl<T: Copy + ToString + FromStr + PartialOrd> NumericEditView<T> {
         self
     }
 
+    /// Sets a placeholder value for this view. Implies `allow_empty(true)`.
+    /// Implies `allow_empty(true)`.
+    ///
+    /// # Arguments
+    /// `placeholder` - The placeholder value to set for this view.
+    #[allow(unused)]
+    pub fn placeholder(mut self, placeholder: T) -> Self {
+        self.placeholder = Some(placeholder);
+        self.allow_empty(true)
+    }
+
+    /// Returns the current value of the view. If a placeholder is defined and
+    /// no value is currently set, the placeholder value is returned.
+    ///
+    /// **This should only be called when `allow_empty = false` or a placeholder
+    /// is set.**
     pub fn get_content(&self) -> Result<T, <T as FromStr>::Err> {
-        assert!(!self.allow_empty);
-        self.inner().get_content().parse()
+        let content = self.inner().get_content();
+
+        if content.is_empty() {
+            if let Some(placeholder) = self.placeholder {
+                return Ok(placeholder);
+            }
+        }
+
+        assert!(!(self.allow_empty && self.placeholder.is_none()));
+        content.parse()
     }
 
+    /// Returns the current value of the view, or [`None`] if the view is
+    /// currently empty.
     pub fn get_content_maybe(&self) -> Option<Result<T, <T as FromStr>::Err>> {
         let content = self.inner().get_content();
+
         if !content.is_empty() {
-            Some(self.inner().get_content().parse())
+            Some(content.parse())
         } else {
             None
         }
@@ -160,6 +191,25 @@ impl<T: Copy + ToString + FromStr + PartialOrd> NumericEditView<T> {
         std::mem::swap(self.inner_mut(), &mut inner);
         self
     }
+
+    /// Generic `wrap_draw()` implementation for [`ViewWrapper`].
+    ///
+    /// # Arguments
+    /// * `printer` - The [`Printer`] to draw to the base view.
+    fn wrap_draw_inner(&self, printer: &Printer) {
+        self.view.draw(printer);
+
+        if self.inner().get_content().is_empty() && !printer.focused {
+            if let Some(placeholder) = self.placeholder {
+                let placeholder = placeholder.to_string();
+
+                printer.with_color(
+                    (BaseColor::Blue.light(), BaseColor::Blue.dark()).into(),
+                    |printer| printer.print((0, 0), &placeholder),
+                );
+            }
+        }
+    }
 }
 
 pub type FloatEditView = NumericEditView<f64>;
@@ -168,6 +218,10 @@ pub type IntegerEditView = NumericEditView<usize>;
 impl ViewWrapper for FloatEditView {
     cursive::wrap_impl!(self.view: LinearLayout);
 
+    fn wrap_draw(&self, printer: &Printer) {
+        self.wrap_draw_inner(printer);
+    }
+
     fn wrap_on_event(&mut self, event: Event) -> EventResult {
         let original = self.inner_mut().get_content();
 
@@ -207,6 +261,10 @@ impl FloatEditView {
 impl ViewWrapper for IntegerEditView {
     cursive::wrap_impl!(self.view: LinearLayout);
 
+    fn wrap_draw(&self, printer: &Printer) {
+        self.wrap_draw_inner(printer);
+    }
+
     fn wrap_on_event(&mut self, event: Event) -> EventResult {
         let original = self.inner_mut().get_content();
 
-- 
2.45.2



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [PATCH installer v5 2/3] tui: expose arc size setting for zfs bootdisks for all products

[Christoph Heiss]

For non-PVE products, simply use the ZFS defaults (aka. 50%) and leave
unset, if the user never touches that setting.

Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
---
Changes v4 -> v5:
  * rebased on latest master
  * fixed value not persisting across dialog open/close for non-PVE

Changes v3 -> v4:
  * rebased on latest master

Changes v2 -> v3:
  * no changes

Changes v1 -> v2:
  * use new placeholder functionality for non-PVE products

 proxmox-installer-common/src/options.rs     |  3 +-
 proxmox-tui-installer/src/views/bootdisk.rs | 48 +++++++++++++--------
 proxmox-tui-installer/src/views/mod.rs      |  1 -
 3 files changed, 32 insertions(+), 20 deletions(-)

diff --git a/proxmox-installer-common/src/options.rs b/proxmox-installer-common/src/options.rs
index 9375ded..34dfadb 100644
--- a/proxmox-installer-common/src/options.rs
+++ b/proxmox-installer-common/src/options.rs
@@ -214,7 +214,8 @@ impl ZfsBootdiskOptions {
 /// The default ZFS maximum ARC size in MiB for this system.
 fn default_zfs_arc_max(product: ProxmoxProduct, total_memory: usize) -> usize {
     if product != ProxmoxProduct::PVE {
-        // Use ZFS default for non-PVE
+        // For products other the PVE, just let ZFS decide on its own. Setting `0`
+        // causes the installer to skip writing the `zfs_arc_max` module parameter.
         0
     } else {
         ((total_memory as f64) / 10.)
diff --git a/proxmox-tui-installer/src/views/bootdisk.rs b/proxmox-tui-installer/src/views/bootdisk.rs
index 1e22105..dfb1608 100644
--- a/proxmox-tui-installer/src/views/bootdisk.rs
+++ b/proxmox-tui-installer/src/views/bootdisk.rs
@@ -622,7 +622,24 @@ impl ZfsBootdiskOptionsView {
         options: &ZfsBootdiskOptions,
         product_conf: &ProductConfig,
     ) -> Self {
-        let is_pve = product_conf.product == ProxmoxProduct::PVE;
+        let arc_max_view = {
+            let view = IntegerEditView::new_with_suffix("MiB").max_value(runinfo.total_memory);
+
+            // For PVE "force" the default value, for other products place the recommended value
+            // only in the placeholder. This causes for the latter to not write the module option
+            // if the value is never modified by the user.
+            if product_conf.product == ProxmoxProduct::PVE {
+                view.content(options.arc_max)
+            } else {
+                let view = view.placeholder(runinfo.total_memory / 2);
+
+                if options.arc_max != 0 {
+                    view.content(options.arc_max)
+                } else {
+                    view
+                }
+            }
+        };
 
         let inner = FormView::new()
             .child("ashift", IntegerEditView::new().content(options.ashift))
@@ -654,13 +671,7 @@ impl ZfsBootdiskOptionsView {
                 "copies",
                 IntegerEditView::new().content(options.copies).max_value(3),
             )
-            .child_conditional(
-                is_pve,
-                "ARC max size",
-                IntegerEditView::new_with_suffix("MiB")
-                    .max_value(runinfo.total_memory)
-                    .content(options.arc_max),
-            )
+            .child("ARC max size", arc_max_view)
             .child("hdsize", DiskSizeEditView::new().content(options.disk_size));
 
         let view = MultiDiskOptionsView::new(&runinfo.disks, &options.selected_disks, inner)
@@ -682,21 +693,22 @@ impl ZfsBootdiskOptionsView {
     fn get_values(&mut self) -> Option<(Vec<Disk>, ZfsBootdiskOptions)> {
         let (disks, selected_disks) = self.view.get_disks_and_selection()?;
         let view = self.view.get_options_view()?;
-        let has_arc_max = view.len() >= 6;
-        let disk_size_index = if has_arc_max { 5 } else { 4 };
 
         let ashift = view.get_value::<IntegerEditView, _>(0)?;
         let compress = view.get_value::<SelectView<_>, _>(1)?;
         let checksum = view.get_value::<SelectView<_>, _>(2)?;
         let copies = view.get_value::<IntegerEditView, _>(3)?;
-        let disk_size = view.get_value::<DiskSizeEditView, _>(disk_size_index)?;
-
-        let arc_max = if has_arc_max {
-            view.get_value::<IntegerEditView, _>(4)?
-                .max(ZFS_ARC_MIN_SIZE_MIB)
-        } else {
-            0 // use built-in ZFS default value
-        };
+        let disk_size = view.get_value::<DiskSizeEditView, _>(5)?;
+
+        // If a value is set, return that and clamp it to at least [`ZFS_ARC_MIN_SIZE_MIB`].
+        //
+        // Otherwise, if no value was set or an error occurred return `0`. The former simply means
+        // that the placeholder value is still there.
+        let arc_max = view
+            .get_child::<IntegerEditView>(4)?
+            .get_content_maybe()
+            .map_or(Ok(0), |v| v.map(|v| v.max(ZFS_ARC_MIN_SIZE_MIB)))
+            .unwrap_or(0);
 
         Some((
             disks,
diff --git a/proxmox-tui-installer/src/views/mod.rs b/proxmox-tui-installer/src/views/mod.rs
index c7491fc..a41dac3 100644
--- a/proxmox-tui-installer/src/views/mod.rs
+++ b/proxmox-tui-installer/src/views/mod.rs
@@ -96,7 +96,6 @@ impl<T: Copy + ToString + FromStr + PartialOrd> NumericEditView<T> {
     ///
     /// # Arguments
     /// `placeholder` - The placeholder value to set for this view.
-    #[allow(unused)]
     pub fn placeholder(mut self, placeholder: T) -> Self {
         self.placeholder = Some(placeholder);
         self.allow_empty(true)
-- 
2.45.2



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [PATCH installer v5 3/3] proxinstall: expose arc size setting for zfs bootdisks for all products

[Christoph Heiss]

For non-PVE products, simply use the ZFS defaults (aka. 50%) and leave
unset, if the user never touches that setting.

Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
---
Changes v4 -> v5:
  * no changes

Changes v3 -> v4:
  * no changes

Changes v2 -> v3:
  * rework based on Maximilano's suggestion using Gtk3::Adjustment

Changes v1 -> v2:
  * add some more explanatory comments

 Proxmox/Install/RunEnv.pm |  3 ++-
 proxinstall               | 26 +++++++++++++-------------
 2 files changed, 15 insertions(+), 14 deletions(-)

diff --git a/Proxmox/Install/RunEnv.pm b/Proxmox/Install/RunEnv.pm
index 7eaf96a..8322603 100644
--- a/Proxmox/Install/RunEnv.pm
+++ b/Proxmox/Install/RunEnv.pm
@@ -329,7 +329,8 @@ our $ZFS_ARC_SYSMEM_PERCENTAGE = 0.1; # use 10% of available system memory by de
 # Calculates the default upper limit for the ZFS ARC size.
 # Returns the default ZFS maximum ARC size in MiB.
 sub default_zfs_arc_max {
-    # Use ZFS default on non-PVE
+    # For products other the PVE, just let ZFS decide on its own. Setting `0`
+    # causes the installer to skip writing the `zfs_arc_max` module parameter.
     return 0 if Proxmox::Install::ISOEnv::get('product') ne 'pve';
 
     my $default_mib = get('total_memory') * $ZFS_ARC_SYSMEM_PERCENTAGE;
diff --git a/proxinstall b/proxinstall
index 12f3eaa..5e991da 100755
--- a/proxinstall
+++ b/proxinstall
@@ -1138,20 +1138,20 @@ my $create_raid_advanced_grid = sub {
     $spinbutton_copies->set_value($copies);
     push @$labeled_widgets, ['copies', $spinbutton_copies];
 
-    if ($iso_env->{product} eq 'pve') {
-	my $total_memory = Proxmox::Install::RunEnv::get('total_memory');
+    my $total_memory = Proxmox::Install::RunEnv::get('total_memory');
+    my $arc_max = Proxmox::Install::Config::get_zfs_opt('arc_max') || ($total_memory * 0.5);
+
+    my $arc_max_adjustment = Gtk3::Adjustment->new(
+	$arc_max, $Proxmox::Install::RunEnv::ZFS_ARC_MIN_SIZE_MIB,
+	$total_memory, 1, 10, 0);
+    my $spinbutton_arc_max = Gtk3::SpinButton->new($arc_max_adjustment, 1, 0);
+    $spinbutton_arc_max->set_tooltip_text('Maximum ARC size in megabytes');
+    $spinbutton_arc_max->signal_connect('value-changed' => sub {
+	my $w = shift;
+	Proxmox::Install::Config::set_zfs_opt('arc_max', $w->get_value_as_int());
+    });
 
-	my $spinbutton_arc_max = Gtk3::SpinButton->new_with_range(
-	    $Proxmox::Install::RunEnv::ZFS_ARC_MIN_SIZE_MIB, $total_memory, 1);
-	$spinbutton_arc_max->set_tooltip_text('Maximum ARC size in megabytes');
-	$spinbutton_arc_max->signal_connect('value-changed' => sub {
-	    my $w = shift;
-	    Proxmox::Install::Config::set_zfs_opt('arc_max', $w->get_value_as_int());
-	});
-	my $arc_max = Proxmox::Install::Config::get_zfs_opt('arc_max');
-	$spinbutton_arc_max->set_value($arc_max);
-	push @$labeled_widgets, ['ARC max size', $spinbutton_arc_max, 'MiB'];
-    }
+    push @$labeled_widgets, ['ARC max size', $spinbutton_arc_max, 'MiB'];
 
     push @$labeled_widgets, ['hdsize', $hdsize_btn, 'GB'];
     return $create_label_widget_grid->($labeled_widgets);;
-- 
2.45.2



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel