Skip to content

fix(e2e): handle neighbor entry GC race in bridge refresh test #339

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

Open
fedepaol wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix(e2e): handle neighbor entry GC race in bridge refresh test #339

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
37 changes: 34 additions & 3 deletions e2etests/tests/bridgerefresh.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ package tests

import (
"context"
"errors"
"fmt"
"strings"
"time"
Expand Down Expand Up @@ -295,6 +296,11 @@ var _ = Describe("BridgeRefresher E2E - Type 2 Route Persistence", Ordered, func
return checkType2RouteExists(cs, migratingPodIPOnly, vtepIPOnly, l2VNI)
}, 3*time.Minute, 5*time.Second).ShouldNot(HaveOccurred())

By("Verifying neighbor entry exists on the router before migration")
Eventually(func() error {
return checkNeighborExists(cs, migratingPodIPOnly, l2VNI, migratingPod.Spec.NodeName)
}, time.Minute, 2*time.Second).ShouldNot(HaveOccurred())
Copy link
Copy Markdown
Contributor

@maiqueb maiqueb Apr 27, 2026

Choose a reason for hiding this comment

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

isn't should succeed the same ? I think it reads better.


By("Deleting migrating pod (simulating pod eviction/migration)")
err = cs.CoreV1().Pods(testNamespace).Delete(
context.Background(),
Expand All @@ -316,7 +322,11 @@ var _ = Describe("BridgeRefresher E2E - Type 2 Route Persistence", Ordered, func
// This is important to trigger the deadlock situation described in https://github.com/FRRouting/frr/issues/14156
By("Waiting for neighbor entry to go STALE on the router on the same node")
Eventually(func() error {
return checkNeighborStale(cs, migratingPodIPOnly, l2VNI, migratingPod.Spec.NodeName)
err := checkNeighborStale(cs, migratingPodIPOnly, l2VNI, migratingPod.Spec.NodeName)
if errors.Is(err, ErrNoNeighborEntry) { // if the neighbor entry is expired no need to wait
return nil
}
return err
}, 3*time.Minute, 2*time.Second).ShouldNot(HaveOccurred())
Comment on lines +325 to 330
Copy link
Copy Markdown
Contributor

@maiqueb maiqueb Apr 27, 2026

Choose a reason for hiding this comment

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

ok, got it.

can't we move this logic to the matcher instead ? like, the error should either: not have occurred, or match ErrNoNeighborEntry ?

I think it would read better.


By("Recreating migrating pod on node B (the other node) with same IP")
Expand Down Expand Up @@ -370,6 +380,28 @@ func checkType2RouteExists(cs clientset.Interface, podIP, vtepIP string, vni int
return nil
}

var ErrNoNeighborEntry = errors.New("no neighbor entry")
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot Apr 24, 2026

Choose a reason for hiding this comment

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

medium

The exported variable ErrNoNeighborEntry should be moved to the top of the file, following the repository's newspaper code structure guidelines (line 33). This ensures that exported symbols are easily discoverable at the beginning of the file.

References
  1. Exported methods and types go at the top of the file (link)


func checkNeighborExists(cs clientset.Interface, podIP string, vni int, nodeName string) error {
bridgeDev := fmt.Sprintf("br-pe-%d", vni)
currentRouters, err := openperouter.Get(cs, HostMode)
if err != nil {
return err
}
exec, err := openperouter.ExecutorForNode(currentRouters, nodeName)
if err != nil {
return err
}
out, err := exec.Exec("ip", "neigh", "show", podIP, "dev", bridgeDev)
if err != nil {
return fmt.Errorf("failed to check neighbor on router %s: %w", exec.Name(), err)
}
if strings.TrimSpace(out) == "" {
return fmt.Errorf("no neighbor entry for %s on %s in router %s", podIP, bridgeDev, exec.Name())
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot Apr 24, 2026

Choose a reason for hiding this comment

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

medium

For consistency with checkNeighborStale, consider using the ErrNoNeighborEntry sentinel error here as well.

Suggested change
return fmt.Errorf("no neighbor entry for %s on %s in router %s", podIP, bridgeDev, exec.Name())
return fmt.Errorf("%w for %s on %s in router %s", ErrNoNeighborEntry, podIP, bridgeDev, exec.Name())

Copy link
Copy Markdown
Contributor

@maiqueb maiqueb Apr 27, 2026

Choose a reason for hiding this comment

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

this could be interesting.

}
return nil
}

func checkNeighborStale(cs clientset.Interface, podIP string, vni int, nodeName string) error {
bridgeDev := fmt.Sprintf("br-pe-%d", vni)
currentRouters, err := openperouter.Get(cs, HostMode)
Expand All @@ -386,11 +418,10 @@ func checkNeighborStale(cs clientset.Interface, podIP string, vni int, nodeName
}
out = strings.TrimSpace(out)
if out == "" {
return fmt.Errorf("no neighbor entry for %s on %s in router %s", podIP, bridgeDev, exec.Name())
return fmt.Errorf("%w for %s on %s in router %s", ErrNoNeighborEntry, podIP, bridgeDev, exec.Name())
Copy link
Copy Markdown
Contributor

@maiqueb maiqueb Apr 27, 2026

Choose a reason for hiding this comment

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

ditto

}
if strings.Contains(out, "STALE") || strings.Contains(out, "FAILED") {
return nil
}
return fmt.Errorf("neighbor %s on %s in router %s is not STALE yet: %s", podIP, bridgeDev, exec.Name(), out)
}

Loading