Skip to content
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

fix flaky test on dra TestPrepareResources/should_timeout #119709

Merged
merged 1 commit into from
Aug 16, 2023
Merged

fix flaky test on dra TestPrepareResources/should_timeout #119709

merged 1 commit into from
Aug 16, 2023

Conversation

charles-chenzz
Copy link
Member

What type of PR is this?

/kind bug

What this PR does / why we need it:

fix flaky test on dra manager_test.go

Which issue(s) this PR fixes:

Fixes #119701

Special notes for your reviewer:

Does this PR introduce a user-facing change?

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

@k8s-ci-robot k8s-ci-robot added kind/bug Categorizes issue or PR as related to a bug. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Aug 1, 2023
@k8s-ci-robot
Copy link
Contributor

Please note that we're already in Test Freeze for the release-1.28 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.28.0 release.

Fast forwards are scheduled to happen every 6 hours, whereas the most recent run was: Tue Aug 1 10:18:23 UTC 2023.

@k8s-ci-robot k8s-ci-robot added do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Aug 1, 2023
@k8s-ci-robot k8s-ci-robot requested a review from dims August 1, 2023 15:00
@k8s-ci-robot k8s-ci-robot added do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. sig/node Categorizes an issue or PR as relevant to SIG Node. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Aug 1, 2023
@bart0sh bart0sh added this to Triage in SIG Node PR Triage Aug 1, 2023
@bart0sh
Copy link
Contributor

bart0sh commented Aug 2, 2023

/kind failing-test
/release-note-none
/priority important-soon
/triage accepted
/assign @pohly

@k8s-ci-robot k8s-ci-robot added the release-note-none Denotes a PR that doesn't merit a release note. label Aug 2, 2023
@k8s-ci-robot k8s-ci-robot added kind/failing-test Categorizes issue or PR as related to a consistently or frequently failing test. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. 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. labels Aug 2, 2023
@bart0sh bart0sh moved this from Triage to Needs Reviewer in SIG Node PR Triage Aug 2, 2023
@pohly
Copy link
Contributor

pohly commented Aug 2, 2023 •
edited

@bart0sh: you are better qualified to review this piece of code. Can you take this PR?

@charles-chenzz: I'll leave this to the final reviewer, but if I were to review, I would point out that your change comes with no explanation why it solves the problem and why this is the right way of fixing it.

/unassign

@bart0sh
Copy link
Contributor

bart0sh commented Aug 2, 2023

@pohly my bad. I thought it's fixing dra e2e tests.
/assign

@charles-chenzz I agree with @pohly. Can you provide an explanation for this PR?

@bart0sh bart0sh moved this from Needs Reviewer to Waiting on Author in SIG Node PR Triage Aug 2, 2023
@charles-chenzz
Copy link
Member Author

charles-chenzz commented Aug 2, 2023 •
edited

imo the reason why the test will flaky is because we set wantTimeOut and wantError and the same time and while the test hit the if-else block, the wantTimeOut will be catch by wantError and PrepareResource return nil that's what we don't want to see, it should be catch by if wantTimeout block

@ndixita ndixita moved this from Triage to PRs Waiting on Author in SIG Node CI/Test Board Aug 2, 2023
@charles-chenzz @TommyStarK
fix flaky test on dra TestPrepareResources/should_timeout
ba9ce3a 
Co-authored-by: TommyStarK <thomasmilox@gmail.com>
@TommyStarK
Copy link
Contributor

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 3, 2023
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: 2c6921d7e01ae35b9c25d869dfb8528c1db5418f

bart0sh
bart0sh reviewed Aug 4, 2023
View reviewed changes
Copy link
Contributor

@bart0sh bart0sh left a comment

Choose a reason for hiding this comment

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

/lgtm

@bart0sh bart0sh moved this from Waiting on Author to Needs Approver in SIG Node PR Triage Aug 4, 2023
@bart0sh
Copy link
Contributor

bart0sh commented Aug 4, 2023

/assign @klueska @pohly @mrunalp
For the final approval

pohly
pohly reviewed Aug 7, 2023
View reviewed changes
pkg/kubelet/cm/dra/manager_test.go
@@ -93,7 +93,7 @@ func setupFakeDRADriverGRPCServer(shouldTimeout bool) (string, tearDown, error)
driverName: driverName,
}
if shouldTimeout {
timeout := plugin.PluginClientTimeout + time.Millisecond
timeout := plugin.PluginClientTimeout + 10*time.Millisecond
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm wondering whether this is large enough for the CI. We'll see...

/approve

@pohly pohly mentioned this pull request Aug 15, 2023
Merged
@pohly
Copy link
Contributor

pohly commented Aug 16, 2023

/retest

@pohly
Copy link
Contributor

pohly commented Aug 16, 2023

@klueska: please approve. This test flake keeps coming up in different unrelated PRs.

@klueska
Copy link
Contributor

klueska commented Aug 16, 2023

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: charles-chenzz, klueska, pohly, TommyStarK

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

The pull request process is described 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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 16, 2023
@pohly
Copy link
Contributor

pohly commented Aug 16, 2023

/retest

@pohly
Copy link
Contributor

pohly commented Aug 16, 2023

#119670 might not have been merged yet, perhaps the OOMKiller flake is gone now.

@k8s-ci-robot k8s-ci-robot merged commit 419df23 into kubernetes:master Aug 16, 2023
14 checks passed
SIG Node CI/Test Board automation moved this from PRs Waiting on Author to Done Aug 16, 2023
SIG Node PR Triage automation moved this from Needs Approver to Done Aug 16, 2023
@k8s-ci-robot k8s-ci-robot added this to the v1.29 milestone Aug 16, 2023
@charles-chenzz charles-chenzz deleted the fix_flaky branch August 16, 2023 13:47
This was referenced Nov 3, 2023
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pohly pohly pohly left review comments

@bart0sh bart0sh bart0sh left review comments

@TommyStarK TommyStarK TommyStarK approved these changes

@dims dims Awaiting requested review from dims

@mtaufen mtaufen Awaiting requested review from mtaufen

Assignees

@pohly pohly

@mrunalp mrunalp

@klueska klueska

@TommyStarK TommyStarK

@bart0sh bart0sh

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/kubelet cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. 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. lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. release-note-none Denotes a PR that doesn't merit a release note. sig/node Categorizes an issue or PR as relevant to SIG Node. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
SIG Node CI/Test Board
Done
SIG Node CI/Test Board
Status: Done
SIG Node PR Triage
Archived in project
SIG Node PR Triage
Done
Milestone
v1.29
Development

Successfully merging this pull request may close these issues.

pkg/kubelet/cm/dra TestPrepareResources/should_timeout is flaking
7 participants
@charles-chenzz @k8s-ci-robot @bart0sh @pohly @TommyStarK @klueska @mrunalp