Add "simple session" module and tests.

[stephen-soltesz]

This change adds a simple session class that simplifies the interface to netsnmp.Session. As well, this change adds a fake session class for use in tests in other modules that take a session object.

---

This change is

[coveralls]

Coverage remained the same at 98.956% when pulling c0626d3 on stephen-soltesz:disco-session into c47ba2b on m-lab:master.

[mtlynch]

Review status: 0 of 2 files reviewed at latest revision, 9 unresolved discussions.

---

site-packages/mlab/disco/simple_session.py, line 1 [r1] (raw file):
This feels a bit too brief and doesn't give much information beyond what the reader would guess based on the filename. "A simple interface for SNMP sessions"?

---

site-packages/mlab/disco/simple_session.py, line 22 [r1] (raw file):
It seems strange to give the caller no way to get any information about the error. Does it make sense for us to detect when an error occurs and then raise an exception based on the ErrorStr value?

---

site-packages/mlab/disco/simple_session.py, line 25 [r1] (raw file):
I feel like passing around dicts instead of defining classes is generally more confusing. Can we define an SnmpVariable class (or whatever the appropriate name for this structure would be)?

If not, extraneous "a" after "containing"

---

site-packages/mlab/disco/simple_session.py, line 57 [r1] (raw file):
_mibs is an unexpected name because we're not using it anywhere else. _varbinds?

---

site-packages/mlab/disco/simple_session.py, line 59 [r1] (raw file):
I think we should be consistent and use either (key, val) or (tag, val) throughout. Probably tag, val if that's what netsnmp uses.

---

site-packages/mlab/disco/simple_session.py, line 64 [r1] (raw file):
It seems like we're treating it as a normal dict instead of a defaultdict. Won't self._mibs[mib] do what we want?

---

site-packages/mlab/disco/simple_session_test.py, line 14 [r1] (raw file):
Why not use a real one?

---

site-packages/mlab/disco/simple_session_test.py, line 39 [r1] (raw file):
Can we add a test for the error case?

---

site-packages/mlab/disco/simple_session_test.py, line 52 [r1] (raw file):
I think we could use more exhaustive tests for walk like if it results in a list > 1 and if it results in a list with errors.

---

Comments from Reviewable

[coveralls]

Coverage remained the same at 98.956% when pulling 206dc7f on stephen-soltesz:disco-session into c47ba2b on m-lab:master.

[coveralls]

Coverage remained the same at 98.956% when pulling 81dd0a1 on stephen-soltesz:disco-session into c47ba2b on m-lab:master.

[stephen-soltesz]

Review status: 0 of 2 files reviewed at latest revision, 9 unresolved discussions.

---

site-packages/mlab/disco/simple_session.py, line 1 [r1] (raw file):
Updated.

---

site-packages/mlab/disco/simple_session.py, line 22 [r1] (raw file):
This is a good idea. The error handling / reporting of netsnmp is fairly poor. The following factors prompted me to try to write what I considered a more straight forward and consistent interface.

The two errors we care most about are:

• connection error, e.g. Timeout
• unknown or missing OID

netsnmp does appear to set ErrorStr for Timeout. So raising an exception for this case makes sense to me. I've added SNMPError for this case.

But, netsnmp reports nothing in ErrorStr for unknown or missing OIDs. And, the behavior for get and walk are different in the "unknown or missing OID" case.

While a VarList is passed to both get and walk, the contents of the list changes after a call to walk. On success, elements are replaced or added. On error, the elements are removed entirely (empty list). For get, the content remains the same, except on error the Varbind object still has an uninitialized val attribute.

My thinking was to make the error case for get and walk look the same from the caller's perspective, i.e. an empty list.

Since netsnmp doesn't set ErrorStr for this case, I'm didn't think raising an exception would be as helpful to the caller.

What do you think? What would be more helpful?

---

site-packages/mlab/disco/simple_session.py, line 25 [r1] (raw file):
Hmm, I imagined that a dict was more generic and simpler than a custom type. If we want to use actual objects, we could use netsnmp.Varbind objects, but I also wanted to isolate users from netsnmp.

Can you say more about the value of a custom class over dict?

---

site-packages/mlab/disco/simple_session.py, line 57 [r1] (raw file):
You're right. In my code and communication I've been using "mib" interchangeably with "oid", but they are different things.

