feat(inputs.slurm): Add a SLURM input plugin #15700

pcolladosoto · 2024-08-01T11:02:29Z

Summary

As set forth in the associated issue, this PR strives to add a new input plugin capable of recollecting data from a SLURM cluster. In our particular use case, this would make gathering and integrating job, node and other metadata into our regular monitoring infrastructure that much easier.

Also, given SLURM implements a REST API interface acquiring all this data doesn't call for parsing a ton of text and is instead a matter of implementing the appropriate clients. This effort has been undertaken in an accompanying repository so as to relocate some of the complexity outside of Telegraf. The repository in question is pcolladosoto/goslurm.

Thank you both for your time and for making Telegraf: it's an awesome tool!

If there are any errors or if there's anything missing I'd like first of all to apologise and second to make you aware that you shouldn't hesitate to contact me to follow up on any issue.

Thanks again!

Checklist

No AI generated code was used in this PR

Related issues

resolves #15699

powersj

Hi,

Thanks for the issue and PR! Looks like tests failed as you will ned to run go mod tidy. I have put some initial observations below, let us know if you have any questions.

Thanks again!

edit: and some linter issues:

plugins/inputs/slurm/slurm.go:42:68   unparam     (*Slurm).createHTTPClient - result 1 (error) is always nil
plugins/inputs/slurm/slurm.go:84:10   perfsprint  fmt.Errorf can be replaced with errors.New
plugins/inputs/slurm/slurm.go:140:2   gocritic    rangeValCopy: each iteration copies 928 bytes (consider pointers or indexing)
plugins/inputs/slurm/slurm.go:145:20  perfsprint  fmt.Sprintf can be replaced with faster strconv.Itoa
plugins/inputs/slurm/slurm.go:255:69  bodyclose   response body must be closed
plugins/inputs/slurm/slurm.go:261:48  errorlint   non-wrapping format verb for fmt.Errorf. Use `%w` to format errors
plugins/inputs/slurm/slurm.go:267:72  bodyclose   response body must be closed
plugins/inputs/slurm/slurm.go:273:48  errorlint   non-wrapping format verb for fmt.Errorf. Use `%w` to format errors
plugins/inputs/slurm/slurm.go:279:74  bodyclose   response body must be closed
plugins/inputs/slurm/slurm.go:285:49  errorlint   non-wrapping format verb for fmt.Errorf. Use `%w` to format errors
plugins/inputs/slurm/slurm.go:292:17  bodyclose   response body must be closed
plugins/inputs/slurm/slurm.go:298:54  errorlint   non-wrapping format verb for fmt.Errorf. Use `%w` to format errors
plugins/inputs/slurm/slurm.go:305:17  bodyclose   response body must be closed
plugins/inputs/slurm/slurm.go:311:56  errorlint   non-wrapping format verb for fmt.Errorf. Use `%w` to format errors

plugins/inputs/slurm/README.md

plugins/inputs/slurm/slurm.go

plugins/inputs/slurm/slurm_test.go

plugins/inputs/slurm/slurm.go

plugins/inputs/slurm/README.md

plugins/inputs/slurm/slurm.go

plugins/inputs/slurm/slurm_test.go

Hipska · 2024-08-07T14:34:15Z

PS You can mark the conversations as resolved when you applied the suggestions (I cannot do this myself)

pcolladosoto · 2024-08-07T14:38:13Z

Whops! I thought you could do that yourself... I went ahead and resolved all the conversations to which you reacted with 👍.

It looks like the last one remaining is the one on the Gather* function tests which was opened by @powersj.

Thanks a ton for letting me know about having to resolve conversations myself!

plugins/inputs/slurm/slurm.go

powersj

Thank you for your efforts!

We just tackled a single occurrence of the `bodyclose` error to try and see if we can fix it like we did... Judging by the implementation of the `Execute()` method the actual body of the request should be closed, but maybe the linter is not seeing it clearly...

TODO: handle `panic()`s within `Gather()`.

Instead of explicitly checking for `nil` pointers on each assignment we'd rather recover from `panic()`s caused by `nil` pointer dereferences to centralize the management of this type of errors.

Hipska

Some minor markup remarks

plugins/inputs/slurm/slurm.go

srebhan

Thanks for your update @pcolladosoto! I tried to clarify some of your questions, see my comments. However, the more I think about the auto-typing of the "tres" entries, the more I think it is a bad idea. Could we please either make really sure we always get the same datatype for a field or default to parsing as float and explicitly ask the user which fields should be integers?

srebhan · 2024-08-08T15:29:56Z

plugins/inputs/slurm/sample.conf

+  ##   nodes, partitions, reservations
+  ## Please note incorrect endpoints will be silently ignore and
+  ## that endpoint names are case insensitive.
+  # ignored_endpoints = []


I think having it swapped is better. Just set the corresponding endpoints in Init() if the list is empty and be done. This makes it also easier to extend the list and it will show the user what is actually collected.

srebhan · 2024-08-08T15:31:28Z

plugins/inputs/slurm/slurm.go

+	if s.ResponseTimeout == 0 {
+		s.ResponseTimeout = config.Duration(time.Second * 5)
+	}


Let me help you out here... Please set the ResponseTimeout: config.Duration(5 * time.Second) in init() (all lowercase) as this allows the user to overwrite the value in the config and set it to zero.

srebhan · 2024-08-08T15:56:46Z

plugins/inputs/slurm/slurm.go

+		parsedInt, err := strconv.ParseInt(val, 10, 64)
+		if err == nil && factor >= 1 {
+			parsedValues[tag] = parsedInt * int64(factor)
+			continue
+		}
+		parsedFloat, err := strconv.ParseFloat(val, 64)
+		if err == nil {
+			parsedValues[tag] = parsedFloat * factor
+			continue
+		}
+		parsedValues[tag] = val


Let me give you an example. Assume we got the following string values for the same tag (e.g. temperature):

temperature=23.9

temperature=23.1

temperature=23

The first two ones will clearly produce an error when trying to parse them as an int so you will get a field named temperature with a float value for the first two examples. However, the third line perfectly parses as an integer and therefore you will get a resulting temperature field being aninteger instead of a float. This will lead to type conflicts in e.g. InfluxDB as the same column cannot hold different datatypes.

Thinking more about it, you will have the same issue with

mem=10M resulting in integer 10

mem=1G resulting in integer 1024

mem=100k resulting in float 0.09765625

Thanks a ton for the input! Co-authored-by: Thomas Casteleyn <thomas.casteleyn@me.com>

pcolladosoto · 2024-08-12T12:46:56Z

Hi @srebhan! I'm waiting for the linters to finish up, but if everything passes how are we looking for a merge 🤔?

I believe I have addressed all the outstanding concerns, but please do let me know if I left any of the out.

Thanks a ton for your time, patience and helpful input 😄

srebhan

@pcolladosoto thank you very much for the quick update and the kind words. I have some more minor comments, mostly on pre-allocation and checking the endpoint list but nothing really big. I think if those are done, we are ready to merge.

plugins/inputs/slurm/slurm.go

plugins/inputs/slurm/sample.conf.in

Co-authored-by: Sven Rebhan <36194019+srebhan@users.noreply.github.com>

plugins/inputs/slurm/slurm.go

A correction was committed before we pushed our already corrected version... Sorry for the noise!

telegraf-tiger · 2024-08-14T10:15:25Z

Download PR build artifacts for linux_amd64.tar.gz, darwin_arm64.tar.gz, and windows_amd64.zip.
Downloads for additional architectures and packages are available below.

☺️ This pull request doesn't significantly change the Telegraf binary size (less than 1%)

📦 Click here to get additional PR build artifacts

Artifact URLs

