AFTS reboot changes #4969

paramasivamn1 · 2026-01-05T09:12:44Z

Pease review AFTS reboot related changes.

gemini-code-assist · 2026-01-05T09:13:04Z

Summary of Changes

Hello @paramasivamn1, 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 comprehensive changes to the AFTS reboot test suite, primarily focusing on enhancing the reliability and clarity of network protocol configurations and device reboot verification. The modifications aim to streamline the setup of BGP and ISIS, improve the robustness of post-reboot checks, and refine how test-related data is handled, ultimately leading to more stable and accurate test results.

Highlights

Refactored BGP and ISIS Configuration: The BGP and ISIS configuration logic within the configureDUT function has been significantly refactored. New helper functions like createBGPNeighbor and updateNeighborMaxPrefix are introduced for BGP, and the isissession library is now utilized for ISIS setup, replacing previous cfgplugins usage.
Enhanced Reboot Verification: The reboot verification process has been improved. The waitForReboot function now directly monitors the DUT's boot time, and the rebootDUT function includes additional steps such as a 10-minute sleep, re-dialing the GNOI client, and watching for system uptime to ensure the device is fully reachable post-reboot.
Updated Constants and Types: Several constants related to reboot timing and BGP AS numbers have been updated or introduced. New types BGPNeighbor and IPFamily were added to facilitate the new BGP configuration structure.
Metadata and AFT Cache Adjustments: The metadata.textproto file was updated to remove the explicit_port_speed deviation. Additionally, the aft_cache.go file now ensures that AFT notifications are always written if they exist, regardless of missing or failing prefixes.

🧠 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 refactors the AFTS reboot test, notably improving BGP and ISIS configuration by using dedicated helpers. However, the reboot handling logic introduces a critical issue: a hardcoded time.Sleep that violates the style guide and significantly slows down the test. The logic for waiting for the reboot is also complex and contains an incorrect context cancellation. Additionally, a new function for ensuring gNMI configuration persistence has been added but is not used, and there's a minor readability issue with type definitions. My review provides suggestions to fix these issues.

gemini-code-assist · 2026-01-05T09:17:03Z

feature/afts/otg_tests/afts_reboot/afts_reboot_test.go

 	if _, err = gnoiClient.System().Reboot(ctxWithTimeout, rebootRequest); err != nil {
 		t.Fatalf("Failed to reboot chassis with unexpected err: %v", err)
 	}
+	startReboot := time.Now()
+	t.Logf("Reboot request sent at %v", startReboot)
+	// Wait for the device to become reachable again.
+	// tc.waitForReboot(t, bootTimeBeforeReboot)
+	time.Sleep(10 * time.Minute)
+	gnoiClient, err = tc.dut.RawAPIs().BindingDUT().DialGNOI(t.Context())
+	if err != nil {
+		t.Fatalf("Error dialing gNOI: %v", err)
+	}
+	_, ok := gnmi.Watch(t, tc.dut, gnmi.OC().System().UpTime().State(), maxRebootTime, func(val *ygnmi.Value[uint64]) bool {
+		_, ok := val.Val()
+		return ok
+	}).Await(t)
+	if !ok {
+		t.Fatalf("Timeout exceeded: DUT did not reboot within %v", maxRebootTime)
+	}
+	bootTimeAfterReboot := gnmi.Get(t, tc.dut, gnmi.OC().System().BootTime().State())
+	t.Logf("DUT boot time after reboot: %v", bootTimeAfterReboot)
+	t.Logf("Device boot time after reboot: %.2f seconds.", time.Since(startReboot).Seconds())
+	if bootTimeAfterReboot < bootTimeBeforeReboot {
+		cancel()
+	}


The reboot handling logic in this function has several issues:

time.Sleep Violation: The use of time.Sleep(10 * time.Minute) on line 426 violates the style guide (rule 53-54), which states to use gnmi.Watch for waiting on conditions. This hardcoded sleep makes the test inefficient and potentially flaky.

Incorrect cancel(): The cancel() call on line 442 is ineffective. The context it cancels was for the unary Reboot RPC, which has already completed. To fail the test if the boot time is incorrect, t.Fatalf should be used. The waitForReboot function already contains this logic.

Unnecessary Complexity: The function re-implements logic to wait for the DUT to become reachable, which is already handled correctly by the waitForReboot function. The rebootDUT function's responsibility should be to only send the reboot command.

Please refactor this to remove the time.Sleep and the manual reachability check, and instead call tc.waitForReboot(t, bootTimeBeforeReboot). Also, consider changing t.Fatalf on the Reboot RPC error to t.Logf, as a transport error is expected when the device reboots.

Suggested change

if _, err = gnoiClient.System().Reboot(ctxWithTimeout, rebootRequest); err != nil {

t.Fatalf("Failed to reboot chassis with unexpected err: %v", err)

}

startReboot := time.Now()

t.Logf("Reboot request sent at %v", startReboot)

