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

Feat(eos_designs): Support for Ipv6 network services #1760

Merged
merged 8 commits into from
May 11, 2022
Merged

Feat(eos_designs): Support for Ipv6 network services #1760

merged 8 commits into from
May 11, 2022

Conversation

onurgashi
Copy link
Contributor

Change Summary

This PR will add IPv6 support to tenants structure.

 < tenant_a >
    vrfs:
      < tenant_a_vrf_1 >:
        svis:
          < 1-4096 >:
            ipv6_address_virtual: < IPv6_address/Mask >
            ipv6_address_virtual_secondaries:
              - < IPv6_address/Mask >
            ipv6_virtual_router_addresses:
              - < IPv6_address/Mask | IPv6_address >
            nodes:
              < l3_leaf_inventory_hostname_1 >:
                ipv6_address: < IPv6_address/Mask >   
        ipv6_static_routes:
          - destination_address_prefix: < IPv6_address/Mask >
            gateway: < IPv6_address >
            distance: < 1-255 >
            tag: < 0-4294967295 >
            name: < description >
            metric: < 0-4294967295 >
            interface: < interface >
            nodes: [ < node_1 >, < node_2 >]

Related Issue(s)

Depends on VARPv6 PR for cli_config_gen role

Component(s) name

arista.avd.eos_designs

Proposed changes

How to test

Molecule eos_designs_unit_tests

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)

@onurgashi onurgashi added EVPN design guide role: eos_designs issue related to eos_designs role labels May 4, 2022
@onurgashi onurgashi requested a review from a team as a code owner May 4, 2022 08:43
@onurgashi onurgashi self-assigned this May 4, 2022
@github-actions github-actions bot added role: eos_cli_config_gen issue related to eos_cli_config_gen role state: CI Updated CI scenario have been updated in the PR state: Documentation role Updated state: conflict PR with conflict labels May 4, 2022
@github-actions
Copy link

github-actions bot commented May 4, 2022

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

@github-actions github-actions bot removed the state: conflict PR with conflict label May 4, 2022
@github-actions
Copy link

github-actions bot commented May 4, 2022

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

@ClausHolbechArista ClausHolbechArista changed the title Ipv6 tenants Feat(eos_designs): Support for Ipv6 network services May 4, 2022
@github-actions
Copy link

github-actions bot commented May 9, 2022

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 May 9, 2022
@github-actions github-actions bot removed the state: conflict PR with conflict label May 9, 2022
@github-actions
Copy link

github-actions bot commented May 9, 2022

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

@github-actions github-actions bot added the type: code quality CI and development toolset label May 9, 2022
@tgodaA
Copy link
Contributor

tgodaA commented May 9, 2022

Sorry @onurgashi , I think I messed up your branch. I didn't realize that your branch wasn't rebase off the devel by me. So all of the old commits ended up in your branch.

ClausHolbechArista
ClausHolbechArista approved these changes May 10, 2022
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.

LGTM

@ClausHolbechArista ClausHolbechArista requested a review from a team May 10, 2022 15:47
jonxstill
jonxstill reviewed May 10, 2022
View reviewed changes
...e_collections/arista/avd/molecule/eos_designs_unit_tests/inventory/group_vars/DC1_FABRIC.yml Outdated
tenants: [ Tenant_A, Tenant_B, Tenant_C ]
tags: [ opzone, web, app, db, vmotion, nfs ]
tenants: [ Tenant_A, Tenant_B, Tenant_C, Tenant_D ]
tags: [ opzone, web, app, db, vmotion, nfs, wan ]
Copy link
Contributor

Choose a reason for hiding this comment

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

It's not a problem now because you've already had the reviews necessary - but adding the wan tag here vastly increases the blast radius (because there's changes to other tenants) of your change on the molecule artefacts, meaning there's a lot more work for people to do to review the change (or rather a lot more cruft for people to wade through). This could have been a wanv6 tag for example that would have only affected Tenant_D and a more limited number of devices.

Copy link
Contributor

Choose a reason for hiding this comment

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

+1 this annoyed me too, but since he was in a hurry...

Copy link
Contributor

Choose a reason for hiding this comment

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

BTW note we need 2 approvals on ansible-avd repo

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep, just added a 2nd.

@ClausHolbechArista ClausHolbechArista requested a review from a team May 10, 2022 17:11
jonxstill
jonxstill approved these changes May 10, 2022
View reviewed changes
Copy link
Contributor

@jonxstill jonxstill left a comment

Choose a reason for hiding this comment

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

LGTM - I can see my comment about IPv6 VARP applies for IPv4 too, so disregard that!

ansible_collections/arista/avd/roles/eos_designs/doc/network-services.md
- < IPv4_address/Mask | IPv4_address >

# ipv6 virtual-router address
# note, also requires an IPv6 address to be configured on the SVI where it is applied.
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a check that verifies that the ipv6 interface address has been specified if we try to use a VARPv6 address?

ClausHolbechArista
ClausHolbechArista requested changes May 10, 2022
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.

I will block this from merging and fix the molecule to be more precise.

ClausHolbechArista
ClausHolbechArista approved these changes May 11, 2022
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.

LGTM

@ClausHolbechArista ClausHolbechArista merged commit 247d9f3 into aristanetworks:devel May 11, 2022
ccsnw pushed a commit to ccsnw/ansible-avd that referenced this pull request May 11, 2022
@onurgashi @ClausHolbechArista
@tgodaA @ccsnw
Feat(eos_designs): Support for Ipv6 network services (aristanetworks#…
863a03c 
…1760)

Co-authored-by: Claus Holbech <holbech@arista.com>
Co-authored-by: tgodaA <tgoda@arista.com>
jonxstill pushed a commit to jonxstill/ansible-avd that referenced this pull request May 31, 2022
@onurgashi @ClausHolbechArista
@tgodaA @jonxstill
Feat(eos_designs): Support for Ipv6 network services (aristanetworks#…
bc69eff 
…1760)

Co-authored-by: Claus Holbech <holbech@arista.com>
Co-authored-by: tgodaA <tgoda@arista.com>
@melkypie melkypie mentioned this pull request Sep 21, 2022
Closed
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jonxstill jonxstill jonxstill approved these changes

@gmuloc gmuloc gmuloc left review comments

@ClausHolbechArista ClausHolbechArista ClausHolbechArista approved these changes

Assignees

@onurgashi onurgashi

Labels
EVPN design guide role: eos_cli_config_gen issue related to eos_cli_config_gen role role: eos_designs issue related to eos_designs role state: CI Updated CI scenario have been updated in the PR state: Documentation role Updated type: code quality CI and development toolset
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@onurgashi @tgodaA @gmuloc @jonxstill @ClausHolbechArista