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

Unique asn #961

Merged
merged 8 commits into from
Feb 22, 2018
Merged

Unique asn #961

merged 8 commits into from
Feb 22, 2018

Conversation

ratulm
Copy link
Member

@ratulm ratulm commented Feb 20, 2018

A question to check if routers are using unique BGP ASNs.


This change is Reviewable

@ratulm ratulm requested a review from dhalperi February 20, 2018 06:28
@dhalperi
Copy link
Member

Reviewed 2 of 3 files at r1.
Review status: 2 of 3 files reviewed at latest revision, all discussions resolved.


projects/question/src/main/java/org/batfish/question/UniqueBgpAsnQuestionPlugin.java, line 24 at r1 (raw file):

@AutoService(Plugin.class)
public class UniqueBgpAsnQuestionPlugin extends QuestionPlugin {

Is it worth making this return all devices associated with an ASN, and having a flag to return only ones used by multiple nodes?


projects/question/src/main/java/org/batfish/question/UniqueBgpAsnQuestionPlugin.java, line 24 at r1 (raw file):

@AutoService(Plugin.class)
public class UniqueBgpAsnQuestionPlugin extends QuestionPlugin {

UniqueBgpAsn does not seem to describe what it does. ReusedBgpAsn? (that's crap, too.)


projects/question/src/main/java/org/batfish/question/UniqueBgpAsnQuestionPlugin.java, line 44 at r1 (raw file):

Bgp

BGP, when printing in a string?


projects/question/src/main/java/org/batfish/question/UniqueBgpAsnQuestionPlugin.java, line 109 at r1 (raw file):

bf_answer("UniqueBgpAsns", nodeRegex='as2.*')

what is this?


Comments from Reviewable

@dhalperi
Copy link
Member

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


projects/question/src/main/java/org/batfish/question/UniqueBgpAsnQuestionPlugin.java, line 71 at r1 (raw file):

      Map<String, Configuration> configurations = _batfish.loadConfigurations();
      Set<String> nodes = question.getNodeRegex().getMatchingNodes(configurations);
      SortedMap<Integer, SortedSet<String>> asns = new TreeMap<>();

Will TreeMultimap simplify this for you? https://google.github.io/guava/releases/22.0/api/docs/com/google/common/collect/TreeMultimap.html

Note it does assume both keys and values are comparable so will give you the property you need.


Comments from Reviewable

@ratulm
Copy link
Member Author

ratulm commented Feb 22, 2018

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


projects/question/src/main/java/org/batfish/question/UniqueBgpAsnQuestionPlugin.java, line 24 at r1 (raw file):

Previously, dhalperi (Dan Halperin) wrote…

UniqueBgpAsn does not seem to describe what it does. ReusedBgpAsn? (that's crap, too.)

moot now, with the name change


projects/question/src/main/java/org/batfish/question/UniqueBgpAsnQuestionPlugin.java, line 24 at r1 (raw file):

Previously, dhalperi (Dan Halperin) wrote…

Is it worth making this return all devices associated with an ASN, and having a flag to return only ones used by multiple nodes?

added a mincount parameter


projects/question/src/main/java/org/batfish/question/UniqueBgpAsnQuestionPlugin.java, line 44 at r1 (raw file):

Previously, dhalperi (Dan Halperin) wrote…

Bgp

BGP, when printing in a string?

Done.


projects/question/src/main/java/org/batfish/question/UniqueBgpAsnQuestionPlugin.java, line 71 at r1 (raw file):

Previously, dhalperi (Dan Halperin) wrote…

Will TreeMultimap simplify this for you? https://google.github.io/guava/releases/22.0/api/docs/com/google/common/collect/TreeMultimap.html

Note it does assume both keys and values are comparable so will give you the property you need.

Good idea. The pom.xml changes are to accommodate this.


projects/question/src/main/java/org/batfish/question/UniqueBgpAsnQuestionPlugin.java, line 109 at r1 (raw file):

Previously, dhalperi (Dan Halperin) wrote…

bf_answer("UniqueBgpAsns", nodeRegex='as2.*')

what is this?

legacy; auto-generating docs: https://github.com/batfish/batfish/wiki/Questions. We don't do that anymore. I left it there for consistency; we can go in and remove from everywhere.


Comments from Reviewable

@dhalperi
Copy link
Member

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


projects/pom.xml, line 532 at r2 (raw file):

jackson-datatype-guava

Per the documentation, this library is deprecated. Use jackson-datatype-collections instead and then you can just use jackson.version.


projects/question/src/main/java/org/batfish/question/BgpAsnUseQuestionPlugin.java, line 102 at r2 (raw file):

   *
   * @type BgpAsnUse multifile
   * @param minCount Only report ASNs that are used at more than this number of nodes

suspect that > will be confusing for something called minCount -- it should be >=.

Either change name or semantics.. having a hard time thinking of a better name, so change semantics and switch number in the test?


Comments from Reviewable

@ratulm
Copy link
Member Author

ratulm commented Feb 22, 2018

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


projects/pom.xml, line 532 at r2 (raw file):

Previously, dhalperi (Dan Halperin) wrote…

jackson-datatype-guava

Per the documentation, this library is deprecated. Use jackson-datatype-collections instead and then you can just use jackson.version.

Done.


projects/question/src/main/java/org/batfish/question/BgpAsnUseQuestionPlugin.java, line 102 at r2 (raw file):

Previously, dhalperi (Dan Halperin) wrote…

suspect that > will be confusing for something called minCount -- it should be >=.

Either change name or semantics.. having a hard time thinking of a better name, so change semantics and switch number in the test?

Changed semantics.


Comments from Reviewable

@dhalperi
Copy link
Member

:lgtm:


Reviewed 2 of 8 files at r2, 4 of 4 files at r3.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@ratulm ratulm merged commit 3bed9af into master Feb 22, 2018
@ratulm ratulm deleted the unique-asn branch February 22, 2018 17:49
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

Successfully merging this pull request may close these issues.

None yet

2 participants