Aft 1.1 fix by manan-patel · Pull Request #4950 · openconfig/featureprofiles

manan-patel · 2025-12-21T22:29:31Z

No description provided.

OpenConfigBot · 2025-12-21T22:29:51Z

Pull Request Functional Test Report for #4950 / `e6ef7b2`

Virtual Devices

Device	Status	Test Documentation
Arista cEOS	AFT-1.1: pending authorization	AFT-1.1: AFTs Base
Cisco 8000E	AFT-1.1: pending authorization	AFT-1.1: AFTs Base
Cisco XRd	AFT-1.1: pending authorization	AFT-1.1: AFTs Base
Juniper ncPTX	AFT-1.1: pending authorization	AFT-1.1: AFTs Base
Nokia SR Linux	AFT-1.1: pending authorization	AFT-1.1: AFTs Base
Openconfig Lemming	AFT-1.1: pending authorization	AFT-1.1: AFTs Base

Hardware Devices

Device	Status	Test Documentation
Arista	AFT-1.1: pending authorization	AFT-1.1: AFTs Base
Cisco	AFT-1.1: pending authorization	AFT-1.1: AFTs Base
Juniper	AFT-1.1: pending authorization	AFT-1.1: AFTs Base
Nokia	AFT-1.1: pending authorization	AFT-1.1: AFTs Base

Help

gemini-code-assist · 2025-12-21T22:29:51Z

Summary of Changes

Hello @manan-patel, I'm Gemini Code Assist¹! 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 introduces several key improvements to AFT (Address Family Table) tests. It enhances test flexibility by allowing dynamic IPv6 subnet mask adjustments based on device deviations, refines interface state manipulation for more reliable churn simulations, and corrects a bug in the AFT cache to ensure accurate next-hop group data. These changes collectively aim to improve the stability and correctness of network device testing, particularly for BGP and AFT convergence scenarios.

Highlights

IPv6 Subnet Mask Flexibility: Introduced a new deviation subnet_mask_change to allow dynamic adjustment of IPv6 subnet mask length (from /128 to /64) in BGP tests, accommodating devices with different subnet mask requirements.
Enhanced Interface Churn Simulation: Switched the method for controlling interface states (up/down) during churn tests from tc.otgInterfaceState to gnmi.Replace, providing a more robust way to simulate interface changes.
AFT Cache Next-Hop Deduplication: Fixed a bug in the AFT cache parsing logic to ensure that next-hop IDs are unique, preventing duplicates from being added to the NHIDs list and improving the accuracy of next-hop group representation.
AFT Convergence Time Adjustment: Increased the aftConvergenceTime constant from 20 minutes to 30 minutes, potentially allowing more time for AFT convergence in tests.

🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console.

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 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 counter productive. 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.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

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

Code Review

This pull request introduces a new deviation, SubnetMaskChange, to allow for different IPv6 prefix lengths in the AFT base test. The changes include adding new constants for different prefix lengths, modifying the test logic to select the prefix length based on the deviation, and defining the new deviation in the proto and Go files. Additionally, there's a fix in aft_cache.go to handle duplicate next-hop IDs.

My review has identified a critical bug in afts_base_test.go where interfaces are incorrectly disabled instead of being enabled, which will cause test failures. I've also pointed out several medium-severity issues, including a redundant else block, and a failure to adhere to the repository style guide for deviation definitions by omitting a required issue tracker URL. There are also opportunities to improve comment clarity for the new deviation.

gemini-code-assist · 2025-12-21T22:31:09Z

+// SetSubnetMaskChange returns true if device requires changing subnet mask length
+func SubnetMaskChange(dut *ondatra.DUTDevice) bool {
+	return lookupDUTDeviations(dut).GetSubnetMaskChange()
+}


According to the repository style guide, deviation accessor functions must include a comment with a URL to an issue tracker. This helps in tracking the removal of the deviation. Please add the issue tracker URL.

Additionally, the function comment // SetSubnetMaskChange... is slightly misleading as the function name is SubnetMaskChange. It's better to keep them consistent for clarity.

Suggested change

// SetSubnetMaskChange returns true if device requires changing subnet mask length

func SubnetMaskChange(dut *ondatra.DUTDevice) bool {

return lookupDUTDeviations(dut).GetSubnetMaskChange()

}

// SubnetMaskChange returns true if device requires changing subnet mask length.

// https://issuetracker.google.com/issues/xxxxx

func SubnetMaskChange(dut *ondatra.DUTDevice) bool {

return lookupDUTDeviations(dut).GetSubnetMaskChange()

}

References

The repository style guide requires that accessor functions for deviations include a comment with a URL to an issue tracker. This is specified in lines 83-86 of the guide. ^(link)

gemini-code-assist · 2025-12-21T22:31:09Z

+    // SetSubnetMaskChange returns true if it needs to be changed
+    bool subnet_mask_change = 358;


The comment // SetSubnetMaskChange returns true if it needs to be changed is inconsistent with the field name subnet_mask_change and is a bit vague. A more descriptive comment would improve clarity for developers using this deviation.

Suggested change

// SetSubnetMaskChange returns true if it needs to be changed

bool subnet_mask_change = 358;

// SubnetMaskChange indicates whether the IPv6 subnet mask length needs to be changed for the test.

bool subnet_mask_change = 358;

coveralls · 2025-12-21T22:38:51Z

Pull Request Test Coverage Report for Build 21276411438

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

For more information on this, see Tracking coverage changes with pull request builds.
To avoid this issue with future PRs, see these Recommended CI Configurations.
For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

0 of 7 (0.0%) changed or added relevant lines in 1 file are covered.
766 unchanged lines in 3 files lost coverage.
Overall coverage decreased (-0.006%) to 9.907%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
internal/telemetry/aftcache/aft_cache.go	0	7	0.0%

Files with Coverage Reduction	New Missed Lines	%
internal/deviations/deviations.go	7	0.0%
internal/cfgplugins/bgp.go	43	0.0%
proto/metadata_go_proto/metadata.pb.go	716	0.0%

Totals
Change from base Build 21269668258:	-0.006%
Covered Lines:	2227
Relevant Lines:	22479

💛 - Coveralls

paramasivamn1 · 2025-12-22T04:04:34Z

    bool config_acl_oc_unsupported = 357;

+    // SetSubnetMaskChange returns true if it needs to be changed
+    bool subnet_mask_change = 358;


@manan-patel, Can you kindly elaborate why is this deviation is needed and also create bug with the details so that we can review and track the changes if needed.

when we have /128 subnet mask for Ipv6, we use CEM resources. With case BGP v6 routes, those resources are getting exhausted.

cmd to validate the status
show controllers npu debugshell 0 "script resource_usage" location 0/7/CPU0 | i CENTRAL_EM

with /64 (not entire ip match) LPM resources are used

show controllers npu debugshell 0 "script resource_usage" location 0/7/CPU0 | i LPM

We can control the allocation across CEM and LPM using hw-module CLIs; however, there are inherent limitations, and these CLIs must be configured with the appropriate profile in mind

can we make 900K 64 and 100K /128 routes then.

Hi Param, we have validated locally 900k /64 and 100k /128 routes. Should we uplift the FNT to cover the combination?

…tion for v6 prefixes for low and full scale, base profile distributes 90% /64 and 10% /128 v6 scale

ksgireesha · 2026-09-10T15:27:12Z

Hi @manan-patel, could you please resolve the merge conflicts and rebase the branch so the checks turn green?

ashishpawar-google · 2026-09-11T16:42:28Z

Hi @manan-patel , Please rebase / squash the commits so the commit history remains clean and only includes test changes.

manan-patel added 3 commits December 21, 2025 15:39

added AFT-1.1 deviation and updated code

821b7a9

updated metadata.pb

3ae48ce

fixed logic to handle duplicate NHs in an NHG

d06668c

manan-patel requested review from a team as code owners December 21, 2025 22:29

removed unnecessary else clause

54a602a

gemini-code-assist Bot reviewed Dec 21, 2025

View reviewed changes

removed suggested ixia side link shutdown

0df0860

manan-patel added the cisco-pr label Dec 21, 2025

paramasivamn1 requested changes Dec 22, 2025

View reviewed changes

manan-patel added 7 commits December 22, 2025 22:35

addressed comments

371e618

fixed order of the call and rebase

6c862fd

removed subnetmask deviation, and provided support to run any combina…

c61118f

…tion for v6 prefixes for low and full scale, base profile distributes 90% /64 and 10% /128 v6 scale

fixed typo

579f3b0

rebased and added hw-module configuration under FNT

ad09d66

fixed typo

eea6f6c

trimmed down hw-module list needed for FNT

e53875c

karthikeya-remilla mentioned this pull request Aug 18, 2026

testing #4937

Closed

ksgireesha assigned singhavnish2516 Aug 27, 2026

singhavnish2516 assigned ashishpawar-google and unassigned singhavnish2516 Sep 3, 2026

manan-patel added 3 commits September 26, 2026 11:38

rebase

d37dede

Merge branch 'main' into AFT-1.1_fix

f14448d

rebase

e6ef7b2

		// SetSubnetMaskChange returns true if it needs to be changed
		bool subnet_mask_change = 358;

Conversation

manan-patel commented Dec 21, 2025

Uh oh!

OpenConfigBot commented Dec 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Functional Test Report for #4950 / e6ef7b2

Virtual Devices

Hardware Devices

Uh oh!

gemini-code-assist Bot commented Dec 21, 2025

Summary of Changes

Highlights

Footnotes

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gemini-code-assist Bot Dec 21, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot Dec 21, 2025

Choose a reason for hiding this comment

Uh oh!

coveralls commented Dec 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 21276411438

Warning: This coverage report may be inaccurate.

Details

💛 - Coveralls

Uh oh!

paramasivamn1 Dec 22, 2025

Choose a reason for hiding this comment

Uh oh!

manan-patel Dec 23, 2025

Choose a reason for hiding this comment

Uh oh!

paramasivamn1 Dec 23, 2025

Choose a reason for hiding this comment

Uh oh!

manan-patel Jan 8, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ksgireesha commented Sep 10, 2026

Uh oh!

ashishpawar-google commented Sep 11, 2026

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

OpenConfigBot commented Dec 21, 2025 •

edited

Loading

Pull Request Functional Test Report for #4950 / `e6ef7b2`

coveralls commented Dec 21, 2025 •

edited

Loading