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.

Sign up for GitHub

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

Jump to bottom

Set origin incomplete on routes from MLAG iBGP peer #600

Merged
merged 9 commits into from
Jan 13, 2021
Merged

Set origin incomplete on routes from MLAG iBGP peer #600

merged 9 commits into from
Jan 13, 2021

Conversation

emilotep
Copy link
Contributor

@emilotep emilotep commented Jan 11, 2021 •
edited by ClausHolbechArista
Loading

Change Summary

Add variable knob and functionality for setting origin to incomplete on routes received from MLAG iBGP peer on l3leafs. The aim is to avoid suboptimal forwarding over the peerlink for some traffic.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes)
  • Documentation content changes
  • Other (please describe):

Related Issue(s)

Fixes #572

Component(s) name

eos_l3ls_evpn/README.md
eos_l3ls_evpn/templates/evpn-fabric-l3leaf-yml.j2
eos_l3ls_evpn/templates/fabric/bgp_base/leaf-router-bgp-peer-groups.j2
eos_l3ls_evpn/templates/fabric/bgp_base/route-maps.j2
eos_cli_config_gen/templates/eos/router-bgp.j2

eos_cli_config_gen
eos_l3ls_evpn

Proposed changes

Add a new knob "mlag_ibgp_origin_incomplete: true/false, default -> true" under fabric variables, either as default for all l3leafs or individually per l3leaf node group to override the default.

Additions to eos_l3ls_evpn and eos_cli_config_gen templates have been made to define a route-map RM-MLAG-PEER that sets the origin and applies it inbound to the MLAG iBGP peer group: MLAG-IPv4-UNDERLAY-PEER. The route-map is not created if the knob is set to false.

The new variable is documented in the README for eos_l3ls_evpn role under the section that deals with fabric variables.

Example:

l3leaf:
  defaults:
    ...
    mlag_ibgp_origin_incomplete: true
  node_groups:
    leaf1:
      ...
      mlag_ibgp_origin_incomplete: false

How to test

Have tried rendering EVPN configurations with most combinations of defining the knob under l3leaf defaults as well as overriding the default per node group. Templates render as expected regardless of whether the knob is undefined (implicitly true) or defined (explicitly true or false) at different levels.

Checklist:

  • My code has been rebased from devel before I start
  • I have read the CONTRIBUTING document.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have updated molecule CI testing accordingly
  • All new and existing tests passed (pre-commit, make linting and make sanity-lint).

@github-actions github-actions bot added role: eos_cli_config_gen issue related to eos_cli_config_gen role role: eos_l3ls_evpn issue related to eos_l3ls_evpn role state: Documentation role Updated labels Jan 11, 2021
ClausHolbechArista
ClausHolbechArista requested changes Jan 11, 2021
View reviewed changes
Copy link
Contributor

@ClausHolbechArista ClausHolbechArista left a comment

Choose a reason for hiding this comment

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

A few changed and also we need to discuss setting this as default. Reach out if you need help on the new test/filter.

ansible_collections/arista/avd/roles/eos_l3ls_evpn/README.md Outdated Show resolved Hide resolved
ansible_collections/arista/avd/roles/eos_l3ls_evpn/templates/fabric/bgp_base/route-maps.j2 Outdated Show resolved Hide resolved
...ions/arista/avd/roles/eos_l3ls_evpn/templates/fabric/bgp_base/leaf-router-bgp-peer-groups.j2 Outdated Show resolved Hide resolved
ansible_collections/arista/avd/roles/eos_l3ls_evpn/templates/fabric/bgp_base/route-maps.j2 Outdated Show resolved Hide resolved
ansible_collections/arista/avd/roles/eos_cli_config_gen/templates/eos/router-bgp.j2 Outdated Show resolved Hide resolved
ansible_collections/arista/avd/roles/eos_l3ls_evpn/templates/evpn-fabric-l3leaf-yml.j2 Outdated Show resolved Hide resolved
@github-actions github-actions bot added the state: CI Updated CI scenario have been updated in the PR label Jan 11, 2021
@github-actions
Copy link

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@github-actions github-actions bot added the state: conflict PR with conflict label Jan 11, 2021
ClausHolbechArista
ClausHolbechArista requested changes Jan 11, 2021
View reviewed changes
Makefile Outdated Show resolved Hide resolved
ansible_collections/arista/avd/molecule/eos_cli_config_gen/inventory/host_vars/route-maps.yml Outdated Show resolved Hide resolved
...ollections/arista/avd/molecule/eos_cli_config_gen/inventory/host_vars/router-bgp-v4-evpn.yml Outdated Show resolved Hide resolved
ansible_collections/arista/avd/molecule/eos_cli_config_gen/molecule.yml Outdated Show resolved Hide resolved
ansible_collections/arista/avd/roles/eos_cli_config_gen/templates/eos/router-bgp.j2 Show resolved Hide resolved
ansible_collections/arista/avd/roles/eos_l3ls_evpn/README.md Outdated Show resolved Hide resolved
ansible_collections/arista/avd/roles/eos_l3ls_evpn/templates/fabric/bgp_base/route-maps.j2 Show resolved Hide resolved
ansible_collections/arista/avd/roles/eos_l3ls_evpn/templates/fabric/bgp_base/route-maps.j2 Outdated Show resolved Hide resolved
@ClausHolbechArista
Copy link
Contributor