DEB	RPM	TAR GZ	ZIP
amd64.deb	aarch64.rpm	darwin_amd64.tar.gz	windows_amd64.zip
arm64.deb	armel.rpm	darwin_arm64.tar.gz	windows_arm64.zip
armel.deb	armv6hl.rpm	freebsd_amd64.tar.gz	windows_i386.zip
armhf.deb	i386.rpm	freebsd_armv7.tar.gz
i386.deb	ppc64le.rpm	freebsd_i386.tar.gz
mips.deb	riscv64.rpm	linux_amd64.tar.gz
mipsel.deb	s390x.rpm	linux_arm64.tar.gz
ppc64el.deb	x86_64.rpm	linux_armel.tar.gz
riscv64.deb		linux_armhf.tar.gz
s390x.deb		linux_i386.tar.gz
		linux_mips.tar.gz
		linux_mipsel.tar.gz
		linux_ppc64le.tar.gz
		linux_riscv64.tar.gz
		linux_s390x.tar.gz

srebhan

Awesome! Thanks @pcolladosoto for all your work!

srebhan · 2024-08-15T08:56:18Z

@DStrand1 or @powersj I assigned this back to you to take another look as the code changed quite a bit. If you feel this looks good, please go ahead and merge!

powersj

Thanks for working through this with us!

pcolladosoto · 2024-08-15T21:32:27Z

Hi @powersj, @Hipska and @srebhan! It's been a pleasure to work on this PR with you. You have all been super helpful, kind and patient: I really appreciate it!

I'll continue to use Telegraf on a daily basis, but now I know there's a wonderful team of people behind it 😄

Thanks again for everything.

telegraf-tiger bot added feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin plugin/input 1. Request for new input plugins 2. Issues/PRs that are related to input plugins labels Aug 1, 2024

powersj reviewed Aug 1, 2024

View reviewed changes

powersj self-assigned this Aug 1, 2024

pcolladosoto force-pushed the master branch from 1bf812f to 82bd797 Compare August 2, 2024 00:10

Hipska suggested changes Aug 6, 2024

View reviewed changes

powersj reviewed Aug 6, 2024

View reviewed changes

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

Hipska suggested changes Aug 7, 2024

View reviewed changes

plugins/inputs/slurm/slurm_test.go Outdated Show resolved Hide resolved

plugins/inputs/slurm/slurm_test.go Outdated Show resolved Hide resolved

Hipska reviewed Aug 7, 2024

View reviewed changes

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

Hipska approved these changes Aug 7, 2024

View reviewed changes

powersj approved these changes Aug 7, 2024

View reviewed changes

pcolladosoto added 17 commits August 7, 2024 10:49

feat(inputs.slurm): Add a SLURM input plugin

2afc2c2

feat(inputs.slurm): Condense boolean into slice

5581231

feat(inputs.slurm): Fix remaining linter issues

28a343a

feat(inputs.slurm): Add link to LICENSE

d2616c8

feat(inputs.slurm): Add URL as a tag

ed130f4

feat(inputs.slurm): Add trailing newline

37ed8d8

feat(inputs.slurm): Remove domain and trim records

41b445d

feat(inputs.slurm): Dereference records

9fca629

TODO: handle `panic()`s within `Gather()`.

feat(inputs.slurm): Add Gather tests

e5062f7

feat(inputs.slurm): Add panic() recovery

d830f24

Instead of explicitly checking for `nil` pointers on each assignment we'd rather recover from `panic()`s caused by `nil` pointer dereferences to centralize the management of this type of errors.

feat(inputs.slurm): Add explicitl nil checks

c67a94a

feat(inputs.slurm): Fix linter errors

4e43799

feat(inputs.slurm): Fix linter issues

f05e2a2

feat(inputs.slurm): Fix linter issue

ac2b45f

feat(inputs.slurm): Rename url to source

c38bb21

feat(inputs.slurm): Use internal User-Agent

427252e

Hipska approved these changes Aug 8, 2024

View reviewed changes

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

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

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

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

srebhan requested changes Aug 8, 2024

View reviewed changes

pcolladosoto and others added 6 commits August 9, 2024 09:39

feat(inputs.slurm): Fix timeout configuration

d477095

feat(inputs.slurm): Leverage t.Run() w/ tweaks

