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

Remove pipe to wc and prevent zombie on err #1558

Merged
merged 1 commit into from
Jan 5, 2017
Merged

Remove pipe to wc and prevent zombie on err #1558

merged 1 commit into from
Jan 5, 2017

Conversation

sykesm
Copy link
Contributor

@sykesm sykesm commented Dec 18, 2016 •
edited
Loading

GetDirInodeUsage requires extended functionality provided by the GNU find implementation to work correctly. When deployed on a system with the BusyBox find implementation, the extra arguments are unrecognized and find terminates. (#1556, kubernetes/minikube#923, kubernetes/kubernetes#38894)

When find exits with an error, the wait for the wc process never happens and wc becomes a zombie. On systems without GNU find, these zombies can accrue quickly.

In lieu of a solution that would work everywhere (ie. not relying on GNU find), this PR simply replaces wc with a writer implementation that counts what has been written. This simplifies the code a bit and removes the pipe.

@k8s-ci-robot
Copy link
Collaborator

Hi @sykesm. 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 @k8s-bot 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.

If you have questions or suggestions related to this bot's behavior, please file an issue against the kubernetes/test-infra repository.

@googlebot
Copy link
Collaborator

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.

  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
  • If you signed the CLA as a corporation, please let us know the company's name.

@googlebot
Copy link
Collaborator

CLAs look good, thanks!

@sykesm sykesm changed the title Cleanup wc process when find fails Remove pipe to wc and prevent zombie on err Dec 19, 2016
@r2d4 r2d4 mentioned this pull request Dec 20, 2016
Closed
@euank
Copy link
Collaborator

euank commented Dec 23, 2016

@k8s-bot ok to test
(I don't think the bot will listen to me, but I'll try anyways)

LGTM

@dashpole
Copy link
Collaborator

dashpole commented Jan 4, 2017

@k8s-bot ok to test

@dashpole
Copy link
Collaborator

dashpole commented Jan 4, 2017

@k8s-bot test this

1 similar comment
@dashpole
Copy link
Collaborator

dashpole commented Jan 4, 2017

@k8s-bot test this

@dashpole
Copy link
Collaborator

dashpole commented Jan 4, 2017

it passed the e2e_node InodeEviction test, so /LGTM

@dashpole
Copy link
Collaborator

dashpole commented Jan 4, 2017

ill have to figure out why the pull is failing...

@timstclair
Copy link
Contributor

@k8s-bot test this

@k8s-ci-robot
Copy link
Collaborator

Jenkins GCE e2e failed for commit c7bd164. Full PR test history.

The magic incantation to run this job again is @k8s-bot test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

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.

@dashpole dashpole mentioned this pull request Jan 4, 2017
Merged
@dashpole
Copy link
Collaborator

dashpole commented Jan 4, 2017

#1567 has merged. Try rebasing and see if the problem goes away.

@sykesm
Copy link
Contributor Author

sykesm commented Jan 5, 2017

Build was successful after a rebase. Thanks.

@timstclair timstclair merged commit ba33b5a into google:master Jan 5, 2017
@dashpole dashpole mentioned this pull request Jan 5, 2017
Merged
k8s-github-robot pushed a commit to kubernetes/kubernetes that referenced this pull request Jan 10, 2017
Merge pull request #39477 from dashpole/zombie_wc
c03ec46 
Automatic merge from submit-queue (batch tested with PRs 39486, 37288, 39477, 39455, 39542)

Fix wc zombie goroutine issue in volume util

See [Cadvisor #1558](google/cadvisor#1558).  This should solve problems for those using images that do not support "wc".
cc: @timstclair
@euank euank mentioned this pull request Jan 16, 2017
Closed
This was referenced Jan 17, 2017
Closed
Merged
k8s-github-robot pushed a commit to kubernetes/kubernetes that referenced this pull request Jan 18, 2017
Merge pull request #40095 from dashpole/update_godeps
a768f15 
Automatic merge from submit-queue (batch tested with PRs 40105, 40095)

Update dependencies: aws-sdk-go to 1.6.10; also cadvisor

updating cadvisor mainly to include [this bugfix #1558](google/cadvisor#1558)

Because cadvisor vendors a newer version of aws than kubernetes, the aws dependency needed to be updated as well.
cc: @justinsb @zmerlynn @timstclair
This was referenced Mar 15, 2017
Merged
Open
k8s-github-robot pushed a commit to kubernetes/kubernetes that referenced this pull request Mar 22, 2017
Merge pull request #43113 from dashpole/release-1.5
6aa1195 
Automatic merge from submit-queue

[Release 1.5] Update cadvisor godeps to v0.25.0

This PR updates the cAdvisor Godeps for the 1.5 branch.  This includes a number of critical bugfixes including:
[cadvisor#1558](google/cadvisor#1558), [cadvisor#1573](google/cadvisor#1573)

This is a large change on account of the aws dependency update.  However, many affected users have requested that this be cherrypicked into 1.5 and 1.4 (coming soon)

```release-note
- Disable thin_ls due to excessive iops
- Ignore .mount cgroups, fixing dissappearing stats
- Fix wc goroutine leak
- Update aws-sdk-go dependency to 1.6.10
```

cc @calebamiles [remember this?](#40095 (comment)) @jessfraz @timstclair
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dashpole dashpole dashpole approved these changes

Assignees

@dashpole dashpole

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@sykesm @k8s-ci-robot @googlebot @euank @dashpole @timstclair