[pve-devel] [RFC proxmox-backup/storage/zfsonlinux] improve zpool import for freshly created zpools

[pve-devel] [RFC proxmox-backup/storage/zfsonlinux] improve zpool import for freshly created zpools

[Stoiko Ivanov]

The issue of created zpools not being imported on boot, due to a
missing/partial/corrupt zpool.cache file is something which we see quite often
reported in our various support-channels.

With PVE the issue was mitigated (and it did not show up so often), since
pvestatd does activate all storages (which imports all referenced zpools).

This does not hold for proxmox-backup (see the thread in our community forum
[0]).

After a short off-list discussion with Fabian (G.) the following patchset
addresses the issue by adding 'zpool-import@.service' to our zfsonlinux repo
and instantiating the template unit in the API paths where we create
zpools (for storages or datastores)

quickly tested on testsetups of mine (by adding a pool and then checking
when it got imported according to the journal).


[0]: https://forum.proxmox.com/threads/pbs-chunks-error-after-update.75467/

zfsonlinux:
Stoiko Ivanov (1):
  Add systemd-unit for importing specific pools

 ...md-unit-for-importing-specific-pools.patch | 75 +++++++++++++++++++
 debian/patches/series                         |  1 +
 debian/zfsutils-linux.install                 |  1 +
 3 files changed, 77 insertions(+)
 create mode 100644 debian/patches/0008-Add-systemd-unit-for-importing-specific-pools.patch

proxmox-backup:
Stoiko Ivanov (1):
  api2/node/disks/zfs: instantiate import service

 src/api2/node/disks/zfs.rs | 5 +++++
 1 file changed, 5 insertions(+)

pve-storage:
Stoiko Ivanov (1):
  Disks: instantiate import unit for created zpool

 PVE/API2/Disks/ZFS.pm | 6 ++++++
 1 file changed, 6 insertions(+)

-- 
2.20.1

[pve-devel] [RFC zfsonlinux 1/1] Add systemd-unit for importing specific pools

[Stoiko Ivanov]

This patch addresses the problems some users experience when some zpools are
created/imported with cachefile (which then causes other pools not to get
imported during boot) - when our tooling creates a pool we explictly
instantiate the service with the pool's name, ensuring that it will get
imported by scanning.

Suggested-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
Signed-off-by: Stoiko Ivanov <s.ivanov@proxmox.com>
---
 ...md-unit-for-importing-specific-pools.patch | 75 +++++++++++++++++++
 debian/patches/series                         |  1 +
 debian/zfsutils-linux.install                 |  1 +
 3 files changed, 77 insertions(+)
 create mode 100644 debian/patches/0008-Add-systemd-unit-for-importing-specific-pools.patch