a5c910f

feat(inputs.slurm): Switch to enabled inputs logic

f943d9d

Apply suggestions from code review

34d2a7b

Thanks a ton for the input! Co-authored-by: Thomas Casteleyn <thomas.casteleyn@me.com>

feat(inputs.slurm): Rework testing structure

24e01e4

feat(inputs.slurm): Just parse float64 on TRES

ab5b932

srebhan reviewed Aug 12, 2024

View reviewed changes

pcolladosoto and others added 3 commits August 14, 2024 10:25

Check enabled endpoints to catch typos

3378da5

Co-authored-by: Sven Rebhan <36194019+srebhan@users.noreply.github.com>

feat(inputs.slurm): Fix endpoint checks

ed20b4d

feat(inputs.slurm): Less allocations & scope fix

dacf8cb

srebhan reviewed Aug 14, 2024

View reviewed changes

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

srebhan and others added 5 commits August 14, 2024 11:15

Update plugins/inputs/slurm/slurm.go

8cbbde4

feat(inputs.slurm): Add support for the P suffix

3b5a534

feat(inputs.slurm): Preallocate tags and records

96e6a42

feat(inputs.slurm): Update endpoints comment

e734140

Merge branch 'master' of pcolladosoto/telegraf

8d7316f

A correction was committed before we pushed our already corrected version... Sorry for the noise!

srebhan approved these changes Aug 15, 2024

View reviewed changes

srebhan assigned powersj and unassigned srebhan Aug 15, 2024

powersj approved these changes Aug 15, 2024

View reviewed changes

powersj merged commit 7b54626 into influxdata:master Aug 15, 2024
27 checks passed

github-actions bot added this to the v1.32.0 milestone Aug 15, 2024

BrewTestBot mentioned this pull request Sep 9, 2024

telegraf 1.32.0 Homebrew/homebrew-core#184076

Merged

asaharn pushed a commit to asaharn/telegraf that referenced this pull request Oct 16, 2024

feat(inputs.slurm): Add a SLURM input plugin (influxdata#15700)

e812321

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(inputs.slurm): Add a SLURM input plugin #15700

feat(inputs.slurm): Add a SLURM input plugin #15700

pcolladosoto commented Aug 1, 2024

powersj left a comment •

edited

Loading

Hipska commented Aug 7, 2024

pcolladosoto commented Aug 7, 2024

powersj left a comment

Hipska left a comment

srebhan left a comment

srebhan Aug 8, 2024

srebhan Aug 8, 2024

srebhan Aug 8, 2024

pcolladosoto commented Aug 12, 2024

srebhan left a comment

telegraf-tiger bot commented Aug 14, 2024

Artifact URLs

srebhan left a comment

srebhan commented Aug 15, 2024

powersj left a comment

pcolladosoto commented Aug 15, 2024

feat(inputs.slurm): Add a SLURM input plugin #15700

feat(inputs.slurm): Add a SLURM input plugin #15700

Conversation

pcolladosoto commented Aug 1, 2024

Summary

Checklist

Related issues

powersj left a comment • edited Loading

Choose a reason for hiding this comment

Hipska commented Aug 7, 2024

pcolladosoto commented Aug 7, 2024

powersj left a comment

Choose a reason for hiding this comment

Hipska left a comment

Choose a reason for hiding this comment

srebhan left a comment

Choose a reason for hiding this comment

srebhan Aug 8, 2024

Choose a reason for hiding this comment

srebhan Aug 8, 2024

Choose a reason for hiding this comment

srebhan Aug 8, 2024

Choose a reason for hiding this comment

pcolladosoto commented Aug 12, 2024

srebhan left a comment

Choose a reason for hiding this comment

telegraf-tiger bot commented Aug 14, 2024

Artifact URLs

srebhan left a comment

Choose a reason for hiding this comment

srebhan commented Aug 15, 2024

powersj left a comment

Choose a reason for hiding this comment

pcolladosoto commented Aug 15, 2024

powersj left a comment •

edited

Loading