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

Feat(eos_designs): Add RD and RT override for VRFs #3419

Merged
merged 9 commits into from
Dec 14, 2023

Conversation

bjmeuer
Copy link
Contributor

@bjmeuer bjmeuer commented Dec 12, 2023

Change Summary

Added the possibility to override the RD and RT values for VRFs.

Related Issue(s)

Fixes #3296

Component(s) name

arista.avd.eos_designs

Proposed changes

Added RD and RT override for VRFs in similar way it was already available for l2vlans

tenants:
  - name: TENANT1
    mac_vrf_vni_base: 10000
    vrfs:
      - name: VRF1
        vrf_id: 1
        rd_override: 111:222
        rt_override: 333:444
        ...
      - name: VRF2
        vrf_id: 2
        rd_override: 999
        rt_override: 999
        ...

How to test

tenants:
  - name: TENANT1
    mac_vrf_vni_base: 10000
    vrfs:
      - name: VRF1
        vrf_id: 1
        rd_override: 111:222
        rt_override: 333:444
        ...
      - name: VRF2
        vrf_id: 2
        rd_override: 999
        rt_override: 999
        ...

Checklist

User Checklist

  • N/A

Repository 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 and documentation have been updated accordingly.
  • I have updated molecule CI testing accordingly. (check the box if not applicable)

@bjmeuer bjmeuer requested review from a team as code owners December 12, 2023 11:10
@github-actions github-actions bot added state: CI Updated CI scenario have been updated in the PR state: Documentation role Updated role: eos_designs issue related to eos_designs role labels Dec 12, 2023
@ClausHolbechArista ClausHolbechArista requested a review from a team December 13, 2023 12:50
@ClausHolbechArista ClausHolbechArista added the one approval This PR has one approval and is only missing one more. label Dec 13, 2023
Copy link
Contributor

@philippebureau philippebureau left a comment

Choose a reason for hiding this comment

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

Tested, LGTM
I would commit Carl`s suggestions on the documentation

Copy link
Member

@carlbuchmann carlbuchmann left a comment

Choose a reason for hiding this comment

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

LGTM, small suggestion on the documentation

carlbuchmann and others added 5 commits December 14, 2023 13:02
…a_fragments/defs_network_services.schema.yml

Co-authored-by: Carl Buchmann <carl.buchmann@arista.com>
…a_fragments/defs_network_services.schema.yml

Co-authored-by: Carl Buchmann <carl.buchmann@arista.com>
…a_fragments/defs_network_services.schema.yml

Co-authored-by: Carl Buchmann <carl.buchmann@arista.com>
…a_fragments/defs_network_services.schema.yml

Co-authored-by: Carl Buchmann <carl.buchmann@arista.com>
@bjmeuer
Copy link
Contributor Author

bjmeuer commented Dec 14, 2023

LGTM, small suggestion on the documentation

Thanks for the review, I commited your suggestions

Copy link
Member

@carlbuchmann carlbuchmann left a comment

Choose a reason for hiding this comment

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

LGTM

@carlbuchmann carlbuchmann merged commit 4e87353 into aristanetworks:devel Dec 14, 2023
38 checks passed
@forcecity
Copy link

I have 2 points to double check with you:

  1. You have added rt_override, but for the logic of it, I’m just a little bit confused from your source code (router_bgp.py):
    `def get_vrf_rt(self, vrf: dict) -> str:
    """
    Return a string with the route-target for one VRF
    """
    rt_override = default(vrf.get("rt_override"))

     if ":" in str(rt_override):
         return rt_override
    
     if rt_override is not None:
         return f"{rt_override}:{rt_override}"
     elif self._vrf_rt_admin_subfield is not None:
         admin_subfield = self._vrf_rt_admin_subfield
     elif self.shared_utils.overlay_rt_type["vrf_admin_subfield"] == "vrf_vni":
         admin_subfield = self.get_vrf_vni(vrf)
     else:
         # Both for 'id' and 'vrf_id' options.
         admin_subfield = self.get_vrf_id(vrf)
     return f"{admin_subfield}:{self.get_vrf_id(vrf)}"
    

`
On this line: return f"{rt_override}:{rt_override}", would it be return f”{ admin_subfield }:{ rt_override }”? Or you could define a new var like “assigned_number_subfield” and assign the rt_override value to it.

If I understand correctly from the eos_designs.schema, A single number will be used in the RT assigned number subfield (second part of the RT). But if you specify like the yellow line, the result will be different. By default, the “admin_subfield” will be vrf_id, and if I set different vrf_id and rt_override. In our deployment, it should be OK, but other customers may confuse about this behavior. What do you think?
2. I think there is a typo in schema (arista/avd/roles/eos_designs/schemas/schema_fragments/defs_network_services.schema.yml), line 308, it should be "the VRF RT will be derived"

@ClausHolbechArista
Copy link
Contributor

@forcecity very good catch! We should also be respecting the admin_subfield setting if you give rt_override (without colon). I will fix this in a new PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
one approval This PR has one approval and is only missing one more. rn: Feat(eos_designs) role: eos_designs issue related to eos_designs role state: CI Updated CI scenario have been updated in the PR state: Documentation role Updated
Projects
None yet
Development

Successfully merging this pull request may close these issues.

It is better to have a possibility to customize the RD/RT of VRF
5 participants