[pve-devel] [PATCH installer 0/4] adaptation to kernel cmdline handling and 2 small ZFS related improvements

[pve-devel] [PATCH installer 0/4] adaptation to kernel cmdline handling and 2 small ZFS related improvements

[Stoiko Ivanov]

This patch-series contains mostly unrelated patches, which still can be
tested well together:
* patch 1/4 fixes the parsing of our kernel cmdline parameters during the
  install (they were ignored if being the last on the line).
  additionally everything not used by our installer now end up in the
  target system's kernel cmdline - so if you need to add something for
  the system to boot - you do not need to do so 3 times (for the install,
  for the first boot, for the boot-loader config)
* patch 2/4 is a minor glitch I introduced, which caused me to debug the
  wrong thing for too long
* patch 3/4 has been a long-standing and quite sensible request
* patch 4/4 fixes a minor inconvenience on machines with local users and
  ZFS on root (dmesg gets filled with journald-messages upon
  journal-rotation and user-logins)

Tested the changes a bit (4/4 on my machine, 3/4 was straight-forward, 2/4
with a quick look, 1/4 with a few installer-options and net.ifnames=0)

Stoiko Ivanov (4):
  fix #4747: pass kernel cmdline parameters to target system
  unconfigured.sh: set serial to a number to prevent warning
  fix #1410: zfs: create /var/lib/vz as separate dataset
  zfs: set acltype=posix for root-dataset

 Proxmox/Install.pm        | 16 ++++++++++------
 Proxmox/Install/Config.pm | 25 +++++++++++++------------
 unconfigured.sh           |  1 +
 3 files changed, 24 insertions(+), 18 deletions(-)

-- 
2.39.2

[pve-devel] [PATCH installer 1/4] fix #4747: pass kernel cmdline parameters to target system

[Stoiko Ivanov]

Parameters needed for booting during installation are best preserved
in the target cmdline as well - e.g. if you need a particular
cmdline switch for your system to boot at all - not having to add it
for the first boot of the installed system and manually adding it to
the bootloader config is an improvement.

This additionally enables us to drop the console parameter handling
for serial consoles (it is just one of the parameters to pass along).

Finally it fixes the regular expressions for the installer settings we
read from the cmdline (swapsize, maxroot,...) which were broken if
added as last entry.

Signed-off-by: Stoiko Ivanov <s.ivanov@proxmox.com>
---
 Proxmox/Install.pm        | 11 +++++------
 Proxmox/Install/Config.pm | 25 +++++++++++++------------
 2 files changed, 18 insertions(+), 18 deletions(-)

diff --git a/Proxmox/Install.pm b/Proxmox/Install.pm
index 66adb2d..c868992 100644
--- a/Proxmox/Install.pm
+++ b/Proxmox/Install.pm
@@ -1152,11 +1152,10 @@ _EOD
 	}
 
 	update_progress(0.8, 0.95, 1, "make system bootable");
