[pve-devel] [PATCH installer v2 0/3] add check/rename for already-existing ZFS rpool

[pve-devel] [PATCH installer v2 0/3] add check/rename for already-existing ZFS rpool

[Christoph Heiss]

Pretty straight forward overall, implements a check for an existing
`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.

v1: https://lists.proxmox.com/pipermail/pve-devel/2024-May/063874.html

Notable changes v1 -> v2:
  * incorporated Aaron suggestions from v1
  * rewrote tests to use a pre-defined input instead
  * moved pool renaming to own subroutine
  * documented all new subroutines
  * split out tests into own patch

Christoph Heiss (3):
  proxmox: add zfs module for retrieving importable zpool info
  test: add test cases for new zfs module
  low-level: install: check for already-existing `rpool` on install

 Proxmox/Install.pm        |  30 +++++++++++
 Proxmox/Makefile          |   1 +
 Proxmox/Sys/ZFS.pm        | 109 ++++++++++++++++++++++++++++++++++++++
 test/Makefile             |   7 ++-
 test/zfs-get-pool-list.pl |  57 ++++++++++++++++++++
 5 files changed, 203 insertions(+), 1 deletion(-)
 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] [PATCH installer v2 1/3] proxmox: add zfs module for retrieving importable zpool info

[Christoph Heiss]

Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
---
Changes v1 -> v2:
  * incorporated Aaron suggestion to use anonymous arrays instead
  * added documentation
  * renamed parsing function parse_pool_list -> zpool_import_parse_output
  * split out tests into separate patch

 Proxmox/Makefile   |  1 +
 Proxmox/Sys/ZFS.pm | 93 ++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 94 insertions(+)
 create mode 100644 Proxmox/Sys/ZFS.pm

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..9232d1a
--- /dev/null
+++ b/Proxmox/Sys/ZFS.pm
@@ -0,0 +1,93 @@
+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);
+
+# Parses the output of running `zpool import`, which shows all importable
+# ZFS pools.
+# Unfortunately, `zpool` does not support JSON or any other machine-readable
+# format for output, so we have to do it the hard way.
+#
+# Example output of `zpool import` looks like this:
+#
+#   root@proxmox:/# zpool import
+#      pool: testpool
+#        id: 4958685680270539150
+#     state: ONLINE
+#    action: The pool can be imported using its name or numeric identifier.
+#    config:
+#
+#           testpool    ONLINE
+#             vdc       ONLINE
+#             vdd       ONLINE
+#
+#      pool: rpool
+#        id: 9412322616744093413
+#     state: ONLINE
+#   status: The pool was last accessed by another system.
+#    action: The pool can be imported using its name or numeric identifier and
+#           the '-f' flag.
+#      see: https://openzfs.github.io/openzfs-docs/msg/ZFS-8000-EY
+#    config:
+#
+#           rpool       ONLINE
+#             mirror-0  ONLINE
+#               vda3    ONLINE
+#               vdb3    ONLINE
+#
+sub zpool_import_parse_output {
+    my ($fh) = @_;
+
+    my $pools = []; # all found pools
+    my $pool = {}; # last found pool in output
+
+    while (my $line = <$fh>) {
+	# first, if we find the start of a new pool, add it to the list with
+	# its name
+	if ($line =~ /^\s+pool: (.+)$/) {
+	    # push the previous parsed pool to the result list
+	    push @$pools, $pool if %$pool;
+	    $pool = { name => $1 };
+	    next;
+	}
+
+	# ignore any (garbage) output before the actual list, just in case
+	next if !%$pool;
+
+	# add any possibly-useful attribute to the last (aka. current) pool
+	if ($line =~ /^\s*(id|state|status|action): (.+)$/) {
+	    chomp($pool->{$1} = $2);
+	    next;
+	}
+    }
+
+    # add the final parsed pool to the list
+    push @$pools, $pool if %$pool;
+    return $pools;
+}
+
+# Returns an array of all importable ZFS pools on the system.
+# Each entry is a hash of the format:
+#
+# {
+#     name => '<pool name>',
+#     id => <numeric pool-id>,
+#     /* see also zpool_state_to_name() in lib/libzfs/libzfs_pool.c /*
+#     state => 'OFFLINE' | 'REMOVED' | 'FAULTED' | 'SPLIT' | 'UNAVAIL' \
+#	| 'FAULTED' | 'DEGRADED' | 'ONLINE',
+#     status => '<human-readable status string>', optional
+#     action => '<human-readable action string>', optional
+# }
+sub get_exported_pools {
+    my $raw = run_command(['zpool', 'import']);
+    open (my $fh, '<', \$raw) or die 'failed to open in-memory stream';
+
+    return zpool_import_parse_output($fh);
+}
+
+1;
-- 
2.45.1



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

[pve-devel] [PATCH installer v2 2/3] test: add test cases for new zfs module

[Christoph Heiss]

Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
---
Changes v1 -> v2:
  * new patch, split out from patch #1
  * rewrote tests to use a pre-defined input instead, thus being able
    to enable the tests unconditionally

 test/Makefile             |  7 ++++-
 test/zfs-get-pool-list.pl | 57 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 63 insertions(+), 1 deletion(-)
 create mode 100755 test/zfs-get-pool-list.pl

diff --git a/test/Makefile b/test/Makefile
index 99bf14e..c473af8 100644
--- a/test/Makefile
+++ b/test/Makefile
@@ -3,12 +3,13 @@ all:
 export PERLLIB=..
 
 .PHONY: check
-check: test-zfs-arc-max test-run-command test-parse-fqdn test-ui2-stdio
+check: test-zfs-arc-max test-run-command test-parse-fqdn test-ui2-stdio test-zfs-get-pool-list
 
 .PHONY: test-zfs-arc-max
 test-zfs-arc-max:
 	./zfs-arc-max.pl
 
+.PHONY: test-run-command
 test-run-command:
 	./run-command.pl
 
@@ -19,3 +20,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..34e6b20
--- /dev/null
+++ b/test/zfs-get-pool-list.pl
@@ -0,0 +1,57 @@
+#!/usr/bin/perl
+
+use strict;
+use warnings;
+
+use File::Temp;
+use Test::More tests => 8;
+
+use Proxmox::Sys::ZFS;
+use Proxmox::UI;
+
+my $log_file = File::Temp->new();
+Proxmox::Log::init($log_file->filename);
+
+Proxmox::UI::init_stdio();
+
+our $ZPOOL_IMPORT_TEST_OUTPUT = <<EOT;
+   pool: testpool
+     id: 4958685680270539150
+  state: ONLINE
+ action: The pool can be imported using its name or numeric identifier.
+ config:
+
+        testpool    ONLINE
+          vdc       ONLINE
+          vdd       ONLINE
+
+   pool: rpool
+     id: 9412322616744093413
+  state: FAULTED
+status: The pool was last accessed by another system.
+ action: The pool can be imported using its name or numeric identifier and
+        the '-f' flag.
+   see: https://openzfs.github.io/openzfs-docs/msg/ZFS-8000-EY
+ config:
+
+        rpool       ONLINE
+          mirror-0  ONLINE
+            vda3    ONLINE
+            vdb3    ONLINE
+EOT
+
+my $pools = {
+    testpool => { id => '4958685680270539150', state => 'ONLINE' },
+    rpool => { id => '9412322616744093413', state => 'FAULTED' },
+};
+
+open(my $fh, '<', \$ZPOOL_IMPORT_TEST_OUTPUT);
+my $result = Proxmox::Sys::ZFS::zpool_import_parse_output($fh);
+while (my ($name, $info) = each %$pools) {
+    my ($p) = grep { $_->{name} eq $name } @$result;
+    ok(defined($p), "pool $name was found");
+    is($p->{id}, $info->{id}, "pool $name has correct id");
+    is($p->{state}, $info->{state}, "pool $name has correct state");
+    like($p->{action}, qr/^The pool can be imported using its name or numeric identifier/,
+	"pool $name can be imported");
+}
-- 
2.45.1



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

[pve-devel] [PATCH installer v2 3/3] low-level: install: check for already-existing `rpool` on install

[Christoph Heiss]

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

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

Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
---
Changes v1 -> v2:
  * use unique pool id for renaming instead of generating random id
    ourselves
  * moved renaming functionality to own subroutine in new
    Proxmox::Sys::ZFS module

 Proxmox/Install.pm | 30 ++++++++++++++++++++++++++++++
 Proxmox/Sys/ZFS.pm | 20 ++++++++++++++++++--
 2 files changed, 48 insertions(+), 2 deletions(-)

diff --git a/Proxmox/Install.pm b/Proxmox/Install.pm
index c0f8955..2517c24 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?
@@ -169,9 +170,38 @@ sub btrfs_create {
     syscmd($cmd);
 }
 
+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-$_->{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 " .
+	    "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) {
+	    Proxmox::Sys::ZFS::rename_pool($_->{id}, $renamed_pool);
+	} 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();
diff --git a/Proxmox/Sys/ZFS.pm b/Proxmox/Sys/ZFS.pm
index 9232d1a..c5a807e 100644
--- a/Proxmox/Sys/ZFS.pm
+++ b/Proxmox/Sys/ZFS.pm
@@ -3,10 +3,10 @@ package Proxmox::Sys::ZFS;
 use strict;
 use warnings;
 
-use Proxmox::Sys::Command qw(run_command);
+use Proxmox::Sys::Command qw(run_command syscmd);
 
 use base qw(Exporter);
-our @EXPORT_OK = qw(get_exported_pools);
+our @EXPORT_OK = qw(get_exported_pools rename_pool);
 
 # Parses the output of running `zpool import`, which shows all importable
 # ZFS pools.
@@ -90,4 +90,20 @@ sub get_exported_pools {
     return zpool_import_parse_output($fh);
 }
 
+# Renames a importable ZFS pool by importing it with a new name and then
+# exporting again.
+#
+# Arguments:
+#
+# $poolid - Unique, numeric identifier of the pool to rename
+# $new_name - New name for the pool
+sub rename_pool {
+    my ($poolid, $new_name) = @_;
+
+    syscmd("zpool import -f $poolid $new_name") == 0 ||
+	die "failed to import zfs pool with id '$poolid' with new name '$new_name'\n";
+    syscmd("zpool export $new_name") == 0 ||
+	warn "failed to export renamed zfs pool '$new_name'\n";
+}
+
 1;
-- 
2.45.1



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

Re: [pve-devel] [PATCH installer v2 0/3] add check/rename for already-existing ZFS rpool

[Aaron Lauterer]

gave it a test on a VM with the GUI installer.

consider this series:

Tested-By: Aaron Lauterer <a.lauterer@proxmox.com>
Reviewed-By: Aaron Lauterer <a.lauterer@proxmox.com>

On  2024-07-11  11:56, Christoph Heiss wrote:
> Pretty straight forward overall, implements a check for an existing
> `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.
> 
> v1: https://lists.proxmox.com/pipermail/pve-devel/2024-May/063874.html
> 
> Notable changes v1 -> v2:
>    * incorporated Aaron suggestions from v1
>    * rewrote tests to use a pre-defined input instead
>    * moved pool renaming to own subroutine
>    * documented all new subroutines
>    * split out tests into own patch
> 
> Christoph Heiss (3):
>    proxmox: add zfs module for retrieving importable zpool info
>    test: add test cases for new zfs module
>    low-level: install: check for already-existing `rpool` on install
> 
>   Proxmox/Install.pm        |  30 +++++++++++
>   Proxmox/Makefile          |   1 +
>   Proxmox/Sys/ZFS.pm        | 109 ++++++++++++++++++++++++++++++++++++++
>   test/Makefile             |   7 ++-
>   test/zfs-get-pool-list.pl |  57 ++++++++++++++++++++
>   5 files changed, 203 insertions(+), 1 deletion(-)
>   create mode 100644 Proxmox/Sys/ZFS.pm
>   create mode 100755 test/zfs-get-pool-list.pl
> 


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

Re: [pve-devel] [PATCH installer v2 0/3] add check/rename for already-existing ZFS rpool

[Christoph Heiss]

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

On Thu, Jul 11, 2024 at 11:56:37AM GMT, Christoph Heiss wrote:
> Pretty straight forward overall, implements a check for an existing
> `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.
>
> v1: https://lists.proxmox.com/pipermail/pve-devel/2024-May/063874.html
>
> Notable changes v1 -> v2:
>   * incorporated Aaron suggestions from v1
>   * rewrote tests to use a pre-defined input instead
>   * moved pool renaming to own subroutine
>   * documented all new subroutines
>   * split out tests into own patch
>
> Christoph Heiss (3):
>   proxmox: add zfs module for retrieving importable zpool info
>   test: add test cases for new zfs module
>   low-level: install: check for already-existing `rpool` on install
>
>  Proxmox/Install.pm        |  30 +++++++++++
>  Proxmox/Makefile          |   1 +
>  Proxmox/Sys/ZFS.pm        | 109 ++++++++++++++++++++++++++++++++++++++
>  test/Makefile             |   7 ++-
>  test/zfs-get-pool-list.pl |  57 ++++++++++++++++++++
>  5 files changed, 203 insertions(+), 1 deletion(-)
>  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