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

Update to work with libcontainer #41

Merged
merged 2 commits into from
May 16, 2014
Merged

Conversation

cpuguy83
Copy link
Contributor

Makes pipework work with the new docker native exec driver (libcontainer).

@crosbymichael
Copy link

Actually can you just use the PID from docker inspect? I fixed the lxc issue where it was not reporting back the correct nspid so it should work now for both drivers.

@cpuguy83
Copy link
Contributor Author

Awesome, much much cleaner

@cpuguy83
Copy link
Contributor Author

Done

@jpetazzo
Copy link
Owner

Yes but — this won't work anymore for plain LXC containers (and I know a few people are using pipework for that). Do you think you could also keep the code to handle non-Docker containers...?

Thanks a lot anyway!

@jpetazzo
Copy link
Owner

Yes but — this won't work anymore for plain LXC containers (and I know a few people are using pipework for that). Do you think you could also keep the code to handle non-Docker containers...?

Thanks a lot anyway!

@cpuguy83
Copy link
Contributor Author

Did you test it with LXC? It should still work with it.

I changed the part the deals with Docker and put a check to see if the DOCKERPID was set later on and use that, else we do all the lookups for lxc stuff.

@jpetazzo
Copy link
Owner

Oops, you're right, and I'm wrong. OK, I'll try to do a quick test when I can, and merge that if everything goes fine.

Thanks, and sorry!

Smana pushed a commit to Smana/pipework that referenced this pull request Mar 14, 2014
@jpetazzo
Copy link
Owner

Hrmm, sorry, I again merged things in the wrong order it seems :-)

Could you please rebase this? Then I'll do this quick test and merge as well...

Thank you!

Makes pipework work with the new docker native exec driver (libcontainer).
Since docker returns a pid for the container use that instead of doing all the other hackery.
@cpuguy83
Copy link
Contributor Author

Done

@jpetazzo
Copy link
Owner

Tested and works for me, but got a report that it didn't work correctly with an older version of Docker :-/

I'll need a few more days to investigate. (I wonder if it might be because PID is not available in older versions... But again, I have to investigate.)

Thanks nonetheless!

@cpuguy83
Copy link
Contributor Author

Yes, @crosbymichael mentioned that PID did not work correctly in older Docker versions.

pjkundert pushed a commit to pjkundert/pipework that referenced this pull request Mar 19, 2014
o Shamelessly copy jpetazzo#41, and modify
  it to more simply fall-back to docker if cgroups lookup doesn't work.
o Ensure we properly test for failures when seeking data from cgroups
  or docker, to avoid silent failure due to the "set -e"
o Consolidate all indentation to use spaces instead of inconsistent tabs
@inthecloud247
Copy link

any progress on this? seems to be stalled.

using latest version of pipework along with docker 0.9.1
it can't show containers...

@cpuguy83
Copy link
Contributor Author

@inthecloud247 What do you mean it can't show containers? Pipework doesn't ever show containers.

@cpuguy83
Copy link
Contributor Author

BTW, this will work 100% with at least Docker 0.9+

@inthecloud247
Copy link

Sorry for not being clear. I tested out your branch of pipework and it seems to work fine with docker 0.9.1 running on ubuntu 13.10

When it wasn't working on the current master, the error message was showing "cannot find container xxxxxx". That's what I was referring to.

Sent from my iPhone

On Mar 31, 2014, at 6:01 AM, Brian Goff [email protected] wrote:

BTW, this will work 100% with at least Docker 0.9+


Reply to this email directly or view it on GitHub.

jpetazzo pushed a commit that referenced this pull request May 16, 2014
Update to work with libcontainer
@jpetazzo jpetazzo merged commit 3e5bf6d into jpetazzo:master May 16, 2014
@jpetazzo
Copy link
Owner

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants