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 keys to set mlag_peer_ipv4 and mlag_peer_l3_ipv4 directly #4205

Open
wants to merge 1 commit into
base: devel
Choose a base branch
from

Conversation

jmussmann
Copy link

This PR adds two node level keys:

mlag_peer_ipv4 and mlag_peer_l3_ipv4 to set the addresses directly without using pools.

Related Issue(s)

Fixes #4202

Component(s) name

arista.avd.eos_designs

Proposed changes

  • The two keys take precedence over the _pool version
  • AVD does not check if they are unique (or valid - it only knows them as string)

How to test

added a molecule test in DC1_FABRIC.yml

Checklist

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)

@jmussmann jmussmann requested review from a team as code owners July 11, 2024 15:47
Copy link

Review docs on Read the Docs

To test this pull request:

# Create virtual environment for this testing below the current directory
python -m venv test-avd-pr-4205
# Activate the virtual environment
source test-avd-pr-4205/bin/activate
# Install all requirements including PyAVD
pip install "pyavd[ansible] @ git+https://github.com/jmussmann/avd.git@issue/4202#subdirectory=python-avd" --force
# Install Ansible collection
ansible-galaxy collection install git+https://github.com/jmussmann/avd.git#/ansible_collections/arista/avd/,issue/4202 --force
# Optional: Install AVD examples
cd test-avd-pr-4205
ansible-playbook arista.avd.install_examples

@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 Jul 11, 2024
Comment on lines +156 to +157
if self._mlag_peer_l3_ipv4_address:
return self._mlag_peer_l3_ipv4_address
Copy link
Contributor

Choose a reason for hiding this comment

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

so this is not 100% correct - the problem here is that you will end up with the same value for primary and secondary which should not be the case

Copy link
Author

Choose a reason for hiding this comment

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

Yes, thanks for the hint. Maybe it would be better to set the IPs on the group level like this:

# If two nodes (and only two) are in the same node_group, they will automatically form an MLAG pair
  node_groups:
    # Definition of a node group that will include two devices in MLAG.
    # Definitions under the group will be inherited by both nodes in the group
    - group: DC1_L3_LEAF1
      # ASN to be used by BGP for the group. Both devices in the MLAG pair will use the same BGP ASN
      bgp_as: 65101
      mlag_peer_primary_ipv4_address: 192.168.42.40
      mlag_peer_secondary_ipv4_address: 192.168.42.41
      mlag_peer_l3_primary_ipv4_address: 192.168.43.40
      mlag_peer_l3_secondary_ipv4_address: 192.168.43.41
      nodes:
        # Definition of hostnames under the node_group
        - name: dc1-leaf1a
          id: 1
          mgmt_ip: 172.16.1.101/24
          loopback_ipv4_address: 172.16.1.101/24
          vtep_loopback_ipv4_address: 172.16.1.101/24
          # Definition of the port to be used in the uplink device facing this device.
          # Note that the number of elements in this list must match the length of 'uplink_switches' as well as 'uplink_interfaces'
          uplink_switch_interfaces:
            - Ethernet1
            - Ethernet1
        - name: dc1-leaf1b
          id: 2
          mgmt_ip: 172.16.1.102/24
          uplink_switch_interfaces:
            - Ethernet2
            - Ethernet2

An alternative might be something like this, but I think the "group" level would be better. What do you think?

def mlag_ip_primary(self) -> str:
  if self.shared_utils.mlag_role == "primary":
              if self._mlag_peer_ipv4_address:
                  return self._mlag_peer_ipv4_address
          else:
              if self.shared_utils.get_mlag_peer_fact("mlag_peer_ipv4_address"):
                  return self.shared_utils.get_mlag_peer_fact("mlag_peer_ipv4_address")

Copy link
Contributor

Choose a reason for hiding this comment

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

IMO we should just have the direct ip options for those who wants to control things. They have to set it per node or build inline jinja.
Note we already have the option of reusing the same mlag subnet under fabric_ip_addressing, and this could be used together with setting mlag_peer_ipv4_pool per node group to override things per group.

Comment on lines +92 to +93
if self._mlag_peer_ipv4_address:
return self._mlag_peer_ipv4_address
Copy link
Contributor

Choose a reason for hiding this comment

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

same as below - this is not really correct to always pick this address for both primary and secondary

Comment on lines +145 to +146
mlag_peer_ipv4_address: 192.168.44.41
mlag_peer_l3_ipv4_address: 192.168.44.41
Copy link
Contributor

Choose a reason for hiding this comment

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

let's not use the same address for non l3 and l3 in the test to avoid confusion in the results (like 192.168.45.1 for the next one)

Copy link
Contributor

Choose a reason for hiding this comment

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

Also the proper 192.168.44.40/31 and 192.168.44.41/31 rather than 41 and 42 if we want it to be "networkingly" correct

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we could just expose mlag_peer_ipv4_subnet and always assign the first and second host address of the given subnet. That would make it easier to use group inheritance etc.

action: permit 172.31.11.6/31
- sequence: 20
action: permit 192.168.44.42/31
Copy link
Contributor

Choose a reason for hiding this comment

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

so here really we should be careful regarding the generated config as it should be really the network IP of the subnet - this comes from the fact that the primary IP is the same as the secondary IP

@github-actions github-actions bot added the state: conflict PR with conflict label Aug 27, 2024
Copy link

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
role: eos_designs issue related to eos_designs role state: CI Updated CI scenario have been updated in the PR state: conflict PR with conflict state: Documentation role Updated
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feat(eos_designs): Create options to set mlag_peer_ipv4 and mlag_peer_l3_ipv4 direcly
3 participants