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

[pve-devel] [PATCH installer v3 0/4] 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
v2: https://lists.proxmox.com/pipermail/pve-devel/2024-July/064619.html

Notable changes v2 -> v3:
  * make low-level option lvm_auto_rename more generic
  * skip rename prompt in auto-install mode

Notable changes v1 -> v2:
  * incorporated Aarons 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 (4):
  test: add test cases for new zfs module
  low-level: install: check for already-existing `rpool` on install
  install: config: rename option lvm_auto_rename ->
    existing_storage_auto_rename
  low-level: install: automatically rename existing rpools on
    auto-installs

 Proxmox/Install.pm                            | 35 +++++++++++-
 Proxmox/Install/Config.pm                     |  6 +-
 Proxmox/Sys/ZFS.pm                            | 20 ++++++-
 proxmox-auto-installer/src/utils.rs           |  2 +-
 .../resources/parse_answer/disk_match.json    |  2 +-
 .../parse_answer/disk_match_all.json          |  2 +-
 .../parse_answer/disk_match_any.json          |  2 +-
 .../tests/resources/parse_answer/minimal.json |  2 +-
 .../resources/parse_answer/nic_matching.json  |  2 +-
 .../resources/parse_answer/specific_nic.json  |  2 +-
 .../tests/resources/parse_answer/zfs.json     |  2 +-
 proxmox-installer-common/src/setup.rs         |  2 +-
 proxmox-tui-installer/src/setup.rs            |  2 +-
 test/Makefile                                 |  7 ++-
 test/zfs-get-pool-list.pl                     | 57 +++++++++++++++++++
 15 files changed, 128 insertions(+), 17 deletions(-)
 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 v3 1/4] proxmox: add zfs module for retrieving importable zpool info

[Christoph Heiss]

Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
---
Changes v2 -> v3:
  * no changes

Changes v1 -> v2:
  * incorporated Aarons 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 v3 2/4] test: add test cases for new zfs module

[Christoph Heiss]

Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
---
Changes v2 -> v3:
  * no changes

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 v3 3/4] install: config: rename option lvm_auto_rename -> existing_storage_auto_rename

[Christoph Heiss]

As this is an internal option for the low-level installer anyway, no
real functional changes here.

Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
---
Changes v2 -> v3:
  * new patch

 Proxmox/Install.pm                                          | 2 +-
 Proxmox/Install/Config.pm                                   | 6 +++---
 proxmox-auto-installer/src/utils.rs                         | 2 +-
 .../tests/resources/parse_answer/disk_match.json            | 2 +-
 .../tests/resources/parse_answer/disk_match_all.json        | 2 +-
 .../tests/resources/parse_answer/disk_match_any.json        | 2 +-
 .../tests/resources/parse_answer/minimal.json               | 2 +-
 .../tests/resources/parse_answer/nic_matching.json          | 2 +-
 .../tests/resources/parse_answer/specific_nic.json          | 2 +-
 .../tests/resources/parse_answer/zfs.json                   | 2 +-
 proxmox-installer-common/src/setup.rs                       | 2 +-
 proxmox-tui-installer/src/setup.rs                          | 2 +-
 12 files changed, 14 insertions(+), 14 deletions(-)

diff --git a/Proxmox/Install.pm b/Proxmox/Install.pm
index c0f8955..7342e4b 100644
--- a/Proxmox/Install.pm
+++ b/Proxmox/Install.pm
@@ -384,7 +384,7 @@ sub ask_existing_vg_rename_or_abort {
 	$vg->{new_vgname} = "$vgname-OLD-$short_uid";
     }
 
-    my $response_ok = Proxmox::Install::Config::get_lvm_auto_rename();
+    my $response_ok = Proxmox::Install::Config::get_existing_storage_auto_rename();
     if (!$response_ok) {
 	my $message = "Detected existing '$vgname' Volume Group(s)! Do you want to:\n";
 
diff --git a/Proxmox/Install/Config.pm b/Proxmox/Install/Config.pm
index ecd8a74..e449039 100644
--- a/Proxmox/Install/Config.pm
+++ b/Proxmox/Install/Config.pm
@@ -82,7 +82,7 @@ my sub init_cfg {
 	# TODO: single disk selection config
 	target_hd => undef,
 	disk_selection => {},
-	lvm_auto_rename => 0,
+	existing_storage_auto_rename => 0,
 
 	# locale
 	country => $country,
@@ -244,7 +244,7 @@ sub get_dns { return get('dns'); }
 sub set_target_cmdline { set_key('target_cmdline', $_[0]); }
 sub get_target_cmdline { return get('target_cmdline'); }
 
-sub set_lvm_auto_rename { set_key('lvm_auto_rename', $_[0]); }
-sub get_lvm_auto_rename { return get('lvm_auto_rename'); }
+sub set_existing_storage_auto_rename { set_key('existing_storage_auto_rename', $_[0]); }
+sub get_existing_storage_auto_rename { return get('existing_storage_auto_rename'); }
 
 1;
diff --git a/proxmox-auto-installer/src/utils.rs b/proxmox-auto-installer/src/utils.rs
index 202ad41..cc47f5f 100644
--- a/proxmox-auto-installer/src/utils.rs
+++ b/proxmox-auto-installer/src/utils.rs
@@ -328,7 +328,7 @@ pub fn parse_answer(
         zfs_opts: None,
         target_hd: None,
         disk_selection: BTreeMap::new(),
-        lvm_auto_rename: 1,
+        existing_storage_auto_rename: 1,
 
         country: answer.global.country.clone(),
         timezone: answer.global.timezone.clone(),
diff --git a/proxmox-auto-installer/tests/resources/parse_answer/disk_match.json b/proxmox-auto-installer/tests/resources/parse_answer/disk_match.json
index 3a117b6..2618fd4 100644
--- a/proxmox-auto-installer/tests/resources/parse_answer/disk_match.json
+++ b/proxmox-auto-installer/tests/resources/parse_answer/disk_match.json
@@ -10,7 +10,7 @@
 	"8": "8",
 	"9": "9"
   },
-  "lvm_auto_rename": 1,
+  "existing_storage_auto_rename": 1,
   "filesys": "zfs (RAID10)",
   "gateway": "192.168.1.1",
   "hdsize": 223.57088470458984,
diff --git a/proxmox-auto-installer/tests/resources/parse_answer/disk_match_all.json b/proxmox-auto-installer/tests/resources/parse_answer/disk_match_all.json
index 5325fc3..6cfb96a 100644
--- a/proxmox-auto-installer/tests/resources/parse_answer/disk_match_all.json
+++ b/proxmox-auto-installer/tests/resources/parse_answer/disk_match_all.json
@@ -7,7 +7,7 @@
   "disk_selection": {
 	"9": "9"
   },
-  "lvm_auto_rename": 1,
+  "existing_storage_auto_rename": 1,
   "filesys": "zfs (RAID0)",
   "gateway": "192.168.1.1",
   "hdsize": 223.57088470458984,
diff --git a/proxmox-auto-installer/tests/resources/parse_answer/disk_match_any.json b/proxmox-auto-installer/tests/resources/parse_answer/disk_match_any.json
index 18e22d1..1921b34 100644
--- a/proxmox-auto-installer/tests/resources/parse_answer/disk_match_any.json
+++ b/proxmox-auto-installer/tests/resources/parse_answer/disk_match_any.json
@@ -14,7 +14,7 @@
 	"8": "8",
 	"9": "9"
   },
-  "lvm_auto_rename": 1,
+  "existing_storage_auto_rename": 1,
   "filesys": "zfs (RAID10)",
   "gateway": "192.168.1.1",
   "hdsize": 2980.820640563965,
diff --git a/proxmox-auto-installer/tests/resources/parse_answer/minimal.json b/proxmox-auto-installer/tests/resources/parse_answer/minimal.json
index bb72713..38112e4 100644
--- a/proxmox-auto-installer/tests/resources/parse_answer/minimal.json
+++ b/proxmox-auto-installer/tests/resources/parse_answer/minimal.json
@@ -7,7 +7,7 @@
   "filesys": "ext4",
   "gateway": "192.168.1.1",
   "hdsize": 223.57088470458984,
-  "lvm_auto_rename": 1,
+  "existing_storage_auto_rename": 1,
   "hostname": "pveauto",
   "keymap": "de",
   "mailto": "mail@no.invalid",
diff --git a/proxmox-auto-installer/tests/resources/parse_answer/nic_matching.json b/proxmox-auto-installer/tests/resources/parse_answer/nic_matching.json
index de94165..6eb3b8a 100644
--- a/proxmox-auto-installer/tests/resources/parse_answer/nic_matching.json
+++ b/proxmox-auto-installer/tests/resources/parse_answer/nic_matching.json
@@ -7,7 +7,7 @@
   "filesys": "ext4",
   "gateway": "10.10.10.1",
   "hdsize": 223.57088470458984,
-  "lvm_auto_rename": 1,
+  "existing_storage_auto_rename": 1,
   "hostname": "pveauto",
   "keymap": "de",
   "mailto": "mail@no.invalid",
diff --git a/proxmox-auto-installer/tests/resources/parse_answer/specific_nic.json b/proxmox-auto-installer/tests/resources/parse_answer/specific_nic.json
index 5b4fcfc..9791535 100644
--- a/proxmox-auto-installer/tests/resources/parse_answer/specific_nic.json
+++ b/proxmox-auto-installer/tests/resources/parse_answer/specific_nic.json
@@ -7,7 +7,7 @@
   "filesys": "ext4",
   "gateway": "10.10.10.1",
   "hdsize": 223.57088470458984,
-  "lvm_auto_rename": 1,
+  "existing_storage_auto_rename": 1,
   "hostname": "pveauto",
   "keymap": "de",
   "mailto": "mail@no.invalid",
diff --git a/proxmox-auto-installer/tests/resources/parse_answer/zfs.json b/proxmox-auto-installer/tests/resources/parse_answer/zfs.json
index 65724a8..85049cb 100644
--- a/proxmox-auto-installer/tests/resources/parse_answer/zfs.json
+++ b/proxmox-auto-installer/tests/resources/parse_answer/zfs.json
@@ -8,7 +8,7 @@
 	"6": "6",
 	"7": "7"
   },
