Change nsenter to support docker 'runin'#141
Change nsenter to support docker 'runin'#141crosbymichael merged 3 commits intodocker-archive:masterfrom
Conversation
|
Ping @crosbymichael @rjnagal |
There was a problem hiding this comment.
We need to have some way to recognize that this is or is not the command we want no?
There was a problem hiding this comment.
Yes. Look further down in this file and there will be a check for "nsenter".
There was a problem hiding this comment.
Ah, why do we need to parse the flags before that?
There was a problem hiding this comment.
Thats because the flags need to be placed before any other args. I can scan the args and skip if nsenter is not found here. Will update the patch in a bit.
There was a problem hiding this comment.
I'd say that nsenter is a "private" command so I'm okay not documenting these since no user should need to call it.
|
LGTM overall, mostly nits |
|
@vmarmol PTAL |
There was a problem hiding this comment.
We want to proceed and not return is "nsenter" is encountered in args.
There was a problem hiding this comment.
won't we just return right after the loop?
|
Thanks @vishh! This LGTM. |
|
The only problem that i see is sending the container json via a cli flag but I think @vmarmol is working on using a pipe to send this. If not let me know and i'll fix it |
|
Yes. The flag needs to be fixed. Is anything else stopping you from merging this patch? |
|
Nope, just testing |
|
Running nsinit to exec into a docker container I get this error when i exit: |
|
@crosbymichael I got the same warning when using nsenter, but only when using Didn't consider it a big problem at the time, and haven't looked at it further |
|
@thaJeztah true, i do not receive the error when running bash |
|
I'm not entirely sure how to place this PR (don't see any docs yet); Is this PR also meant to be used to open an interactive shell in a running container, thus like https://github.com/jpetazzo/nsenter ? If so, does this functionality;
I'm sorry if this is just noise, I ran into the above issues when using the nsenter / docker-enter script and hope this functionality can be built into Docker. For reference how it was solved there, see https://github.com/jpetazzo/nsenter/blob/master/docker-enter |
|
@thaJeztah This PR is re-factoring existing code without any user visible changes. |
|
@vishh thx. May have been a silly question of my side, thought I was looking at docker issues, but this is the libcontainer repository |
|
LGTM |
Docker does not require RunIn API. Hence that API has been removed. nsinit CLI has been modified to work around the nsenter changes. Docker-DCO-1.1-Signed-off-by: Vishnu Kannan <[email protected]> (github: vishh)
Docker-DCO-1.1-Signed-off-by: Vishnu Kannan <[email protected]> (github: vishh)
Docker-DCO-1.1-Signed-off-by: Vishnu Kannan <[email protected]> (github: vishh)
|
@vishh what did you just change? |
|
@crosbymichael I did a rebase. Haven't changed code. I was thinking that the cgutil.go issue might have been fixed on HEAD already :) |
|
Ok, just checking |
Change nsenter to support docker 'runin'
5 participants
Refactor nsenter to support docker runin