vincentbernat / snimpy

interactive SNMP tool with Python
http://snimpy.readthedocs.org/
185 stars 44 forks source link

Table traversal #46

Open ghost opened 8 years ago

ghost commented 8 years ago

Not a real issue, just some notes to table traversal: (1) the recipe for table traversal in documentation is wrong (very inefficient):

for index in manager.ifDescr:
    print(manager.ifDescr[index]) # results in one more (useless) SNMP get
                                  # for every iteration

it shoud be rewritten (should work in head, but tested only with 0.8.3) as:

for index, ifDescr in manager.ifDescr.iteritems():
    print(ifDescr)

or without the index if you also implement a ProxyColumn.itervalues(), (2) in last versions, one is allowed to change the default bulk parameters, but I would propose to change them also "per traversal" - like .iteritems(bulk = False) or .iteritems(max_bulk = 10) - it is quite usefull, (3) why does the .iteritems() method walk all values before yielding first item? it could yield first value(s) after first (bulk) response -- this would not only speed up first results, but would be also more efficient if the caller does not consume all the values (i.e. break in the mentioned for ... constructs).

vincentbernat commented 8 years ago

Hey!

The main goal of Snimpy is to feel "pythonic". The use of iteritems() is "less" pythonic. But I agree this is less efficient. There is also caching that could help here.

As for iteritems, I don't remember why I implement that instead of doing all that in iter. I may need to dig a bit about that. Depending on that, adding a bulk parameter would be possible or not.

For (3), which version of PySNMP are you using?

ghost commented 8 years ago

Hello Vincent,

I understand and agree you've chosen MIB "columns" to be accessible like Python dicts. I just proposed to document how to write the traversal more efficiently (the historical coincidence Python dicts iterate over keys by default, which causes Snimpy iterator to fetch again the same values it has just received -- I agree your current API is more pythonic, its right).

I believe you implemented .iteritems() just as a more efficient alternative to .__iter__() :-) I noticed this behavior (the additional SNMP get(s) when traversing with default iterator as in the readthedocs) by an accident (wasn't looking for efficiency): I have a buggy agent (SNMP card for a UPS) which returns tabular data only to 'get next' (is not able to send a table item in response to specific 'get').

I discovered (3) at first on my Debian stable: Python 2.7.9 + Snimpy 0.8.3. I've just tried with Debian testing as well: Python 3.4.3 + Snimpy 0.8.8. But it behaves the same: walks (SNMP bulk get next) all the values:

#!/usr/bin/python3
import snimpy.manager
snimpy.manager.load('IF-MIB')
m = snimpy.manager.Manager(..., version = 2)
for index, if_descr in manager.ifDescr.iteritems():
    break
vincentbernat commented 8 years ago

For (3), I would have believed that I should have yield each bulk get. I'll check. I know this is not the case anymore with PySNMP 4.3 which turns a GETBULK request into a complete walk, but if you are using Debian packages, PySNMP is not at this version yet.

For (2), I believe that this could be done with contexts instead through the use of with. This way, we wouldn't need to rely on the fact that iteritems() is not the standard Python function.

For (1), my solution would have been to use caching. I have updated the documentation in respect of that.

vincentbernat commented 8 years ago

For (3), you are right. While I yield values in the higher levels, in the lower layers, I am using the simple command generator in PySNMP and it doesn't seem to provide an iterator on the results with versions < 4.3.0. I'll either upgrade to 4.3 at some point or rollback to using NetSNMP (for independent reasons). I'll fix that at this moment.

ghost commented 8 years ago

Thank you. Not only for this small change, but for the (idea of) Snimpy in general: I haven't found any "ORM" SNMP analogy for any scripting langugage yet, your library is my favorite as being nearest. Not as a toy to retrieve few statistical SNMP variables, but for a larger project to autom8 configuration of a carrier L2/L3 device: my friend ended up with entering tens of OIDs in a messy PHP script to achieve the same... Btw, do you have any idea/plan to GET multiple variables in a single request (SET is already possible in with: context)?

vincentbernat commented 8 years ago

The difficulty for that is to find a pythonic way to do that. Until now, I didn't find anything. This is one of the main feature of Snimpy that I want to keep "true".

vincentbernat commented 8 years ago

So, about iteritems(), there is still no "abstract" way to do that in Python (I mean, no __iteritems__() in the different abstract classes). So iteritems() is here to stay. I'll add contextual bulk parameters shortly.

ghost commented 8 years ago

Sorry for touching this again: I found an unexpected behavior just now. When trying to traverse an empty column, at least with SNMPv1 (as tested), the Session._op() raises SNMPEndOfMibView(), which is not handled in the iterator: instead of returning no items (throwin StopIteration), it leaks the SNMPEndOfMibView. Tested with 0.8.3 (the mentioned Debian setup) and skimming through master version here didn't reveal any patch for this.

vincentbernat commented 8 years ago

Could you show the exception? I think I see where the problem is but can't look at it right now. The exception would help.

ghost commented 8 years ago

Sure. Please note it is 0.8.3 so are the line numbers. The ups_snmp in my script is a Manager instance, SNMPv1, with standard UPS-MIB loaded.

Traceback (most recent call last):
  File "/usr/local/bin/eatonbtest", line 193, in <module>
    for idx, upsAlarmDescr in ups_snmp.upsAlarmDescr.iteritems():
  File "/usr/lib/python2.7/dist-packages/snimpy/manager.py", line 379, in iteritems
    for noid, result in self.session.walk(oid):
  File "/usr/lib/python2.7/dist-packages/snimpy/snmp.py", line 268, in walk
    return self._op(self._cmdgen.nextCmd, *oids)
  File "/usr/lib/python2.7/dist-packages/snimpy/snmp.py", line 246, in _op
    raise SNMPEndOfMibView("no more stuff after this OID")  # nopep8
SNMPEndOfMibView: no more stuff after this OID

tcpdump: 1x get-next-request (OID=upsAlarmDescr), 1x get-response (OID>upsAlarmDescr)

vincentbernat commented 8 years ago

OK, that should be easy to fix.

vincentbernat commented 8 years ago

I have pushed a8551d731e5e for that.

cbueche commented 8 years ago

Hi Vincent,

I'm right now confronted with the efficiency issue (thousands of devices, and many OID's per device), that's probably what I should call "success" :-)

Question: any idea when you will have contextual bulk parameters implemented ?

Thx, Charles

vincentbernat commented 8 years ago

I am currently swamped, so I can't give you any timeline on this.

cbueche commented 8 years ago

ok no problem, I can wait, thanks for your work Vincent !

cbueche commented 8 years ago

Hi,

As I understand ghost's request and Vincent's answer on 1.12.2015, there is currently no efficient / getbulk way of getting multiple "columns" for a table using a single iteritem() loop ?

My goal would be to have one loop over ifTable (1.3.6.1.2.1.2.2) and get a few values for each index, e.g. the following code produces a bulk-get (quick) and then loops over the ifMtu column, very "slowly".

for index, desc in m.ifDescr.iteritems():
    interface = {}
    interface['index'] = index
    interface['ifDescr'] = desc
    interface['ifMtu'] = m.ifMtu[index]

As I understand, the only way would be to to multiple distinct loops, one for each column I need, and then merge the results together. Or any other suggestion ?

Using pysnmp==4.3.2 and snimpy==0.8.10

Thx ! Charles

vincentbernat commented 8 years ago

@cbueche We could create a proxy object where we register several OID:

for index, desc, mtu in m.ifDescr.and.ifMtu.iteritems():
    ...

This is a bit more Ruby than Python.

cbueche commented 8 years ago

with the 7-8 columns I need, the line length might be indecent :-) Anyway, thx, I will implement multiple iteritems() and merge the results together.

vincentbernat commented 8 years ago

Well, you could do that in several times:

proxy = m.ifDescr
proxy &= m.ifMtu
proxy &= ...

More pythonic to just overload &.