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

Move hostPID tests to common #83368

Merged
merged 1 commit into from
Oct 22, 2019
Merged

Move hostPID tests to common #83368

merged 1 commit into from
Oct 22, 2019

Conversation

Jefftree
Copy link
Member

@Jefftree Jefftree commented Oct 1, 2019 •
edited

What type of PR is this?
/kind cleanup

What this PR does / why we need it:
Move hostpid tests from test/e2e_node to test/e2e/common. These tests should be eligible to be promoted to Conformance.

Which issue(s) this PR fixes:

Fixes # #82547

Special notes for your reviewer:

These tests are a duplicate of the hostpid tests in e2e_node. I removed the check for shared pid namespace support (https://github.com/kubernetes/kubernetes/blob/master/test/e2e_node/security_context_test.go#L77) since it was only checking the Docker version (https://github.com/kubernetes/kubernetes/blob/master/test/e2e_node/docker_util.go#L53). Docker should not be an assumption we should be making about the container runtime.

Does this PR introduce a user-facing change?:

NONE

/area conformance
/priority important-longterm
/ok-to-test
/sig node
/cc @johnbelamaric
/cc @tallclair

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. release-note-none Denotes a PR that doesn't merit a release note. labels Oct 1, 2019
@k8s-ci-robot k8s-ci-robot added kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. area/conformance Issues or PRs related to kubernetes conformance tests size/L Denotes a PR that changes 100-499 lines, ignoring generated files. priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. sig/node Categorizes an issue or PR as relevant to SIG Node. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Oct 1, 2019
@k8s-ci-robot
Copy link
Contributor

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

@k8s-ci-robot
Copy link
Contributor

@Jefftree: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

What type of PR is this?
/kind cleanup

What this PR does / why we need it:
Move hostpid tests from test/e2e_node to test/e2e/common. These tests should be eligible to be promoted to Conformance.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

NONE

/area conformance
/priority important-longterm
/ok-to-test
/sig node
/cc @johnbelamaric
/cc @tallclair

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.

@Jefftree Jefftree marked this pull request as ready for review October 1, 2019 18:36
@k8s-ci-robot k8s-ci-robot added area/test sig/testing Categorizes an issue or PR as relevant to SIG Testing. and removed do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels Oct 1, 2019
@Jefftree
Copy link
Member Author

Jefftree commented Oct 1, 2019

/assign @derekwaynecarr

johnbelamaric
johnbelamaric reviewed Oct 1, 2019
View reviewed changes
Copy link
Member

@johnbelamaric johnbelamaric left a comment

Choose a reason for hiding this comment

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

/ok-to-test

test/e2e/common/security_context.go Outdated
}
})

ginkgo.It("processes in containers sharing a pod namespace should be able to see each other [Alpha]", func() {
Copy link
Member

Choose a reason for hiding this comment

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

According to https://kubernetes.io/docs/reference/command-line-tools-reference/feature-gates/ this has been Beta since 1.12. Probably should remove the [Alpha] tag. This also means we cannot make this a conformance test unless it gets promoted to GA in 1.17.

Copy link
Member Author

Choose a reason for hiding this comment

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

ack. I searched through the codebase and didn't see a [Beta] tag. Would that be unnecessary?

Copy link
Member

Choose a reason for hiding this comment

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

yeah

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Oct 1, 2019
@johnbelamaric
Copy link
Member

/assign

@hh hh mentioned this pull request Oct 1, 2019
Closed
@Jefftree
Copy link
Member Author

Jefftree commented Oct 2, 2019

/retest

johnbelamaric
johnbelamaric reviewed Oct 2, 2019
View reviewed changes
Copy link
Member

@johnbelamaric johnbelamaric left a comment

Choose a reason for hiding this comment

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

We're only trying to promote hostPID not shared PID namespace so I think you should reduce the scope of the PR

@Jefftree
Copy link
Member Author

Jefftree commented Oct 2, 2019

/retest

@Jefftree
Copy link
Member Author

Jefftree commented Oct 2, 2019

/retest

@johnbelamaric
Copy link
Member

/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 3, 2019
@johnbelamaric
Copy link
Member

/assign @tallclair

@tallclair
Copy link
Member

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Jefftree, tallclair

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 21, 2019
@Jefftree
Copy link
Member Author

/retest

@k8s-ci-robot k8s-ci-robot merged commit c981bce into kubernetes:master Oct 22, 2019
@k8s-ci-robot k8s-ci-robot added this to the v1.17 milestone Oct 22, 2019
@BenTheElder
Copy link
Member

These tests flaked on both GCE and kind in this PR and now they are flaking elsewhere :/
https://prow.k8s.io/view/gcs/kubernetes-jenkins/pr-logs/pull/83368/pull-kubernetes-e2e-gce/1186396152421945347/

@aojea
Copy link
Member

aojea commented Oct 22, 2019

Some snippets from jobs failing

return empty pid
https://prow.k8s.io/view/gcs/kubernetes-jenkins/pr-logs/pull/83368/pull-kubernetes-e2e-gce/1186396152421945347/

I1021 22:04:36.729] Oct 21 22:04:32.745: INFO: Got nginx's pid "" from pod "busybox-hostpid-7bf4cdcc-813a-4947-8973-3076ee3f8250"
I1021 22:04:36.729] Oct 21 22:04:32.745: FAIL: nginx's pid should be seen by hostpid containers

return multiples pids

https://prow.k8s.io/view/gcs/kubernetes-jenkins/pr-logs/pull/82334/pull-kubernetes-e2e-kind/1186594584491200513

Oct 22 10:59:22.534: INFO: Got nginx's pid "95667 95664 95631 95618 95592 95574 95555 95554 95548 95547" from pod "busybox-hostpid-c5853456-0f07-43dd-8737-238f46a5ef88"
Oct 22 10:59:22.534: FAIL: nginx's pid should be seen by hostpid containers

@BenTheElder BenTheElder mentioned this pull request Oct 22, 2019
Closed
@BenTheElder
Copy link
Member

definitely flaking broadly, filed #84209

@liggitt
Copy link
Member

liggitt commented Oct 23, 2019

@Jefftree @tallclair can you open a revert to clear the queue while the flakiness is root caused?

@liggitt
Copy link
Member

liggitt commented Oct 23, 2019

opened revert at #84212

@Jefftree Jefftree deleted the move-hostpid-tests branch December 7, 2021 17:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@johnbelamaric johnbelamaric johnbelamaric left review comments

@tallclair tallclair tallclair left review comments

Assignees

@derekwaynecarr derekwaynecarr

@johnbelamaric johnbelamaric

@tallclair tallclair

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/test cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. 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/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
None yet
Milestone
v1.17
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@Jefftree @k8s-ci-robot @johnbelamaric @tallclair @BenTheElder @aojea @liggitt @derekwaynecarr