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

Table traversal #46

Open
ghost opened this issue Nov 30, 2015 · 19 comments
Open

Table traversal #46

ghost opened this issue Nov 30, 2015 · 19 comments

Comments

@ghost
Copy link

ghost commented Nov 30, 2015

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
Copy link
Owner

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
Copy link
Author

ghost commented Dec 1, 2015

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
Copy link
Owner

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
Copy link
Owner

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
Copy link
Author

ghost commented Dec 1, 2015

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
Copy link
Owner

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
Copy link
Owner

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
Copy link
Author

ghost commented Dec 2, 2015

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
Copy link
Owner

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
Copy link
Author

ghost commented Dec 2, 2015

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
Copy link
Owner

OK, that should be easy to fix.

@vincentbernat
Copy link
Owner

I have pushed a8551d7 for that.

@cbueche
Copy link

cbueche commented Feb 11, 2016

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
Copy link
Owner

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

@cbueche
Copy link

cbueche commented Feb 12, 2016

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

@cbueche
Copy link

cbueche commented Apr 14, 2016

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
Copy link
Owner

@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
Copy link

cbueche commented Apr 14, 2016

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
Copy link
Owner

Well, you could do that in several times:

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

More pythonic to just overload &.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants