* [pve-devel] [PATCH installer v5 1/3] tui: NumericEditView: add optional placeholder value
2024-08-14 13:25 [pve-devel] [PATCH installer v5 0/3] expose zfs arc size setting for all products Christoph Heiss
@ 2024-08-14 13:25 ` Christoph Heiss
2024-08-14 13:25 ` [pve-devel] [PATCH installer v5 2/3] tui: expose arc size setting for zfs bootdisks for all products Christoph Heiss
` (2 subsequent siblings)
3 siblings, 0 replies; 5+ messages in thread
From: Christoph Heiss @ 2024-08-14 13:25 UTC (permalink / raw)
To: pve-devel
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
^ permalink raw reply [flat|nested] 5+ messages in thread* [pve-devel] [PATCH installer v5 2/3] tui: expose arc size setting for zfs bootdisks for all products
2024-08-14 13:25 [pve-devel] [PATCH installer v5 0/3] expose zfs arc size setting for all products Christoph Heiss
2024-08-14 13:25 ` [pve-devel] [PATCH installer v5 1/3] tui: NumericEditView: add optional placeholder value Christoph Heiss
@ 2024-08-14 13:25 ` Christoph Heiss
2024-08-14 13:25 ` [pve-devel] [PATCH installer v5 3/3] proxinstall: " Christoph Heiss
2024-11-10 18:52 ` [pve-devel] applied-series: [PATCH installer v5 0/3] expose zfs arc size setting " Thomas Lamprecht
3 siblings, 0 replies; 5+ messages in thread
From: Christoph Heiss @ 2024-08-14 13:25 UTC (permalink / raw)
To: pve-devel
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
^ permalink raw reply [flat|nested] 5+ messages in thread* [pve-devel] [PATCH installer v5 3/3] proxinstall: expose arc size setting for zfs bootdisks for all products
2024-08-14 13:25 [pve-devel] [PATCH installer v5 0/3] expose zfs arc size setting for all products Christoph Heiss
2024-08-14 13:25 ` [pve-devel] [PATCH installer v5 1/3] tui: NumericEditView: add optional placeholder value Christoph Heiss
2024-08-14 13:25 ` [pve-devel] [PATCH installer v5 2/3] tui: expose arc size setting for zfs bootdisks for all products Christoph Heiss
@ 2024-08-14 13:25 ` Christoph Heiss
2024-11-10 18:52 ` [pve-devel] applied-series: [PATCH installer v5 0/3] expose zfs arc size setting " Thomas Lamprecht
3 siblings, 0 replies; 5+ messages in thread
From: Christoph Heiss @ 2024-08-14 13:25 UTC (permalink / raw)
To: pve-devel
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
^ permalink raw reply [flat|nested] 5+ messages in thread* [pve-devel] applied-series: [PATCH installer v5 0/3] expose zfs arc size setting for all products
2024-08-14 13:25 [pve-devel] [PATCH installer v5 0/3] expose zfs arc size setting for all products Christoph Heiss
` (2 preceding siblings ...)
2024-08-14 13:25 ` [pve-devel] [PATCH installer v5 3/3] proxinstall: " Christoph Heiss
@ 2024-11-10 18:52 ` Thomas Lamprecht
3 siblings, 0 replies; 5+ messages in thread
From: Thomas Lamprecht @ 2024-11-10 18:52 UTC (permalink / raw)
To: Proxmox VE development discussion, Christoph Heiss
Am 14.08.24 um 15:25 schrieb 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(-)
>
applied series, thanks!
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 5+ messages in thread