Add module for loading model and OID configuration.

[stephen-soltesz]

This PR includes a configuration file for known switch models and a module for loading and interpreting the configuration.

---

This change is

[mtlynch]

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

---

site-packages/mlab/disco/models.py, line 33 [r1] (raw file):
The filename vs. config semantics are a bit confusing. I'm assuming this is for unit testing? I haven't thought enough about it to suggest alternatives, but if we could eliminate the "one parameter makes the other parameter meaningless" semantics, that'd be good.

---

site-packages/mlab/disco/models.py, line 124 [r1] (raw file):
It seems like if we inherit from yaml.YAMLObject we can do elegant parsing, if I'm understanding correctly

http://pyyaml.org/wiki/PyYAMLDocumentation#LoadingYAML

---

Comments from Reviewable

[coveralls]

Coverage remained the same at 98.643% when pulling 159c9a8 on stephen-soltesz:disco-models into 2d3420c on m-lab:master.

[coveralls]

Coverage remained the same at 98.643% when pulling ecee03b on stephen-soltesz:disco-models into 2d3420c on m-lab:master.

[stephen-soltesz]

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

---

site-packages/mlab/disco/models.py, line 33 [r1] (raw file):
Okay. I was using the pattern from #32 but the filename is user-provided rather than static. I think it's fine to push the file opening to the caller.

---

site-packages/mlab/disco/models.py, line 124 [r1] (raw file):
Can you say more? I'm not understanding yet. If I'm understanding correctly, the logic wouldn't change much. Perhaps load_switch_config could be a class method instead?

It looks like yaml.safe_load is preferable in load_switch_config.

---

Comments from Reviewable

[coveralls]

Coverage remained the same at 98.643% when pulling db920d9 on stephen-soltesz:disco-models into 2d3420c on m-lab:master.

[mtlynch]

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

---

site-packages/mlab/disco/models.py, line 124 [r1] (raw file):
Yeah, I don't have experience with PyYAML, but from a cursory look at the documentation, it seems like we can push the heavy lifting into yaml.safe_load instead of parsing into a dictionary and manually matching dicionary values to attributes in a SwitchConfig instance. It seems like if we inherit from yaml.YAMLObjectand arrange the class attributes to match the YAML format, PyYAML will just populate the fields automatically for us, but I may be misunderstanding.

---

Comments from Reviewable

[coveralls]

Coverage decreased (-4.7%) to 93.919% when pulling df2e87a on stephen-soltesz:disco-models into 2d3420c on m-lab:master.

[coveralls]

Coverage decreased (-4.7%) to 93.919% when pulling dcf8172 on stephen-soltesz:disco-models into 2d3420c on m-lab:master.

[coveralls]

Coverage increased (+0.03%) to 98.671% when pulling bc9feb4 on stephen-soltesz:disco-models into 2d3420c on m-lab:master.

[coveralls]

Coverage increased (+0.03%) to 98.671% when pulling e91110b on stephen-soltesz:disco-models into 2d3420c on m-lab:master.

[coveralls]

Coverage increased (+0.03%) to 98.671% when pulling a813002 on stephen-soltesz:disco-models into 2d3420c on m-lab:master.

[stephen-soltesz]

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

---

site-packages/mlab/disco/models.py, line 124 [r1] (raw file):
Aah. Okay. That is possible. The file is a little longer than it was, but I hope that all parts are simpler now. It looks like the only complex logic is in Config.__init__ for initializing the sets of model oids based on the default_oids.

There are a few caveats to yaml.YAMLObject documented in the comments for BaseYAMLObject. Though, I'm especially happy that we can report the line number in the exception for the offending yaml node.

---

Comments from Reviewable

[coveralls]

Coverage increased (+0.03%) to 98.672% when pulling 79d7552 on stephen-soltesz:disco-models into 2d3420c on m-lab:master.

[coveralls]

Coverage increased (+0.03%) to 98.672% when pulling 5f56c04 on stephen-soltesz:disco-models into 2d3420c on m-lab:master.

[mtlynch]

Reviewed 3 of 4 files at r4, 1 of 1 files at r5.
Review status: all files reviewed at latest revision, 13 unresolved discussions.

---

disco/models.yaml, line 1 [r5] (raw file):
(not a deficiency in the code, just my YAML ignorance) Is my understanding correct that the !config etc is to define it as a config type in YAML so that PyYAML can translate it to a Config object?

---

disco/models.yaml, line 13 [r5] (raw file):
It doesn't look like we're documenting vlan or oids (and children of oids)

---

disco/models.yaml, line 16 [r5] (raw file):
Pattern seems to be property names in single quotes

---

disco/models.yaml, line 37 [r5] (raw file):
(more YAML ignorance) Is this just a string that happens to have a curly brace in it or does {...} have special meaning in YAML? I've seen double curly braces for variable substitution in Ansible, but I don't know if that's an Ansible thing or a YAML thing.

Edit: After reading the rest of the code, I think the answer is that it's just a string. I'm not sure how obvious this would be to someone who's comfortable enough with YAML to recognize it as not YAML syntax. Maybe we could document that rx and tx are format strings meant to be formatted in code?

I'm still a little confused about why the format string variable is called ifIndex but gets populated by a parameter named port.

---

site-packages/mlab/disco/models.py, line 18 [r5] (raw file):
We seem to use OID in other comments.

---

site-packages/mlab/disco/models.py, line 28 [r5] (raw file):
Looks like this is missing a word or two.

---

