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

Refactor(eos_designs): Refactor eos_designs structured_config code for router_path_selection #5002

Open
wants to merge 12 commits into
base: devel
Choose a base branch
from

Conversation

laxmikantchintakindi
Copy link
Contributor

@laxmikantchintakindi laxmikantchintakindi commented Feb 10, 2025

Change Summary

Refactor eos_designs structured_config code for router_path_selection.py.

Related Issue(s)

Fixes #

Component(s) name

arista.avd.eos_designs

Proposed changes

Refactor eos_designs structured_config code for router_path_selection.py.

How to test

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)

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-5002
# Activate the virtual environment
source test-avd-pr-5002/bin/activate
# Install all requirements including PyAVD
pip install "pyavd[ansible] @ git+https://github.com/laxmikantchintakindi/avd.git@refactor/router-path-selection#subdirectory=python-avd" --force
# Point Ansible collections path to the Python virtual environment
export ANSIBLE_COLLECTIONS_PATH=$VIRTUAL_ENV/ansible_collections
# Install Ansible collection
ansible-galaxy collection install git+https://github.com/laxmikantchintakindi/avd.git#/ansible_collections/arista/avd/,refactor/router-path-selection --force
# Optional: Install AVD examples
cd test-avd-pr-5002
ansible-playbook arista.avd.install_examples

@github-actions github-actions bot added state: CI Updated CI scenario have been updated in the PR and removed state: CI Updated CI scenario have been updated in the PR labels Feb 18, 2025
@laxmikantchintakindi laxmikantchintakindi force-pushed the refactor/router-path-selection branch from 0c85f19 to 751ba2d Compare February 19, 2025 12:37
@Vibhu-gslab Vibhu-gslab force-pushed the refactor/router-path-selection branch from 66b7227 to 30eb016 Compare February 24, 2025 05:29
@laxmikantchintakindi laxmikantchintakindi force-pushed the refactor/router-path-selection branch from 30eb016 to f548021 Compare February 25, 2025 14:55
@github-actions github-actions bot added the state: conflict PR with conflict label Feb 27, 2025
Copy link

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 Feb 28, 2025
Copy link

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

"vrfs": vrfs,
},
)
for vrf in vrfs:
Copy link
Contributor

Choose a reason for hiding this comment

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

Above we are iterating on self._filtered_wan_vrfs and here again iterating on vrfs so i think we can from the vrfs data in above loop only with if condition

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.


def _autovpn_policies(self: AvdStructuredConfigNetworkServicesProtocol) -> list:
def _autovpn_policies(self: AvdStructuredConfigNetworkServicesProtocol) -> None:
"""Return a list of policies for AutoVPN."""
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"""Return a list of policies for AutoVPN."""
"""Set the list of policies for AutoVPN."""

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

Comment on lines 59 to 65
policy_item.rules.append(
EosCliConfigGen.RouterPathSelection.PoliciesItem.RulesItem(
id=10 * index,
application_profile=get(match, "application_profile"),
load_balance=match["load_balance_policy"].name if "load_balance_policy" in match else None,
)
)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
policy_item.rules.append(
EosCliConfigGen.RouterPathSelection.PoliciesItem.RulesItem(
id=10 * index,
application_profile=get(match, "application_profile"),
load_balance=match["load_balance_policy"].name if "load_balance_policy" in match else None,
)
)
policy_item.rules.append_new(
id=10 * index,
application_profile=get(match, "application_profile"),
load_balance=match["load_balance_policy"].name if "load_balance_policy" in match else None,
)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@laxmikantchintakindi laxmikantchintakindi changed the title Refactor(eos_designs): Refactor eos_designs structured_config code for router_path_selection.py Refactor(eos_designs): Refactor eos_designs structured_config code for router_path_selection Mar 3, 2025
@gmuloc gmuloc marked this pull request as ready for review March 3, 2025 13:03
@gmuloc gmuloc requested review from a team as code owners March 3, 2025 13:03

# An entry is composed of a list of path-groups in `names` and a `priority`
policy_entries = get(input_dict, "path_groups", [])
# TODO: Decide if the default value in schema for lowest_hop_count is correct.
Copy link
Contributor

Choose a reason for hiding this comment

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

discuss between @gmuloc and @ClausHolbechArista

)
self._set_local_interfaces_for_path_group(path_group.name, path_group_item)
self._set_dynamic_peers(disable_ipsec=disable_dynamic_peer_ipsec, path_group_item=path_group_item)
self._set_static_peers_for_path_group(path_group.name, path_group_item)

if is_local_pg:
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel like we could avoid creating the PathGroupsItem if it is not a local_pg - what do you think?

so above line 54

if not is_local_pg:
    continue

Comment on lines +85 to +87
path_group_item = EosCliConfigGen.RouterPathSelection.PathGroupsItem()
self._generate_ha_path_group(path_group_item=path_group_item)
self.structured_config.router_path_selection.path_groups.append(path_group_item)
Copy link
Contributor

Choose a reason for hiding this comment

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

Replace these 3 lines and rename the function
_set_ha_path_group below

@@ -149,44 +135,48 @@ def _get_path_group_id(self: AvdStructuredConfigOverlayProtocol, path_group_name
return config_id
return 500

def _get_local_interfaces_for_path_group(self: AvdStructuredConfigOverlayProtocol, path_group_name: str) -> list:
def _set_local_interfaces_for_path_group(
self: AvdStructuredConfigOverlayProtocol, path_group_name: str, path_group_item: EosCliConfigGen.RouterPathSelection.PathGroupsItem
Copy link
Contributor

Choose a reason for hiding this comment

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

no need to pass path_group_name anymore I think if you pass path_group_item you can just access it using path_group_item.name

also rename the path_group_item to path_group, that's what it is really

"""
Generate the router_path_selection.local_interfaces list.

For AUTOVPN clients, configure the stun server profiles as appropriate
"""
local_interfaces = []
if path_group_name not in self.shared_utils.wan_local_path_groups:
Copy link
Contributor

Choose a reason for hiding this comment

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

Once your rename the path_group_item to path_group, use:

Suggested change
if path_group_name not in self.shared_utils.wan_local_path_groups:
if path_group.name not in self.shared_utils.wan_local_path_groups:

def _get_static_peers_for_path_group(self: AvdStructuredConfigOverlayProtocol, path_group_name: str) -> list | None:
def _set_static_peers_for_path_group(
self: AvdStructuredConfigOverlayProtocol, path_group_name: str, path_group_item: EosCliConfigGen.RouterPathSelection.PathGroupsItem
) -> None:
"""Retrieves the static peers to configure for a given path-group based on the connected nodes."""
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"""Retrieves the static peers to configure for a given path-group based on the connected nodes."""
"""Set the static peers to configure for a given path-group based on the connected nodes."""

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.

3 participants