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

Setup nerdctl top cmd #223

Merged
merged 1 commit into from
Jun 1, 2021
Merged

Setup nerdctl top cmd #223

merged 1 commit into from
Jun 1, 2021

Conversation

fahedouch
Copy link
Member

same as docker top

Signed-off-by: fahed dorgaa fahed.dorgaa@gmail.com

@fahedouch fahedouch marked this pull request as draft May 20, 2021 22:07
@fahedouch fahedouch changed the title [WIP] setup docker top cmd [WIP] setup nerdctl top cmd May 20, 2021
@fahedouch fahedouch marked this pull request as ready for review May 26, 2021 19:51
@fahedouch fahedouch changed the title [WIP] setup nerdctl top cmd Setup nerdctl top cmd May 27, 2021
@AkihiroSuda
Copy link
Member

AkihiroSuda commented May 27, 2021 •
edited
Loading

Ideally we should use https://github.com/containers/psgo rather than exec-ing ps, but this refactoring can be done later.

@fahedouch fahedouch marked this pull request as draft May 30, 2021 19:07
@fahedouch
Copy link
Member Author

running the ps command in a rootless container with cgroupv1 is not possible because rootless container does not rely on cgroupv1.
the problem does not arise in the case of rootlfull (v1 or v2) && rootless ( v2)
The idea to get around this problem is to join the mount namespace of rootless container when we are dealing with rootless container on cgroupv1 and fetch process info inside the pid mount ns.

FYI : for pid host is like doing a ps on the host and dealing with the host /proc so we should add this to the condition when implementing --pid=host

I prefer making this refactoring later when using psgo rather than exec-ing ps. It handles better this use cases.

@fahedouch
setup docker top cmd
ff8686b 
Signed-off-by: fahed dorgaa <fahed.dorgaa@gmail.com>
@fahedouch fahedouch marked this pull request as ready for review May 31, 2021 17:36
@fahedouch
Copy link
Member Author

Cc @AkihiroSuda

AkihiroSuda
AkihiroSuda approved these changes Jun 1, 2021
View reviewed changes
Copy link
Member

@AkihiroSuda AkihiroSuda left a comment

Choose a reason for hiding this comment

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

LGTM if green, thanks

@AkihiroSuda AkihiroSuda merged commit 02badc3 into containerd:master Jun 1, 2021
AkihiroSuda added a commit to AkihiroSuda/nerdctl that referenced this pull request Jun 1, 2021
@AkihiroSuda
follow-up to "setup docker top cmd" (containerd#223)
8e1d2a7 
- Clarify the origin of the codes copied from Moby
- Fix the bash completion
- Fix the doc
- Fix cgroup manager check
- Fix wording

Signed-off-by: Akihiro Suda <akihiro.suda.cz@hco.ntt.co.jp>
AkihiroSuda added a commit to AkihiroSuda/nerdctl that referenced this pull request Jun 1, 2021
@AkihiroSuda
follow-up to "setup docker top cmd" (containerd#223)
390a9ea 
- Clarify the origin of the codes copied from Moby
- Fix the bash completion
- Fix the doc
- Fix cgroup manager check
- Fix wording

Signed-off-by: Akihiro Suda <akihiro.suda.cz@hco.ntt.co.jp>
AkihiroSuda added a commit that referenced this pull request Jun 1, 2021
@AkihiroSuda
Merge pull request #230 from AkihiroSuda/followup-223
f6c5862 
follow-up to "setup docker top cmd" (#223)
@renovate renovate bot mentioned this pull request Jun 2, 2021
Merged
1 task
@fahedouch
Copy link
Member Author

Pid host is implemented here #273
The case of host pid should be handled when refacto with psgo

@apostasie apostasie mentioned this pull request Aug 6, 2024
Merged
15 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AkihiroSuda AkihiroSuda AkihiroSuda approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@fahedouch @AkihiroSuda