Skip to content

ISIS: cover telemetry gaps in base_adjacencies and isis_scale tests - #6119

Open
ashishpawar-google wants to merge 1 commit into
openconfig:mainfrom
ashishpawar-google:isisgap
Open

ashishpawar-google wants to merge 1 commit into
openconfig:mainfrom
ashishpawar-google:isisgap

Conversation

@ashishpawar-google

Copy link
Copy Markdown
Contributor
  • base_adjacencies_test: check LSP retransmit counter for presence only, since retransmits are legitimate during initial OTG adjacency bring-up.
  • base_adjacencies_test: fall back to level-2 hello-authentication when interface-level authentication config is rejected.
  • Add deviation isis_lsp_retransmit_counter_unsupported (Nokia).
  • isis_scale_test: verify global SPF first-interval state and add the spf-first-interval telemetry path to the README.

@OpenConfigBot

OpenConfigBot commented Oct 1, 2026 •

Copy link
Copy Markdown

Pull Request Functional Test Report for #6119 / 55dc3f0

Virtual Devices

Device Test Status Test Documentation Job Raw Log
Arista cEOS status
status
RT-2.1: pending authorization
RT-2.17: pending authorization
RT-2.1: Base IS-IS Process and Adjacencies
RT-2.17: IS-IS scale test
Cisco 8000E status
status
RT-2.1: pending authorization
RT-2.17: pending authorization
RT-2.1: Base IS-IS Process and Adjacencies
RT-2.17: IS-IS scale test
Cisco XRd status
status
RT-2.1: pending authorization
RT-2.17: pending authorization
RT-2.1: Base IS-IS Process and Adjacencies
RT-2.17: IS-IS scale test
Juniper ncPTX status
status
RT-2.1: pending authorization
RT-2.17: pending authorization
RT-2.1: Base IS-IS Process and Adjacencies
RT-2.17: IS-IS scale test
Nokia SR Linux status
status
RT-2.1: pending authorization
RT-2.17: pending authorization
RT-2.1: Base IS-IS Process and Adjacencies
RT-2.17: IS-IS scale test
Openconfig Lemming status
status
RT-2.1: pending authorization
RT-2.17: pending authorization
RT-2.1: Base IS-IS Process and Adjacencies
RT-2.17: IS-IS scale test

Hardware Devices

Device Test Status Test Documentation Raw Log
Arista status
status
RT-2.1: pending authorization
RT-2.17: pending authorization
RT-2.1: Base IS-IS Process and Adjacencies
RT-2.17: IS-IS scale test
Cisco status
status
RT-2.1: pending authorization
RT-2.17: pending authorization
RT-2.1: Base IS-IS Process and Adjacencies
RT-2.17: IS-IS scale test
Juniper status
status
RT-2.1: pending authorization
RT-2.17: pending authorization
RT-2.1: Base IS-IS Process and Adjacencies
RT-2.17: IS-IS scale test
Nokia status
status
RT-2.1: pending authorization
RT-2.17: pending authorization
RT-2.1: Base IS-IS Process and Adjacencies
RT-2.17: IS-IS scale test

Help

@gemini-code-assist

Copy link
Copy Markdown
Contributor

Summary of Changes

Hello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request focuses on improving the robustness and telemetry coverage of IS-IS tests. It addresses specific gaps in adjacency and scale testing by refining how counters are verified, adding support for platform-specific deviations, and ensuring configuration cleanup is more comprehensive. These changes improve test reliability across different hardware platforms and ensure better alignment with expected telemetry paths.

Highlights

  • Test Improvements: Updated base_adjacencies_test to handle LSP retransmit counters more flexibly and added a fallback to level-2 hello-authentication when interface-level authentication is rejected.
  • Telemetry Coverage: Added coverage for the IS-IS LSP retransmit counter and verified the global SPF first-interval state in isis_scale_test.
  • Deviation Management: Introduced a new deviation 'isis_lsp_retransmit_counter_unsupported' to support platforms where this counter is not available.
  • Infrastructure Updates: Improved test reliability by increasing timeouts for LAG interface status checks and route count verification, and enhanced configuration cleanup logic.
Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment @gemini-code-assist Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help /gemini help Displays a list of available commands.

Customization

To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here.