-	my $console_param='';
-	if (my $console = Proxmox::Install::Config::get_console()) {
-	    $console_param="console=$console";
-	    my $console_snippet = "GRUB_CMDLINE_LINUX=\"\$GRUB_CMDLINE_LINUX $console_param\"";
-	    file_write_all("$targetdir/etc/default/grub.d/console.cfg", $console_snippet);
+	my $target_cmdline='';
+	if ($target_cmdline = Proxmox::Install::Config::get_target_cmdline()) {
+	    my $target_cmdline_snippet = "GRUB_CMDLINE_LINUX=\"\$GRUB_CMDLINE_LINUX $target_cmdline\"";
+	    file_write_all("$targetdir/etc/default/grub.d/installer.cfg", $target_cmdline_snippet);
 	}
 
 	if ($use_zfs) {
@@ -1164,7 +1163,7 @@ _EOD
 	    my $zfs_snippet = "GRUB_CMDLINE_LINUX=\"\$GRUB_CMDLINE_LINUX root=ZFS=$zfs_pool_name/ROOT/$zfs_root_volume_name boot=zfs\"";
 	    file_write_all("$targetdir/etc/default/grub.d/zfs.cfg", $zfs_snippet);
 
-	    file_write_all("$targetdir/etc/kernel/cmdline", "root=ZFS=$zfs_pool_name/ROOT/$zfs_root_volume_name boot=zfs $console_param\n");
+	    file_write_all("$targetdir/etc/kernel/cmdline", "root=ZFS=$zfs_pool_name/ROOT/$zfs_root_volume_name boot=zfs $target_cmdline\n");
 
 	    zfs_setup_module_conf($targetdir);
 	}
diff --git a/Proxmox/Install/Config.pm b/Proxmox/Install/Config.pm
index 5e80255..b1acebc 100644
--- a/Proxmox/Install/Config.pm
+++ b/Proxmox/Install/Config.pm
@@ -16,36 +16,37 @@ my sub parse_kernel_cmdline {
 
     my $cmdline = Proxmox::Install::RunEnv::get('kernel_cmdline');
 
-    if ($cmdline =~ m/\s(ext4|xfs)(\s.*)?$/) {
+    if ($cmdline =~ s/\b(ext4|xfs)\s?//i) {
 	$cfg->{filesys} = $1;
     }
 
-    if ($cmdline =~ m/hdsize=(\d+(\.\d+)?)[\s\n]/i) {
+    if ($cmdline =~ s/\bhdsize=(\d+(\.\d+)?)\s?//i) {
 	$cfg->{hdsize} = $1;
     }
 
-    if ($cmdline =~ m/swapsize=(\d+(\.\d+)?)[\s\n]/i) {
+    if ($cmdline =~ s/\bswapsize=(\d+(\.\d+)?)\s?//i) {
 	$cfg->{swapsize} = $1;
     }
 
-    if ($cmdline =~ m/maxroot=(\d+(\.\d+)?)[\s\n]/i) {
+    if ($cmdline =~ s/\bmaxroot=(\d+(\.\d+)?)\s?//i) {
 	$cfg->{maxroot} = $1;
     }
 
-    if ($cmdline =~ m/minfree=(\d+(\.\d+)?)[\s\n]/i) {
+    if ($cmdline =~ s/\bminfree=(\d+(\.\d+)?)\s?//i) {
 	$cfg->{minfree} = $1;
     }
 
     my $iso_env = Proxmox::Install::ISOEnv::get();
     if ($iso_env->{product} eq 'pve') {
-	if ($cmdline =~ m/maxvz=(\d+(\.\d+)?)[\s\n]/i) {
+	if ($cmdline =~ s/\bmaxvz=(\d+(\.\d+)?)\s?//i) {
 	    $cfg->{maxvz} = $1;
 	}
     }
 
-    if ($cmdline =~ m/console=(\S+)[\s\n]?/i) {
-	    $cfg->{console} = $1;
-    }
+    $cmdline =~ s/(?:BOOT_IMAGE|root|ramdisk_size|splash|vga)=\S+\s?//gi;
+    $cmdline =~ s/ro|rw|quiet|proxdebug|proxtui|nomodeset//gi;
+
+    $cfg->{target_cmdline}= $cmdline;
 
     return $cfg;
 }
@@ -101,7 +102,7 @@ my sub init_cfg {
 	cidr => undef,
 	gateway => undef,
 	dns => undef,
-	console => undef,
+	target_cmdline => undef,
     };
 
     $initial = parse_kernel_cmdline($initial);
@@ -235,8 +236,8 @@ sub get_gateway { return get('gateway'); }
 sub set_dns { set_key('dns', $_[0]); }
 sub get_dns { return get('dns'); }
 
-sub set_console { set_key('console', $_[0]); }
-sub get_console { return get('console'); }
+sub set_target_cmdline { set_key('target_cmdline', $_[0]); }
+sub get_target_cmdline { return get('target_cmdline'); }
 
 
 1;
-- 
2.39.2

[pve-devel] [PATCH installer 2/4] unconfigured.sh: set serial to a number to prevent warning

[Stoiko Ivanov]

caught me off-guard while debugging other things - the message:
` [: : integer expression expected`
made me look a bit too long at the wrong place.

Fixes: a31259b1597447a0b431cd5c81a6db2bc80f1ddf
Signed-off-by: Stoiko Ivanov <s.ivanov@proxmox.com>
---
 unconfigured.sh | 1 +
 1 file changed, 1 insertion(+)

diff --git a/unconfigured.sh b/unconfigured.sh
index a361a20..6b3a8bf 100755
--- a/unconfigured.sh
+++ b/unconfigured.sh
@@ -7,6 +7,7 @@ trap "err_reboot" ERR
 parse_cmdline() {
     proxdebug=0
     proxtui=0
+    serial=0
     # shellcheck disable=SC2013 # per word splitting is wanted here
     for par in $(cat /proc/cmdline); do
         case $par in
-- 
2.39.2

[pve-devel] [PATCH installer 3/4] fix #1410: zfs: create /var/lib/vz as separate dataset

[Stoiko Ivanov]

this enables the users to set reservations on / separate from
/var/lib/vz - where backups, ISOs, and other data might fill the
complete pool.

Signed-off-by: Stoiko Ivanov <s.ivanov@proxmox.com>
---
 Proxmox/Install.pm | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/Proxmox/Install.pm b/Proxmox/Install.pm
index c868992..a96249e 100644
--- a/Proxmox/Install.pm
+++ b/Proxmox/Install.pm
@@ -189,6 +189,9 @@ sub zfs_create_rpool {
     syscmd("zfs create $pool_name/ROOT/$root_volume_name")  == 0 ||
 	die "unable to create zfs $pool_name/ROOT/$root_volume_name volume\n";
 
+    syscmd("zfs create -p $pool_name/ROOT/$root_volume_name/var/lib/vz")  == 0 ||
+	die "unable to create zfs $pool_name/ROOT/$root_volume_name/var/lib/vz volume\n";
+
     # default to `relatime` on, fast enough for the installer and production
     syscmd("zfs set atime=on relatime=on $pool_name") == 0 || die "unable to set zfs properties\n";
 
-- 
2.39.2

[pve-devel] [PATCH installer 4/4] zfs: set acltype=posix for root-dataset

[Stoiko Ivanov]

journald as a core component tries setting a ACL on the journal files
for (non-root) users and fails on our ZFS installs.
Resulting in dmesg being spammed with messages from journald upon each
journal-rotation for each user upon their first login.

This is also suggested by OpenZFS in their Debian guide for root on
ZFS:
https://openzfs.github.io/openzfs-docs/Getting%20Started/Debian/Debian%20Bookworm%20Root%20on%20ZFS.html

Tested by setting this on a machine of mine, where this has been
bugging for quite a while.

Signed-off-by: Stoiko Ivanov <s.ivanov@proxmox.com>
---
 Proxmox/Install.pm | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Proxmox/Install.pm b/Proxmox/Install.pm
index a96249e..4045a97 100644
--- a/Proxmox/Install.pm
+++ b/Proxmox/Install.pm
@@ -203,6 +203,8 @@ sub zfs_create_rpool {
 
     $value = $zfs_opts->{copies} // 1;
     syscmd("zfs set copies=$value $pool_name") if defined($value) && $value != 1;
+
+    syscmd("zfs set acltype=posix $pool_name/ROOT/$root_volume_name");
 }
 
 my $get_raid_devlist = sub {
-- 
2.39.2

Re: [pve-devel] [PATCH installer 3/4] fix #1410: zfs: create /var/lib/vz as separate dataset

[Stoiko Ivanov]

this is wrong!
the dataset is only needed for pve
I'll resend a v2

sorry for the noise!


On Thu, 16 Nov 2023 16:00:40 +0100
Stoiko Ivanov <s.ivanov@proxmox.com> wrote:

> this enables the users to set reservations on / separate from
> /var/lib/vz - where backups, ISOs, and other data might fill the
> complete pool.
> 
> Signed-off-by: Stoiko Ivanov <s.ivanov@proxmox.com>
> ---
>  Proxmox/Install.pm | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/Proxmox/Install.pm b/Proxmox/Install.pm
> index c868992..a96249e 100644
> --- a/Proxmox/Install.pm
> +++ b/Proxmox/Install.pm
> @@ -189,6 +189,9 @@ sub zfs_create_rpool {
>      syscmd("zfs create $pool_name/ROOT/$root_volume_name")  == 0 ||
>  	die "unable to create zfs $pool_name/ROOT/$root_volume_name volume\n";
>  
> +    syscmd("zfs create -p $pool_name/ROOT/$root_volume_name/var/lib/vz")  == 0 ||
> +	die "unable to create zfs $pool_name/ROOT/$root_volume_name/var/lib/vz volume\n";
> +
>      # default to `relatime` on, fast enough for the installer and production
>      syscmd("zfs set atime=on relatime=on $pool_name") == 0 || die "unable to set zfs properties\n";
>  

[pve-devel] applied-series: [PATCH installer 0/4] adaptation to kernel cmdline handling and 2 small ZFS related improvements

[Thomas Lamprecht]

Am 16/11/2023 um 16:00 schrieb Stoiko Ivanov:
> This patch-series contains mostly unrelated patches, which still can be
> tested well together:
> * patch 1/4 fixes the parsing of our kernel cmdline parameters during the
>   install (they were ignored if being the last on the line).
>   additionally everything not used by our installer now end up in the
>   target system's kernel cmdline - so if you need to add something for
>   the system to boot - you do not need to do so 3 times (for the install,
>   for the first boot, for the boot-loader config)
> * patch 2/4 is a minor glitch I introduced, which caused me to debug the
>   wrong thing for too long
> * patch 3/4 has been a long-standing and quite sensible request
> * patch 4/4 fixes a minor inconvenience on machines with local users and
>   ZFS on root (dmesg gets filled with journald-messages upon
>   journal-rotation and user-logins)
> 
> Tested the changes a bit (4/4 on my machine, 3/4 was straight-forward, 2/4
> with a quick look, 1/4 with a few installer-options and net.ifnames=0)
> 
> Stoiko Ivanov (4):
>   fix #4747: pass kernel cmdline parameters to target system
>   unconfigured.sh: set serial to a number to prevent warning
>   fix #1410: zfs: create /var/lib/vz as separate dataset
>   zfs: set acltype=posix for root-dataset
> 
>  Proxmox/Install.pm        | 16 ++++++++++------
>  Proxmox/Install/Config.pm | 25 +++++++++++++------------
>  unconfigured.sh           |  1 +
>  3 files changed, 24 insertions(+), 18 deletions(-)
> 


applied this one a bit ago, please send further fixes as follow up, thanks!