bgpv1: Retry peer checks in NeighborAddDel test to avoid flakes

[rastislavs]

Fixes flakes due to potential timing issue in the Test_NeighborAddDel test.

In the test we wait for peers to transition into their expected state. However, remote peer's session state does not have to immediately match our session's state (e.g. peer may be already in Established but we still in OpenConfirm until we receive a Keepalive from the peer), so we should retry the check with a timeout if our state of the session does not immediately match.

Fixes: #25637

[harsimran-pabla]

Thank you!

[rastislavs]

/test

Job 'Cilium-PR-K8s-1.26-kernel-net-next' failed:

Click to show.

Test Name

K8sDatapathServicesTest Checks N/S loadbalancing With host policy Tests NodePort

Failure Output

FAIL: Can not connect to service "http://192.168.56.11:32520" from outside cluster (1/10)

Jenkins URL: https://jenkins.cilium.io/job/Cilium-PR-K8s-1.26-kernel-net-next/137/

If it is a flake and a GitHub issue doesn't already exist to track it, comment /mlh new-flake Cilium-PR-K8s-1.26-kernel-net-next so I can create one.

Then please upload the Jenkins artifacts to that issue.

[ldelossa]

Just out of curiosity, if we suspect a flake, was this test ran over multiple trials to see if the flake was removed?

[rastislavs]

@ldelossa

Just out of curiosity, if we suspect a flake, was this test ran over multiple trials to see if the flake was removed?

This is a bit tricky, as I could not reproduce the flake when running it locally (not before nor after fix). Somehow it is reproducible only in CI - we can re-run the CI test multiple times before merging.

[ldelossa]

yeah, can totally understand that, @rastislavs - rerunning a few times in CI sounds like a good idea to me, if it doesn't spam our CI. May want to task in public testing channel whether we have a precedent for this. Really not too sure.

[rastislavs]

The test-1.26-net-next job is failing due to #25605. All other are green.

It seems that the test-runtime job that was flaking due to this issue has been pretty successful in the past runs, so not sure if re-running it once or twice would even prove anything. Maybe we should just merge this and observe it over a larger time period.

[harsimran-pabla]

It seems that the test-runtime job that was flaking due to this issue has been pretty successful in the past runs, so not sure if re-running it once or twice would even prove anything. Maybe we should just merge this and observe it over a larger time period.

Locally I was able to reproduce same failure few time running like this
PRIVILEGED_TESTS=1 go test -exec 'sudo -E' ./... -count 10 -run Test_NeighborAddDel from bgpv1/test directory.

You can try to validate if this change fixes it.

[rastislavs]

Locally I was able to reproduce same failure few time running like this PRIVILEGED_TESTS=1 go test -exec 'sudo -E' ./... -count 10 -run Test_NeighborAddDel from bgpv1/test directory.

You can try to validate if this change fixes it.

@harsimran-pabla Thanks for the tip, I was able to reproduce the issue (without the fix) with some more iterations and it seems that the fix indeed helped as I could not reproduce it with 10x more iterations with the fix.

[rastislavs]

As I was able to verify the fix locally and the test-1.26-net-next job is failing due to #25605, I think this is good to go, marking as ready-to-merge.