My best understanding right now is that "mib" is analogous to "database" and "oid" is analogous to "index to a row of database".

I've updated the instance variable to _mib (singular) and parameters to from mib to oid to be more consistent.

---

site-packages/mlab/disco/simple_session.py, line 59 [r1] (raw file):
I think you're right. Changed to 'tag'.

---

site-packages/mlab/disco/simple_session.py, line 64 [r1] (raw file):
Also correct. I was using plain dicts at one point, but neglected to update the usage of .get after changing to defaultdict.

---

site-packages/mlab/disco/simple_session_test.py, line 14 [r1] (raw file):
Good question. I thought there would be a conflict with creating a real object and simultaneously mocking the netsnmp.Varbind object. But, it turns out that there is no conflict, and using a real object is better because I was missing part of the 'tag' value returned by SimpleSession.get and walk.

---

site-packages/mlab/disco/simple_session_test.py, line 39 [r1] (raw file):
Yes. There are two error case tests now: empty list return value, and for SNMPError.

---

site-packages/mlab/disco/simple_session_test.py, line 52 [r1] (raw file):
Yes, I was lazy with the walk test. I've updated test_walk to return multiple variables (which is typical) and added the empty-list and SNMPError cases.

---

Comments from Reviewable

[mtlynch]

Review status: 0 of 2 files reviewed at latest revision, 5 unresolved discussions.

---

site-packages/mlab/disco/simple_session.py, line 22 [r1] (raw file):
(optional) Empty list feels like too subtle a way to signal an error, especially if a connection error raises an exception now. Could we raise an SNMPError for both the get and walk case for unknown or missing OID that's like SNMPError('unknown netsnmp error') or maybe SNMPError('unknown netsnmp error, OID %s may be invalid' % oid)

---

site-packages/mlab/disco/simple_session.py, line 25 [r1] (raw file):
Yeah, I should have tagged this one as (optional) as I don't feel that strongly, about it, but I do prefer objects. The biggest case I feel dictionaries are confusing are in docstrings because it's much easier to say (and easier for the reader to understand) "a list of netsnmp.Varbind objects" than "a list of dictionaries with the following key, value pairs: ..."

I understand that we're violating encapsulation by using netsnmp.Varbind because the promise of this interface is that the user doesn't have to worry about the netsnmp APIs. Another way around this might be to write a wrapper object like simple_session.Varbind.

---

site-packages/mlab/disco/simple_session.py, line 73 [r2] (raw file):
I think we've exceeded complexity for readable list comprehensions in this case:

Each portion must fit on one line: mapping expression, for clause, filter expression.

https://google.github.io/styleguide/pyguide.html?showone=List_Comprehensions#List_Comprehensions

---

site-packages/mlab/disco/simple_session_test.py, line 14 [r2] (raw file):
Can we do netsnmp.Varbind(tag, val=val)? Actually at that point we probably don't even need the helper function.

---

site-packages/mlab/disco/simple_session_test.py, line 28 [r2] (raw file):
This is only used in one test right? Suggest defining it there rather than in setUp so that the reader doesn't have to jump around to understand that test.

---

Comments from Reviewable

[stephen-soltesz]

Review status: 0 of 2 files reviewed at latest revision, 5 unresolved discussions, some commit checks failed.

---

site-packages/mlab/disco/simple_session.py, line 22 [r1] (raw file):
Okay. The code that's not been reviewed yet that calls session.get or session.walk was checking for the empty result and raising an exception anyway. This should be very nearly equivalent. So, Done.

Sometimes there is more context at the call site than within the called function, which is helpful for logging or error reporting. But, in these cases, I think it's okay to raise the exception here.

---

site-packages/mlab/disco/simple_session.py, line 25 [r1] (raw file):
Okay. I understand. I agree that providing an object interface is easier to document. At this point, my feeling is that the netsnmp library is actually low quality. Ultimately, we would like to use a library like http://easysnmp.readthedocs.org/en/latest/, which has the rational interface that it sounds like we're both imagining.

So, maybe in the spirit of creating a minimal interface that looks like the one we actually want, I've added an SNMPVariable object as used by the easysnmp library. And, added TODOs to remove them when easysnmp is available.

How does it look?

---

site-packages/mlab/disco/simple_session.py, line 73 [r2] (raw file):
lol, now that you say so, it clearly has. Done.

---

site-packages/mlab/disco/simple_session_test.py, line 14 [r2] (raw file):
Oh! I missed this possibility. Done.

---

site-packages/mlab/disco/simple_session_test.py, line 28 [r2] (raw file):
Yes, I didn't like separating the setup of these values from the use in the assert check. I didn't like the alternatives much either :-)

This is a case when the mock patches interfere with the test arrange block. If we call netsnmp.Varbind within a test then we're calling the mocked version.

Options I see to move the var_list definition closer to its use:

• move the @mock.patch.object(netsnmp, 'Varbind') decorator from the test class to individual functions, then use the context form of patch within test_walk. i.e.

  def test_walk(self, mock_varlist):
      var_list = [...]
  
      with mock.patch.object(netsnmp, 'Varbind'):
          actual = self.session.walk('...')
  
          self.assertItemsEqual(...)
  

• define a new test class for walk tests so setUp is closer to usage.

• just move the walk tests first so that walk immediately follows setUp.

  • something else I'm not thinking of.

What do you think?

---

Comments from Reviewable

[coveralls]

Coverage remained the same at 98.956% when pulling 94ccf61 on stephen-soltesz:disco-session into 2f2d17b on m-lab:master.

[mtlynch]

Reviewed 2 of 2 files at r3.
Review status: all files reviewed at latest revision, 2 unresolved discussions.

---

site-packages/mlab/disco/simple_session.py, line 25 [r1] (raw file):
I like it!

---

site-packages/mlab/disco/simple_session_test.py, line 28 [r2] (raw file):
Oh I completely overlooked that we're mocking Varbind and Varlist. This seems strange since they're types. They have a little bit of logic, but they're mostly dumb structs. Can we get rid of these mocks and limit our mocking to netsnmp.Session APIs?

I think the current mocking actually tests implementation details rather than interfaces. For example, we're asserting this:

mock_varbind.assert_called_with('sysDescr.0')

because our implementation is like this:

oids = netsnmp.VarList(netsnmp.Varbind(oid))

But my reading of Varbind is that the following implementation is also valid, but would break our tests:

var = netsnmp.Varbind()
var.tag = oid
oids = netsnmp.VarList(var)

If we abstain from mocking these types, we don't run into that issue.

Those types don't define equality operators, so we might need to define custom self.assertVarbindEqual methods.

---

Comments from Reviewable

[coveralls]

Coverage remained the same at 98.956% when pulling 1215a5e on stephen-soltesz:disco-session into 2f2d17b on m-lab:master.

[coveralls]

Coverage remained the same at 98.956% when pulling c9bf56e on stephen-soltesz:disco-session into 2f2d17b on m-lab:master.

[coveralls]

Coverage remained the same at 98.956% when pulling c9bf56e on stephen-soltesz:disco-session into 2f2d17b on m-lab:master.

[coveralls]

Coverage remained the same at 98.956% when pulling 9eb8f3b on stephen-soltesz:disco-session into 2f2d17b on m-lab:master.

[coveralls]

Coverage remained the same at 98.956% when pulling 9eb8f3b on stephen-soltesz:disco-session into 2f2d17b on m-lab:master.

[stephen-soltesz]

Review status: 1 of 2 files reviewed at latest revision, 1 unresolved discussion.

---

site-packages/mlab/disco/simple_session_test.py, line 28 [r2] (raw file):
Ah ha. Where there's a will, there's a way.

Because netsnmp.Session.get and walk modify the given VarList (and we are mocking netsnmp.Session so that it cannot act on the real VarList value), I thought we needed to at least control the return value of netsnmp.VarList.

It turns out we can redefine the get and walk functions on demand to modify the varlist as the real versions might.

How does it look now?

---

Comments from Reviewable

[mtlynch]

---

Review status: 1 of 2 files reviewed at latest revision, 1 unresolved discussion.

---

site-packages/mlab/disco/simple_session_test.py, line 28 [r2] (raw file):
I like it

---

Comments from Reviewable

[mtlynch]

Reviewed 1 of 1 files at r5.
Review status: all files reviewed at latest revision, 1 unresolved discussion, all commit checks successful.

---

Comments from Reviewable

[stephen-soltesz]

Review status: all files reviewed at latest revision, 1 unresolved discussion, all commit checks successful.

---

site-packages/mlab/disco/simple_session_test.py, line 28 [r2] (raw file):
Woohoo!

---

Comments from Reviewable