- Download Diff

Summary:

11818 IPMI topo plugin shouldn't return data from unavailable sensors

Review Request #2397 — Created Oct. 15, 2019 and submitted Nov. 6, 2019, 9:46 a.m.

Information
Owner:	mscheler
Repository:	illumos-gate
Branch:
Bugs:	11818
Depends On:
Reviewers
Groups:	general
People:

Description

This change accomplishes two things:

- It add an "unusable" (that is its name, not its state) topo method to the IPMI provider. This method will mark unavailable sensor as unusable to indicate to consumers that they cannot use them.

- It will not return data read from an unavailable IPMI sensor.

Testing Done

1.) Recorded the output of "/usr/lib/fm/fmd/fmtopo -S" on a Supermicro X9 2U.

2.) Run full nightly, used "onu" to create new BE and booted into it.

3.) Recorded the new output of "/usr/lib/fm/fmd/fmtopo -S" on the same machine. The differences are as expected:
--- before.txt  2019-11-05 10:40:24.143900715 +0000

+++ after.txt   2019-11-05 11:24:05.281979308 +0000

@@ -1,5 +1,5 @@

 TIME                 UUID

-Nov 05 10:40:23 591d6fa7-b2a0-ee13-ab35-a08b376cd8b2

+Nov 05 11:24:05 48db20e3-81b7-431c-d266-c3c20ecbf8fa
hc://:product-id=T3300-B1:server-id=zebi-46:chassis-id=TS1502-0076/chassis=0

        Present: true

@@ -31,7 +31,7 @@
hc://:product-id=T3300-B1:server-id=zebi-46:chassis-id=TS1502-0076/chassis=0/fan=1

        Present: true

-       Unusable: false

+       Unusable: true
hc://:product-id=T3300-B1:server-id=zebi-46:chassis-id=TS1502-0076/chassis=0/fan=1?sensor=Fan2

        Present: true

@@ -39,7 +39,7 @@
hc://:product-id=T3300-B1:server-id=zebi-46:chassis-id=TS1502-0076/chassis=0/fan=2

        Present: true

-       Unusable: false

+       Unusable: true
hc://:product-id=T3300-B1:server-id=zebi-46:chassis-id=TS1502-0076/chassis=0/fan=2?sensor=Fan3

        Present: true

@@ -63,7 +63,7 @@
hc://:product-id=T3300-B1:server-id=zebi-46:chassis-id=TS1502-0076/chassis=0/fan=5

        Present: true

-       Unusable: false

+       Unusable: true
hc://:product-id=T3300-B1:server-id=zebi-46:chassis-id=TS1502-0076/chassis=0/fan=5?sensor=Fan6

        Present: true
"ipmitool sensor" confirms that the correct sensors were marked as unusable:
Fan1             | 6256.000   | RPM        | ok    | 340.000   | 408.000   | 476.000   | 17204.000 | 17272.000 | 17340.000

Fan2             | na         | RPM        | na    | 340.000   | 408.000   | 476.000   | 17204.000 | 17272.000 | 17340.000

Fan3             | na         | RPM        | na    | 340.000   | 408.000   | 476.000   | 17204.000 | 17272.000 | 17340.000

Fan4             | 6324.000   | RPM        | ok    | 340.000   | 408.000   | 476.000   | 17204.000 | 17272.000 | 17340.000

Fan5             | 6188.000   | RPM        | ok    | 340.000   | 408.000   | 476.000   | 17204.000 | 17272.000 | 17340.000

Fan6             | na         | RPM        | na    | 340.000   | 408.000   | 476.000   | 17204.000 | 17272.000 | 17340.000

Issues

0
0
1
0
All Issues: 1

Description	From	Last Updated
Should there be a call to topo_mod_strfree(mode, name) here?	pwinder	Nov. 5, 2019, 6:44 p.m.

Testing Done:

		1.) Recorded the output of "/usr/lib/fm/fmd/fmtopo -e" on a Supermicro X9 2U.
		2.) Run full nightly, used "onu" to create new BE and booted into it.
		3.) Recorded the new output of "/usr/lib/fm/fmd/fmtopo -e" on the same machine. The differences are as expected:

		--- before.txt 2019-10-15 13:21:53.133213816 +0000
		+++ after.txt 2019-10-15 15:06:34.855121633 +0000
		@@ -4,9 +4,3 @@
		/chassis0/fan1
		/chassis0/fan2
		/chassis0/fan2
		-/chassis0/fan3
		-/chassis0/fan3
		-/chassis0/fan4
		-/chassis0/fan4
		-/chassis0/fan5
		-/chassis0/fan5

		4.) Run "/usr/lib/fm/fmd/fmtopo" to check that the right chassis fan sensors were skipped:

		hc://:product-id=T3300-B1:server-id=zebi-46:chassis-id=TS1502-0076/chassis=0/fan=0

		hc://:product-id=T3300-B1:server-id=zebi-46:chassis-id=TS1502-0076/chassis=0/fan=0?sensor=Fan1

		hc://:product-id=T3300-B1:server-id=zebi-46:chassis-id=TS1502-0076/chassis=0/fan=1

		hc://:product-id=T3300-B1:server-id=zebi-46:chassis-id=TS1502-0076/chassis=0/fan=1?sensor=Fan4

		hc://:product-id=T3300-B1:server-id=zebi-46:chassis-id=TS1502-0076/chassis=0/fan=2

		hc://:product-id=T3300-B1:server-id=zebi-46:chassis-id=TS1502-0076/chassis=0/fan=2?sensor=Fan5

~		These are indeed the sensor that the "ipmitool" reports values for:
	~	These are indeed the sensors that the "ipmitool" reports values for:

		Fan1 \| 6460.000 \| RPM \| ok \| 340.000 \| 408.000 \| 476.000 \| 17204.000 \| 17272.000 \| 17340.000
		Fan2 \| na \| RPM \| na \| 340.000 \| 408.000 \| 476.000 \| 17204.000 \| 17272.000 \| 17340.000
		Fan3 \| na \| RPM \| na \| 340.000 \| 408.000 \| 476.000 \| 17204.000 \| 17272.000 \| 17340.000
		Fan4 \| 6596.000 \| RPM \| ok \| 340.000 \| 408.000 \| 476.000 \| 17204.000 \| 17272.000 \| 17340.000
		Fan5 \| 6392.000 \| RPM \| ok \| 340.000 \| 408.000 \| 476.000 \| 17204.000 \| 17272.000 \| 17340.000
		Fan6 \| na \| RPM \| na \| 340.000 \| 408.000 \| 476.000 \| 17204.000 \| 17272.000 \| 17340.000

Ship it!

```
Ship It!
```

Change Summary:

Completely reworked fix based on feedback on Illumos developer mailing list

Summary:

-	11818 Skip unavailable sensors in IPMI topo plugin
+	11818 IPMI topo plugin shouldn't return data from unavailable sensors

Description:

~		This change modifies the sensor enumeration code in the IPMI topo login to check the unavailable flag of each sensor reported by the BMC. If the sensor is unavailable it will be skipped over as we cannot obtain readings anyway.
	~	This change accomplishes two things:
	+	- It add an "unusable" (that is its name, not its state) topo method to the IPMI provider. This method will mark unavailable sensor as unusable to indicate to consumers that they cannot use them.
	+	- It will not return data read from an unavailable IPMI sensor.

Testing Done:

~		1.) Recorded the output of "/usr/lib/fm/fmd/fmtopo -e" on a Supermicro X9 2U.
	~	1.) Recorded the output of "/usr/lib/fm/fmd/fmtopo -S" on a Supermicro X9 2U.
		2.) Run full nightly, used "onu" to create new BE and booted into it.
~		3.) Recorded the new output of "/usr/lib/fm/fmd/fmtopo -e" on the same machine. The differences are as expected:
	~	3.) Recorded the new output of "/usr/lib/fm/fmd/fmtopo -S" on the same machine. The differences are as expected:

~		--- before.txt 2019-10-15 13:21:53.133213816 +0000
~		+++ after.txt 2019-10-15 15:06:34.855121633 +0000
~		@@ -4,9 +4,3 @@
~		/chassis0/fan1
~		/chassis0/fan2
~		/chassis0/fan2
~		-/chassis0/fan3
~		-/chassis0/fan3
~		-/chassis0/fan4
~		-/chassis0/fan4
~		-/chassis0/fan5
~		-/chassis0/fan5
	~	--- before.txt 2019-11-05 10:40:24.143900715 +0000
	~	+++ after.txt 2019-11-05 11:24:05.281979308 +0000
	~	@@ -1,5 +1,5 @@
	~	TIME UUID
	~	-Nov 05 10:40:23 591d6fa7-b2a0-ee13-ab35-a08b376cd8b2
	~	+Nov 05 11:24:05 48db20e3-81b7-431c-d266-c3c20ecbf8fa
	~
	~	hc://:product-id=T3300-B1:server-id=zebi-46:chassis-id=TS1502-0076/chassis=0
	~	Present: true
	~	@@ -31,7 +31,7 @@
	~
	~	hc://:product-id=T3300-B1:server-id=zebi-46:chassis-id=TS1502-0076/chassis=0/fan=1
	+	Present: true
	+	- Unusable: false
	+	+ Unusable: true
	+
	+	hc://:product-id=T3300-B1:server-id=zebi-46:chassis-id=TS1502-0076/chassis=0/fan=1?sensor=Fan2
	+	Present: true
	+	@@ -39,7 +39,7 @@
	+
	+	hc://:product-id=T3300-B1:server-id=zebi-46:chassis-id=TS1502-0076/chassis=0/fan=2
	+	Present: true
	+	- Unusable: false
	+	+ Unusable: true
	+
	+	hc://:product-id=T3300-B1:server-id=zebi-46:chassis-id=TS1502-0076/chassis=0/fan=2?sensor=Fan3
	+	Present: true
	+	@@ -63,7 +63,7 @@
	+
	+	hc://:product-id=T3300-B1:server-id=zebi-46:chassis-id=TS1502-0076/chassis=0/fan=5
	+	Present: true
	+	- Unusable: false
	+	+ Unusable: true