Footnotes

  1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request introduces support for verifying the IS-IS up-timestamp and retransmit counters, adds checks for the global SPF first interval, and optimizes route counting using AFT summaries. However, there are a few critical issues in the implementation. First, a potential nil pointer dereference exists in NewAggregateInterface when creating aggregate interfaces without IP attributes, as subinterface 0 is accessed unconditionally. Second, the retry loops in findProtocolRouteCount for both IPv4 and IPv6 prematurely break when the route count is zero, defeating the polling mechanism.

Comment thread internal/cfgplugins/interface.go Outdated
Comment thread feature/isis/otg_tests/isis_scale_test/isis_scale_test.go
Comment thread feature/isis/otg_tests/isis_scale_test/isis_scale_test.go
@ashishpawar-google
ashishpawar-google force-pushed the isisgap branch 2 times, most recently from 0985239 to a796e84 Compare October 8, 2026 07:33
@ashishpawar-google

ashishpawar-google commented Oct 9, 2026 •

Copy link
Copy Markdown
Contributor Author

@jayakumarramisetty

Copy link
Copy Markdown

/gemini review

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request introduces several updates to the IS-IS base adjacency and scale tests, including adding verification for the up-timestamp and retransmit counters, validating the global SPF first interval, and introducing the ISISLspRetransmitCounterUnsupported deviation. It also refactors the aggregate interface configuration in cfgplugins and improves cleanup registration. A critical issue was raised in the review regarding NewAggregateInterface in internal/cfgplugins/interface.go, where subinterface 0 is not created if both IPv4 and IPv6 attributes are empty, causing the RequireRoutedSubinterface0 deviation logic to be skipped. A code suggestion was provided to ensure subinterface 0 is explicitly created and enabled when required.

Comment thread internal/cfgplugins/interface.go Outdated
@jayakumarramisetty

Copy link
Copy Markdown

A few verification notes on base_adjacencies_test:
In the PR test results comment for base_adjacencies_test:

  • Nokia is listed twice (6dcea624 and 875a8428) and Arista is missing — could you please share an Arista Sponge run?
  • The linked Juniper run (3b25772b) was executed with TESTBRIDGE_TEST_ONLY=TestHelloPadding|TestAuthentication, so TestBasic (which contains the new up-timestamp and lsp/state/retransmit checks at lines 157, 254, and 319) didn't run in that invocation. Could you share a Juniper run that includes TestBasic?

Comment thread feature/isis/otg_tests/isis_scale_test/isis_scale_test.go Outdated
@ashishpawar-google

Copy link
Copy Markdown
Contributor Author

A few verification notes on base_adjacencies_test: In the PR test results comment for base_adjacencies_test:

  • Nokia is listed twice (6dcea624 and 875a8428) and Arista is missing — could you please share an Arista Sponge run?
  • The linked Juniper run (3b25772b) was executed with TESTBRIDGE_TEST_ONLY=TestHelloPadding|TestAuthentication, so TestBasic (which contains the new up-timestamp and lsp/state/retransmit checks at lines 157, 254, and 319) didn't run in that invocation. Could you share a Juniper run that includes TestBasic?

Update the latest run results on mirror PR.

Comment thread internal/cfgplugins/interface.go Outdated
Comment thread internal/cfgplugins/interface.go Outdated
Comment thread internal/cfgplugins/interface.go
Comment thread internal/cfgplugins/interface.go Outdated
base_adjacencies_test (RT-2.1):
- Verify adjacency up-timestamp is reported and non-zero.
- Check LSP retransmit counter: zero before adjacency, present after
  (retransmits are legitimate during initial OTG adjacency bring-up).
- Fall back to level-2 hello-authentication when interface-level
  authentication config is rejected by the DUT.
- Add deviation isis_lsp_retransmit_counter_unsupported for Nokia
  (https://partnerissuetracker.corp.google.com/issues/570885559) and
  regenerate metadata.pb.go.

isis_scale_test (RT-2.17):
- Verify global SPF first-interval state (200ms) and document the path.
- Fix dynamic test DUT data (copy LAG attrs instead of mutating shared
  attrs; use both LAGs) and enable hierarchical FIB where supported.
- Make ISIS adjacency/route-count checks robust: nil-safe level lookup,
  honor nominal count in non-stream path, prefer AFT summary counters
  with fallback to per-entry watch, longer convergence timeouts.
- Register cleanup before DUT config and also remove LAG interfaces so
  failed runs don't leak config.

This branch has not been deployed

No deployments
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.

4 participants