Add pool health /proc entry, "SUSPENDED" pools #7563

tonyhutter · 2018-05-25T16:46:46Z

Description

Add a proc entry to display the pool's health:

$ cat /proc/spl/kstat/zfs/tank/health
ONLINE

This is done without using the spa config locks, so it will never hang.

Also, fix zpool status and zpool list -o health output to print SUSPENDED instead of ONLINE for suspended pools.

Motivation and Context

#7331

How Has This Been Tested?

Added test case

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Performance enhancement (non-breaking change which improves efficiency)
Code cleanup (non-breaking change which makes code smaller or more readable)
Breaking change (fix or feature that would cause existing functionality to change)
Documentation (a change to man pages or other documentation)

Checklist:

My code follows the ZFS on Linux code style requirements.
I have updated the documentation accordingly.
I have read the contributing document.
I have added tests to cover my changes.
All new and existing tests passed.
All commit messages are properly formatted and contain Signed-off-by.
Change has been approved by a ZFS on Linux member.

richardelling · 2018-05-25T19:31:18Z

module/zcommon/zfs_comutil.c

@@ -207,10 +208,53 @@ const char *zfs_history_event_names[ZFS_NUM_LEGACY_HISTORY_EVENTS] = {
 	"pool split",
 };

+#if defined(_KERNEL)
+/* Dummy gettext() for kernel builds */


The preferred method is to use:

#if defined(_KERNEL) #define gettext(x) x #endif

Fixed in latest push

behlendorf · 2018-05-29T18:40:05Z

lib/libzfs/libzfs_pool.c

+ * "SUSPENDED", etc).
+ */
+const char *
+zpool_get_health_str_from_zhp(zpool_handle_t *zhp)


nit: I'd suggest dropping the _from_zhp suffix.

behlendorf · 2018-05-29T19:32:49Z

module/zcommon/zfs_comutil.c

@@ -207,10 +208,48 @@ const char *zfs_history_event_names[ZFS_NUM_LEGACY_HISTORY_EVENTS] = {
 	"pool split",
 };

+#if defined(_KERNEL)
+#define	gettext(x) x
+#endif


We should avoid allowing the localization logic to seep down in to the shared user/kernel code. Which means in this case we're going to have to live with a little code duplication. I'd suggest moving the zpool_state_to_name back to where it was and adding a new spa_state_to_name(spa_t *) function without gettext to spa_misc.c. This has the additional advantage of not disrupting the existing libzfs API.

behlendorf · 2018-05-29T19:39:00Z

module/zfs/spa_stats.c

+{
+	return (spa_suspended(spa) && (spa_get_failmode(spa)
+	    != ZIO_FAILURE_MODE_CONTINUE));
+}


After adding spa_state_to_name you might find it clearer to move this check and the associated strlcpy in to the new function. Then you wouldn't need a conditional at all in spa_health_data.

tonyhutter · 2018-05-29T21:54:56Z

@behlendorf thanks for looking at it. I included your changes in my latest push.

richardelling · 2018-05-30T18:42:56Z

lib/libzfs/libzfs_pool.c

+		str = gettext("FAULTED");
+	} else if (status == ZPOOL_STATUS_IO_FAILURE_WAIT ||
+	    status == ZPOOL_STATUS_IO_FAILURE_MMP) {
+		str = gettext("SUSPENDED");


since suspended state is in addition to other states, do we need a separate kstat for suspension?

I don't think so. The kstat is there to be a quick "is the pool healthy?" check for pacemaker to use. If you wanted to know why the pool was suspended, you could always look at the zpool status output.

I wish it were that easy, in the zpool command suspension message is sent to stderr, so most screen-scrapers miss it.

An implementation could be as easy as exposing the value of spa->spa_suspended and it can wait for another PR.

From a practical implementation of HA perspective, it is not always desirable to failover if a pool is suspended. Hence we do want to know why, in addition to the rest of the state, of course.

the zpool command suspension message is sent to stderr, so most screen-scrapers miss it.

@richardelling which zpool status suspension message are you referring too? My recollection is that this has always been a point of confusion for users because a pool may be suspended yet appear totally healthy in the zpool status output.

yes, it is confusing :-( zfs_standard_error[_fmt]() has many callers and buried in there is the case where users are notified over stderr that the pool is suspended. In any case, I think exposing spa_suspended can be useful, but belongs in another PR

behlendorf · 2018-05-29T22:13:54Z

module/zfs/spa_misc.c

+		return ("DEGRADED");
+	case VDEV_STATE_HEALTHY:
+		return ("ONLINE");
+


nit: extra whitespace

behlendorf · 2018-05-30T18:50:21Z

include/spl/sys/kstat.h

@@ -69,6 +69,7 @@
 #define	KSTAT_FLAG_WRITABLE	0x04
 #define	KSTAT_FLAG_PERSISTENT	0x08
 #define	KSTAT_FLAG_DORMANT	0x10
+#define	KSTAT_FLAG_NO_HEADERS	0x20


We should keep this aligned with the Illumos ones for consistency and add yours to the end.

#define KSTAT_FLAG_VIRTUAL 0x01 #define KSTAT_FLAG_VAR_SIZE 0x02 #define KSTAT_FLAG_WRITABLE 0x04 #define KSTAT_FLAG_PERSISTENT 0x08 #define KSTAT_FLAG_DORMANT 0x10 #define KSTAT_FLAG_INVALID 0x20 #define KSTAT_FLAG_LONGSTRINGS 0x40

tonyhutter · 2018-05-31T17:49:35Z

@behlendorf my latest push fixes your last two comments.

behlendorf · 2018-05-31T19:50:26Z

include/spl/sys/kstat.h

@@ -69,6 +69,9 @@
 #define	KSTAT_FLAG_WRITABLE	0x04
 #define	KSTAT_FLAG_PERSISTENT	0x08
 #define	KSTAT_FLAG_DORMANT	0x10
+#define	KSTAT_FLAG_INVALID	0x20
+#define	KSTAT_FLAG_LONGSTRINGS	0x40
+#define	KSTAT_FLAG_NO_HEADERS	0x60


s/0x60/0x80

behlendorf · 2018-05-31T19:52:46Z

include/spl/sys/kstat.h

@@ -69,6 +69,9 @@
 #define	KSTAT_FLAG_WRITABLE	0x04
 #define	KSTAT_FLAG_PERSISTENT	0x08
 #define	KSTAT_FLAG_DORMANT	0x10
+#define	KSTAT_FLAG_INVALID	0x20
+#define	KSTAT_FLAG_LONGSTRINGS	0x40
+#define	KSTAT_FLAG_NO_HEADERS	0x60
 #define	KSTAT_FLAG_UNSUPPORTED	\
 	(KSTAT_FLAG_VAR_SIZE | KSTAT_FLAG_WRITABLE | \
 	KSTAT_FLAG_PERSISTENT | KSTAT_FLAG_DORMANT)


Can you also remove KSTAT_FLAG_UNSUPPORTED and it's one use too in __kstat_create

behlendorf · 2018-05-31T19:53:06Z

lib/libspl/include/sys/kstat.h

@@ -304,6 +304,8 @@ typedef struct kstat32 {
 #define	KSTAT_FLAG_PERSISTENT		0x08
 #define	KSTAT_FLAG_DORMANT		0x10
 #define	KSTAT_FLAG_INVALID		0x20
+#define	KSTAT_FLAG_LONGSTRINGS		0x40
+#define	KSTAT_FLAG_NO_HEADERS		0x60


s/0x60/0x80

behlendorf · 2018-05-31T20:03:00Z

lib/libzfs/libzfs_pool.c

+		str = gettext("FAULTED");
+	} else if (status == ZPOOL_STATUS_IO_FAILURE_WAIT ||
+	    status == ZPOOL_STATUS_IO_FAILURE_MMP) {
+		str = gettext("SUSPENDED");


the zpool command suspension message is sent to stderr, so most screen-scrapers miss it.

@richardelling which zpool status suspension message are you referring too? My recollection is that this has always been a point of confusion for users because a pool may be suspended yet appear totally healthy in the zpool status output.

codecov · 2018-06-01T02:48:15Z

Codecov Report

Merging #7563 into master will increase coverage by 0.08%.
The diff coverage is 85.48%.

@@            Coverage Diff             @@
##           master    #7563      +/-   ##
==========================================
+ Coverage   77.44%   77.53%   +0.08%     
==========================================
  Files         361      362       +1     
  Lines      110322   110338      +16     
==========================================
+ Hits        85442    85551     +109     
+ Misses      24880    24787      -93

Flag	Coverage Δ
#kernel	`78.14% <85.36%> (+0.16%)`	⬆️
#user	`66.5% <51.66%> (+0.12%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2d9142c...c272d1d. Read the comment docs.

behlendorf

Looks good after the ZTS fix.

behlendorf · 2018-06-01T16:42:14Z

tests/zfs-tests/tests/functional/kstat/Makefile.am

@@ -0,0 +1,5 @@
+pkgdatadir = $(datadir)/@PACKAGE@/zfs-tests/tests/functional/health


Should be functional/kstat instead of functional/health, which caused them to be installed in the wrong location.

Thanks, I'll fix that

beren12 · 2018-06-01T16:53:22Z

Would it be possible to use this to generate a proc file like mdstat, maybe /proc/zfsstat that has some appreciated info on pool status? maybe things like # vdevs and/or members, health, scrubbing vs resilvering, degraded, etc?

rpool: scrubbing [22%] zraid1 sdf1 sdj1 sda1 + mirror sdn1 sdd1
586006323200 512b blocks

Something more compact than zpool status but more than zpool list.
if it gets too complex with expanders and all, maybe just list the number of devices present/total of each vdev

richardelling · 2018-06-04T16:26:15Z

@beren12 can you start a conversation on the mail list regarding zpool status and procfs?

tonyhutter · 2018-06-04T17:12:07Z

One more last minute change - I'm going to rename the proc name from health to state to match zpool status:

  pool: mypool
 state: ONLINE
  scan: none requested
config:

	NAME                             STATE     READ WRITE CKSUM
	mypool                           ONLINE       0     0     0
	  /home/hutter/proc-health/file  ONLINE       0     0     0

ofaaland

Looks great! I have one question, about zpool_get_health_str().

I agree with the plan to call the kstat 'state' instead of 'health'.

ofaaland · 2018-06-05T00:19:08Z

lib/libzfs/libzfs_pool.c

+
+	if (zpool_get_state(zhp) == POOL_STATE_UNAVAIL) {
+		str = gettext("FAULTED");
+	} else if (status == ZPOOL_STATUS_IO_FAILURE_WAIT ||


Are the contents of status really valid here? I don't see an assignment or a pointer being passed.

Good gravy. The assignment is right there. Nevermind.

ofaaland

LGTM

1. Add a proc entry to display the pool's state: $ cat /proc/spl/kstat/zfs/tank/state ONLINE This is done without using the spa config locks, so it will never hang. 2. Fix 'zpool status' and 'zpool list -o health' output to print "SUSPENDED" instead of "ONLINE" for suspended pools. Signed-off-by: Tony Hutter <hutter2@llnl.gov> Closes openzfs#7331

bmerchant · 2018-06-13T21:40:28Z

Can this be added to the 0.7.10 todo list?

1. Add a proc entry to display the pool's state: $ cat /proc/spl/kstat/zfs/tank/state ONLINE This is done without using the spa config locks, so it will never hang. 2. Fix 'zpool status' and 'zpool list -o health' output to print "SUSPENDED" instead of "ONLINE" for suspended pools. Reviewed-by: Olaf Faaland <faaland1@llnl.gov> Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Reviewed by: Richard Elling <Richard.Elling@RichardElling.com> Signed-off-by: Tony Hutter <hutter2@llnl.gov> Closes openzfs#7331 Closes openzfs#7563

gmelikov · 2018-10-24T08:48:12Z

JFYI Updated ZFS resource agent is released in resource-agents v4.2.0 ClusterLabs/resource-agents@2bdeee4#diff-2f9687bda2dc6253e000b30aaea222d9

richardelling reviewed May 25, 2018

View reviewed changes

tonyhutter force-pushed the proc-health branch 3 times, most recently from b9e2785 to c98aa0b Compare May 29, 2018 18:22

behlendorf requested changes May 29, 2018

View reviewed changes

tonyhutter force-pushed the proc-health branch from c98aa0b to c36adec Compare May 29, 2018 21:15

tonyhutter force-pushed the proc-health branch 2 times, most recently from bb0e94f to 214c865 Compare May 30, 2018 00:14

tonyhutter mentioned this pull request May 30, 2018

Add KSTAT_FLAG_NO_HEADERS for kstats openzfs/spl#705

Closed

tonyhutter force-pushed the proc-health branch 2 times, most recently from 2558e24 to 0783f1b Compare May 30, 2018 00:38

tonyhutter changed the base branch from proc-health to master May 30, 2018 00:46

tonyhutter force-pushed the proc-health branch from 0783f1b to 91330f7 Compare May 30, 2018 01:09

richardelling reviewed May 30, 2018

View reviewed changes

behlendorf reviewed May 30, 2018

View reviewed changes

tonyhutter force-pushed the proc-health branch from 91330f7 to af8ebe0 Compare May 30, 2018 20:06

behlendorf requested changes May 31, 2018

View reviewed changes

tonyhutter force-pushed the proc-health branch from af8ebe0 to 56893d6 Compare May 31, 2018 20:36

behlendorf approved these changes Jun 1, 2018

View reviewed changes

behlendorf reviewed Jun 1, 2018

View reviewed changes

ofaaland suggested changes Jun 5, 2018

View reviewed changes

ofaaland approved these changes Jun 5, 2018

View reviewed changes

tonyhutter force-pushed the proc-health branch from 56893d6 to c272d1d Compare June 5, 2018 17:15

behlendorf merged commit f0ed6c7 into openzfs:master Jun 6, 2018

gmelikov mentioned this pull request Oct 17, 2018

ZFS agent: check status without locks when possible ClusterLabs/resource-agents#1247

Merged

danielnelson mentioned this pull request May 8, 2020

Issue #2616: ZFS Zpool properties on Linux influxdata/telegraf#6724

Closed

gmelikov mentioned this pull request Feb 27, 2023

Manual pages for userspace utils should document exit status? #14535

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add pool health /proc entry, "SUSPENDED" pools #7563

Add pool health /proc entry, "SUSPENDED" pools #7563

tonyhutter commented May 25, 2018 •

edited

Loading

richardelling May 25, 2018

tonyhutter May 29, 2018

behlendorf May 29, 2018

behlendorf May 29, 2018

behlendorf May 29, 2018

tonyhutter commented May 29, 2018

richardelling May 30, 2018

tonyhutter May 30, 2018

richardelling May 30, 2018

behlendorf May 31, 2018

richardelling Jun 1, 2018

behlendorf May 29, 2018

behlendorf May 30, 2018

tonyhutter commented May 31, 2018

behlendorf May 31, 2018

behlendorf May 31, 2018

behlendorf May 31, 2018

behlendorf May 31, 2018

codecov bot commented Jun 1, 2018 •

edited

Loading

behlendorf left a comment

behlendorf Jun 1, 2018

tonyhutter Jun 4, 2018

beren12 commented Jun 1, 2018

richardelling commented Jun 4, 2018

tonyhutter commented Jun 4, 2018

ofaaland left a comment

ofaaland Jun 5, 2018

ofaaland Jun 5, 2018

ofaaland left a comment

bmerchant commented Jun 13, 2018

gmelikov commented Oct 24, 2018

		@@ -0,0 +1,5 @@
		pkgdatadir = $(datadir)/@PACKAGE@/zfs-tests/tests/functional/health

Add pool health /proc entry, "SUSPENDED" pools #7563

Add pool health /proc entry, "SUSPENDED" pools #7563

Conversation

tonyhutter commented May 25, 2018 • edited Loading

Description

Motivation and Context

How Has This Been Tested?

Types of changes

Checklist:

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

tonyhutter commented May 29, 2018

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

tonyhutter commented May 31, 2018

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

codecov bot commented Jun 1, 2018 • edited Loading

Codecov Report

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

beren12 commented Jun 1, 2018

richardelling commented Jun 4, 2018

tonyhutter commented Jun 4, 2018

ofaaland left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ofaaland left a comment

Choose a reason for hiding this comment

bmerchant commented Jun 13, 2018

gmelikov commented Oct 24, 2018

tonyhutter commented May 25, 2018 •

edited

Loading

codecov bot commented Jun 1, 2018 •

edited

Loading