diff --git a/debian/patches/0008-Add-systemd-unit-for-importing-specific-pools.patch b/debian/patches/0008-Add-systemd-unit-for-importing-specific-pools.patch
new file mode 100644
index 00000000..61c4e94a
--- /dev/null
+++ b/debian/patches/0008-Add-systemd-unit-for-importing-specific-pools.patch
@@ -0,0 +1,75 @@
+From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
+From: Stoiko Ivanov <s.ivanov@proxmox.com>
+Date: Tue, 15 Sep 2020 19:07:24 +0200
+Subject: [PATCH] Add systemd-unit for importing specific pools
+
+The unit can be instantiated with a specific poolname, which will get imported
+by scanning /dev/disk/by-id, irrespective of the existence and content of
+/etc/zfs/zpool.cache.
+
+the instance name is used unescaped (see systemd.unit(5)), since zpool names
+can contain characters which will be escaped by systemd.
+
+Signed-off-by: Stoiko Ivanov <s.ivanov@proxmox.com>
+---
+ etc/systemd/system/50-zfs.preset.in       |  1 +
+ etc/systemd/system/Makefile.am            |  2 ++
+ etc/systemd/system/zfs-import@.service.in | 17 +++++++++++++++++
+ 3 files changed, 20 insertions(+)
+ create mode 100644 etc/systemd/system/zfs-import@.service.in
+
+diff --git a/etc/systemd/system/50-zfs.preset.in b/etc/systemd/system/50-zfs.preset.in
+index e4056a92c..030611419 100644
+--- a/etc/systemd/system/50-zfs.preset.in
++++ b/etc/systemd/system/50-zfs.preset.in
+@@ -1,6 +1,7 @@
+ # ZFS is enabled by default
+ enable zfs-import-cache.service
+ disable zfs-import-scan.service
++enable zfs-import@.service
+ enable zfs-import.target
+ enable zfs-mount.service
+ enable zfs-share.service
+diff --git a/etc/systemd/system/Makefile.am b/etc/systemd/system/Makefile.am
+index 4e14467a0..ae72102af 100644
+--- a/etc/systemd/system/Makefile.am
++++ b/etc/systemd/system/Makefile.am
+@@ -5,6 +5,7 @@ systemdunit_DATA = \
+ 	zfs-zed.service \
+ 	zfs-import-cache.service \
+ 	zfs-import-scan.service \
++	zfs-import@.service \
+ 	zfs-mount.service \
+ 	zfs-share.service \
+ 	zfs-volume-wait.service \
+@@ -16,6 +17,7 @@ EXTRA_DIST = \
+ 	$(top_srcdir)/etc/systemd/system/zfs-zed.service.in \
+ 	$(top_srcdir)/etc/systemd/system/zfs-import-cache.service.in \
+ 	$(top_srcdir)/etc/systemd/system/zfs-import-scan.service.in \
++	$(top_srcdir)/etc/systemd/system/zfs-import@.service.in \
+ 	$(top_srcdir)/etc/systemd/system/zfs-mount.service.in \
+ 	$(top_srcdir)/etc/systemd/system/zfs-share.service.in \
+ 	$(top_srcdir)/etc/systemd/system/zfs-import.target.in \
+diff --git a/etc/systemd/system/zfs-import@.service.in b/etc/systemd/system/zfs-import@.service.in
+new file mode 100644
+index 000000000..2db9fdaab
+--- /dev/null
++++ b/etc/systemd/system/zfs-import@.service.in
+@@ -0,0 +1,17 @@
++[Unit]
++Description=Import ZFS pool %i
++Documentation=man:zpool(8)
++DefaultDependencies=no
++Requires=systemd-udev-settle.service
++After=systemd-udev-settle.service
++After=cryptsetup.target
++After=multipathd.target
++Before=zfs-import.target
++
++[Service]
++Type=oneshot
++RemainAfterExit=yes
++ExecStart=@sbindir@/zpool import -N -d /dev/disk/by-id -o cachefile=none %I
++
++[Install]
++WantedBy=zfs-import.target
diff --git a/debian/patches/series b/debian/patches/series
index 33a76ef1..e1d7c485 100644
--- a/debian/patches/series
+++ b/debian/patches/series
@@ -5,3 +5,4 @@
 0005-Enable-zed-emails.patch
 0006-dont-symlink-zed-scripts.patch
 0007-Use-installed-python3.patch
+0008-Add-systemd-unit-for-importing-specific-pools.patch
diff --git a/debian/zfsutils-linux.install b/debian/zfsutils-linux.install
index aa62d505..229ff2ea 100644
--- a/debian/zfsutils-linux.install
+++ b/debian/zfsutils-linux.install
@@ -5,6 +5,7 @@ etc/zfs/zpool.d/
 lib/systemd/system-preset/
 lib/systemd/system/zfs-import-cache.service
 lib/systemd/system/zfs-import-scan.service
+lib/systemd/system/zfs-import@.service
 lib/systemd/system/zfs-import.target
 lib/systemd/system/zfs-import.service
 lib/systemd/system/zfs-mount.service
-- 
2.20.1

Re: [pve-devel] [RFC zfsonlinux 1/1] Add systemd-unit for importing specific pools

[Thomas Lamprecht]

