Add illumos FMD ZFS logic to ZED -- phase 2 (WIP) #5343

don-brady · 2016-10-27T04:57:54Z

Fault Management Logic for ZED

The integration of Fault Management Daemon (FMD) logic from illumos
is being deployed in three phases. This logic is encapsulated in several
software modules inside ZED. This phase 2 portion primarily entails the
Diagnosis Engine and the Retire Agent modules. It also includes
infrastructure to support a crude FMD environment to host these modules.

The Diagnosis Engine consumes I/O and checksum ereports and
feeds them into a SERD engine which will generate a corresponding
fault diagnosis when the SERD engine fires. All the diagnosis state
data is collected into cases, one case per-vdev being tracked.
The Retire Agent responds to diagnosed faults by isolating the
faulty VDEV. It will notify the ZFS kernel module of the new VDEV
state (degraded or faulted). This agent is also responsible for
managing hot spares across pools. When it encounters a device
fault or a device removal it replaces the device with an appropriate
spare if available.

Implementation Notes

The FMD module API required for logic modules is emulated and implemented
in the fmd_api.c and fmd_serd.c source files. This support includes
module registration, memory allocation, module property accessors, basic
case management, one-shot timers and SERD engines.
The event subscriptions for the modules (located in a module specific
configuration file on illumos) are currently hard-coded into the ZED
zfs_agent_dispatch() function.
The FMD modules are called one at a time from a single thread that
consumes events queued to the modules. These events are sourced from
the normal ZED events and also include events posted from the diagnosis
engine and the libudev disk event monitor.
The FMD code modules have minimal changes and were intentionally left
as similar as possible to their upstream source files.
The sysevent namespace in ZED differs from illumos. For example:
- illumos uses "resource.sysevent.EC_zfs.ESC_ZFS_vdev_remove"
- Linux uses "sysevent.fs.zfs.vdev_remove"
The FMD Modules port was produced by Intel Federal, LLC under award
number B609815 between the U.S. Department of Energy (DOE) and Intel
Federal, LLC.

mention-bot · 2016-10-27T04:57:55Z

@don-brady, thanks for your PR! By analyzing the history of the files in this pull request, we identified @dun, @behlendorf and @tonyhutter to be potential reviewers.

behlendorf

Nice work. Questions and comments inline.

behlendorf · 2016-10-27T17:33:31Z

cmd/zed/Makefile.am

 	$(top_builddir)/lib/libzpool/libzpool.la \
 	$(top_builddir)/lib/libzfs/libzfs.la \
-	$(top_builddir)/lib/libzfs_core/libzfs_core.la
+	$(top_builddir)/lib/libzfs_core/libzfs_core.la \
+	-lrt


The -lrt flags are for the POSIX timer support right? If it's only used by the zed I believe these should be added as zed_LDFLAGS = -lrt.

Got it. Thanks.

behlendorf · 2016-10-27T17:59:42Z

cmd/zed/agents/README.md

+ereports and feeds them into a SERD engine which will generate a
+corresponding fault diagnosis when the SERD engine fires. The initial
+N and T values for the SERD engines are estimates inherited from
+illumos (10 errors in 10 minutes).


Are these values tunable? If so how?

Not directly yet. As stated in the README this was slated for Phase 3.

behlendorf · 2016-10-27T18:02:38Z

cmd/zed/agents/README.md

+  module registration, memory allocation, module property accessors, basic
+  case management, one-shot timers and SERD engines.
+  For detailed information on the FMD module API, see the document --
+  _"Fault Management Daemon Programmer's Reference Manual"_.


I had no idea this even existed, let alone was so extensive.

I see some results when Googling for this. Is there a canonical source such that you list a URL here?

Not aware of a stable/canonical source. It was originally hosted on opensolaris.org

behlendorf · 2016-10-27T18:04:25Z

cmd/zed/agents/fmd_api.c

+
+/*
+ * Copyright (c) 2004, 2010, Oracle and/or its affiliates. All rights reserved.
+ *


Unneeded extra empty line.

behlendorf · 2016-10-27T18:06:10Z

cmd/zed/agents/fmd_api.c

+ *
+ * In the ZED runtime, the modules are called from a single thread so no
+ * locking is required in this emulated FMD environment.
+ */


This comment should probably be above the includes.

behlendorf · 2016-10-28T00:31:21Z

cmd/zed/agents/zfs_diagnosis.c

+		    "ereport time %lld.%lld, pool load time = %lld.%lld",
+		    pool_guid, er_when.ertv_sec, er_when.ertv_nsec,
+		    pool_load.ertv_sec, pool_load.ertv_nsec);
+#endif


Why doesn't think work on Linux? What bit of infrastructure are we missing?

We originally did have a time value for er_when. We do now. Also, these log message should be limited to the rare case that we are skipping an event because it occurred before the pool was imported. I have move them accordingly and they are not excluded now.

behlendorf · 2016-10-28T00:34:34Z

cmd/zed/agents/zfs_diagnosis.c

+	{ "io_T", FMD_TYPE_TIME, "10min" },
+	{ "remove_timeout", FMD_TYPE_TIME, "15sec" },
+	{ NULL, 0, NULL }
+};


These values could be initialized via command line options to the zed.

Yep. Property config is slated for the next phase.

behlendorf · 2016-10-28T00:41:25Z

cmd/zed/agents/zfs_diagnosis.c

 {
+	zcp->zc_data.zc_version = CASE_DATA_VERSION_SERD;
+}


Is the intention to implement persistent records at a latter date?

Yes, the checkpoints of case data is a future feature. The current diagnosis is just SERD values so not losing much by not check-pointing. As the diagnosis/prediction model gets more complex we may want to checkpoint it to avoid losing valuable state.

behlendorf · 2016-10-28T00:44:27Z

cmd/zed/zed.d/checksum-spare.sh

@@ -1 +0,0 @@
-io-spare.sh


These scripts will also need to be removed from the Makefile.am.

good catch. fixed.

behlendorf · 2016-10-28T00:45:00Z

cmd/zed/zed_disk_event.c

-			zed_log_msg(LOG_INFO, "zed_udev_monitor: %s waiting "
-			    "for slice", udev_device_get_devnode(dev));
+	//		zed_log_msg(LOG_INFO, "zed_udev_monitor: %s waiting "
+	//		    "for slice", udev_device_get_devnode(dev));


Did you intend to comment this out?

I removed it. It was just a noisy log entry.

rlaager · 2016-11-01T04:54:39Z

cmd/zed/agents/README.md

+### ZFS Fault Management Overview ###
+
+The primary purpose with ZFS fault management is automated diagnosis
+and isolation of VDEV faults. A Fault is something we can associate


Is Fault intentionally capitalized here?

A typo, will fix in next commit.

rlaager · 2016-11-01T04:55:34Z

cmd/zed/agents/README.md

+  3. A _Disk Add Agent_ module (`agents/zfs_mod.c`)
+
+To begin with, a **Diagnosis Engine** consumes per-vdev I/O and checksum
+ereports and feeds them into a SERD engine which will generate a


What is "SERD"? I actually don't know, but also should it be expanded here?

I can add a basic definition.

rlaager · 2016-11-01T04:57:43Z

cmd/zed/agents/README.md

+  module registration, memory allocation, module property accessors, basic
+  case management, one-shot timers and SERD engines.
+  For detailed information on the FMD module API, see the document --
+  _"Fault Management Daemon Programmer's Reference Manual"_.


I see some results when Googling for this. Is there a canonical source such that you list a URL here?

rlaager · 2016-11-01T05:01:16Z

cmd/zed/agents/fmd_serd.c

+	 * function can also be used in combination with fmd_serd_eng_record().
+	 */
+	if (sgp->sg_flags & FMD_SERD_FIRED) {
+#if 1


Is this #if 1 leftover?

rlaager · 2016-11-01T05:01:50Z

cmd/zed/agents/fmd_serd.c

+}
+
+/*
+ * sourced from fmd_string.c


There are a couple of these "sourced from" comments. Is this copy-and-paste of some code? Should it be done differently?

Its code from the FMD runtime and under the same license and copyright. It's just a consolidation. For example: https://github.com/openzfs/openzfs/tree/master/usr/src/cmd/fm/fmd/common

rlaager · 2016-11-01T05:04:46Z

cmd/zed/agents/zfs_diagnosis.c

+
+	/*
+	 * There is no way to open a pool by GUID, or lookup a vdev by GUID.  No
+	 * matter what we do, we're going to have to stomach a O(vdevs * cases)


"a" should be "an".

Typo, can fix in next commit.

rlaager · 2016-11-01T05:06:43Z

cmd/zed/agents/zfs_retire.c

+ * The ZFS retire agent is responsible for managing hot spares across all pools.
+ * When we see a device fault or a device removal, we try to open the associated
+ * pool and look for any hot spares.  We iterate over any available hot spares
+ * and attempt a 'zpool replace' for each one.


Policy for which ones to try (or the order) is for later?

Policy in this phase is same as upstream (i.e very basic). Subsequent patches will improve the hot spare matching criteria (e.g. like matching virtual spares for dRAID or matching by size and device types).

rlaager · 2016-11-01T05:08:17Z

cmd/zed/agents/fmd_api.c

+	fmd_module_t *mp = (fmd_module_t *)hdl;
+
+	if (fmd_serd_eng_lookup(&mp->mod_serds, name) != NULL) {
+		zed_log_msg(LOG_ERR, "failed to create serd engine '%s': "


Should these error messages use all-caps "SERD" for consistency?

Hey this is UNIX/Linux. But point taken for being consistent.

rlaager · 2016-11-01T05:09:27Z

My comments here are pretty much all trivial proofreading stuff. If you think that's a waste of time, feel free to ignore them.

don-brady · 2016-11-02T06:31:00Z

@rlaager Thanks for taking time to review. All input is welcomed here. We also need to update the man page for ZED. I'll take a stab at it in the next commit and would welcome your input for that as well.

The phase 2 work primarily entails the Diagnosis Engine and the Retire Agent modules. It also includes infrastructure to support a crude FMD environment to host these modules. The Diagnosis Engine consumes I/O and checksum ereports and feeds them into a SERD engine which will generate a corres- ponding fault diagnosis when the SERD engine fires. All the diagnosis state data is collected into cases, one case per vdev being tracked. The Retire Agent responds to diagnosed faults by isolating the faulty VDEV. It will notify the ZFS kernel module of the new VDEV state (degraded or faulted). This agent is also responsible for managing hot spares across pools. When it encounters a device fault or a device removal it replaces the device with an appropriate spare if available.

tonyhutter · 2016-11-07T21:38:33Z

I've been testing this on our JBODs and haven't hit any major issues. I tested auto-online, auto-replace, and spares on a multipath pool and it worked as advertised. LGTM

The phase 2 work primarily entails the Diagnosis Engine and the Retire Agent modules. It also includes infrastructure to support a crude FMD environment to host these modules. The Diagnosis Engine consumes I/O and checksum ereports and feeds them into a SERD engine which will generate a corres- ponding fault diagnosis when the SERD engine fires. All the diagnosis state data is collected into cases, one case per vdev being tracked. The Retire Agent responds to diagnosed faults by isolating the faulty VDEV. It will notify the ZFS kernel module of the new VDEV state (degraded or faulted). This agent is also responsible for managing hot spares across pools. When it encounters a device fault or a device removal it replaces the device with an appropriate spare if available. Reviewed-by: Tony Hutter <[email protected]> Reviewed-by: Brian Behlendorf <[email protected]> Signed-off-by: Don Brady <[email protected]> Closes openzfs#5343

behlendorf requested changes Oct 28, 2016

View reviewed changes

behlendorf approved these changes Oct 31, 2016

View reviewed changes

rlaager reviewed Nov 1, 2016

View reviewed changes

behlendorf merged commit 976246f into openzfs:master Nov 7, 2016

tonyhutter mentioned this pull request Nov 8, 2016

Spare not detached after auto-onlining original disk #5374

Closed

mailinglists35 mentioned this pull request Dec 15, 2016

zpool commands block when a disk goes missing / pool suspends #3461

Open

		@@ -1 +0,0 @@
		io-spare.sh

Add illumos FMD ZFS logic to ZED -- phase 2 (WIP) #5343

Add illumos FMD ZFS logic to ZED -- phase 2 (WIP) #5343

Conversation

don-brady commented Oct 27, 2016

Fault Management Logic for ZED

Implementation Notes

mention-bot commented Oct 27, 2016

behlendorf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rlaager commented Nov 1, 2016

don-brady commented Nov 2, 2016

tonyhutter commented Nov 7, 2016