-
Notifications
You must be signed in to change notification settings - Fork 30
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
fix(anta.tests): Cleaning up ISIS tests module #963
base: main
Are you sure you want to change the base?
Conversation
CodSpeed Performance ReportMerging #963 will not alter performanceComparing Summary
|
# Copyright (c) 2023-2024 Arista Networks, Inc. | ||
# Use of this source code is governed by the Apache License 2.0 | ||
# that can be found in the LICENSE file. | ||
"""Module containing input models for routing ISIS tests.""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"""Module containing input models for routing ISIS tests.""" | |
"""Module containing input models for routing IS-IS tests.""" |
@@ -164,23 +126,37 @@ class VerifyISISNeighborState(AntaTest): | |||
@AntaTest.anta_test | |||
def test(self) -> None: | |||
"""Main test function for VerifyISISNeighborState.""" | |||
self.result.is_success() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
show isis neighbors
shows neighbors in VRF default only. We should use show isis neighbors vrf all
instead and add a knob to the inputs:
anta.tests.routing.isis:
- VerifyISISNeighborState:
check_all_vrfs: bool = False
It would be False
by default to avoid breaking change. We should also update the test docstring to reflect this.
anta/tests/routing/isis.py
Outdated
@@ -146,7 +108,7 @@ class VerifyISISNeighborState(AntaTest): | |||
Expected Results | |||
---------------- | |||
* Success: The test will pass if all IS-IS neighbors are in UP state. | |||
* Failure: The test will fail if some IS-IS neighbors are not in UP state. | |||
* Failure: The test will fail if any IS-IS neighbor adjance session is down. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* Failure: The test will fail if any IS-IS neighbor adjance session is down. | |
* Failure: The test will fail if any IS-IS neighbor adjacency is down. |
anta/tests/routing/isis.py
Outdated
@@ -164,23 +126,37 @@ class VerifyISISNeighborState(AntaTest): | |||
@AntaTest.anta_test | |||
def test(self) -> None: | |||
"""Main test function for VerifyISISNeighborState.""" | |||
self.result.is_success() | |||
|
|||
# Verify the ISIS neighbor configure. If not then skip the test. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
# Verify the ISIS neighbor configure. If not then skip the test. | |
# Verify if ISIS neighbors are configured. Skip the test if none are found. |
anta/tests/routing/isis.py
Outdated
command_output = self.instance_commands[0].json_output | ||
if _count_isis_neighbor(command_output) == 0: | ||
neighbor_details = _get_isis_neighbor_details(command_output) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer having a dedicated helper function to check if any neighbors are configured. You could create any_neighbors_configured
that returns True as soon as it find any neighbor in any VRF/instance (depending on the check_all_vrfs
knob), avoiding unnecessary iteration through the entire data.
anta/tests/routing/isis.py
Outdated
@@ -208,43 +184,38 @@ class VerifyISISNeighborCount(AntaTest): | |||
class Input(AntaTest.Input): | |||
"""Input model for the VerifyISISNeighborCount test.""" | |||
|
|||
interfaces: list[InterfaceCount] | |||
interfaces: list[ISISInterface] | |||
"""list of interfaces with their information.""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"""list of interfaces with their information.""" | |
"""List of IS-IS interfaces with their information.""" |
"""Verifies the number of IS-IS neighbors. | ||
|
||
This test performs the following checks for each specified interface: | ||
|
||
1. Validates the IS-IS neighbors configured on specified interface. | ||
2. Validates the number of IS-IS neighbors for each interface at specified level. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as VerifyISISNeighborState, we should use show isis interface brief vrf all
command instead.
"""Verifies the operational mode of IS-IS Interfaces. | ||
|
||
This test performs the following checks: | ||
|
||
1. Validates that all specified IS-IS interfaces are configured. | ||
2. Validates the operational mode of each IS-IS interface (e.g., "active," "passive," or "unset"). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as others: show isis interface brief vrf all
:). It's actually a bug for this test so let's fix it!
"""Adjacency type""" | ||
address: IPv4Address | ||
"""IP address of remote end of segment.""" | ||
instances: list[ISISInstance] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IS-IS SR is only supported in default VRF.
On EOS:
PE11(config-router-isis-vrf-DEV)#segment-routing mpls
% Unavailable command (not supported in non-default vrf)
We should add a Pydantic validator here to check if the VRF in each ISISInstance
model is left to default
. If not, we can raise an error and say that as of 4.33.1F, IS-IS SR is only supported in default VRF. Refer to https://www.arista.com/en/support/toi/eos-4-17-0f/13789-isis-segment-routing in the limitations section.
"""VRF name where ISIS instance is configured.""" | ||
dataplane: Literal["MPLS", "mpls", "unset"] = "MPLS" | ||
"""Configured dataplane for the instance.""" | ||
instances: list[ISISInstance] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment as above about ISIS SR supported in default VRF only.
2dd7370
to
9f92ab1
Compare
Quality Gate passedIssues Measures |
Description
Refactoring ISIS tests module to address the following issues:
Tests: