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.

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

Use EPOLL_CLOEXEC to prevent fd leaks. #74691

Merged
merged 3 commits into from Jul 11, 2019

Conversation

@cpuguy83
Copy link
Contributor

commented Feb 28, 2019

What type of PR is this?

Uncomment only one /kind <> line, hit enter to put that in a new line, and remove leading whitespaces from that line:

/kind bug

What this PR does / why we need it:

Use EPOLL_CLOEXEC to prevent fd leaks.

Which issue(s) this PR fixes:

Special notes for your reviewer:

There's still an issue with fsnotify which is used in other places.
Reaching out to the maintainer, but he backed out of the project some
time ago and there's no active committers AFAIK.

Does this PR introduce a user-facing change?:

Use O_CLOEXEC to ensure file descriptors do not leak to subprocesses.
@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Feb 28, 2019

Hi @cpuguy83. 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.

@cpuguy83

This comment has been minimized.

Copy link
Contributor Author

commented Feb 28, 2019

/sig node

@k8s-ci-robot k8s-ci-robot added sig/node and removed needs-sig labels Feb 28, 2019
@k8s-ci-robot k8s-ci-robot requested review from dims and resouer Feb 28, 2019
@cpuguy83

This comment has been minimized.

Copy link
Contributor Author

commented Feb 28, 2019

/assign @derekwaynecarr

@ddebroy

This comment has been minimized.

Copy link
Member

commented Feb 28, 2019

/ok-to-test

@khenidak

This comment has been minimized.

Copy link
Contributor

commented Feb 28, 2019

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm label Feb 28, 2019
@cpuguy83 cpuguy83 force-pushed the cpuguy83:epoll_use_cloexec branch from 44220ef to 9df0fe7 Mar 15, 2019
@cpuguy83

This comment has been minimized.

Copy link
Contributor Author

commented Apr 15, 2019

/test pull-kubernetes-kubemark-e2e-gce-big

@dims

This comment has been minimized.

Copy link
Member

commented May 23, 2019

/priority important-soon
/retest
/milestone v1.15

@timmycarr

This comment has been minimized.

Copy link

commented May 29, 2019

Hi! Friendly reminder from your release team: we are starting the code freeze for 1.15 tomorrow EOD. Seeing some good progress here, just checking in to see if this is still planned for the 1.15 cycle? See that milestone added a couple of days ago and awaiting a LGTM. Assuming that's the case. Can I get a quick confirmation here?

@soggiest

This comment has been minimized.

Copy link
Contributor

commented Jun 3, 2019

@dims Is this release blocking? I see you've approved this PR but it is lacking a LGTM label, I'm inclined to move this to 1.16.

@dims

This comment has been minimized.

Copy link
Member

commented Jun 3, 2019

@soggiest sounds good, my approval is not enough, need folks from sig-node to approve unfortunately :(

@soggiest

This comment has been minimized.

Copy link
Contributor

commented Jun 3, 2019

/milestone v1.16

@k8s-ci-robot k8s-ci-robot modified the milestones: v1.15, v1.16 Jun 3, 2019
@dims

This comment has been minimized.

Copy link
Member

commented Jul 8, 2019

/uncc

@derekwaynecarr

This comment has been minimized.

Copy link
Member

commented Jul 8, 2019

apologies for the delay, for some reason this PR was lost in my review queue. i assume you were observing leaks from this area, but its not clear at what rate? we should have metrics for this and i am wondering why it hasn't come up prior.

@sjenning can you take a look? i think you were the original author for using memcg notifications in this area, and it would be good for you to weigh in.

@dims dims referenced this pull request Jul 8, 2019
@sjenning

This comment has been minimized.

Copy link
Contributor

commented Jul 8, 2019

related fsnotify/fsnotify#219
/lgtm
I don't think we are leaking fd's that I know of but this is good hygiene unless you are intentionally wanting to share fd's with child processes (which we don't).
needs a rebase though.

@cpuguy83

This comment has been minimized.

Copy link
Contributor Author

commented Jul 8, 2019

Yes, I went on a hunt for possible leaks after a user reported some issues running out of inotify handles (without much to go on to debug with). These are just what I ran across as possible, and unlikely to want to be shared with sub-processes.

@dims

This comment has been minimized.

Copy link
Member

commented Jul 9, 2019

@cpuguy83 let's rebase and ping folks for approval again :)

cpuguy83 added 3 commits Feb 28, 2019
This prevents fd's from leaking to subprocesses.
This prevents fd's from leaking to subprocesses.
This prevents fd's from leaking to subprocesses.
@cpuguy83 cpuguy83 force-pushed the cpuguy83:epoll_use_cloexec branch from 9df0fe7 to 7077bbd Jul 9, 2019
@k8s-ci-robot k8s-ci-robot removed the lgtm label Jul 9, 2019
@derekwaynecarr

This comment has been minimized.

Copy link
Member

commented Jul 9, 2019

thanks @cpuguy83 and @sjenning for commenting.

kubelet changes lgtm.

/approve
/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm label Jul 9, 2019
@dims

This comment has been minimized.

Copy link
Member

commented Jul 10, 2019

/assign @thockin @dchen1107

(We need pkg/ OWNERS approval, please see @derekwaynecarr 's /approve above)

@thockin

This comment has been minimized.

Copy link
Member

commented Jul 10, 2019

Thanks!

/lgtm
/approve

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Jul 10, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cpuguy83, derekwaynecarr, thockin

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

@fejta-bot

This comment has been minimized.

Copy link

commented Jul 11, 2019

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@k8s-ci-robot k8s-ci-robot merged commit 514a157 into kubernetes:master Jul 11, 2019
22 of 23 checks passed
22 of 23 checks passed
pull-kubernetes-kubemark-e2e-gce-big Job triggered.
Details
cla/linuxfoundation cpuguy83 authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-conformance-image-test Skipped.
pull-kubernetes-cross Skipped.
pull-kubernetes-dependencies Job succeeded.
Details
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-csi-serial Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gce-iscsi Skipped.
pull-kubernetes-e2e-gce-iscsi-serial Skipped.
pull-kubernetes-e2e-gce-storage-slow Job succeeded.
Details
pull-kubernetes-godeps Skipped.
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-local-e2e Skipped.
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-node-e2e-containerd Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
pull-publishing-bot-validate Skipped.
tide In merge pool.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.