site-packages/mlab/disco/models.py, line 92 [r5] (raw file):
Are we deliberately omitting documentation of kwargs?

---

site-packages/mlab/disco/models.py, line 93 [r5] (raw file):
Can we do if not models or not default_oids? It seems like any falsy values for these variables would be invalid. Is it legal to have a config with an empty list of models?

---

site-packages/mlab/disco/models.py, line 124 [r5] (raw file):
If multiple patterns match, it will return the first one. Might be worth documenting or throwing an exception for.

---

site-packages/mlab/disco/models.py, line 167 [r5] (raw file):
ditto about falsy values

---

site-packages/mlab/disco/models_test.py, line 11 [r5] (raw file):
Very elegant unit test!

---

site-packages/mlab/disco/models_test.py, line 28 [r5] (raw file):
I don't think we're covering the overwrite case here, and we probably want to. Suggest having a model with OIDs A and B and default_oids with OIDs B and C so we can see add (A), overwrite (B), and inherit unchanged (C).

---

site-packages/mlab/disco/models_test.py, line 90 [r5] (raw file):
Seems like weird line breaks. Is YAPF forcing this? I don't think it's worth a # yapf: disable, but if we can remove the early break and have YAPF accept it, suggest that.

---

Comments from Reviewable

[coveralls]

Coverage increased (+0.04%) to 98.682% when pulling abd9c11 on stephen-soltesz:disco-models into 2d3420c on m-lab:master.

[coveralls]

Coverage increased (+0.04%) to 98.682% when pulling abd9c11 on stephen-soltesz:disco-models into 2d3420c on m-lab:master.

[stephen-soltesz]

Review status: 1 of 4 files reviewed at latest revision, 13 unresolved discussions.

---

disco/models.yaml, line 1 [r5] (raw file):
That's right. The convention seems to be that yaml supports "tags" http://www.yaml.org/refcard.html and PyYAML uses tags to identify the object type.

The models.Config class includes a class variable yaml_tag to know when to use the Config class.

---

disco/models.yaml, line 13 [r5] (raw file):
I've added more notes here and deferred the documentation for oids and oid to the default_oids section below.

How does it look?

---

disco/models.yaml, line 16 [r5] (raw file):
Yes. Fixed throughout.

---

disco/models.yaml, line 37 [r5] (raw file):
I've added more clarifying text that the OIDs are "format strings". That is helpful to clarify.

"ifIndex" is an SNMP thing that basically means port... most of the time. I changed the parameter to lookup_oids to use ifindex instead of port for consistency.

I was trying to be helpful to humans who may understand what a switch port is but not be sure of what an ifindex is. But, this may be another example where some domain familiarity is necessary and using ifIndex throughout is actually more helpful by hinting that "you need to know what an ifindex is."

---

site-packages/mlab/disco/models.py, line 18 [r5] (raw file):
Fixed. I've also updated the use of Oid in class and function names to OID for consistency.

---

site-packages/mlab/disco/models.py, line 28 [r5] (raw file):
lol. NotInitialized is not used. Removed.

---

site-packages/mlab/disco/models.py, line 92 [r5] (raw file):
Not deliberately. Fixed. Evidently none of our pre-checks flag this.

---

site-packages/mlab/disco/models.py, line 93 [r5] (raw file):
Okay. I tend to favor being specific. But, a config with an empty list of models would not be so helpful, you're right. I still want to preserve and empty default_oids for testing, since a model could declare all of it's oids.

---

site-packages/mlab/disco/models.py, line 124 [r5] (raw file):
That's a good idea. Multiple matches is a problem with configuration that deserves to be flagged. Added.

---

site-packages/mlab/disco/models.py, line 167 [r5] (raw file):
Done.

---

site-packages/mlab/disco/models_test.py, line 11 [r5] (raw file):
Thank you! I love it when we don't need mocks.

---

site-packages/mlab/disco/models_test.py, line 28 [r5] (raw file):
Oh, great point. Added.

---

site-packages/mlab/disco/models_test.py, line 90 [r5] (raw file):
Yeah, this is yapf. I thought it was weird too. I tried initializing the dict differently to prevent this. Unfortunate to work around the auto-formatter, but better than the weird formatting too I think.

---

Comments from Reviewable

[mtlynch]

---

Reviewed 3 of 3 files at r6.
Review status: all files reviewed at latest revision, 4 unresolved discussions.

---

disco/models.yaml, line 60 [r6] (raw file):
formats -> format

---

disco/models.yaml, line 62 [r6] (raw file):
Suggest a comma after "values"

---

site-packages/mlab/disco/models.py, line 142 [r6] (raw file):
Might be useful to include the matching model patterns in the error message as well.

---

site-packages/mlab/disco/models_test.py, line 90 [r5] (raw file):
Might be worth filing:

https://github.com/google/yapf/issues

---

Comments from Reviewable

[coveralls]

Coverage increased (+0.04%) to 98.683% when pulling e944d68 on stephen-soltesz:disco-models into 2d3420c on m-lab:master.

[stephen-soltesz]

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

---

disco/models.yaml, line 60 [r6] (raw file):
Done.

---

disco/models.yaml, line 62 [r6] (raw file):
Done.

---

site-packages/mlab/disco/models.py, line 142 [r6] (raw file):
I wondered about this. Sounds like we have consensus. Done.

---

site-packages/mlab/disco/models_test.py, line 90 [r5] (raw file):
Done.

google/yapf#245

---

Comments from Reviewable