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

Use an upstream fix for the zombie problem #79

Merged
merged 3 commits into from
Jul 26, 2017
Merged

Use an upstream fix for the zombie problem #79

merged 3 commits into from
Jul 26, 2017

Conversation

juanjux
Copy link
Contributor

@juanjux juanjux commented Jul 25, 2017 •
edited
Loading

The zombie processes problem is a currently open issue of
opencontainers:

opencontainers/runc#1443

Currently there is a PR that I've tested to work with our use cases:

opencontainers/runc#1506

This PR uses the version from that opencontainer's PR that have been tested to fix the problem.

Fixes #42

Workaround to use an upstream fix the zombie problem
10a940c 
The zombie processes problem is a currently open issue of
opencontainers:

opencontainers/runc#1443

Currently there is a PR that I've tested to work with our use cases:

opencontainers/runc#1506

But Glide doesn't seem to be able to get a version from a PR; and if I add
LittleLightLittleFire's to the dependencies (and change the imports in the Go
files) glide then fails with some error related to the other parts of
libopencontainers and the fork having the same dependencies.

So, this (ugly but simple) workaround in the Makefile was created to use the
upstream PR so the machine-learning department can continue slurping all the
code in the world while the upstream PR is merged.
@juanjux juanjux mentioned this pull request Jul 25, 2017
Closed
@juanjux juanjux changed the title Workaround to use an upstream fix the zombie problem Workaround to use an upstream fix for the zombie problem Jul 25, 2017
@codecov
Copy link

codecov bot commented Jul 25, 2017 •
edited
Loading

Codecov Report

Merging #79 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #79   +/-   ##
=======================================
  Coverage   58.78%   58.78%           
=======================================
  Files          14       14           
  Lines         888      888           
=======================================
  Hits          522      522           
  Misses        327      327           
  Partials       39       39

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c86b8fa...a03e9e9. Read the comment docs.

@juanjux juanjux mentioned this pull request Jul 26, 2017
Closed
@juanjux juanjux changed the title Workaround to use an upstream fix for the zombie problem [Dont merge] Workaround to use an upstream fix for the zombie problem Jul 26, 2017
@juanjux juanjux changed the title [Dont merge] Workaround to use an upstream fix for the zombie problem Workaround to use an upstream fix for the zombie problem Jul 26, 2017
@juanjux juanjux changed the title Workaround to use an upstream fix for the zombie problem Use an upstream fix for the zombie problem Jul 26, 2017
@juanjux juanjux requested a review from smola July 26, 2017 09:00
smola
smola approved these changes Jul 26, 2017
View reviewed changes
Copy link
Member

@smola smola left a comment

Choose a reason for hiding this comment

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

Looks good, but you'll have to track this upstream issue.

@juanjux
Copy link
Contributor Author

juanjux commented Jul 26, 2017

@smola yeah I'm watching it.

@abeaumont abeaumont self-assigned this Jul 26, 2017
@juanjux
Copy link
Contributor Author

juanjux commented Jul 26, 2017

Don't merge yet, let me check if I can fork the upstream fix into bblfsh as we talked in Slack.

@codecov-io
Copy link

codecov-io commented Jul 26, 2017 •
edited
Loading

Codecov Report

Merging #79 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #79   +/-   ##
=======================================
  Coverage   58.78%   58.78%           
=======================================
  Files          14       14           
  Lines         888      888           
=======================================
  Hits          522      522           
  Misses        327      327           
  Partials       39       39

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c86b8fa...eef4e91. Read the comment docs.

@abeaumont
Copy link
Contributor

@juanjux CI is failing, I'd say due to having added .git explicitely set in the repo property

@juanjux
Copy link
Contributor Author

juanjux commented Jul 26, 2017 •
edited
Loading

Nah, it's an URL, it was the lack of an https:// at the start, fixed.

@abeaumont abeaumont merged commit 6b85767 into bblfsh:master Jul 26, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@smola smola smola approved these changes

@abeaumont abeaumont Awaiting requested review from abeaumont

@manucorporat manucorporat Awaiting requested review from manucorporat

@mcarmonaa mcarmonaa Awaiting requested review from mcarmonaa

Assignees

@abeaumont abeaumont

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Server does not clean up driver instances when the driver dies
4 participants
@juanjux @codecov-io @abeaumont @smola