public inbox for pve-devel@lists.proxmox.com
 help / color / mirror / Atom feed
* [pve-devel] [PATCH installer 0/3] add check/rename for already-existing ZFS rpool
@ 2024-05-16 10:28 Christoph Heiss
  2024-05-16 10:28 ` [pve-devel] [PATCH installer 1/3] low-level: add zfs module for retrieving importable zpool info Christoph Heiss
                   ` (4 more replies)
  0 siblings, 5 replies; 10+ messages in thread
From: Christoph Heiss @ 2024-05-16 10:28 UTC (permalink / raw)
  To: pve-devel

Pretty straight forward overall, implements a check for an exising
`rpool` on the system and ask the user whether they would like to rename
it, much in the same way as it works for VGs already.

Without this, the installer would silently create a second (and thus
conflicting) `rpool` and cause a boot failure after the installation,
since it does not know which pool to import exactly.

Christoph Heiss (3):
  proxmox: add zfs module for retrieving importable zpool info
  low-level: install: split out random disk uid generation
  low-level: install: check for already-existing `rpool` on install

 Proxmox/Install.pm        | 47 ++++++++++++++++++++++++++++++++-----
 Proxmox/Makefile          |  1 +
 Proxmox/Sys/ZFS.pm        | 43 ++++++++++++++++++++++++++++++++++
 test/Makefile             |  6 +++++
 test/zfs-get-pool-list.pl | 49 +++++++++++++++++++++++++++++++++++++++
 5 files changed, 140 insertions(+), 6 deletions(-)
 create mode 100644 Proxmox/Sys/ZFS.pm
 create mode 100755 test/zfs-get-pool-list.pl

-- 
2.44.0



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


^ permalink raw reply	[flat|nested] 10+ messages in thread

* [pve-devel] [PATCH installer 1/3] low-level: add zfs module for retrieving importable zpool info
  2024-05-16 10:28 [pve-devel] [PATCH installer 0/3] add check/rename for already-existing ZFS rpool Christoph Heiss