@emilotep So after the latest changes you need to rerun molecule if you can. And then you have a merge conflict for eos_l3ls_evpn/README.md, so I suggest to do a rebase.

@emilotep
Set origin incomplete on routes from MLAG iBGP peer
5cc4177 
Add variable knob and functionality for setting origin to incomplete
on routes received from MLAG iBGP peer on l3leafs. The aim is to avoid
suboptimal forwarding over peerlink for some traffic.

The new knob is "mlag_ibgp_origin_incomplete: true/false" and is
defined under fabric variables, either as default for all l3leafs or
individually per l3leaf node group. The setting defaults to true if
omitted.

Additions to eos_l3ls_evpn and eos_cli_config_gen have been made to
define a route-map RM-MLAG-PEER that sets the origin and applies it to
the MLAG iBGP peer group: MLAG-IPv4-UNDERLAY-PEER.

The knob is documented in the README for eos_l3ls_evpn role under the
section that deals with fabric variables.
@emilotep
Update CI artifacts
f9b78e5
@emilotep
Changes requested in PR aristanetworks#600
a16bbd3 
- Add newline at end of route-maps.j2
- Renaming new route-map from RM-MLAG-PEER to RM-MLAG-PEER-IN
- Refactoring to use new arista.avd.defined test in /eos_cli_config_gen/templates/eos/router-bgp.j2
@emilotep
Changes in evpn-fabric-l3leaf-yml.j2 template
09e01a3 
Refactored to using arista.avd.default filter for added variable.
@emilotep
Add description to RM-MLAG-PEER-IN
6776937 
Add descriptive text to the new route-map to clarify the
purpose of the added configuration.
@emilotep
Improve documentation, correcting molecule/Makefiles
9ac9e26 
- Undid unintentional changes to Makefile and molecule.yml
- Update molecule files with correct route-map name
- Added condition in route-maps.yml to check for underlay proto = ebgp
- Improved documentation of new variable knob
@emilotep
Update CI artifacts
2091f69
@emilotep
Update CI artifacts
caefb55
@emilotep
Add support for route_map_out in eos_cli_config_gen
29761e3 
- Support for route_map_out added in router-bgp.j2 template
- Added eos_cli_config_gen role documentation for route_map_in and _out
- Added route_map_out to molecule
- Reran molecule and linting
@github-actions
Copy link

Conflicts have been resolved. A maintainer will review the pull request shortly.

@github-actions github-actions bot removed the state: conflict PR with conflict label Jan 11, 2021
titom73
titom73 approved these changes Jan 12, 2021
View reviewed changes
Copy link
Contributor

@titom73 titom73 left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@titom73 titom73 merged commit c1d8778 into aristanetworks:devel Jan 13, 2021
@titom73 titom73 added this to the v2.0.0rc1 milestone Jan 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ClausHolbechArista ClausHolbechArista ClausHolbechArista approved these changes

@titom73 titom73 titom73 approved these changes

@carlbuchmann carlbuchmann Awaiting requested review from carlbuchmann

@aphillipps aphillipps Awaiting requested review from aphillipps

Assignees

@emilotep emilotep

Labels
role: eos_cli_config_gen issue related to eos_cli_config_gen role role: eos_l3ls_evpn issue related to eos_l3ls_evpn role state: CI Updated CI scenario have been updated in the PR state: Documentation role Updated
Projects
None yet
Milestone
v2.0.0rc1
Development

Successfully merging this pull request may close these issues.

Set origin incomplete over MLAG iBGP in l3ls_evpn
3 participants
@emilotep @ClausHolbechArista @titom73