Add support for raid0 devices in mdadm_linux collector.#253
Add support for raid0 devices in mdadm_linux collector.#253
Conversation
SuperQ
force-pushed
the
superq/md_raid0
branch
2 times, most recently
from
dc3d7f8 to
3711be7
Compare
|
Hmmm, I think I need to make an example of failed raid0. |
SuperQ
changed the title
SuperQ
force-pushed
the
superq/md_raid0
branch
3 times, most recently
from
2c950e1 to
60e134f
Compare
collector/mdadm_linux.go
Outdated
There was a problem hiding this comment.
End comments with periods everywhere.
|
Looks good in general, would be good to get a second opinion from someone with mdadm / RAID experience, like probably @brian-brazil. |
SuperQ
force-pushed
the
superq/md_raid0
branch
3 times, most recently
from
493b1b6 to
5516297
Compare
SuperQ
changed the title
|
Ok, fixed my go newbie bugs, and end-to-end tests. 💚 |
collector/mdadm_linux.go
Outdated
There was a problem hiding this comment.
In the check above we only ensure that the line has at least 3 parts. While I assume a line with less than 4 parts to be invalid, it's possible to run into an out-of-bound panic with this change, no?
Bonus points for adding a test for such invalid short lines.
There was a problem hiding this comment.
Ahh, yea, mainLine should always have atleast 4 parts anyway. I'll update the test.
SuperQ
force-pushed
the
superq/md_raid0
branch
2 times, most recently
from
9ebcf4c to
ce739bf
Compare
collector/mdadm_linux.go
Outdated
| if len(mainLine) < 4 { | ||
| return mdStates, fmt.Errorf("error parsing mdline: %s", l) | ||
| } | ||
| currentMD = mainLine[0] // name of md-device |
There was a problem hiding this comment.
Comments here should be full sentences.
|
👍 |
collector/mdadm_linux.go
Outdated
| // +1 to make it more obvious that the whole string containing the info is also returned as matches[0]. | ||
| if len(matches) < 1+1 { | ||
| return 0, fmt.Errorf("too few matches found in statusline: %s", statusline) | ||
| } else { |
There was a problem hiding this comment.
Reduce the indentation level in go if possible. This else branch is not necessary.
|
Ok, addressed all comments. |
|
👍 |
6 participants
Fix for #219