On 16.09.20 14:14, Stoiko Ivanov wrote:
> [ --8<-- snip --8<-- ]
> +diff --git a/etc/systemd/system/zfs-import@.service.in b/etc/systemd/system/zfs-import@.service.in
> +new file mode 100644
> +index 000000000..2db9fdaab
> +--- /dev/null
> ++++ b/etc/systemd/system/zfs-import@.service.in
> +@@ -0,0 +1,17 @@
> ++[Unit]
> ++Description=Import ZFS pool %i
> ++Documentation=man:zpool(8)
> ++DefaultDependencies=no
> ++Requires=systemd-udev-settle.service

The after below can be OK, but why the require??

"Using this service is not recommended. [...]"
-- https://www.freedesktop.org/software/systemd/man/systemd-udev-settle.service.html

> ++After=systemd-udev-settle.service
> ++After=cryptsetup.target
> ++After=multipathd.target
> ++Before=zfs-import.target
> ++
> ++[Service]
> ++Type=oneshot
> ++RemainAfterExit=yes
> ++ExecStart=@sbindir@/zpool import -N -d /dev/disk/by-id -o cachefile=none %I
> ++
> ++[Install]
> ++WantedBy=zfs-import.target

[pve-devel] applied: [RFC zfsonlinux 1/1] Add systemd-unit for importing specific pools

[Thomas Lamprecht]

On 16.09.20 14:14, Stoiko Ivanov wrote:
> This patch addresses the problems some users experience when some zpools are
> created/imported with cachefile (which then causes other pools not to get
> imported during boot) - when our tooling creates a pool we explictly
> instantiate the service with the pool's name, ensuring that it will get
> imported by scanning.
> 
> Suggested-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
> Signed-off-by: Stoiko Ivanov <s.ivanov@proxmox.com>
> ---
>  ...md-unit-for-importing-specific-pools.patch | 75 +++++++++++++++++++
>  debian/patches/series                         |  1 +
>  debian/zfsutils-linux.install                 |  1 +
>  3 files changed, 77 insertions(+)
>  create mode 100644 debian/patches/0008-Add-systemd-unit-for-importing-specific-pools.patch
> 
>

applied, thanks! Dropped the "Require=systemd-udev-settle.service", though.
But, it's still ordered after.

[pve-devel] [RFC proxmox-backup 1/1] api2/node/disks/zfs: instantiate import service

[Stoiko Ivanov]

When creating a new zpool for a datastore, also instantiate an
import-unit for it. This helps in cases where '/etc/zfs/zool.cache'
get corrupted and thus the pool is not imported upon boot.

This patch needs the corresponding addition of 'zfs-import@.service' in
the zfsonlinux repository.

Suggested-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
Signed-off-by: Stoiko Ivanov <s.ivanov@proxmox.com>
---
 src/api2/node/disks/zfs.rs | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/src/api2/node/disks/zfs.rs b/src/api2/node/disks/zfs.rs
index c5d6f6a6..0f0e4220 100644
--- a/src/api2/node/disks/zfs.rs
+++ b/src/api2/node/disks/zfs.rs
@@ -25,6 +25,8 @@ use crate::server::WorkerTask;
 
 use crate::api2::types::*;
 
+use crate::tools::systemd;
+
 pub const DISK_ARRAY_SCHEMA: Schema = ArraySchema::new(
     "Disk name list.", &BLOCKDEVICE_NAME_SCHEMA)
     .schema();
@@ -355,6 +357,9 @@ pub fn create_zpool(
             let output = crate::tools::run_command(command, None)?;
             worker.log(output);
 
+            let import_unit = format!("zfs-import@{}.service", systemd::escape_unit(&name, false));
+            systemd::enable_unit(&import_unit)?;
+
             if let Some(compression) = compression {
                 let mut command = std::process::Command::new("zfs");
                 command.args(&["set", &format!("compression={}", compression), &name]);
-- 
2.20.1

[pve-devel] applied: [pbs-devel] [RFC proxmox-backup 1/1] api2/node/disks/zfs: instantiate import service

[Thomas Lamprecht]

On 16.09.20 14:14, Stoiko Ivanov wrote:
> When creating a new zpool for a datastore, also instantiate an
> import-unit for it. This helps in cases where '/etc/zfs/zool.cache'
> get corrupted and thus the pool is not imported upon boot.
> 
> This patch needs the corresponding addition of 'zfs-import@.service' in
> the zfsonlinux repository.
> 
> Suggested-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
> Signed-off-by: Stoiko Ivanov <s.ivanov@proxmox.com>
> ---
>  src/api2/node/disks/zfs.rs | 5 +++++
>  1 file changed, 5 insertions(+)
> 
>

applied, thanks!

did the following (-w white-space ignored) change, similar to the PVE one.

diff --git a/src/api2/node/disks/zfs.rs b/src/api2/node/disks/zfs.rs
index 0f0e4220..16534199 100644
--- a/src/api2/node/disks/zfs.rs
+++ b/src/api2/node/disks/zfs.rs
@@ -357,8 +357,10 @@ pub fn create_zpool(
             let output = crate::tools::run_command(command, None)?;
             worker.log(output);
 
+            if std::path::Path::new("/lib/systemd/system/zfs-import@.service").exists() {
                 let import_unit = format!("zfs-import@{}.service", systemd::escape_unit(&name, false));
                 systemd::enable_unit(&import_unit)?;
+            }
 
             if let Some(compression) = compression {
                 let mut command = std::process::Command::new("zfs");

[pve-devel] [RFC storage 1/1] Disks: instantiate import unit for created zpool

[Stoiko Ivanov]

When creating a new ZFS storage, also instantiate an import-unit for the pool.
This should help mitigate the case where some pools don't get imported during
boot, because they are not listed in an existing zpool.cache file.

This patch needs the corresponding addition of 'zfs-import@.service' in
the zfsonlinux repository.

Suggested-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
Signed-off-by: Stoiko Ivanov <s.ivanov@proxmox.com>
---
 PVE/API2/Disks/ZFS.pm | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/PVE/API2/Disks/ZFS.pm b/PVE/API2/Disks/ZFS.pm
index 7a633a7..66b26e9 100644
--- a/PVE/API2/Disks/ZFS.pm
+++ b/PVE/API2/Disks/ZFS.pm
@@ -5,6 +5,7 @@ use warnings;
 
 use PVE::Diskmanage;
 use PVE::JSONSchema qw(get_standard_option);
+use PVE::Systemd;
 use PVE::API2::Storage::Config;
 use PVE::Storage;
 use PVE::Tools qw(run_command lock_file trim);
@@ -393,6 +394,11 @@ __PACKAGE__->register_method ({
 		print "# ", join(' ', @$cmd), "\n";
 		run_command($cmd);
 
+		my $importunit = 'zfs-import@'. PVE::Systemd::escape_unit($name, undef) . '.service';
+		$cmd = ['systemctl', 'enable', $importunit];
+		print "# ", join(' ', @$cmd), "\n";
+		run_command($cmd);
+
 		if ($param->{add_storage}) {
 		    my $storage_params = {
 			type => 'zfspool',
-- 
2.20.1

[pve-devel] applied: [RFC storage 1/1] Disks: instantiate import unit for created zpool

[Thomas Lamprecht]

On 16.09.20 14:14, Stoiko Ivanov wrote:
> When creating a new ZFS storage, also instantiate an import-unit for the pool.
> This should help mitigate the case where some pools don't get imported during
> boot, because they are not listed in an existing zpool.cache file.
> 
> This patch needs the corresponding addition of 'zfs-import@.service' in
> the zfsonlinux repository.
> 
> Suggested-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
> Signed-off-by: Stoiko Ivanov <s.ivanov@proxmox.com>
> ---
>  PVE/API2/Disks/ZFS.pm | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
>

applied, thanks! As we have no dependency on zfsutils-linux here to do a
bump to the versioned dependency, I added a simple check if the zfs-import@
template service exists (roughly, if -e '/lib/systemd/system/zfs-import@.service')