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.

Sign up for GitHub

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

Jump to bottom

[snmp] convert into network-check to run instances in parallel #2152

Merged
merged 2 commits into from
Jan 20, 2016
Merged

[snmp] convert into network-check to run instances in parallel #2152

merged 2 commits into from
Jan 20, 2016

Conversation

truthbk
Copy link
Member

@truthbk truthbk commented Dec 17, 2015

Multi-threaded SNMP Check (as a Network Check)

This PR allows checking for multiple SNMP instances in parallel, each running in it's own thread (within the allocated thread-pool). The actual OID/walk is still done serially for each instance, but this PR should help organizations with multiple instances configured with a performance benefit as a factor of the thread pool size.

NOTE: tests involving rates now require three runs because network checks report the results of the nth run on the n+1 run.

@truthbk truthbk mentioned this pull request Dec 17, 2015
Closed
@truthbk truthbk force-pushed the jaime/snmp_threadspeed branch 4 times, most recently from c1c8f75 to 1bdddc7 Compare December 21, 2015 20:24
@remh remh added this to the 5.7.0 milestone Dec 22, 2015
@yannmh yannmh self-assigned this Dec 23, 2015
yannmh
yannmh reviewed Dec 23, 2015
View reviewed changes
checks.d/snmp.py
retries = int(instance.get('retries', self.DEFAULT_RETRIES))

# Create SNMP command generator and aliases
self.create_command_generator(self.mibs_path, self.ignore_nonincreasing_oid)
Copy link
Member

Choose a reason for hiding this comment

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

Any reason why this was moved from __init__ to _check ?

@yannmh
Copy link
Member

yannmh commented Dec 23, 2015

Nice changes. Added a few comments here and there 🌵

@truthbk truthbk force-pushed the jaime/snmp_threadspeed branch 2 times, most recently from 4bb335f to 4d644bb Compare January 11, 2016 18:35
@truthbk
Copy link
Member Author

truthbk commented Jan 11, 2016

thanks a lot for your input @yannmh - if you could please take another pass sometime, that'd be great.

@truthbk truthbk force-pushed the jaime/snmp_threadspeed branch from 4d644bb to db0dbaa Compare January 11, 2016 19:26
@truthbk
Copy link
Member Author

truthbk commented Jan 11, 2016

Tests pass locally but not on Travis - check out what's up!

@truthbk truthbk force-pushed the jaime/snmp_threadspeed branch from 073de9d to f52109f Compare January 12, 2016 05:05
@yannmh
Copy link
Member

yannmh commented Jan 12, 2016

Your changes look good to be merged.

Failing tests are due to some flake8 missing dependencies on Travis's Python 2.6 environment.
I am not sure what's going on here but it's certainly unrelated to your changes: let's fix this in a separate PR.

@yannmh
Copy link
Member

yannmh commented Jan 12, 2016

My bad, there are actually multiple Travis CI errors. While it looks unrelated to your changes and specific to Travis CI Python 2.6 , let's investigate more before merging any change.

@truthbk truthbk force-pushed the jaime/snmp_threadspeed branch 2 times, most recently from fa05eca to da949b4 Compare January 12, 2016 22:06
@truthbk truthbk force-pushed the jaime/snmp_threadspeed branch from da949b4 to 146a44d Compare January 12, 2016 22:16
@truthbk
Copy link
Member Author

truthbk commented Jan 12, 2016

After rebasing to master all 2.6 tests are failing with what seems a broken virtualenv on Travis.

Also the snmpd test may sometimes flap because we collect some rates that are based on network stats (udpDatagrams, ifInOctets, etc) so if the stats don't change the rate is 0 and they're not flushed.

@truthbk truthbk force-pushed the jaime/snmp_threadspeed branch from 146a44d to eb9a5d7 Compare January 12, 2016 23:09
@truthbk
[snmp] converting snmp into network check - parallel instances.
058a3d0 
[snmp] Add instance name if not present before calling constructor.

[snmp] send Status, not check state.

[snmp] fixing tests

[snmp] if a force reload is issued, we should first stop all threads.
@truthbk
[snmp] fixing snmp tests, cleaning-up.
8af116a 
[snmp] fixing-up exception related issues now that we're dealing with threads. Clean-up.
@truthbk truthbk force-pushed the jaime/snmp_threadspeed branch from eb9a5d7 to 8af116a Compare January 19, 2016 19:35
@truthbk truthbk closed this Jan 19, 2016
@truthbk
Copy link
Member Author

truthbk commented Jan 19, 2016

closing in favor of #2214 to fix TravisCI issues.

@truthbk truthbk reopened this Jan 19, 2016
@yannmh
Copy link
Member

yannmh commented Jan 19, 2016

:shipit:

@truthbk truthbk mentioned this pull request Jan 19, 2016
Closed
truthbk added a commit that referenced this pull request Jan 20, 2016
@truthbk
Merge pull request #2152 from DataDog/jaime/snmp_threadspeed
b875555 
[snmp] convert into network-check to run instances in parallel
@truthbk truthbk merged commit b875555 into master Jan 20, 2016
@truthbk truthbk deleted the jaime/snmp_threadspeed branch January 20, 2016 21:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@yannmh yannmh

Labels
None yet
Projects
None yet
Milestone
5.7.0
Development

Successfully merging this pull request may close these issues.
3 participants
@truthbk @yannmh @remh