@ 2024-05-16 10:28 ` Christoph Heiss
  2024-07-08 14:24   ` Aaron Lauterer
  2024-05-16 10:28 ` [pve-devel] [PATCH installer 2/3] low-level: install: split out random disk uid generation Christoph Heiss
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 10+ messages in thread
From: Christoph Heiss @ 2024-05-16 10:28 UTC (permalink / raw)
  To: pve-devel

Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
---
 Proxmox/Makefile          |  1 +
 Proxmox/Sys/ZFS.pm        | 43 ++++++++++++++++++++++++++++++++++
 test/Makefile             |  6 +++++
 test/zfs-get-pool-list.pl | 49 +++++++++++++++++++++++++++++++++++++++
 4 files changed, 99 insertions(+)
 create mode 100644 Proxmox/Sys/ZFS.pm
 create mode 100755 test/zfs-get-pool-list.pl

diff --git a/Proxmox/Makefile b/Proxmox/Makefile
index 9561d9b..035626b 100644
--- a/Proxmox/Makefile
+++ b/Proxmox/Makefile
@@ -17,6 +17,7 @@ PERL_MODULES=\
     Sys/File.pm \
     Sys/Net.pm \
     Sys/Udev.pm \
+    Sys/ZFS.pm \
     UI.pm \
     UI/Base.pm \
     UI/Gtk3.pm \
diff --git a/Proxmox/Sys/ZFS.pm b/Proxmox/Sys/ZFS.pm
new file mode 100644
index 0000000..4c732ca
--- /dev/null
+++ b/Proxmox/Sys/ZFS.pm
@@ -0,0 +1,43 @@
+package Proxmox::Sys::ZFS;
+
+use strict;
+use warnings;
+
+use Proxmox::Sys::Command qw(run_command);
+
+use base qw(Exporter);
+our @EXPORT_OK = qw(get_exported_pools);
+
+my sub parse_pool_list {
+    my ($fh) = @_;
+
+    my @pools;
+    my $pool = {}; # last found pool in output
+
+    while (my $line = <$fh>) {
+	if ($line =~ /^\s+pool: (.+)$/) {
+	    push @pools, $pool if %$pool;
+	    $pool = { name => $1 };
+	    next;
+	}
+
+	next if !%$pool;
+
+	if ($line =~ /^\s*(id|state|status|action): (.+)$/) {
+	    chomp($pool->{$1} = $2);
+	    next;
+	}
+    }
+
+    push @pools, $pool if %$pool;
+    return \@pools;
+}
+
+sub get_exported_pools {
+    my $raw = run_command(['zpool', 'import']);
+    open (my $fh, '<', \$raw) or die 'failed to open in-memory stream';
+
+    return parse_pool_list($fh);
+}
+
+1;
diff --git a/test/Makefile b/test/Makefile
index 99bf14e..2d9decc 100644
--- a/test/Makefile
+++ b/test/Makefile
@@ -2,6 +2,7 @@ all:
 
 export PERLLIB=..
 
+# test-zfs-get-pool-list is not run by default, due to requiring root access
 .PHONY: check
 check: test-zfs-arc-max test-run-command test-parse-fqdn test-ui2-stdio
 
@@ -9,6 +10,7 @@ check: test-zfs-arc-max test-run-command test-parse-fqdn test-ui2-stdio
 test-zfs-arc-max:
 	./zfs-arc-max.pl
 
+.PHONY: test-run-command
 test-run-command:
 	./run-command.pl
 
@@ -19,3 +21,7 @@ test-parse-fqdn:
 .PHONY: test-ui2-stdio
 test-ui2-stdio:
 	./ui2-stdio.pl
+
+.PHONY: test-zfs-get-pool-list
+test-zfs-get-pool-list:
+	./zfs-get-pool-list.pl
diff --git a/test/zfs-get-pool-list.pl b/test/zfs-get-pool-list.pl
new file mode 100755
index 0000000..072da2f
--- /dev/null
+++ b/test/zfs-get-pool-list.pl
@@ -0,0 +1,49 @@
+#!/usr/bin/perl
+
+use strict;
+use warnings;
+
+use File::Temp;
+use Test::More tests => 6;
+
+use Proxmox::Sys::Command qw(syscmd run_command);
+use Proxmox::Sys::ZFS;
+use Proxmox::UI;
+
+my $log_file = File::Temp->new();
+Proxmox::Log::init($log_file->filename);
+
+Proxmox::UI::init_stdio();
+
+our $POOL_PREFIX = 'pve-installer';
+my $pools = { 'test-pool1' => {}, 'test-pool2' => {} };
+
+foreach (keys %$pools) {
+    my $f = File::Temp->new();
+    print "$_: $f\n";
+
+    syscmd("truncate -s 1G $f");
+    my $dev = run_command("losetup --find --show $f");
+
+    syscmd("zpool create $POOL_PREFIX-$_ $dev");
+    syscmd("zpool export $POOL_PREFIX-$_");
+
+    $pools->{$_} = {
+	file => $f,
+	dev => $dev,
+    };
+}
+
+my $exported = Proxmox::Sys::ZFS::get_exported_pools();
+while (my ($name, $info) = each %$pools) {
+    my ($p) = grep { $_->{name} eq "$POOL_PREFIX-$name" } @$exported;
+    ok(defined($p), "pool $name was found");
+    is($p->{state}, 'ONLINE', "pool $name is online");
+    is($p->{action}, 'The pool can be imported using its name or numeric identifier.',
+	"pool $name can be imported");
+}
+
+keys %$pools;
+while (my ($name, $info) = each %$pools) {
+    syscmd("losetup --detach $info->{dev}");
+}
-- 
2.44.0



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


^ permalink raw reply	[flat|nested] 10+ messages in thread

* [pve-devel] [PATCH installer 2/3] low-level: install: split out random disk uid generation
  2024-05-16 10:28 [pve-devel] [PATCH installer 0/3] add check/rename for already-existing ZFS rpool Christoph Heiss
  2024-05-16 10:28 ` [pve-devel] [PATCH installer 1/3] low-level: add zfs module for retrieving importable zpool info Christoph Heiss
@ 2024-05-16 10:28 ` Christoph Heiss
  2024-05-16 10:28 ` [pve-devel] [PATCH installer 3/3] low-level: install: check for already-existing `rpool` on install Christoph Heiss
                   ` (2 subsequent siblings)
  4 siblings, 0 replies; 10+ messages in thread
From: Christoph Heiss @ 2024-05-16 10:28 UTC (permalink / raw)
  To: pve-devel

Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
---
 Proxmox/Install.pm | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/Proxmox/Install.pm b/Proxmox/Install.pm
index c0f8955..c86a574 100644
--- a/Proxmox/Install.pm
+++ b/Proxmox/Install.pm
@@ -169,6 +169,13 @@ sub btrfs_create {
     syscmd($cmd);
 }
 
+# no high randomnes properties, but this is only for the cases where
+# we either have multiple volume groups or zpools from multiple old PVE disks,
+# or we have a disk with both a "$vgname" and "$vgname-old" ...
+sub random_short_uid {
+    return sprintf "%08X", rand(0xffffffff);
+}
+
 sub zfs_create_rpool {
     my ($vdev, $pool_name, $root_volume_name) = @_;
 
@@ -376,12 +383,7 @@ sub ask_existing_vg_rename_or_abort {
 
     for my $vg_uuid (keys %$duplicate_vgs) {
 	my $vg = $duplicate_vgs->{$vg_uuid};
-
-	# no high randomnes properties, but this is only for the cases where
-	# we either have multiple "$vgname" vgs from multiple old PVE disks, or
-	# we have a disk with both a "$vgname" and "$vgname-old"...
-	my $short_uid = sprintf "%08X", rand(0xffffffff);
-	$vg->{new_vgname} = "$vgname-OLD-$short_uid";
+	$vg->{new_vgname} = "$vgname-OLD-" . random_short_uid();
     }
 
     my $response_ok = Proxmox::Install::Config::get_lvm_auto_rename();
-- 
2.44.0



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


^ permalink raw reply	[flat|nested] 10+ messages in thread

* [pve-devel] [PATCH installer 3/3] low-level: install: check for already-existing `rpool` on install
  2024-05-16 10:28 [pve-devel] [PATCH installer 0/3] add check/rename for already-existing ZFS rpool Christoph Heiss
  2024-05-16 10:28 ` [pve-devel] [PATCH installer 1/3] low-level: add zfs module for retrieving importable zpool info Christoph Heiss
  2024-05-16 10:28 ` [pve-devel] [PATCH installer 2/3] low-level: install: split out random disk uid generation Christoph Heiss
@ 2024-05-16 10:28 ` Christoph Heiss
  2024-07-08 14:16   ` Aaron Lauterer
  2024-07-08 11:27 ` [pve-devel] [PATCH installer 0/3] add check/rename for already-existing ZFS rpool Christoph Heiss
  2024-07-11 12:00 ` Christoph Heiss
  4 siblings, 1 reply; 10+ messages in thread
From: Christoph Heiss @ 2024-05-16 10:28 UTC (permalink / raw)
  To: pve-devel

.. in the same manner as the detection for LVM works.

zpools can only be renamed by importing them with a new name, so
unfortunaly the import-export dance is needed.

Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
---
 Proxmox/Install.pm | 33 +++++++++++++++++++++++++++++++++
 1 file changed, 33 insertions(+)

diff --git a/Proxmox/Install.pm b/Proxmox/Install.pm
index c86a574..531ef1c 100644
--- a/Proxmox/Install.pm
+++ b/Proxmox/Install.pm
@@ -16,6 +16,7 @@ use Proxmox::Install::StorageConfig;
 use Proxmox::Sys::Block qw(get_cached_disks wipe_disk partition_bootable_disk);
 use Proxmox::Sys::Command qw(run_command syscmd);
 use Proxmox::Sys::File qw(file_read_firstline file_write_all);
+use Proxmox::Sys::ZFS;
 use Proxmox::UI;
 
 # TODO: move somewhere better?
@@ -176,9 +177,41 @@ sub random_short_uid {
     return sprintf "%08X", rand(0xffffffff);
 }
 
+sub zfs_ask_existing_zpool_rename {
+    my ($pool_name) = @_;
+
+    # At this point, no pools should be imported/active
+    my $exported_pools = Proxmox::Sys::ZFS::get_exported_pools();
+
+    foreach (@$exported_pools) {
+	next if $_->{name} ne $pool_name || $_->{state} ne 'ONLINE';
+	my $renamed_pool = "$_->{name}-OLD-" . random_short_uid();
+
+	my $response_ok = Proxmox::UI::prompt(
+	    "A ZFS pool named '$_->{name}' (id $_->{id}) already exists on the system.\n\n" .
+	    "Do you want to rename the pool to '$renamed_pool' before continuing\n" .
+	    "or cancel the installation?"
+	);
+
+	# Import the pool using its id, as that is unique and works even if there are
+	# multiple zpools with the same name.
+	if ($response_ok) {
+	    syscmd("zpool import -f $_->{id} $renamed_pool") == 0 ||
+		die "failed to import zfs pool '$_->{name}' with new name '$renamed_pool'\n";
+	    syscmd("zpool export $renamed_pool") == 0 ||
+		warn "failed to export renamed zfs pool '$renamed_pool'\n";
+	} else {
+	    warn "Canceled installation as requested by user, due to already existing ZFS pool '$pool_name'\n";
+	    die "\n"; # causes abort without re-showing an error dialogue
+	}
+    }
+}
+
 sub zfs_create_rpool {
     my ($vdev, $pool_name, $root_volume_name) = @_;
 
+    zfs_ask_existing_zpool_rename($pool_name);
+
     my $iso_env = Proxmox::Install::ISOEnv::get();
 
     my $zfs_opts = Proxmox::Install::Config::get_zfs_opt();
-- 
2.44.0



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


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [pve-devel] [PATCH installer 0/3] add check/rename for already-existing ZFS rpool
  2024-05-16 10:28 [pve-devel] [PATCH installer 0/3] add check/rename for already-existing ZFS rpool Christoph Heiss
                   ` (2 preceding siblings ...)
  2024-05-16 10:28 ` [pve-devel] [PATCH installer 3/3] low-level: install: check for already-existing `rpool` on install Christoph Heiss
@ 2024-07-08 11:27 ` Christoph Heiss
  2024-07-11 12:00 ` Christoph Heiss
  4 siblings, 0 replies; 10+ messages in thread
From: Christoph Heiss @ 2024-07-08 11:27 UTC (permalink / raw)
  Cc: Proxmox VE development discussion

Ping. Series still applies cleanly.

On Thu, May 16, 2024 at 12:28:32PM GMT, Christoph Heiss wrote:
> Pretty straight forward overall, implements a check for an exising
> `rpool` on the system and ask the user whether they would like to rename
> it, much in the same way as it works for VGs already.
>
> Without this, the installer would silently create a second (and thus
> conflicting) `rpool` and cause a boot failure after the installation,
> since it does not know which pool to import exactly.
>
> Christoph Heiss (3):
>   proxmox: add zfs module for retrieving importable zpool info
>   low-level: install: split out random disk uid generation
>   low-level: install: check for already-existing `rpool` on install
>
>  Proxmox/Install.pm        | 47 ++++++++++++++++++++++++++++++++-----
>  Proxmox/Makefile          |  1 +
>  Proxmox/Sys/ZFS.pm        | 43 ++++++++++++++++++++++++++++++++++
>  test/Makefile             |  6 +++++
>  test/zfs-get-pool-list.pl | 49 +++++++++++++++++++++++++++++++++++++++
>  5 files changed, 140 insertions(+), 6 deletions(-)
>  create mode 100644 Proxmox/Sys/ZFS.pm
>  create mode 100755 test/zfs-get-pool-list.pl
>
> --
> 2.44.0
>
>
>
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>
>


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


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [pve-devel] [PATCH installer 3/3] low-level: install: check for already-existing `rpool` on install
  2024-05-16 10:28 ` [pve-devel] [PATCH installer 3/3] low-level: install: check for already-existing `rpool` on install Christoph Heiss
@ 2024-07-08 14:16   ` Aaron Lauterer
  2024-07-08 15:16     ` Christoph Heiss
  0 siblings, 1 reply; 10+ messages in thread
From: Aaron Lauterer @ 2024-07-08 14:16 UTC (permalink / raw)
  To: Proxmox VE development discussion, Christoph Heiss



On  2024-05-16  12:28, Christoph Heiss wrote:
> .. in the same manner as the detection for LVM works.
> 
> zpools can only be renamed by importing them with a new name, so
> unfortunaly the import-export dance is needed.
> 
> Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
> ---
>   Proxmox/Install.pm | 33 +++++++++++++++++++++++++++++++++
>   1 file changed, 33 insertions(+)
> 
> diff --git a/Proxmox/Install.pm b/Proxmox/Install.pm
> index c86a574..531ef1c 100644
> --- a/Proxmox/Install.pm
> +++ b/Proxmox/Install.pm
> @@ -16,6 +16,7 @@ use Proxmox::Install::StorageConfig;
>   use Proxmox::Sys::Block qw(get_cached_disks wipe_disk partition_bootable_disk);
>   use Proxmox::Sys::Command qw(run_command syscmd);
>   use Proxmox::Sys::File qw(file_read_firstline file_write_all);
> +use Proxmox::Sys::ZFS;
>   use Proxmox::UI;
>   
>   # TODO: move somewhere better?
> @@ -176,9 +177,41 @@ sub random_short_uid {
>       return sprintf "%08X", rand(0xffffffff);
>   }
>   
> +sub zfs_ask_existing_zpool_rename {
> +    my ($pool_name) = @_;
> +
> +    # At this point, no pools should be imported/active
> +    my $exported_pools = Proxmox::Sys::ZFS::get_exported_pools();
> +
> +    foreach (@$exported_pools) {
> +	next if $_->{name} ne $pool_name || $_->{state} ne 'ONLINE';
> +	my $renamed_pool = "$_->{name}-OLD-" . random_short_uid();

since the pool already has a unigue numerical id, couln't we use that 
instead of generating a new one?

we even have everything in place with $_->{id}.

> +
> +	my $response_ok = Proxmox::UI::prompt(
> +	    "A ZFS pool named '$_->{name}' (id $_->{id}) already exists on the system.\n\n" .
> +	    "Do you want to rename the pool to '$renamed_pool' before continuing\n" .
> +	    "or cancel the installation?"
> +	);
> +
> +	# Import the pool using its id, as that is unique and works even if there are
> +	# multiple zpools with the same name.
> +	if ($response_ok) {
> +	    syscmd("zpool import -f $_->{id} $renamed_pool") == 0 ||
> +		die "failed to import zfs pool '$_->{name}' with new name '$renamed_pool'\n";
> +	    syscmd("zpool export $renamed_pool") == 0 ||
> +		warn "failed to export renamed zfs pool '$renamed_pool'\n";
> +	} else {
> +	    warn "Canceled installation as requested by user, due to already existing ZFS pool '$pool_name'\n";
> +	    die "\n"; # causes abort without re-showing an error dialogue
> +	}
> +    }
> +}
> +
>   sub zfs_create_rpool {
>       my ($vdev, $pool_name, $root_volume_name) = @_;
>   
> +    zfs_ask_existing_zpool_rename($pool_name);
> +
>       my $iso_env = Proxmox::Install::ISOEnv::get();
>   
>       my $zfs_opts = Proxmox::Install::Config::get_zfs_opt();


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


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [pve-devel] [PATCH installer 1/3] low-level: add zfs module for retrieving importable zpool info
  2024-05-16 10:28 ` [pve-devel] [PATCH installer 1/3] low-level: add zfs module for retrieving importable zpool info Christoph Heiss
@ 2024-07-08 14:24   ` Aaron Lauterer
  2024-07-08 15:13     ` Christoph Heiss
  0 siblings, 1 reply; 10+ messages in thread
From: Aaron Lauterer @ 2024-07-08 14:24 UTC (permalink / raw)
  To: Proxmox VE development discussion, Christoph Heiss



On  2024-05-16  12:28, Christoph Heiss wrote:
> Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
> ---
>   Proxmox/Makefile          |  1 +
>   Proxmox/Sys/ZFS.pm        | 43 ++++++++++++++++++++++++++++++++++
>   test/Makefile             |  6 +++++
>   test/zfs-get-pool-list.pl | 49 +++++++++++++++++++++++++++++++++++++++
>   4 files changed, 99 insertions(+)
>   create mode 100644 Proxmox/Sys/ZFS.pm
>   create mode 100755 test/zfs-get-pool-list.pl
> 
> diff --git a/Proxmox/Makefile b/Proxmox/Makefile
> index 9561d9b..035626b 100644
> --- a/Proxmox/Makefile
> +++ b/Proxmox/Makefile
> @@ -17,6 +17,7 @@ PERL_MODULES=\
>       Sys/File.pm \
>       Sys/Net.pm \
>       Sys/Udev.pm \
> +    Sys/ZFS.pm \
>       UI.pm \
>       UI/Base.pm \
>       UI/Gtk3.pm \
> diff --git a/Proxmox/Sys/ZFS.pm b/Proxmox/Sys/ZFS.pm
> new file mode 100644
> index 0000000..4c732ca
> --- /dev/null
> +++ b/Proxmox/Sys/ZFS.pm
> @@ -0,0 +1,43 @@
> +package Proxmox::Sys::ZFS;
> +
> +use strict;
> +use warnings;
> +
> +use Proxmox::Sys::Command qw(run_command);
> +
> +use base qw(Exporter);
> +our @EXPORT_OK = qw(get_exported_pools);
> +

Some of the flow in this function is difficult to understand without 
having a sample of the text it is parsing. Could we have a small 
example, maybe added as comment?
That could help people to see what is it trying to parse, even if they 
are not too familiar with the expected output

> +my sub parse_pool_list {
> +    my ($fh) = @_;
> +
> +    my @pools;
> +    my $pool = {}; # last found pool in output
> +
> +    while (my $line = <$fh>) {
> +	if ($line =~ /^\s+pool: (.+)$/) {
> +	    push @pools, $pool if %$pool;
> +	    $pool = { name => $1 };
> +	    next;
> +	}
> +
> +	next if !%$pool;
> +
> +	if ($line =~ /^\s*(id|state|status|action): (.+)$/) {
> +	    chomp($pool->{$1} = $2);
> +	    next;
> +	}
> +    }
> +
> +    push @pools, $pool if %$pool;
> +    return \@pools;

not too sure, but we usually tend to use anonymous arrays, $pools = [];
then we could just return $pools
The downside is of course that we need to dereference it in all the 
other places, AFAICT all the `push` lines:
push @$pools ...

But IME this is more in line with how usually handle such code.


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


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [pve-devel] [PATCH installer 1/3] low-level: add zfs module for retrieving importable zpool info
  2024-07-08 14:24   ` Aaron Lauterer
@ 2024-07-08 15:13     ` Christoph Heiss
  0 siblings, 0 replies; 10+ messages in thread
From: Christoph Heiss @ 2024-07-08 15:13 UTC (permalink / raw)
  To: Aaron Lauterer; +Cc: Proxmox VE development discussion

Thanks for the review!

On Mon, Jul 08, 2024 at 04:24:16PM GMT, Aaron Lauterer wrote:
> On  2024-05-16  12:28, Christoph Heiss wrote:
> > [..]
> > diff --git a/Proxmox/Sys/ZFS.pm b/Proxmox/Sys/ZFS.pm
> > new file mode 100644
> > index 0000000..4c732ca
> > --- /dev/null
> > +++ b/Proxmox/Sys/ZFS.pm
> > @@ -0,0 +1,43 @@
> > +package Proxmox::Sys::ZFS;
> > +
> > +use strict;
> > +use warnings;
> > +
> > +use Proxmox::Sys::Command qw(run_command);
> > +
> > +use base qw(Exporter);
> > +our @EXPORT_OK = qw(get_exported_pools);
> > +
>
> Some of the flow in this function is difficult to understand without having
> a sample of the text it is parsing. Could we have a small example, maybe
> added as comment?
> That could help people to see what is it trying to parse, even if they are
> not too familiar with the expected output

Makes sense, will do that!

I'll also add some tests using verbatim outputs of `zfs import`, such
that these can always be run. Probably easier than creating test pools
and calling `zfs import` at test run time.

>
> > +my sub parse_pool_list {
> > +    my ($fh) = @_;
> > +
> > +    my @pools;
> > +    my $pool = {}; # last found pool in output
> > +
> > +    while (my $line = <$fh>) {
> > +	if ($line =~ /^\s+pool: (.+)$/) {
> > +	    push @pools, $pool if %$pool;
> > +	    $pool = { name => $1 };
> > +	    next;
> > +	}
> > +
> > +	next if !%$pool;
> > +
> > +	if ($line =~ /^\s*(id|state|status|action): (.+)$/) {
> > +	    chomp($pool->{$1} = $2);
> > +	    next;
> > +	}
> > +    }
> > +
> > +    push @pools, $pool if %$pool;
> > +    return \@pools;
>
> not too sure, but we usually tend to use anonymous arrays, $pools = [];
> then we could just return $pools
> The downside is of course that we need to dereference it in all the other
> places, AFAICT all the `push` lines:
> push @$pools ...
>
> But IME this is more in line with how usually handle such code.

Sure, I'll rewrite it in that style for v2.


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


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [pve-devel] [PATCH installer 3/3] low-level: install: check for already-existing `rpool` on install
  2024-07-08 14:16   ` Aaron Lauterer
@ 2024-07-08 15:16     ` Christoph Heiss
  0 siblings, 0 replies; 10+ messages in thread
From: Christoph Heiss @ 2024-07-08 15:16 UTC (permalink / raw)
  To: Aaron Lauterer; +Cc: Proxmox VE development discussion

On Mon, Jul 08, 2024 at 04:16:23PM GMT, Aaron Lauterer wrote:
> On  2024-05-16  12:28, Christoph Heiss wrote:
> > [..]
> > +sub zfs_ask_existing_zpool_rename {
> > +    my ($pool_name) = @_;
> > +
> > +    # At this point, no pools should be imported/active
> > +    my $exported_pools = Proxmox::Sys::ZFS::get_exported_pools();
> > +
> > +    foreach (@$exported_pools) {
> > +	next if $_->{name} ne $pool_name || $_->{state} ne 'ONLINE';
> > +	my $renamed_pool = "$_->{name}-OLD-" . random_short_uid();
>
> since the pool already has a unigue numerical id, couln't we use that
> instead of generating a new one?

Good point, I will change that. Only thing I might have to look into is
the maximum pool name length, since the numerical IDs are quite long.
But in the worst case, they should still be unique enough even after
shortening to an appropriate length.

>
> we even have everything in place with $_->{id}.
>


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


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [pve-devel] [PATCH installer 0/3] add check/rename for already-existing ZFS rpool
  2024-05-16 10:28 [pve-devel] [PATCH installer 0/3] add check/rename for already-existing ZFS rpool Christoph Heiss
                   ` (3 preceding siblings ...)
  2024-07-08 11:27 ` [pve-devel] [PATCH installer 0/3] add check/rename for already-existing ZFS rpool Christoph Heiss
@ 2024-07-11 12:00 ` Christoph Heiss
  4 siblings, 0 replies; 10+ messages in thread
From: Christoph Heiss @ 2024-07-11 12:00 UTC (permalink / raw)
  To: Proxmox VE development discussion

v3 (already) out: https://lists.proxmox.com/pipermail/pve-devel/2024-July/064635.html

On Thu, May 16, 2024 at 12:28:32PM GMT, Christoph Heiss wrote:
> Pretty straight forward overall, implements a check for an exising
> `rpool` on the system and ask the user whether they would like to rename
> it, much in the same way as it works for VGs already.
>
> Without this, the installer would silently create a second (and thus
> conflicting) `rpool` and cause a boot failure after the installation,
> since it does not know which pool to import exactly.
>
> Christoph Heiss (3):
>   proxmox: add zfs module for retrieving importable zpool info
>   low-level: install: split out random disk uid generation
>   low-level: install: check for already-existing `rpool` on install
>
>  Proxmox/Install.pm        | 47 ++++++++++++++++++++++++++++++++-----
>  Proxmox/Makefile          |  1 +
>  Proxmox/Sys/ZFS.pm        | 43 ++++++++++++++++++++++++++++++++++
>  test/Makefile             |  6 +++++
>  test/zfs-get-pool-list.pl | 49 +++++++++++++++++++++++++++++++++++++++
>  5 files changed, 140 insertions(+), 6 deletions(-)
>  create mode 100644 Proxmox/Sys/ZFS.pm
>  create mode 100755 test/zfs-get-pool-list.pl
>
> --
> 2.44.0
>
>
>
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>
>


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


^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2024-07-11 12:00 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-05-16 10:28 [pve-devel] [PATCH installer 0/3] add check/rename for already-existing ZFS rpool Christoph Heiss
2024-05-16 10:28 ` [pve-devel] [PATCH installer 1/3] low-level: add zfs module for retrieving importable zpool info Christoph Heiss
2024-07-08 14:24   ` Aaron Lauterer
2024-07-08 15:13     ` Christoph Heiss
2024-05-16 10:28 ` [pve-devel] [PATCH installer 2/3] low-level: install: split out random disk uid generation Christoph Heiss
2024-05-16 10:28 ` [pve-devel] [PATCH installer 3/3] low-level: install: check for already-existing `rpool` on install Christoph Heiss
2024-07-08 14:16   ` Aaron Lauterer
2024-07-08 15:16     ` Christoph Heiss
2024-07-08 11:27 ` [pve-devel] [PATCH installer 0/3] add check/rename for already-existing ZFS rpool Christoph Heiss
2024-07-11 12:00 ` Christoph Heiss

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
Service provided by Proxmox Server Solutions GmbH | Privacy | Legal