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

sysctl-test: use status reason check instead of events check #103906

Merged
merged 1 commit into from
Sep 21, 2021
Merged

sysctl-test: use status reason check instead of events check #103906

merged 1 commit into from
Sep 21, 2021

Conversation

pacoxu
Copy link
Member

@pacoxu pacoxu commented Jul 26, 2021 •
edited
Loading

/kind bug
/sig node
/cc ehashman @mmiranda96 SergeyKanzhelev

follow up of #101190

  1. unsafe sysctl should not be conformance testing: this is done in Demote sysctl test from its conformance status #103827
  2. unsafe sysctl test case is testing safe sysctl which is not correct and dup with the case below. So I remove it in this PR.
  3. Currently should forbid unsafe sysctls is using event check, it should use pod/container status check instead.
NONE

@k8s-ci-robot k8s-ci-robot added the release-note-none Denotes a PR that doesn't merit a release note. label Jul 26, 2021
@k8s-ci-robot k8s-ci-robot requested a review from ehashman July 26, 2021 06:30
@k8s-ci-robot k8s-ci-robot added kind/bug Categorizes issue or PR as related to a bug. sig/node Categorizes an issue or PR as relevant to SIG Node. labels Jul 26, 2021
@k8s-ci-robot
Copy link
Contributor

@pacoxu: GitHub didn't allow me to request PR reviews from the following users: mmiranda96.

Note that only kubernetes members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

/kind bug
/sig node
/cc ehashman mmiranda96 SergeyKanzhelev

follow up of #101190

  1. unsafe sysctl should not be conformance testing: this is done in Demote sysctl test from its conformance status #103827
  2. unsafe sysctl test case is testing safe sysctl which is not correct and dup with the case below. So I remove it in this PR.
  3. Currently should forbid unsafe sysctls is using event check, it should use pod/container status check instead.
NONE

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/test-infra repository.

@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. 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. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. area/test sig/testing Categorizes an issue or PR as relevant to SIG Testing. labels Jul 26, 2021
@pacoxu pacoxu mentioned this pull request Jul 26, 2021
Closed
@SergeyKanzhelev
Copy link
Member

Currently should forbid unsafe sysctls is using event check, it should use pod/container status check instead.

Is the PR WIP or ready to be looked at?

Thank you much for the follow up!

@mmiranda96 mmiranda96 mentioned this pull request Jul 28, 2021
Closed
@ehashman
Copy link
Member

ehashman commented Aug 2, 2021

/triage accepted
/priority important-longterm

@k8s-ci-robot k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. and removed 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. labels Aug 2, 2021
@pacoxu
Copy link
Member Author

pacoxu commented Aug 3, 2021

Currently should forbid unsafe sysctls is using event check, it should use pod/container status check instead.

Is the PR WIP or ready to be looked at?

Yes. I think it's ready.

matthyx
matthyx approved these changes Aug 4, 2021
View reviewed changes
Copy link
Contributor

@matthyx matthyx left a comment

Choose a reason for hiding this comment

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

/lgtm

test/e2e/common/node/sysctl.go Outdated
})
})

// waitForPodFailedReason wait for pod failed reason in status, for example "SysctlForbidden".
func waitForPodFailedReason(f *framework.Framework, pod *v1.Pod, reason string) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I think we should have this sort of helper func somewhere for DRY

Copy link
Member Author

@pacoxu pacoxu Aug 16, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Done. Move the func to test/e2e/framework/pod/wait.go and solve the import cycle.

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 4, 2021
@k8s-ci-robot k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Aug 18, 2021
@k8s-ci-robot k8s-ci-robot added area/conformance Issues or PRs related to kubernetes conformance tests sig/architecture Categorizes an issue or PR as relevant to SIG Architecture. labels Aug 18, 2021
@pacoxu
correct unsafe sysctls e2e test case & use status reason check instea…
ddaa346 
…d of events watch

Signed-off-by: Paco Xu <paco.xu@daocloud.io>
@pacoxu
Copy link
Member Author

pacoxu commented Aug 19, 2021

/assign @ehashman @fabriziopandini
/retest

@k8s-ci-robot
Copy link
Contributor

k8s-ci-robot commented Aug 19, 2021 •
edited
Loading

@pacoxu: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
pull-kubernetes-node-kubelet-serial-containerd ddaa346 link /test pull-kubernetes-node-kubelet-serial-containerd
pull-kubernetes-node-kubelet-serial ddaa346 link /test pull-kubernetes-node-kubelet-serial

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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/test-infra repository. I understand the commands that are listed here.

@pacoxu
Copy link
Member Author

pacoxu commented Aug 24, 2021 •
edited
Loading

https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/104535/pull-kubernetes-e2e-kind/1430104056465985536

Kubernetes e2e suite: [sig-node] Sysctls [LinuxOnly] [NodeConformance] should support unsafe sysctls which are actually allowed [MinimumKubeletVersion:1.21] expand_less | 5m3s
Aug 24 10:17:58.776: Unexpected error:
    <*errors.errorString | 0xc0001ce240>: {
        s: "timed out waiting for the condition",
    }
    timed out waiting for the condition
occurred
/home/prow/go/src/k8s.io/kubernetes/_output/local/go/src/k8s.io/kubernetes/test/e2e/common/node/sysctl.go:141

This would be a flake that is caused by the event waiting.

@matthyx
Copy link
Contributor

matthyx commented Sep 15, 2021

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 15, 2021
SergeyKanzhelev
SergeyKanzhelev approved these changes Sep 16, 2021
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.

again, thank you for the follow up

/lgtm
/approve
/skip

@pacoxu
Copy link
Member Author

pacoxu commented Sep 16, 2021

/assign fabriziopandini neolit123

@neolit123
Copy link
Member

i don't have much context on the sysctl / node/common change, but the addition in the framework looks sane.

/approve
given @mrunalp approved from Node.

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: matthyx, mrunalp, neolit123, pacoxu, SergeyKanzhelev

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 Sep 21, 2021
@k8s-ci-robot k8s-ci-robot merged commit 92ddd4d into kubernetes:master Sep 21, 2021
@k8s-ci-robot k8s-ci-robot added this to the v1.23 milestone Sep 21, 2021
@pacoxu pacoxu deleted the unsafe-e2e-test branch May 10, 2022 06:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@matthyx matthyx matthyx approved these changes

@SergeyKanzhelev SergeyKanzhelev SergeyKanzhelev approved these changes

@mrunalp mrunalp mrunalp approved these changes

@ehashman ehashman Awaiting requested review from ehashman

Assignees

@neolit123 neolit123

@fabriziopandini fabriziopandini

@SergeyKanzhelev SergeyKanzhelev

@matthyx matthyx

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/conformance Issues or PRs related to kubernetes conformance tests area/e2e-test-framework Issues or PRs related to refactoring the kubernetes e2e test framework area/test cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/bug Categorizes issue or PR as related to a bug. lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. release-note-none Denotes a PR that doesn't merit a release note. sig/architecture Categorizes an issue or PR as relevant to SIG Architecture. 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/M Denotes a PR that changes 30-99 lines, ignoring generated files. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
SIG Node CI/Test Board
Archived in project
SIG Node: code and documentation PRs
Archived in project
Milestone
v1.23
Development

Successfully merging this pull request may close these issues.
8 participants
@pacoxu @k8s-ci-robot @SergeyKanzhelev @ehashman @matthyx @neolit123 @mrunalp @fabriziopandini