Redfish plugin for hardware monitoring #7082

sarvanikonda · 2020-02-25T09:25:02Z

This PR consists of redfish APIs for hardware monitoring of DELL and HP servers.

Required for all PRs:

Signed CLA.
Associated README.md updated.
Has appropriate unit tests.

Signed-off-by: admin account <vzwadmin@tpa_poc_influxdb_1-tampa.vici.verizon.com>

ssoroka

Thanks for the PR!
I've included a list of recommendations, as well we may want to remove the .dell_power.json.swp file as it doesn't seem to be relevant to the tests.

plugins/inputs/redfish/README.md

plugins/inputs/redfish/redfish.go

ssoroka · 2020-05-08T14:58:01Z

plugins/inputs/redfish/redfish.go

+func getThermal(host, username, password, id string) error {
+	r := fmt.Sprint(host, "/redfish/v1/Chassis/", id, "/Thermal")
+	client := &http.Client{}
+	http.DefaultTransport.(*http.Transport).TLSClientConfig = &tls.Config{InsecureSkipVerify: true}


Couple issues here:

client should be set globally and reused; it's thread safe (goroutine safe) by default.

We probably don't want to use InsecureSkipVerify by default, as it doesn't validate anything about where it's connecting to. If possible we want to default to something a little more secure and give users the option to set this, with similar scary naming.

Rather than modifying the TLS config on the default transport, you want to set TLS config on the client's transport that you're creating.

I think we probably made this a little less secure by turning TLS off by default. Do we know if Redfish typically has https available by default?

From the docs I read and by some practical knowledge I understood that redfish only supports https by default and even python redfish library only supports https protocol, I have changed the code accordingly.

plugins/inputs/redfish/redfish.go

danielnelson

The produced data model doesn't align very closely with what I see in the Redfish specification, I'd like to mirror the types as I think this will make it easier to extend to new types and understand for users familiar with Redfish. The most straightforward way to me seems to be to one metric for each supported resource type. For example we could have one type for Thermal, one for Power.

plugins/inputs/redfish/README.md

plugins/inputs/redfish/redfish.go

plugins/inputs/redfish/redfish_test.go

plugins/inputs/redfish/README.md

danielnelson · 2020-05-11T19:52:08Z

plugins/inputs/redfish/README.md

+
+### Metrics for Dell Servers
+
+- cpu_temperatures


W should name these redfish_thermal, redfish_power which will fit in well with the output of other plugins.

The produced data model doesn't align very closely with what I see in the Redfish specification, I'd like to mirror the types as I think this will make it easier to extend to new types and understand for users familiar with Redfish. The most straightforward way to me seems to be to one metric for each supported resource type. For example we could have one type for Thermal, one for Power.

We have two type data coming from each resource(thermal,power) , if we send all the data from each resource to a single measurement it would be a problem as in the thermal resource the number of fans and CPUs temperature varies, for example there might be 50 CPUs but only 7 Fans, Similar for Power resource as well. It would be appropriate to store them in different measurements like redfish_thermal_temperatures, redfish_thermal_fans,redfish_power_powersupplies, redfish_power_voltages. What would you think of it?

plugins/inputs/redfish/redfish.go

ssoroka

I think you're pretty much there. A few small comments.

ssoroka · 2020-05-25T15:28:01Z

plugins/inputs/redfish/redfish.go

+	State  string
+	Health string
+}
+type speed struct {


struct casing isn't consistent. Use Capital first letter for public structs, lowercase for private.

ssoroka · 2020-05-25T15:28:05Z

plugins/inputs/redfish/redfish.go

+			}
+
+		} else {
+			return fmt.Errorf("received status code %d (%s), expected 200",


This stops processing as soon as it finds a url that doesn't return a 200. This is probably okay, but you might want to return whatever data you can manage, instead of no data.

It would be appropriate to stop the process when it does not return 200, in some scenarios for example user might give wrong credentials or ID, the process will stop at 1st API call and also if there is a proxy or any connectivity problem the process will be stopped at 1st API call itself. there might be rare situations where we receive data for one API and not for other APIs.

ssoroka · 2020-05-25T15:29:28Z

plugins/inputs/redfish/redfish.go

+	Status         CpuStatus `json:"Status"`
+}
+type Payload struct {
+	Temperatures  []*Cpu   `json:",omitempty"`


might want to use slices of structs instead of slices of pointers to structs. This way you don't need to worry about the nil pointer case

danielnelson · 2020-06-05T17:51:53Z

plugins/inputs/redfish/redfish.go

+	}
+	if (len(r.ClientConfig.TLSCA) == 0) || (len(r.ClientConfig.TLSCert) == 0 && len(r.ClientConfig.TLSKey) == 0) {
+		var insecuretls tls.ClientConfig
+		insecuretls.InsecureSkipVerify = true


Use the tls.ClientConfig type to provide the insecure option and feed whatever configuration it provides into the http.Transport. Change the host option to be a base url so that it will include the scheme:

address = "https://127.0.0.1"

I will change it as suggested by you but by default redfish only supports HTTPS protocol, if we just feed tls.ClientConfig config to http.Transport , users might not provide TLS configuration in conf file and errors might occur (errors like cannot validate certificate, etc might occur.)

danielnelson · 2020-06-05T17:55:39Z

plugins/inputs/redfish/redfish.go

+	if len(r.Host) == 0 || len(r.BasicAuthUsername) == 0 || len(r.BasicAuthPassword) == 0 {
+		return fmt.Errorf("Did not provide IP or username and password")
+	}
+	if len(r.Id) == 0 {
+		return fmt.Errorf("Did not provide all the ID of the resource")
+	}


Check for Host and ID in the Init() function. Don't check for the username/password, but set it on the request only if one of the options is non-empty.

updated to check Host, username/password and ID in the Init() function(included username/password check as well because for redfish APIs the authorisation is mandatory)

danielnelson · 2020-06-05T17:59:38Z

plugins/inputs/redfish/redfish.go

+		resp, err := r.client.Do(req)
+		if err != nil {
+			return err
+		}
+		if resp.StatusCode == 200 {


We need to close the response body anytime Do returns without error. This will ensure that connections are properly reused and not "leaked". The easiest way to do this is to use defer, but to do so you will need to split the request code into it's own function, since defer code runs when the function ends.

danielnelson · 2020-06-05T18:00:10Z

plugins/inputs/redfish/redfish.go

+		if resp.StatusCode == 200 {
+			body, err := ioutil.ReadAll(resp.Body)
+			if err != nil {
+				return fmt.Errorf("%v", err)


return err

danielnelson · 2020-06-05T18:06:26Z

plugins/inputs/redfish/redfish.go

+			tags["room"] = payload.Location.PostalAddress.Room
+			tags["rack"] = payload.Location.Placement.Rack
+			tags["row"] = payload.Location.Placement.Row
+			tags["severity"] = Severity(j.UpperThresholdCritical, j.UpperThresholdFatal, j.ReadingCelsius)


We should remove the severity tag, we avoid adding computed values since they are best calculated at query time or optionally in a processor. Especially since it seems that this is a new concept, not part of the redfish specification.

danielnelson · 2020-06-05T19:55:25Z

plugins/inputs/redfish/README.md

+
+### Example Output For Dell
+```
+redfish_thermal_temperatures,source=test-hostname,name=CPU1\ Temp,source_ip=http://190.0.0.1,host=test-telegraf,datacenter="Tampa",health="OK",rack="12",room="tbc",row="3",state="Enabled",severity="OK" temperature=41,upper_threshold_critical=59,upper_threshold_fatal=64 1582114112000000000


I know we talked about this earlier, but I'm having second thoughts about the source_ip tag. It seems that we don't necessarily have an IP Address, it could just as easily be a hostname. The source_ip also doesn't necessarily correspond to the source tag, so I feel the name doesn't fit well.

How about we call it address instead?

Let's also make sure it is only the hostname, not a URL.

danielnelson · 2020-06-05T20:02:54Z

plugins/inputs/redfish/testdata/dell_hostname.json

@@ -0,0 +1,329 @@
+{


Rename dell_computer_system.json

renamed it as dell_systems.json as it sounds more appropriate.

danielnelson · 2020-06-05T20:03:13Z

plugins/inputs/redfish/testdata/dell_location.json

@@ -0,0 +1,187 @@
+{


Rename dell_chassis

danielnelson · 2020-06-05T22:03:17Z

plugins/inputs/redfish/testdata/hp_hostname.json

@@ -0,0 +1,319 @@
+{


Rename hp_computer_system.json

renamed it as hp_systems.json as it sounds more appropriate.

danielnelson · 2020-06-05T22:05:44Z

plugins/inputs/redfish/redfish.go

+
+func (r *Redfish) Gather(acc telegraf.Accumulator) error {
+	var url []string
+	var payload Payload


Parsing all the data into a single Payload type makes it more difficult to determine where the various bits of data are sourced from. It also increases the chances of value conflicts across resources and will make it difficult to support multiple Power or Thermal resources per ComputerSystem. Instead of using this shared type we should parse each resource into its own dedicated type.

I suggest we relay out of the Gather flow as follows:

Create a func get(url, type struct) error that will do requests and parse the json into the provided struct. Having this function will help with the response body closing mentioned in the review.

In Gather Get the ComputerSystem, Chassis resources. In the future it would be nice to follow the @odata.i links in case a system has multiple Thermal or Power resources, or the ID is not shared for all resources, but it is not required now.

Next get the Thermal resource, then I suggest another function that creates the tags and fields using it and the Chassis and ComputerSystem types, and call AddFields.

Finally get the Power resource, also in another function create the tags and fields using it and the Chassis and ComputerSystem types, and call AddFields.

About parsing data into a single Payload was suggested by Steven the other reviewer, previously the json data was parsed into different structs, What shall we do now?

Okay, don't worry about doing this refactor.

danielnelson · 2020-06-10T19:59:33Z

plugins/inputs/redfish/README.md

+  password = "test"
+
+  ## Resource Id for redfish APIs
+  id="System.Embedded.1"


The ComputerSystem naming is taken from Redfish Resource Guid, do you have a link to where the ServerSystem is described? Also, don't forget to update this README.

danielnelson · 2020-06-10T20:00:04Z

plugins/inputs/redfish/redfish.go

+	}
+	tlsCfg, err := r.ClientConfig.TLSConfig()
+	if err != nil {
+		return fmt.Errorf("%v", err)


return err, use this pattern throughout the PR

danielnelson · 2020-06-10T20:00:30Z

plugins/inputs/redfish/redfish.go

+
+func (r *Redfish) Init() error {
+	if len(r.Address) == 0 || len(r.BasicAuthUsername) == 0 || len(r.BasicAuthPassword) == 0 {
+		return fmt.Errorf("Did not provide IP or username and password")


Lowercase error message: https://github.com/golang/go/wiki/CodeReviewComments#error-strings

danielnelson · 2020-06-10T20:03:04Z

plugins/inputs/redfish/README.md

+
+### Example Output 
+```
+redfish_thermal_temperatures,source=test-hostname,name=CPU1\ Temp,address=http://190.0.0.1,host=test-telegraf,datacenter="Tampa",health="OK",rack="12",room="tbc",row="3",state="Enabled" reading_celsius=41,upper_threshold_critical=59,upper_threshold_fatal=64 1582114112000000000


Make sure to update this section

danielnelson · 2020-06-18T19:18:11Z

Merged for 1.15.0, excellent work @sarvanikonda

danielnelson · 2020-06-19T01:53:05Z

Hey @sarvanikonda, I'm trying to get the plugin working with this simulator, and I noticed that the HP test examples don't have a file for the Chassis resource. Do these exist, there is a link listed in the System resource to /redfish/v1/Chassis/1/.

sarvanikonda · 2020-06-19T06:14:19Z

Hey @danielnelson from the Chassis resource I am fetching Location details, for HP the /redfish/v1/Chassis/1/ exists but the location details in the resource are not provided by HP redfish, I used the existing the hp_power.json resource file in testdata as input to the /redfish/v1/Chassis/1/ endpoint in redfish_test.go file as the Location data is anyway not available if this is going to confuse people I will add the Chassis resource file in test data and change the redfish_test.go file.

danielnelson · 2020-06-22T23:22:42Z

Got it, yeah let's add this file for completeness when you have some free time. What I found with this simulator is that the ID is not necessarily the same between the System and the Chassis endpoints.

I am making a small change to the plugin to start with the System and use it's Chassis link. This requires a Chassis response but for now I can just toss in a placeholder.

sarvanikonda · 2020-06-24T06:54:14Z

Hi @danielnelson I was also planning to add LowerThresholdCritical and LowerThresholdFatal values for temperatures,fans and voltages, I see that a PR has been raised by you #7722, would you like to add these values in 7722 PR ? or shall I wait to 7722 PR be merged to master and then add the values and raise a PR.

danielnelson · 2020-06-24T18:00:13Z

I added these values to #7722 and also adjusted some field types match what is listed in the schema. This will probably require you to drop some measurements from your database but better now than after the release.

sarvanikonda · 2020-06-24T21:09:34Z

Hey @danielnelson could you be more specific about dropping some measurements? and thank you very much for adding the lower threshold values, but in the code these values are also added in structs but not added in gather function?

danielnelson · 2020-06-24T21:30:08Z

On the fields that I switched to be a float, you will get "type conflict error" when you insert into InfluxDB. To clear this up you have a few options but the most straightforward way is to delete the old metrics in the influx cli:

drop series from redfish_thermal_temperatures
drop series from redfish_thermal_fans

but in the code these values are also added in structs but not added in gather function?

Uh oh, me trying to move to fast and failing badly.

sarvanikonda · 2020-06-24T21:38:58Z

oh, got it now, when you mentioned to drop measurements I thought you were thinking to delete some measurements in the code.
Thanks again for updating the code. I will test it once the lower threshold values are added in gather function.

admin account and others added 2 commits February 25, 2020 08:07

Added redfish plugin

2e4f6f5

Signed-off-by: admin account <vzwadmin@tpa_poc_influxdb_1-tampa.vici.verizon.com>

Update redfish_test.go

0045edd

sarvanikonda mentioned this pull request Feb 25, 2020

Add an input plugin for iBMC (Huawei Server) #3990

Closed

Did make check

75f5ba0

Signed-off-by: admin account <vzwadmin@tpa_poc_influxdb_1-tampa.vici.verizon.com>

danielnelson added the new plugin label Feb 26, 2020

added license

8a2178f

ssoroka requested review from ssoroka and danielnelson May 7, 2020 15:26

ssoroka suggested changes May 8, 2020

View reviewed changes

sarvanikonda added 2 commits May 11, 2020 19:42

optimised redfish plugin code

3cbf86d

optimised redfish plugin code

06a33dc

sarvanikonda requested a review from ssoroka May 11, 2020 14:46

danielnelson suggested changes May 11, 2020

View reviewed changes

ssoroka reviewed May 13, 2020

View reviewed changes

plugins/inputs/redfish/redfish.go Outdated Show resolved Hide resolved

sarvanikonda added 2 commits May 18, 2020 19:55

changes made as requested

06e6e4a

removed commented lines in redfish_test.go

94d64b1

sarvanikonda requested review from danielnelson and ssoroka May 18, 2020 15:34

removed comments from redfish.go

93cf584

ssoroka reviewed May 25, 2020

View reviewed changes

added threshold data

9015189

ssoroka approved these changes May 26, 2020

View reviewed changes

danielnelson suggested changes Jun 5, 2020

View reviewed changes

made changes as required

36a041e

danielnelson suggested changes Jun 10, 2020

View reviewed changes

updated README.md and redfish.go

c18917a

danielnelson added this to the 1.15.0 milestone Jun 18, 2020

danielnelson approved these changes Jun 18, 2020

View reviewed changes

danielnelson merged commit b0cd913 into influxdata:master Jun 18, 2020

rhajek pushed a commit to bonitoo-io/telegraf that referenced this pull request Jul 13, 2020

Add redfish input plugin (influxdata#7082)

ca8707d

idohalevi pushed a commit to idohalevi/telegraf that referenced this pull request Sep 29, 2020

Add redfish input plugin (influxdata#7082)

7fded79

arstercz pushed a commit to arstercz/telegraf that referenced this pull request Mar 5, 2023

Add redfish input plugin (influxdata#7082)

85639b6

Redfish plugin for hardware monitoring #7082

Redfish plugin for hardware monitoring #7082

Conversation

sarvanikonda commented Feb 25, 2020 • edited Loading

Required for all PRs:

ssoroka 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

danielnelson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ssoroka 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

sarvanikonda Jun 9, 2020 • edited Loading

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

danielnelson commented Jun 18, 2020

danielnelson commented Jun 19, 2020

sarvanikonda commented Jun 19, 2020

danielnelson commented Jun 22, 2020

sarvanikonda commented Jun 24, 2020

danielnelson commented Jun 24, 2020

sarvanikonda commented Jun 24, 2020

danielnelson commented Jun 24, 2020 • edited Loading

sarvanikonda commented Jun 24, 2020

sarvanikonda commented Feb 25, 2020 •

edited

Loading

sarvanikonda Jun 9, 2020 •

edited

Loading

danielnelson commented Jun 24, 2020 •

edited

Loading