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

This is a fix for the EIGRP redistribution problem #8364

Merged
merged 32 commits into from
Jul 4, 2022
Merged

This is a fix for the EIGRP redistribution problem #8364

merged 32 commits into from
Jul 4, 2022

Conversation

Katsuya414
Copy link
Contributor

#8273

Here is a solution to the above problem

@batfish-bot
Copy link

This change is Reviewable

Copy link
Member

@dhalperi dhalperi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @dhalperi and @Katsuya414)


projects/batfish/src/main/java/org/batfish/representation/cisco_nxos/CiscoNxosConfiguration.java line 1111 at r1 (raw file):

                          .collect(ImmutableList.toImmutableList());
                  Conjunction redistExpr = new Conjunction(matchConjuncts);
                  return new If(redistExpr, ImmutableList.of(Statements.ExitAccept.toStaticStatement()));

Where is the process ID matched?

That is, your bug has redistribute eigrp 1 route-map foo -- where is the 1 here?

@Katsuya414
Copy link
Contributor Author

@dhalperi
What value do you mean by process ID?

Format Correction
@codecov
Copy link

codecov bot commented Jun 14, 2022

Codecov Report

Merging #8364 (96c1dbf) into master (5123d6c) will decrease coverage by 0.01%.
The diff coverage is 82.50%.

@@             Coverage Diff              @@
##             master    #8364      +/-   ##
============================================
- Coverage     74.81%   74.80%   -0.02%     
+ Complexity    44202    44198       -4     
============================================
  Files          3428     3428              
  Lines        170497   170527      +30     
  Branches      20339    20343       +4     
============================================
- Hits         127560   127557       -3     
- Misses        33309    33330      +21     
- Partials       9628     9640      +12     
Impacted Files Coverage Δ
...resentation/cisco_nxos/CiscoNxosConfiguration.java 87.90% <82.50%> (-0.45%) ⬇️
...src/main/java/org/batfish/coordinator/PoolMgr.java 54.76% <0.00%> (-5.96%) ⬇️
...fish/bddreachability/BDDLoopDetectionAnalysis.java 83.82% <0.00%> (-2.95%) ⬇️
...bddreachability/BDDReachabilityGraphOptimizer.java 82.82% <0.00%> (-1.02%) ⬇️
...ain/java/org/batfish/coordinator/WorkQueueMgr.java 70.93% <0.00%> (-0.59%) ⬇️
...src/main/java/org/batfish/coordinator/WorkMgr.java 76.02% <0.00%> (-0.55%) ⬇️
...tfish/representation/cisco/CiscoConfiguration.java 84.79% <0.00%> (-0.15%) ⬇️
.../org/batfish/dataplane/ibdp/BgpRoutingProcess.java 83.48% <0.00%> (-0.09%) ⬇️

Copy link
Member

@dhalperi dhalperi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @dhalperi and @Katsuya414)


projects/batfish/src/main/java/org/batfish/representation/cisco_nxos/CiscoNxosConfiguration.java line 1111 at r1 (raw file):

Previously, dhalperi (Dan Halperin) wrote…

Where is the process ID matched?

That is, your bug has redistribute eigrp 1 route-map foo -- where is the 1 here?

You have multiple EIGRP processes, here with numbers 1, 2, and 3:

router eigrp 1
  redistribute eigrp 2
  ...
router eigrp 2
  redistribute eigrp 1
  ...
router eigrp 3
  ...

Note that the redistribution command is not "any EIGRP route" it's "EIGRP routes from process X" -- e.g., redistribute eigrp 1 or redistribute eigrp 2 should only redistribute routes from that EIGRP process, not routes from all EIGRP processes.

How does this code implement that part of the requirements?

- Fixed redistribution by target EIGRP AS number.
- Fixed metric values for "other protocol -> EIGRP" redistribution.
- Corrected code format (part pointed out in pull request)
@Katsuya414
Copy link
Contributor Author

@dhalperi
Regarding issues with the process
Updated.
May I ask you to check?

@Katsuya414
Copy link
Contributor Author

@dhalperi
You said.
I solved the problem about the process ID.
May I ask you to check?

Copy link
Member

@dhalperi dhalperi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 0 of 1 files reviewed, 2 unresolved discussions (waiting on @dhalperi and @Katsuya414)


projects/batfish/src/main/java/org/batfish/representation/cisco_nxos/CiscoNxosConfiguration.java line 1105 at r3 (raw file):

                case EIGRP:
                  assert policy.getInstance().getTag() != null;
                  long eigrpAsn = Long.parseLong(policy.getInstance().getTag());

I do not believe this is the right way to get the ASN from the process. The instance tag on NX-OS can be an arbitrary trying, in which case the user must set it manually. Even if the tag is a long, the user can override that long. See docs: https://www.cisco.com/c/en/us/td/docs/switches/datacenter/nexus9000/sw/6-x/unicast/configuration/guide/l3_cli_nxos/l3_eigrp.html


projects/batfish/src/main/java/org/batfish/representation/cisco_nxos/CiscoNxosConfiguration.java line 1106 at r3 (raw file):

                  assert policy.getInstance().getTag() != null;
                  long eigrpAsn = Long.parseLong(policy.getInstance().getTag());
                  MatchProtocol matchEigrp =

Unfortunately you did not provide us with a network to test, so we are blocked until we are able to find time to build our own network and test.
An example of a PR that did this: https://github.com/batfish/batfish/pull/7943/files
Notice how Ari created the arista_ospf_network file and added a unit test of the new functionality in AristaGrammarTest.java

Copy link
Member

@dhalperi dhalperi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 of 1 files at r3, 1 of 2 files at r4, 1 of 1 files at r6, 3 of 3 files at r7, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @Katsuya414)

commit-id:cf6ab583
commit-id:a2775386
commit-id:4ac4ccba
Metrics need to only be set in the protocol itself.

Metric in test needs to be copied from builder.

commit-id:ff198c56
Copy link
Member

@dhalperi dhalperi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 2 of 2 files at r8, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @Katsuya414)

@dhalperi dhalperi merged commit 61e1141 into batfish:master Jul 4, 2022
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

3 participants