-  "lvm_auto_rename": 1,
+  "existing_storage_auto_rename": 1,
   "filesys": "zfs (RAID1)",
   "gateway": "192.168.1.1",
   "hdsize": 80.0,
diff --git a/proxmox-installer-common/src/setup.rs b/proxmox-installer-common/src/setup.rs
index 64d05af..3082e8c 100644
--- a/proxmox-installer-common/src/setup.rs
+++ b/proxmox-installer-common/src/setup.rs
@@ -479,7 +479,7 @@ pub struct InstallConfig {
     #[serde(skip_serializing_if = "BTreeMap::is_empty")]
     pub disk_selection: BTreeMap<String, String>,
 
-    pub lvm_auto_rename: usize,
+    pub existing_storage_auto_rename: usize,
 
     pub country: String,
     pub timezone: String,
diff --git a/proxmox-tui-installer/src/setup.rs b/proxmox-tui-installer/src/setup.rs
index 8c01e42..02d9ece 100644
--- a/proxmox-tui-installer/src/setup.rs
+++ b/proxmox-tui-installer/src/setup.rs
@@ -17,7 +17,7 @@ impl From<InstallerOptions> for InstallConfig {
             zfs_opts: None,
             target_hd: None,
             disk_selection: BTreeMap::new(),
-            lvm_auto_rename: 0,
+            existing_storage_auto_rename: 0,
 
             country: options.timezone.country,
             timezone: options.timezone.timezone,
-- 
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 v3 4/4] 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 v2 -> v3:
  * skip rename prompt in auto-install mode

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 | 33 +++++++++++++++++++++++++++++++++
 Proxmox/Sys/ZFS.pm | 20 ++++++++++++++++++--
 2 files changed, 51 insertions(+), 2 deletions(-)

diff --git a/Proxmox/Install.pm b/Proxmox/Install.pm
index 7342e4b..ba699fa 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,41 @@ 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::Install::Config::get_existing_storage_auto_rename();
+	if (!$response_ok) {
+	    $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 v3 4/4] low-level: install: check for already-existing `rpool` on install

[Aaron Lauterer]

only one small nit inline

On  2024-07-11  13:57, Christoph Heiss wrote:
> .. 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 v2 -> v3:
>    * skip rename prompt in auto-install mode
> 
> 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 | 33 +++++++++++++++++++++++++++++++++
>   Proxmox/Sys/ZFS.pm | 20 ++++++++++++++++++--
>   2 files changed, 51 insertions(+), 2 deletions(-)
> 
> diff --git a/Proxmox/Install.pm b/Proxmox/Install.pm
> index 7342e4b..ba699fa 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,41 @@ 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::Install::Config::get_existing_storage_auto_rename();
maybe we want to name this differently to avoid confusion?
response_ok -> do_rename
or something in that regard?

but that could be done in a follow up patch as well if we want to

> +	if (!$response_ok) {
> +	    $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
> +	}
> +    }
> +}
> +
[...]


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

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

