Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Workaround of bad SNMP implementation in EDS device. #9801

Merged
merged 3 commits into from Feb 12, 2019

Conversation

Projects
None yet
2 participants
@PipoCanaja
Copy link
Contributor

commented Feb 10, 2019

The EDS devices are sometimes replying to snmpget with an extra ".0" after the index. This patch checks this situation and extracts the correct value if available.

DO NOT DELETE THIS TEXT

Please note

Please read this information carefully. You can run ./scripts/pre-commit.php to check your code before submitting.

Testers

If you would like to test this pull request then please run: ./scripts/github-apply <pr_id>, i.e ./scripts/github-apply 5926
After you are done testing, you can remove the changes with ./scripts/github-remove. If there are schema changes, you can ask on discord how to revert.

@murrant

This comment has been minimized.

Copy link
Member

commented Feb 10, 2019

@PipoCanaja So, you need to omit {{$index }} in the yaml? We can update the tests to allow that.

@PipoCanaja

This comment has been minimized.

Copy link
Contributor Author

commented Feb 10, 2019

@PipoCanaja So, you need to omit {{$index }} in the yaml? We can update the tests to allow that.

Nope, not needed.
You need to snmp[get|walk] the exact . and you get a reply with ..0 = "value" or you snmpwalk the table and you get the list of . as expected ...
So the solution implemented here and tested with the device is the only one I see so far.
And it works with the test data (which are not buggy thanks to snmpsim) and the real device.

@murrant

This comment has been minimized.

Copy link
Member

commented Feb 10, 2019

@PipoCanaja but all this does is change the oid stored in the database then fetch it. Why not just store the correct oid (without the .0)?

@PipoCanaja

This comment has been minimized.

Copy link
Contributor Author

commented Feb 11, 2019

@murrant In fact not:
You need to query the correct OID+index for each sensor (as every the discovery finds it). But the reply you receive when queried with snmpget appears with an extra ".0" which makes the matching request/value fail. So this patch will check if the matching failed, and if yes, will try to match with an extra ".0".
This ensures that a better implementation in a later upgrade of the device (or the test data in snmpsim) works as well as the real buggy device :)

@murrant
Copy link
Member

left a comment

If you query it with the .0 it fails too right? What peach.

@PipoCanaja

This comment has been minimized.

Copy link
Contributor Author

commented Feb 11, 2019

Exact. It is really stupid but at least we have a solution ...

@PipoCanaja

This comment has been minimized.

Copy link
Contributor Author

commented Feb 11, 2019

@murrant
Do you have a way to force the unqueue of travis ? I did force a 2nd run, that succeeded but the original one is still stuck doing nothing ...

@murrant

This comment has been minimized.

Copy link
Member

commented Feb 11, 2019

@PipoCanaja you can restart within the Travis-CI web interface. Do you have access to that?

@PipoCanaja

This comment has been minimized.

Copy link
Contributor Author

commented Feb 11, 2019

I added a dot in a comment to force travis-ci to run again cleanly. It was saying "queued" but when opening the job in Travis, it said "done" ...
We'll see how it goes this time.
I tried to restart but it did not succeed to get out this "locked" situation...

@murrant

This comment has been minimized.

Copy link
Member

commented Feb 11, 2019

Ah, failure between github and travis.

@PipoCanaja

This comment has been minimized.

Copy link
Contributor Author

commented Feb 11, 2019

Ah, failure between github and travis.

Now, it's good.

@murrant
Copy link
Member

left a comment

Probably shouldn't have added support in the first place and waited for the vendor to fix it. My bad :)

@murrant murrant merged commit 1d1962a into librenms:master Feb 12, 2019

5 of 6 checks passed

codeclimate 2 issues to fix
Details
Inspection Summary
Details
Node: analysis
Details
Travis CI - Pull Request Build Passed
Details
WIP Ready for review
Details
license/cla Contributor License Agreement is signed.
Details

@murrant murrant added the Device 🖥 label Feb 12, 2019

@PipoCanaja

This comment has been minimized.

Copy link
Contributor Author

commented Feb 13, 2019

Probably shouldn't have added support in the first place and waited for the vendor to fix it. My bad :)

It seems that we'll have to wait a long time for the fix ... Anyway, it is not too ugly in the end :)

@PipoCanaja PipoCanaja deleted the PipoCanaja:eds_patch branch Feb 13, 2019

@lock lock bot locked as resolved and limited conversation to collaborators Apr 14, 2019

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.