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

make ps command to print the pid in container, not the pid in host #1015

Closed
wants to merge 1 commit into from

Conversation

keloyang
Copy link
Contributor

@keloyang keloyang commented Sep 1, 2016

make ps command to print the pid in container, not the pid in host
this pr depends upon #1013
before this ptch:

root@ubuntu:~/workspace/runc-test# ./runc ps test 
UID        PID  PPID  C STIME TTY          TIME CMD
root     18185 18176  0 20:43 pts/12   00:00:00 sh
root     18192 18185  0 20:43 pts/12   00:00:00 sleep 9000

after this patch:

root@ubuntu:~/workspace/runc-test# ./runc ps test    
UID        PID  PPID  C STIME TTY          TIME CMD
root         1     0  0 20:20 pts/12   00:00:00 sh
root        14     1  0 20:22 pts/12   00:00:00 sleep 100000
root       130   125  0 20:35 pts/5    00:00:00 ps -ef

Signed-off-by: Shukui Yang [email protected]

return fmt.Errorf("open /proc/%d/ns/pid %v", pids[0], err)
}
defer syscall.Close(fd)
err = system.Setns(uintptr(fd), 0)
Copy link
Member

Choose a reason for hiding this comment

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

Ah, okay. This is how I would like this to be done, unfortunately there are several potential problems with trying to Setns in Go:

  1. Since Go programs are multithreaded (with no way of disabling it), this syscall can fail arbitrarily (the interface says that you should only use setns for single-threaded programs).
  2. In SELinux (and rootless container) contexts, you'll also need to join the user namespace as well.

I think this would work better if we made libcontainer.Container.Processes() do all of this internally, where it goes through the full runc exec process (but only joins the User and PID namespaces). Then we send the PIDs to the main runC process over an AF_UNIX socket.

Copy link
Contributor

Choose a reason for hiding this comment

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

I still don't think we need to show inner pids for runc ps, the global pids would be more useful, and we can do runc exec <id> ps -ef if we really need to know the inner pids. Plus, docker top shows global pids all the time and it works well, no one complained about it, why we think runc users would like this?

Even if someone really like it, we should add option like runc ps --inner <id> instead of replacing the default.

Copy link
Member

@cyphar cyphar Sep 2, 2016

Choose a reason for hiding this comment

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

The "send PIDs over an AF_UNIX socket" means you'll get PIDs in your current PID namespace (as close to "global" as you can get). This was one of the extensions to the current set up I was considering (it means you could use libcontainer.Container.Processes() inside a rootless container -- since we don't have cgroups).

@cyphar
Copy link
Member

cyphar commented Sep 2, 2016

I don't remember the usecase that @hqhq gave, but with this patch runc ps isn't much different to runc exec test ps, which makes it a bit useless IMO. I'm not really a huge fan of the current runc ps interface overall, so @hqhq can probably give you some more feedback than I can. :P

@hqhq
Copy link
Contributor

hqhq commented Sep 2, 2016

@cyphar I don't have any usecases, I only mentioned we can give it a try following your comments in #808 😄 And after a bit more think, I don't think it it useful.

@keloyang
Copy link
Contributor Author

keloyang commented Sep 2, 2016

@cyphar http://man7.org/linux/man-pages/man2/setns.2.html
A process may not be reassociated with a new mount namespace if it is multithreaded and A multithreaded process may not change user namespace with setns(),I'm not sure if setns(...pid) is ok, and I will try that libcontainer.Container.Processes() do all of this internally.
@hqhq I do think we can add --inner, How do others think ?

@crosbymichael
Copy link
Member

I think the global pids are more useful for people making the call outside the container. Thanks for the PR @keloyang but I think we shouldn't do this for the other reasons @hqhq said.

Sorry if there was confusion about this feature in that issue.

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

Successfully merging this pull request may close these issues.

5 participants