[Aaron Lauterer]

tested this series more thoroughly and checked with an existing root 
pool all 3 install options:

* TUI
* GUI
* auto

Worked well in 3 cases to rename.

One oddity seems to be if we cancel the rename in the TUI installer. it 
jumps to 100% and stays there and I then need to manually abort. Jumping 
to a "setup cancelled" box that we confirm before quitting would be 
nice. This is how the GUI installer handles it.

Otherwise, besides this and a tiny nit in patch 4, consider this:

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

On  2024-07-11  13:57, 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
> v2: https://lists.proxmox.com/pipermail/pve-devel/2024-July/064619.html
> 
> Notable changes v2 -> v3:
>    * make low-level option lvm_auto_rename more generic
>    * skip rename prompt in auto-install mode
> 
> Notable changes v1 -> v2:
>    * incorporated Aarons 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 (4):
>    test: add test cases for new zfs module
>    low-level: install: check for already-existing `rpool` on install
>    install: config: rename option lvm_auto_rename ->
>      existing_storage_auto_rename
>    low-level: install: automatically rename existing rpools on
>      auto-installs
> 
>   Proxmox/Install.pm                            | 35 +++++++++++-
>   Proxmox/Install/Config.pm                     |  6 +-
>   Proxmox/Sys/ZFS.pm                            | 20 ++++++-
>   proxmox-auto-installer/src/utils.rs           |  2 +-
>   .../resources/parse_answer/disk_match.json    |  2 +-
>   .../parse_answer/disk_match_all.json          |  2 +-
>   .../parse_answer/disk_match_any.json          |  2 +-
>   .../tests/resources/parse_answer/minimal.json |  2 +-
>   .../resources/parse_answer/nic_matching.json  |  2 +-
>   .../resources/parse_answer/specific_nic.json  |  2 +-
>   .../tests/resources/parse_answer/zfs.json     |  2 +-
>   proxmox-installer-common/src/setup.rs         |  2 +-
>   proxmox-tui-installer/src/setup.rs            |  2 +-
>   test/Makefile                                 |  7 ++-
>   test/zfs-get-pool-list.pl                     | 57 +++++++++++++++++++
>   15 files changed, 128 insertions(+), 17 deletions(-)
>   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 v3 4/4] low-level: install: check for already-existing `rpool` on install

[Christoph Heiss]

Thanks for the review!

On Fri, Jul 12, 2024 at 11:36:56AM GMT, Aaron Lauterer wrote:
> only one small nit inline
>
> On  2024-07-11  13:57, 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-$_->{id}";
> > +
> > +	my $response_ok = Proxmox::Install::Config::get_existing_storage_auto_rename();
> maybe we want to name this differently to avoid confusion?
> response_ok -> do_rename
> or something in that regard?
>
> but that could be done in a follow up patch as well if we want to

Makes sense - I'll spin a new revision of this series and address this
directly.


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

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

[Christoph Heiss]

Thanks again!

On Fri, Jul 12, 2024 at 11:42:58AM GMT, Aaron Lauterer wrote:
> tested this series more thoroughly and checked with an existing root pool
> all 3 install options:
>
> * TUI
> * GUI
> * auto
>
> Worked well in 3 cases to rename.
>
> One oddity seems to be if we cancel the rename in the TUI installer. it
> jumps to 100% and stays there and I then need to manually abort. Jumping to
> a "setup cancelled" box that we confirm before quitting would be nice. This
> is how the GUI installer handles it.

Hm .. this must have existed since the inception of the TUI installer,
so it least isn't a (new) regression then. It just probably never
occurred that anyone tested/stumbled upon this in the TUI.

Thanks for the notice tho! I'll address it in a separate patch though,
as it isn't really related to the series.

>
> Otherwise, besides this and a tiny nit in patch 4, consider this:
>
> Reviewed-By: Aaron Lauterer <a.lauterer@proxmox.com>
> Tested-By: Aaron Lauterer <a.lauterer@proxmox.com>
>
> On  2024-07-11  13:57, 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
> > v2: https://lists.proxmox.com/pipermail/pve-devel/2024-July/064619.html
> >
> > Notable changes v2 -> v3:
> >    * make low-level option lvm_auto_rename more generic
> >    * skip rename prompt in auto-install mode
> >
> > Notable changes v1 -> v2:
> >    * incorporated Aarons 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 (4):
> >    test: add test cases for new zfs module
> >    low-level: install: check for already-existing `rpool` on install
> >    install: config: rename option lvm_auto_rename ->
> >      existing_storage_auto_rename
> >    low-level: install: automatically rename existing rpools on
> >      auto-installs
> >
> >   Proxmox/Install.pm                            | 35 +++++++++++-
> >   Proxmox/Install/Config.pm                     |  6 +-
> >   Proxmox/Sys/ZFS.pm                            | 20 ++++++-
> >   proxmox-auto-installer/src/utils.rs           |  2 +-
> >   .../resources/parse_answer/disk_match.json    |  2 +-
> >   .../parse_answer/disk_match_all.json          |  2 +-
> >   .../parse_answer/disk_match_any.json          |  2 +-
> >   .../tests/resources/parse_answer/minimal.json |  2 +-
> >   .../resources/parse_answer/nic_matching.json  |  2 +-
> >   .../resources/parse_answer/specific_nic.json  |  2 +-
> >   .../tests/resources/parse_answer/zfs.json     |  2 +-
> >   proxmox-installer-common/src/setup.rs         |  2 +-
> >   proxmox-tui-installer/src/setup.rs            |  2 +-
> >   test/Makefile                                 |  7 ++-
> >   test/zfs-get-pool-list.pl                     | 57 +++++++++++++++++++
> >   15 files changed, 128 insertions(+), 17 deletions(-)
> >   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 v3 0/4] add check/rename for already-existing ZFS rpool

[Christoph Heiss]

v4 out: https://lists.proxmox.com/pipermail/pve-devel/2024-July/064675.html

On Thu, Jul 11, 2024 at 01:57:52PM 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
> v2: https://lists.proxmox.com/pipermail/pve-devel/2024-July/064619.html
>
> Notable changes v2 -> v3:
>   * make low-level option lvm_auto_rename more generic
>   * skip rename prompt in auto-install mode
>
> Notable changes v1 -> v2:
>   * incorporated Aarons 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 (4):
>   test: add test cases for new zfs module
>   low-level: install: check for already-existing `rpool` on install
>   install: config: rename option lvm_auto_rename ->
>     existing_storage_auto_rename
>   low-level: install: automatically rename existing rpools on
>     auto-installs
>
>  Proxmox/Install.pm                            | 35 +++++++++++-
>  Proxmox/Install/Config.pm                     |  6 +-
>  Proxmox/Sys/ZFS.pm                            | 20 ++++++-
>  proxmox-auto-installer/src/utils.rs           |  2 +-
>  .../resources/parse_answer/disk_match.json    |  2 +-
>  .../parse_answer/disk_match_all.json          |  2 +-
>  .../parse_answer/disk_match_any.json          |  2 +-
>  .../tests/resources/parse_answer/minimal.json |  2 +-
>  .../resources/parse_answer/nic_matching.json  |  2 +-
>  .../resources/parse_answer/specific_nic.json  |  2 +-
>  .../tests/resources/parse_answer/zfs.json     |  2 +-
>  proxmox-installer-common/src/setup.rs         |  2 +-
>  proxmox-tui-installer/src/setup.rs            |  2 +-
>  test/Makefile                                 |  7 ++-
>  test/zfs-get-pool-list.pl                     | 57 +++++++++++++++++++
>  15 files changed, 128 insertions(+), 17 deletions(-)
>  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