Skip to content

Avoid crash on failure to parse int in emerson_temp discovery. - #818

Closed
indigoparadox wants to merge 3 commits into
Checkmk:masterfrom
indigoparadox:indigoparadox-emerson-temp
Closed

Avoid crash on failure to parse int in emerson_temp discovery.#818
indigoparadox wants to merge 3 commits into
Checkmk:masterfrom
indigoparadox:indigoparadox-emerson-temp

Conversation

@indigoparadox

Copy link
Copy Markdown
Contributor

Thank you for your interest in contributing to Checkmk!
Consider looking into Readme regarding process details.

General information

While performing a discovery on certain Emerson/Vertiv devices, they can return an SNMP value to the generic Emerson temperature discovery function that cannot be parsed as an integer, causing this ValueError:
Checkmk_Crash_kale_lab_54b25f28-30d6-11f0-bf2f-005056a12db3_2025-05-14_11-46-04.tar.gz

This results in a perpetural state of "unknown" for the discovery service, and an accumulation of crash dumps:
2025-05-14 11_27_34-Checkmk Local site kale_lab - Services of host PTMONH07_VertivRectifier — Mozill

Proposed changes

There may be a better way to fix this, but it's difficult for me to know if e.g. changing the match string would break the original piece of hardware this check was written for. Python doesn't seem to have a quick or elegant way to comprehensively see if a string can be parsed as an integer other than to try and catch it if it fails, so that's what I opted for, here.

Thank you for your consideration!

@mo-ki

mo-ki commented May 19, 2025

Copy link
Copy Markdown
Member

Hi!
Thank you for your contribution!
The problem with this approach is that it masks all other problems as well. Is there any change to deal with a value like IB2 Temp 1 properly?
If that can't be done, I suggest to not detect the affected devices at all, by setting

detect=all_of(
    startswith(".1.3.6.1.4.1.6302.2.1.1.1.0", "Emerson Network Power"),
    not_ ...   # some condition to exclude affected devices
)

Any chance you can come up with a good condition on which the affected devices are?

@indigoparadox

indigoparadox commented May 19, 2025

Copy link
Copy Markdown
Contributor Author

I agree that there are some pitfalls, so here's an alternate approach:

All of our devices are exhibiting the issue outlined above because they seem to be providing OIDs beyond those the check was originally designed for. These OIDs (1.3.6.1.4.1.6302.2.1.2.7.3.*) do not appear in any MIB I presently have access to (e.g. https://mibbrowser.online/mibdb_search.php?mib=EES-POWER-MIB), and looking closely at their values, it's difficult to tell if they're even providing valid data for temperatures above "Temperature 1".

With this being the case, I've rewritten the OID selection to explicitly confine it to 1.3.6.1.4.1.6302.2.1.2.7.1 and 1.3.6.1.4.1.6302.2.1.2.7.2, thereby limiting it to the (known good and valid) values the check was originally designed for and eliminating the crash when attempting to parse invalid data (because these OIDs return valid temperatures in a sane range on our devices).

What do you think?

Thanks!

@indigoparadox
indigoparadox force-pushed the indigoparadox-emerson-temp branch from 9367c6d to 3570cf0 Compare May 19, 2025 14:30
@indigoparadox
indigoparadox force-pushed the indigoparadox-emerson-temp branch from 3570cf0 to acff302 Compare May 19, 2025 14:31
Comment thread cmk/base/legacy_checks/emerson_temp.py Outdated

def parse_emerson_temp(string_table: StringTable) -> StringTable:
return string_table
return [[x] for x in string_table[0]]

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are many ways of doing this, obviously, but this way a test is failing that ensures we can deal with an emtpy input. I suggest to leave line 52 alone, add a comment, and do this:

def parse_...
    # we only use at most the first two lines because....
    return string_table[:2]

@mo-ki

mo-ki commented May 23, 2025

Copy link
Copy Markdown
Member

I like this approach better.
I just realize that this plugin is not using the system object ID nor the system description in its detect spec. ~35 of our ~1000 plugins do that. It's unfortunate, because it means that one adiitional OID is fetched during every discovery of any of your hosts. Would you share your system description with us? Maybe this is an opportunity to improve this :-)

@indigoparadox

Copy link
Copy Markdown
Contributor Author

Certainly, the approach you outline in your review does seem to work well on our test device, so let's go with that.

The sysOID is: .1.3.6.1.4.1.6302.2.1

I hesitated to change any of the detection logic in the (probably naive) assumption that the original author was using a weird method of detection for a reason and I didn't want to break anyone's existing hardware that I may not have access to (especially if it's already behaving differently from the hardware I do have access to). If changing that does break anything, though, hopefully someone would submit an appropriate fix with a comment to this effect. 😌

@CheckmkCI CheckmkCI closed this in 2127f3b Jul 31, 2025
CheckmkCI pushed a commit that referenced this pull request Jul 31, 2025
Only use the first two sensor values, as values beyond that seem to be
handled in a different structure that we lack a concrete definition for.

A more robust approach would be to come up with a good condition on
which devices are affected, but this was not picked up in this change to
not break existing setups.

CMK-23523

Closes: #818
Co-authored-by: Moritz Kiemer <moritz.kiemer@checkmk.com>
Co-authored-by: Logan Connolly <logan.connolly@checkmk.com>
Change-Id: Iccae1567786b63afe451e7e410808cf8599d1c59
CheckmkCI pushed a commit that referenced this pull request Jul 31, 2025
Only use the first two sensor values, as values beyond that seem to be
handled in a different structure that we lack a concrete definition for.

A more robust approach would be to come up with a good condition on
which devices are affected, but this was not picked up in this change to
not break existing setups.

CMK-23523

Closes: #818
Co-authored-by: Moritz Kiemer <moritz.kiemer@checkmk.com>
Co-authored-by: Logan Connolly <logan.connolly@checkmk.com>
Change-Id: Iccae1567786b63afe451e7e410808cf8599d1c59
@github-actions github-actions Bot locked and limited conversation to collaborators Jul 31, 2025
@logan-connolly

Copy link
Copy Markdown
Member

@indigoparadox thank you for the contribution!

You can find more details regarding when this patch will be released in Werk #18282.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants