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

[pve-devel] [PATCH installer v3 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 and PBS (PMG is handled the same as PBS),
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.

Previous revisions
------------------
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 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

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 | 45 ++++++++------
 proxmox-tui-installer/src/views/mod.rs      | 65 +++++++++++++++++++--
 5 files changed, 105 insertions(+), 37 deletions(-)

--
2.42.0

[pve-devel] [PATCH installer v3 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 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 3244e76..5e5f4fb 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;
@@ -24,6 +25,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,
 }
@@ -36,6 +38,7 @@ impl<T: Copy + ToString + FromStr + PartialOrd> NumericEditView<T> {
         Self {
             view,
             max_value: None,
+            placeholder: None,
             max_content_width: None,
             allow_empty: false,
         }
@@ -54,6 +57,7 @@ impl<T: Copy + ToString + FromStr + PartialOrd> NumericEditView<T> {
         Self {
             view,
             max_value: None,
+            placeholder: None,
             max_content_width: None,
             allow_empty: false,
         }
@@ -84,15 +88,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
         }
@@ -157,6 +188,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>;
@@ -165,6 +215,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();

@@ -204,6 +258,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.44.0

[pve-devel] [PATCH installer v3 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 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 | 45 ++++++++++++---------
 proxmox-tui-installer/src/views/mod.rs      |  1 -
 3 files changed, 29 insertions(+), 20 deletions(-)

diff --git a/proxmox-installer-common/src/options.rs b/proxmox-installer-common/src/options.rs
index 1aa8f65..a210142 100644
--- a/proxmox-installer-common/src/options.rs
+++ b/proxmox-installer-common/src/options.rs
@@ -205,7 +205,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 7e13e91..3d9be50 100644
--- a/proxmox-tui-installer/src/views/bootdisk.rs
+++ b/proxmox-tui-installer/src/views/bootdisk.rs
@@ -564,7 +564,18 @@ 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 {
+                view.placeholder(runinfo.total_memory / 2)
+            }
+        };

         let inner = FormView::new()
             .child("ashift", IntegerEditView::new().content(options.ashift))
@@ -592,14 +603,11 @@ impl ZfsBootdiskOptionsView {
                             .unwrap_or_default(),
                     ),
             )
-            .child("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(
+                "copies",
+                IntegerEditView::new().content(options.copies).max_value(3),
             )
+            .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)
@@ -621,21 +629,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.inner_mut()?;
-        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 occured 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 5e5f4fb..eab258c 100644
--- a/proxmox-tui-installer/src/views/mod.rs
+++ b/proxmox-tui-installer/src/views/mod.rs
@@ -93,7 +93,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.44.0

[pve-devel] [PATCH installer v3 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 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 25b6bb3..4e24da6 100644
--- a/Proxmox/Install/RunEnv.pm
+++ b/Proxmox/Install/RunEnv.pm
@@ -319,7 +319,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 a6a4cfb..bc0e1e4 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.44.0

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

[Christoph Heiss]

v4: https://lists.proxmox.com/pipermail/pve-devel/2024-May/063957.html


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