Skip to content

test: Fix flaky network test for service endpoint updates #131372

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

Jump to bottom
Closed
wants to merge 2 commits into from
Closed

test: Fix flaky network test for service endpoint updates #131372

wants to merge 2 commits into from
+128 −1

Conversation

aryasoni98
Copy link

The test 'Networking Granular Checks: Services should update endpoints: http' was failing intermittently due to a race condition between pod deletion and endpoint updates. This change adds explicit wait for endpoint updates after pod deletion, improves error logging for better debugging, sets appropriate timeout for endpoint update checks, and provides better status reporting during the test.

Fixes #131370

What type of PR is this?

/kind bug
/kind flake
/kind failing-test

What this PR does / why we need it:

This PR fixes a flaky test in the networking e2e test suite. The test was failing intermittently due to a race condition where it would check for endpoint updates too quickly after pod deletion, before the endpoint controller had time to update the endpoints.

Changes made:

  • Added explicit wait condition using wait.PollWithContext after pod deletion
  • Added proper error logging to help diagnose issues
  • Set a reasonable timeout (30 seconds) for endpoint updates
  • Added status reporting during the endpoint update check
  • Improved error messages for debugging failures

Which issue(s) this PR fixes:

Fixes #131370

Special notes for your reviewer:

The main change is the addition of a wait condition after pod deletion. This ensures that we properly wait for the endpoint controller to update the endpoints before proceeding with the test. The wait condition checks every second for up to 30 seconds, which should be more than enough time for the endpoint controller to process the pod deletion and update the endpoints.

The error logging has also been improved to make it easier to diagnose any future issues. If the endpoints don't match the expected state, the logs will now show both the current and expected endpoint lists.

Does this PR introduce a user-facing change?

NONE

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

This change follows the best practices for e2e test reliability as documented in:
- [Test Guidelines]: https://git.k8s.io/community/contributors/devel/sig-testing/testing.md
- [Flaky Tests]: https://git.k8s.io/community/contributors/devel/sig-testing/flaky-tests.md


Let me know if you'd like me to:
1. Modify any part of the PR description
2. Add more details about the changes
3. Include additional documentation links
4. Help with anything else related to the PR

@aryasoni98
test: Fix flaky network test for service endpoint updates
acd2314 
The test 'Networking Granular Checks: Services should update endpoints: http' was failing intermittently due to a race condition between pod deletion and endpoint updates. This change adds explicit wait for endpoint updates after pod deletion, improves error logging for better debugging, sets appropriate timeout for endpoint update checks, and provides better status reporting during the test.

Fixes kubernetes#131370
@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. do-not-merge/invalid-commit-message Indicates that a PR should not merge because it has an invalid commit message. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Apr 18, 2025
@k8s-ci-robot
Copy link
Contributor

Please note that we're already in Test Freeze for the release-1.33 branch. This means every merged PR will be automatically fast-forwarded via the periodic ci-fast-forward job to the release branch of the upcoming v1.33.0 release.

Fast forwards are scheduled to happen every 6 hours, whereas the most recent run was: Fri Apr 18 13:34:58 UTC 2025.

@k8s-ci-robot k8s-ci-robot added kind/bug Categorizes issue or PR as related to a bug. kind/flake Categorizes issue or PR as related to a flaky test. kind/failing-test Categorizes issue or PR as related to a consistently or frequently failing test. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Apr 18, 2025
@k8s-ci-robot
Copy link
Contributor

This issue is currently awaiting triage.

If a SIG or subproject determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@k8s-ci-robot k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Apr 18, 2025
@k8s-ci-robot
Copy link
Contributor