~		4.) Run "/usr/lib/fm/fmd/fmtopo" to check that the right chassis fan sensors were skipped:
	~	hc://:product-id=T3300-B1:server-id=zebi-46:chassis-id=TS1502-0076/chassis=0/fan=5?sensor=Fan6
	+	Present: true

~		hc://:product-id=T3300-B1:server-id=zebi-46:chassis-id=TS1502-0076/chassis=0/fan=0
	~	"ipmitool sensor" confirms that the correct sensors were marked as unusable:

~		hc://:product-id=T3300-B1:server-id=zebi-46:chassis-id=TS1502-0076/chassis=0/fan=0?sensor=Fan1
	~	Fan1 \| 6256.000 \| RPM \| ok \| 340.000 \| 408.000 \| 476.000 \| 17204.000 \| 17272.000 \| 17340.000
-
-		hc://:product-id=T3300-B1:server-id=zebi-46:chassis-id=TS1502-0076/chassis=0/fan=1
-
-		hc://:product-id=T3300-B1:server-id=zebi-46:chassis-id=TS1502-0076/chassis=0/fan=1?sensor=Fan4
-
-		hc://:product-id=T3300-B1:server-id=zebi-46:chassis-id=TS1502-0076/chassis=0/fan=2
-
-		hc://:product-id=T3300-B1:server-id=zebi-46:chassis-id=TS1502-0076/chassis=0/fan=2?sensor=Fan5
-
-		These are indeed the sensors that the "ipmitool" reports values for:
-
-		Fan1 \| 6460.000 \| RPM \| ok \| 340.000 \| 408.000 \| 476.000 \| 17204.000 \| 17272.000 \| 17340.000
		Fan2 \| na \| RPM \| na \| 340.000 \| 408.000 \| 476.000 \| 17204.000 \| 17272.000 \| 17340.000
		Fan3 \| na \| RPM \| na \| 340.000 \| 408.000 \| 476.000 \| 17204.000 \| 17272.000 \| 17340.000
~		Fan4 \| 6596.000 \| RPM \| ok \| 340.000 \| 408.000 \| 476.000 \| 17204.000 \| 17272.000 \| 17340.000
~		Fan5 \| 6392.000 \| RPM \| ok \| 340.000 \| 408.000 \| 476.000 \| 17204.000 \| 17272.000 \| 17340.000
	~	Fan4 \| 6324.000 \| RPM \| ok \| 340.000 \| 408.000 \| 476.000 \| 17204.000 \| 17272.000 \| 17340.000
	~	Fan5 \| 6188.000 \| RPM \| ok \| 340.000 \| 408.000 \| 476.000 \| 17204.000 \| 17272.000 \| 17340.000
		Fan6 \| na \| RPM \| na \| 340.000 \| 408.000 \| 476.000 \| 17204.000 \| 17272.000 \| 17340.000

Diff:

Revision 2 (+192 -65)

Show changes

	usr/src/lib/fm/topo/modules/common/fac_prov_ipmi/fac_prov_ipmi.c
	usr/src/lib/fm/topo/modules/common/ipmi/ipmi_enum.c

Ship it!

In general LGTM, but I kinda dislike the naming of the attribute "unusable" because of the double negative. But I guess that's just a matter of taste and I don't want to block this because of it.

mscheler Nov. 5, 2019, 2:57 p.m.

I didn't invent that name. It is an existing feature of the framework.

usr/src/lib/fm/topo/modules/common/ipmi/ipmi_enum.c (Diff revision 2)

Show all issues

Should there be a call to topo_mod_strfree(mode, name) here?

mscheler Nov. 5, 2019, 4:57 p.m.

No, because "name" is never set to anything other than NULL if we found a node via topo_node_getspecific() and "ep" is non NULL.

However that is non obvious and could change in the future. I'll put the call in.

pwinder Nov. 5, 2019, 5:07 p.m.
```
ok - change and ship it
```

Ship it!

Change Summary:

Small safety change based on review feedback

Diff:

Revision 3 (+193 -65)

Show changes

	usr/src/lib/fm/topo/modules/common/fac_prov_ipmi/fac_prov_ipmi.c
	usr/src/lib/fm/topo/modules/common/ipmi/ipmi_enum.c

Ship it!

```
Ship It!
```

Ship it!

```
Ship It!
```

You have a pending review.

Review Board 3.0.18

11818 IPMI topo plugin shouldn't return data from unavailable sensors

Testing Done:

Change Summary:

Summary:

Description:

Testing Done:

Diff:

Change Summary:

Diff:

Status: Closed (submitted)