Make LRP restore test logic robust and optimized

[aditighag]

The goal of the test is to check if curl to a clusterIP svc endpoint is redirected to both the backends when the original svc entry is restored upon LRP removal. The current test logic expects the same backend should be selected for all the pod clients simultaneously, and this can lengthen test duration. This doesn't seem right since backend selection is not exactly deterministic. More importantly, we only need both backends to be selected at least once for all the client pods.

Flip the order in which we loop over backends and client pods. Loop over client pods first, and then making curl calls to until we hit both the backends on each of the client pods. Also, keep state about which backends have been successfully tested in order to avoid making some of the duplicate curl calls, and make the test logic deterministic.

More details - #16154 (comment).

Deferred to a follow-up PR - Looking at the LRP test case where we check if traffic only goes to the local backend, it doesn't seem reliable since it's possible that the curl request that the test made wasn't redirected to the remote backend by chance. I think the reliable way to validate the correctness is to check for a LocalRedirect service entry and its corresponding backends in the cilium service list .

Fixes: #16154

[aditighag]

test-only --focus="K8sServicesTest.* LRP" --kernel_version="net-next"

[aditighag]

LRP focused tests passed. Marking it as ready for review. /cc @Weil0ng

[aanm]

marking for backport as it failed in one backport PR #16210

[Weil0ng]

LGTM! Thanks for the fix, I think this is the correct test logic as you described.

[nbusseneau]

As discussed in the community meeting, rationale looks sensible to me. Thanks!

[aditighag]

@joamaki Thanks for the review, PTAL.

[aditighag]

test-only --focus="K8sServicesTest.* LRP" --kernel_version="net-next"

[aditighag]

Test failures -

1.16-netnext : https://jenkins.cilium.io/job/Cilium-PR-K8s-1.16-net-next/576/console

Setting status of 07392becc9d9752966c6bbf97d50391be02efea3 to FAILURE with url https://jenkins.cilium.io/job/Cilium-PR-K8s-1.16-net-next/576/ and message: 'Build finished. '
Using context: k8s-1.16-kernel-netnext (test-1.16-netnext)
Global Slack Notifier try posting to slack. However some error occurred
TeamDomain :cilium
Channel :#testing
Message :

java.net.SocketTimeoutException: Read timed out
at java.net.SocketInputStream.socketRead0(Native Method)
at java.net.SocketInputStream.socketRead(SocketInputStream.java:116)
at java.net.SocketInputStream.read(SocketInputStream.java:171)
at java.net.SocketInputStream.read(SocketInputStream.java:141)
at sun.security.ssl.InputRecord.readFully(InputRecord.java:465)
at sun.security.ssl.InputRecord.read(InputRecord.java:503)

1.20-net-next failure looks legit. Marking the PR as draft for now.

[aditighag]

Focused test runs passed - https://jenkins.cilium.io/job/Cilium-PR-Tests-Kernel-Focus/219/testReport/Suite-k8s-1/20/.

[pchaigno]

I'll push a new commit to address Paul's comment about incorrect "Fixes" commit SHA mentioned in the 1st commit.

According to the logs, you also rebased, so let's retrigger the e2e tests 😞

[pchaigno]

test-gke

[pchaigno]

test-1.20-4.19

[pchaigno]

test-1.16-netnext

[aditighag]

I'll push a new commit to address Paul's comment about incorrect "Fixes" commit SHA mentioned in the 1st commit.

According to the logs, you also rebased, so let's retrigger the e2e tests 😞

@pchaigno Why do we need to trigger the e2e tests? The changes are localized to only LRP test cases, and there were no merge conflicts.

[pchaigno]

Because other changes included in the rebase could cause the test to fail (e.g., changes to the agent). The only case where we can merge without restarting tests is when the diff doesn't include any changes that is tested (e.g., only code comment or commit description changes). That's not the case with a rebase.

[aditighag]

Relevant tests have passed. Marking it as ready for merge again.