// Wait for the device to become reachable again.

// tc.waitForReboot(t, bootTimeBeforeReboot)

time.Sleep(10 * time.Minute)

gnoiClient, err = tc.dut.RawAPIs().BindingDUT().DialGNOI(t.Context())

if err != nil {

t.Fatalf("Error dialing gNOI: %v", err)

}

_, ok := gnmi.Watch(t, tc.dut, gnmi.OC().System().UpTime().State(), maxRebootTime, func(val *ygnmi.Value[uint64]) bool {

_, ok := val.Val()

return ok

}).Await(t)

if !ok {

t.Fatalf("Timeout exceeded: DUT did not reboot within %v", maxRebootTime)

}

bootTimeAfterReboot := gnmi.Get(t, tc.dut, gnmi.OC().System().BootTime().State())

t.Logf("DUT boot time after reboot: %v", bootTimeAfterReboot)

t.Logf("Device boot time after reboot: %.2f seconds.", time.Since(startReboot).Seconds())

if bootTimeAfterReboot < bootTimeBeforeReboot {

cancel()

}

if _, err = gnoiClient.System().Reboot(ctxWithTimeout, rebootRequest); err != nil {

// This error is expected because the connection is broken on reboot.

t.Logf("Reboot request failed as expected, or with an unexpected error: %v", err)

}

// Wait for the device to become reachable again and for the boot time to update.

tc.waitForReboot(t, bootTimeBeforeReboot)

gemini-code-assist · 2026-01-05T09:17:03Z

feature/afts/otg_tests/afts_reboot/afts_reboot_test.go

+func (tc *testCase) configureToStoreRunninggNMIConfig(t *testing.T) error {
+	hwProfileConfig := map[enpb.VendorId]string{
+		enpb.VendorId_V_ARISTA: "management api gnmi \n transport grpc default \n operation set persistence \n",
 	}
-	statement.GetOrCreateActions().PolicyResult = applyPolicyType
-	gnmi.Update(t, dut, gnmi.OC().RoutingPolicy().Config(), routePolicy)
+	tc.dut.Config().New().
+		WithAristaText(hwProfileConfig[enpb.VendorId_V_ARISTA]).
+		WithCiscoText(hwProfileConfig[enpb.VendorId_V_CISCOXR]).
+		WithJuniperText(hwProfileConfig[enpb.VendorId_V_JUNIPER]).
+		Append(t)
+	t.Logf("hwProfileConfig: %v added \n", hwProfileConfig[enpb.VendorId_V_ARISTA])
 	return nil
 }


The function configureToStoreRunninggNMIConfig is defined but it is not called anywhere in the test. This seems to be an oversight. If this configuration is necessary for the test to work correctly after a reboot, it should be called, probably at the beginning of configureDUT.

Additionally, the hwProfileConfig map only contains a configuration for Arista. For other vendors like Cisco and Juniper, it will pass an empty string. If this is intentional because no configuration is needed for them, please add a comment to clarify. If configurations are pending, a TODO comment would be helpful.
Example:

hwProfileConfig := map[enpb.VendorId]string{ enpb.VendorId_V_ARISTA: "management api gnmi \n transport grpc default \n operation set persistence \n", // enpb.VendorId_V_CISCOXR: "", // No equivalent config needed. // enpb.VendorId_V_JUNIPER: "", // No equivalent config needed. }

feature/afts/otg_tests/afts_reboot/afts_reboot_test.go

coveralls · 2026-01-05T09:18:26Z

Pull Request Test Coverage Report for Build 20711325825

Details

0 of 1 (0.0%) changed or added relevant line in 1 file are covered.
No unchanged relevant lines lost coverage.
Overall coverage remained the same at 10.023%

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

Totals
Change from base Build 20704703562:	0.0%
Covered Lines:	2227
Relevant Lines:	22219

💛 - Coveralls

typo

OpenConfigBot · 2026-01-05T09:43:42Z

Pull Request Functional Test Report for #4969 / `f3e03a6`

Virtual Devices

Device	Test Documentation	Job	Raw Log
Arista cEOS	AFT-5.1: AFTs DUT Reboot	7222f743	Log
Cisco 8000E	AFT-5.1: AFTs DUT Reboot	44a41c57	Log
Cisco XRd	AFT-5.1: AFTs DUT Reboot	9fba04b2	Log
Juniper ncPTX	AFT-5.1: AFTs DUT Reboot	d9eaba77	Log
Nokia SR Linux	AFT-5.1: AFTs DUT Reboot	a54671dd	Log
Openconfig Lemming	AFT-5.1: AFTs DUT Reboot	1582b8e5	Log

Hardware Devices

Device	Test	Test Documentation	Raw Log
Arista 7808		AFT-5.1: AFTs DUT Reboot
Cisco 8808		AFT-5.1: AFTs DUT Reboot
Juniper PTX10008		AFT-5.1: AFTs DUT Reboot
Nokia 7250 IXR-10e		AFT-5.1: AFTs DUT Reboot

Help

ElodinLaarz

I think most of these changes should not happen. Can you try to make the changes in the cfgplugin library if the test has issues with ISIS / BGP?

ElodinLaarz · 2026-01-05T21:28:06Z

feature/afts/otg_tests/afts_reboot/afts_reboot_test.go

+	if deviations.MultipathUnsupportedNeighborOrAfisafi(dut) {
+		peerGroupV4.GetOrCreateUseMultiplePaths().SetEnabled(true)
+		peerGroupV6.GetOrCreateUseMultiplePaths().SetEnabled(true)
+	} else {
+		afiSAFI.GetOrCreateUseMultiplePaths().GetOrCreateEbgp().SetMaximumPaths(2)
+		asisafi6.GetOrCreateUseMultiplePaths().GetOrCreateEbgp().SetMaximumPaths(2)
+		peerGroupV4AfiSafi.GetOrCreateUseMultiplePaths().SetEnabled(true)
+		peerGroupV6AfiSafi.GetOrCreateUseMultiplePaths().SetEnabled(true)
+	}


Handle the deviation in its own function and handle all vendors, explicitly. (I can't tell what vendors should have this value set or not as-is.)

sure. will modify.

ElodinLaarz · 2026-01-05T21:46:06Z

feature/afts/otg_tests/afts_reboot/afts_reboot_test.go

 	"github.com/openconfig/featureprofiles/internal/cfgplugins"
 	"github.com/openconfig/featureprofiles/internal/deviations"
 	"github.com/openconfig/featureprofiles/internal/fptest"
+	"github.com/openconfig/featureprofiles/internal/isissession"


This package is deprecated. You should not be using it in new changes.

ElodinLaarz · 2026-01-05T21:46:59Z

feature/afts/otg_tests/afts_reboot/afts_reboot_test.go

+		global := isis.GetOrCreateGlobal()
+		global.HelloPadding = oc.Isis_HelloPaddingType_DISABLE
+
+		if deviations.ISISSingleTopologyRequired(ts.DUT) {


Same comment here about properly handling deviations. They should be handled explicitly in their own functions and we should be able to see which vendor(s) are expected to have the value set.

modify accordingly.

feature/afts/otg_tests/afts_reboot/afts_reboot_test.go

ElodinLaarz · 2026-01-05T21:48:47Z

feature/afts/otg_tests/afts_reboot/afts_reboot_test.go

 }

-// configureDUT configures all the interfaces, BGP, and ISIS on the DUT.
+func createBGPNeighbor(peerGrpNameV4, peerGrpNameV6 string, nbrs []*BGPNeighbor, dut *ondatra.DUTDevice) *oc.NetworkInstance_Protocol {


Don't in-line the BGP configuration-- please use / update the cfgplugins library so the config can be used across tests.

ElodinLaarz · 2026-01-08T17:32:01Z

I don't think you uploaded any changes you made? Could you commit and upload before I can review it again?

reboot changes

4baaecd

paramasivamn1 assigned ElodinLaarz and singhavnish2516 Jan 5, 2026

paramasivamn1 requested review from a team as code owners January 5, 2026 09:12

gemini-code-assist bot reviewed Jan 5, 2026

View reviewed changes

Update afts_reboot_test.go

f3e03a6

typo

ElodinLaarz requested changes Jan 5, 2026

View reviewed changes

AFTS reboot changes #4969

Are you sure you want to change the base?

AFTS reboot changes #4969

Uh oh!

Conversation

paramasivamn1 commented Jan 5, 2026

Uh oh!

gemini-code-assist bot commented Jan 5, 2026

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!

gemini-code-assist bot Jan 5, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Jan 5, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

coveralls commented Jan 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 20711325825

Details

💛 - Coveralls

Uh oh!

OpenConfigBot commented Jan 5, 2026

Pull Request Functional Test Report for #4969 / f3e03a6

Virtual Devices

Hardware Devices

Uh oh!

ElodinLaarz left a comment

Choose a reason for hiding this comment

Uh oh!

ElodinLaarz Jan 5, 2026

Choose a reason for hiding this comment

Uh oh!

paramasivamn1 Jan 7, 2026

Choose a reason for hiding this comment

Uh oh!

ElodinLaarz Jan 5, 2026

Choose a reason for hiding this comment

Uh oh!

paramasivamn1 Jan 7, 2026

Choose a reason for hiding this comment

Uh oh!

ElodinLaarz Jan 5, 2026

Choose a reason for hiding this comment

Uh oh!

paramasivamn1 Jan 7, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ElodinLaarz Jan 5, 2026

Choose a reason for hiding this comment

Uh oh!

ElodinLaarz commented Jan 8, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

coveralls commented Jan 5, 2026 •

edited

Loading

Pull Request Functional Test Report for #4969 / `f3e03a6`