Hi @aryasoni98. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@k8s-ci-robot k8s-ci-robot added the needs-priority Indicates a PR lacks a `priority/foo` label and requires one. label Apr 18, 2025
@k8s-ci-robot k8s-ci-robot requested review from bowei and thockin April 18, 2025 17:41
@k8s-ci-robot k8s-ci-robot added area/test sig/network Categorizes an issue or PR as relevant to SIG Network. sig/testing Categorizes an issue or PR as relevant to SIG Testing. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Apr 18, 2025
@aryasoni98
kubelet: log startup probe failures as Normal events
8eeafb2 
Startup probe failures during container startup are expected and should be logged as Normal events rather than Warning events. This is because these failures are part of the normal container startup process and do not indicate a problem that requires operator attention.

This change modifies the prober to: - Log startup probe failures as Normal events - Keep liveness and readiness probe failures as Warning events - Add unit tests to verify the event type behavior

Fixes kubernetes#131370

Signed-off-by: Arya Soni <[email protected]>
@k8s-ci-robot
Copy link
Contributor

Keywords which can automatically close issues and at(@) or hashtag(#) mentions are not allowed in commit messages.

The list of commits with invalid commit messages:

  • acd2314 test: Fix flaky network test for service endpoint updates
  • 8eeafb2 kubelet: log startup probe failures as Normal events

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. area/kubelet sig/node Categorizes an issue or PR as relevant to SIG Node. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Apr 18, 2025
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: aryasoni98
Once this PR has been reviewed and has the lgtm label, please assign thockin, yujuhong for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

danwinship
danwinship reviewed Apr 21, 2025
View reviewed changes
pkg/kubelet/prober/prober.go
@@ -118,7 +118,12 @@ func (pb *prober) probe(ctx context.Context, probeType probeType, pod *v1.Pod, s

case probe.Failure:
klog.V(1).InfoS("Probe failed", "probeType", probeType, "pod", klog.KObj(pod), "podUID", pod.UID, "containerName", container.Name, "probeResult", result, "output", output)
pb.recordContainerEvent(pod, &container, v1.EventTypeWarning, events.ContainerUnhealthy, "%s probe failed: %s", probeType, output)
// For startup probes, failures during startup are expected and should be Normal events
Copy link
Contributor

Choose a reason for hiding this comment

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

Should they even be events at all?

You should split this patch out as its own PR/issue. (In particular, this needs a SIG Node approver rather than a SIG Network approver...)

test/e2e/network/networking.go
}); err != nil {
framework.Failf("failed waiting for endpoints to be updated: %v", err)
}

Copy link
Contributor

Choose a reason for hiding this comment

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

This is essentially just duplicating the code which follows it, with a different set of timeouts.

But looking at the test failures in the linked issue, this is not actually what is failing; there should be either 3 or 4 endpoints at this point (depending on whether the deleted pod has been recreated yet or not), but it's finding 1 endpoint, which isn't even a real endpoint that should exist. See the earlier issue, #123760 for more discussion.

@danwinship
Copy link
Contributor

/close
thanks for the PR, but this is not actually the cause of the problem with that test

@k8s-ci-robot
Copy link
Contributor

@danwinship: Closed this PR.

In response to this:

/close
thanks for the PR, but this is not actually the cause of the problem with that test

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@github-project-automation github-project-automation bot moved this from Triage to Done in SIG Node CI/Test Board Apr 21, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@danwinship danwinship danwinship left review comments

@bowei bowei Awaiting requested review from bowei

@thockin thockin Awaiting requested review from thockin

Assignees
No one assigned
Labels
area/kubelet area/test cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/invalid-commit-message Indicates that a PR should not merge because it has an invalid commit message. kind/bug Categorizes issue or PR as related to a bug. kind/failing-test Categorizes issue or PR as related to a consistently or frequently failing test. kind/flake Categorizes issue or PR as related to a flaky test. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. release-note-none Denotes a PR that doesn't merit a release note. sig/network Categorizes an issue or PR as relevant to SIG Network. sig/node Categorizes an issue or PR as relevant to SIG Node. sig/testing Categorizes an issue or PR as relevant to SIG Testing. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
SIG Node CI/Test Board
Status: Done
Milestone
No milestone
3 participants
@aryasoni98 @k8s-ci-robot @danwinship