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

skip the reason check for OOM reason test if it will fail #120460

Merged
merged 1 commit into from
Oct 10, 2023
Merged

skip the reason check for OOM reason test if it will fail #120460

merged 1 commit into from
Oct 10, 2023

Conversation

tzneal
Copy link
Contributor

@tzneal tzneal commented Sep 6, 2023 •
edited
Loading

What type of PR is this?

/kind flake

What this PR does / why we need it:

This is currently flaking for both containerd/crio due to a race in cgroup deletion and reading the memory.events file were the OOMKill is recorded. Skip the part that fails for now until the issue is resolved.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?

NONE

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

@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. kind/flake Categorizes issue or PR as related to a flaky 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 Sep 6, 2023
@tzneal
Copy link
Contributor Author

tzneal commented Sep 6, 2023

Related to #119600

@k8s-ci-robot k8s-ci-robot added the needs-priority Indicates a PR lacks a `priority/foo` label and requires one. label Sep 6, 2023
@tzneal
Copy link
Contributor Author

tzneal commented Sep 6, 2023

/cc pacoxu

@k8s-ci-robot k8s-ci-robot requested a review from pacoxu September 6, 2023 13:56
@k8s-ci-robot k8s-ci-robot added area/test sig/node Categorizes an issue or PR as relevant to SIG Node. 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 Sep 6, 2023
@tzneal tzneal force-pushed the deflake-oom-tests-on-containerd branch from 774e732 to 75017e2 Compare September 6, 2023 14:31
@tzneal
Copy link
Contributor Author

tzneal commented Sep 6, 2023

/retest

@SergeyKanzhelev
Copy link
Member

Discussing a SIG Node CI meeting. This will fail on CRI-O as well. SO we may be better off by skipping the test altogether.

@SergeyKanzhelev
Copy link
Member

Link from @haircommander: opencontainers/runc#3932

@SergeyKanzhelev SergeyKanzhelev mentioned this pull request Sep 6, 2023
Open
@haircommander
Copy link
Contributor

another xref cri-o/cri-o#7035

@tzneal
skip the reason check for OOM tests if it will fail
94afd6e 
This is currently flaking badly due to a race between cgroup deletion
and the runtime detecting the OOM kill.
@tzneal tzneal force-pushed the deflake-oom-tests-on-containerd branch from 75017e2 to 94afd6e Compare September 6, 2023 17:23
@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Sep 6, 2023
@tzneal tzneal changed the title skip the reason check for OOM tests on containerd skip the reason check for OOM reason test if it will fail Sep 6, 2023
@pacoxu
Copy link
Member

pacoxu commented Sep 7, 2023

/priority important-soon
as it flakes a lot, or important-longterm
/triage accepted

@k8s-ci-robot k8s-ci-robot added 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 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 Sep 7, 2023
@pacoxu
Copy link
Member

pacoxu commented Sep 12, 2023 •
edited
Loading

Discussing a SIG Node CI meeting. This will fail on CRI-O as well. SO we may be better off by skipping the test altogether.

IIRC, can we add [Flaky] to this e2e test in such a case?

@pacoxu pacoxu mentioned this pull request Sep 12, 2023
Closed
@tzneal
Copy link
Contributor Author

tzneal commented Sep 12, 2023

IIRC, can we add [Flaky] to this e2e test in such a case?

I could be convinced either way, but I prefer this method as it still tests that the termination occurs for the single & multi-process cases and just skips the flaking bit instead of the entire test.

@kannon92 kannon92 mentioned this pull request Oct 6, 2023
Closed
@kannon92
Copy link
Contributor

kannon92 commented Oct 6, 2023

Opened up #121031 to help resolve these failures.

kannon92
kannon92 reviewed Oct 10, 2023
View reviewed changes
Copy link
Contributor

@kannon92 kannon92 left a comment

Choose a reason for hiding this comment

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

/lgtm

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

LGTM label has been added.

Git tree hash: a7fffa16fb4e562956ce97366341b49fa3767b1c

@kannon92
Copy link
Contributor

/assign @SergeyKanzhelev

SergeyKanzhelev
SergeyKanzhelev approved these changes Oct 10, 2023
View reviewed changes
Copy link
Member

@SergeyKanzhelev SergeyKanzhelev left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: SergeyKanzhelev, tzneal

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 Oct 10, 2023
@k8s-ci-robot k8s-ci-robot merged commit bdcb73d into kubernetes:master Oct 10, 2023
@k8s-ci-robot k8s-ci-robot added this to the v1.29 milestone Oct 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kannon92 kannon92 kannon92 left review comments

@SergeyKanzhelev SergeyKanzhelev SergeyKanzhelev approved these changes

@pacoxu pacoxu Awaiting requested review from pacoxu

@andrewsykim andrewsykim Awaiting requested review from andrewsykim

@bart0sh bart0sh Awaiting requested review from bart0sh

Assignees

@kannon92 kannon92

@SergeyKanzhelev SergeyKanzhelev

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/test cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/flake Categorizes issue or PR as related to a flaky 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. sig/testing Categorizes an issue or PR as relevant to SIG Testing. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
SIG Node: code and documentation PRs
Archived in project
Milestone
v1.29
Development

Successfully merging this pull request may close these issues.
6 participants
@tzneal @SergeyKanzhelev @haircommander @pacoxu @kannon92 @k8s-ci-robot