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

Refactor(eos_designs): Refactor eos_designs structured_config code for ip_igmp_snooping.py #5012

Draft
wants to merge 4 commits into
base: devel
Choose a base branch
from
+27 −51
Draft

Refactor(eos_designs): Refactor eos_designs structured_config code for ip_igmp_snooping.py #5012

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
04a9f6e
Refactor(eos_designs): Refactor eos_designs structured_config code fo…
Feb 11, 2025
bf93400
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Feb 11, 2025
c838edb
Fix expected error msg
Feb 11, 2025
6d6ea64
Small adjustment of code
Feb 11, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion ..._negative_unit_tests/inventory/host_vars/duplicate-vrfs-tenant-igmp-snooping-conflict.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,6 @@ tenants:
ip_address_virtual: 10.0.100.1/24

expected_error_message: >-
Found duplicate objects with conflicting data while generating configuration for IGMP snooping for SVIs in VRF 'VRF1'.
Found duplicate objects with conflicting data while generating configuration for Vlans.
{'id': 100, 'querier': {'enabled': True, 'address': '192.168.255.101'}} conflicts with {'id': 100, 'querier':
{'enabled': False}}.
76 changes: 26 additions & 50 deletions python-avd/pyavd/_eos_designs/structured_config/network_services/ip_igmp_snooping.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,11 @@
# that can be found in the LICENSE file.
from __future__ import annotations

from functools import cached_property
from typing import TYPE_CHECKING, Protocol

from pyavd._utils import append_if_not_duplicate, default, strip_empties_from_dict
from pyavd._eos_cli_config_gen.schema import EosCliConfigGen
from pyavd._eos_designs.structured_config.structured_config_generator import structured_config_contributor
from pyavd._utils import default, strip_empties_from_dict

if TYPE_CHECKING:
from pyavd._eos_designs.schema import EosDesigns
Expand All @@ -21,51 +22,32 @@ class IpIgmpSnoopingMixin(Protocol):
Class should only be used as Mixin to a AvdStructuredConfig class.
"""

@cached_property
def ip_igmp_snooping(self: AvdStructuredConfigNetworkServicesProtocol) -> dict | None:
"""Return structured config for ip_igmp_snooping."""
@structured_config_contributor
def ip_igmp_snooping(self: AvdStructuredConfigNetworkServicesProtocol) -> None:
"""Set structured config for ip_igmp_snooping."""
if not self.shared_utils.network_services_l2:
return None
return

igmp_snooping_enabled = self.shared_utils.igmp_snooping_enabled
ip_igmp_snooping = {"globally_enabled": igmp_snooping_enabled}
if not igmp_snooping_enabled:
return ip_igmp_snooping
self.structured_config.ip_igmp_snooping.globally_enabled = igmp_snooping_enabled
Copy link
Contributor

Choose a reason for hiding this comment

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

Move out this line from if code so you dont need to write it again.

Copy link
Contributor

@MaheshGSLAB MaheshGSLAB Feb 11, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

and there is no case of igmp_snooping_enabled: false in molecule so can you recheck that it return None or globally_enabled: false

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can we add this coverage part in any other PR. In first part we need to add globally_enabled: false only when igmp_snooping_enabled is false and return. If I move out of if condition code will return even when igmp_snooping_enabled is true.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you please check the false case locally to make sure it works fine

return
Shivani-gslab marked this conversation as resolved.
Show resolved Hide resolved

vlans = []
for tenant in self.shared_utils.filtered_tenants:
for vrf in tenant.vrfs:
for svi in vrf.svis:
if vlan := self._ip_igmp_snooping_vlan(svi, tenant):
append_if_not_duplicate(
list_of_dicts=vlans,
primary_key="id",
new_dict=vlan,
context=f"IGMP snooping for SVIs in VRF '{vrf.name}'",
context_keys=["id"],
)

self._ip_igmp_snooping_vlan(svi, tenant)
for l2vlan in tenant.l2vlans:
if vlan := self._ip_igmp_snooping_vlan(l2vlan, tenant):
append_if_not_duplicate(
list_of_dicts=vlans,
primary_key="id",
new_dict=vlan,
context="IGMP snooping for L2VLANs",
context_keys=["id"],
)

if vlans:
ip_igmp_snooping["vlans"] = vlans
self._ip_igmp_snooping_vlan(l2vlan, tenant)

return ip_igmp_snooping
self.structured_config.ip_igmp_snooping.globally_enabled = igmp_snooping_enabled

def _ip_igmp_snooping_vlan(
self: AvdStructuredConfigNetworkServicesProtocol,
vlan: EosDesigns._DynamicKeys.DynamicNetworkServicesItem.NetworkServicesItem.VrfsItem.SvisItem
| EosDesigns._DynamicKeys.DynamicNetworkServicesItem.NetworkServicesItem.L2vlansItem,
tenant: EosDesigns._DynamicKeys.DynamicNetworkServicesItem.NetworkServicesItem,
) -> dict:
) -> None:
"""
ip_igmp_snooping logic for one vlan.

Expand All @@ -87,24 +69,18 @@ def _ip_igmp_snooping_vlan(
if self.shared_utils.network_services_l3 and self.shared_utils.uplink_type in ["p2p", "p2p-vrfs"]:
igmp_snooping_querier_enabled = default(vlan.igmp_snooping_querier.enabled, tenant.igmp_snooping_querier.enabled)

ip_igmp_snooping_vlan = strip_empties_from_dict(
{
"enabled": igmp_snooping_enabled,
"querier": {
"enabled": igmp_snooping_querier_enabled,
"address": (
default(vlan.igmp_snooping_querier.source_address, tenant.igmp_snooping_querier.source_address, self.shared_utils.router_id)
if igmp_snooping_querier_enabled
else None
),
"version": default(vlan.igmp_snooping_querier.version, tenant.igmp_snooping_querier.version) if igmp_snooping_querier_enabled else None,
},
# IGMP snooping fast-leave feature is enabled only when evpn_l2_multicast is enabled
"fast_leave": default(vlan.igmp_snooping_querier.fast_leave, tenant.evpn_l2_multicast.fast_leave) if evpn_l2_multicast_enabled else None,
}
vlan_item = EosCliConfigGen.IpIgmpSnooping.VlansItem(
enabled=igmp_snooping_enabled,
querier=EosCliConfigGen.IpIgmpSnooping.VlansItem.Querier(
enabled=igmp_snooping_querier_enabled,
address=default(vlan.igmp_snooping_querier.source_address, tenant.igmp_snooping_querier.source_address, self.shared_utils.router_id)
if igmp_snooping_querier_enabled
else None,
version=default(vlan.igmp_snooping_querier.version, tenant.igmp_snooping_querier.version) if igmp_snooping_querier_enabled else None,
),
fast_leave=default(vlan.igmp_snooping_querier.fast_leave, tenant.evpn_l2_multicast.fast_leave) if evpn_l2_multicast_enabled else None,
)

if ip_igmp_snooping_vlan:
return {"id": vlan.id, **ip_igmp_snooping_vlan}

return ip_igmp_snooping_vlan
if strip_empties_from_dict(vlan_item._as_dict()):
Copy link
Contributor

Choose a reason for hiding this comment

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

Need to check if we can avoid to use this method. Will check and update

vlan_item._update(id=vlan.id)
self.structured_config.ip_igmp_snooping.vlans.append(vlan_item)