Skip to content

Setup nerdctl top cmd#223

Merged
AkihiroSuda merged 1 commit intocontainerd:masterfrom
fahedouch:setup-nerdctl-top
Jun 1, 2021
Merged

Setup nerdctl top cmd#223
AkihiroSuda merged 1 commit intocontainerd:masterfrom
fahedouch:setup-nerdctl-top

Conversation

@fahedouch
Copy link
Copy Markdown
Member

@fahedouch fahedouch commented May 20, 2021

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 force-pushed the setup-nerdctl-top branch from a5b8d88 to 61149b8 Compare May 21, 2021 15:13
@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
Copy Markdown
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 fahedouch force-pushed the setup-nerdctl-top branch from 1b4adfb to 082a99f Compare May 30, 2021 19:22
@fahedouch
Copy link
Copy Markdown
Member Author

fahedouch commented May 31, 2021

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 force-pushed the setup-nerdctl-top branch from c2e97c7 to ff8686b Compare May 31, 2021 15:53
@fahedouch fahedouch marked this pull request as ready for review May 31, 2021 17:36
@fahedouch
Copy link
Copy Markdown
Member Author

fahedouch commented May 31, 2021

Cc @AkihiroSuda

AkihiroSuda
AkihiroSuda approved these changes Jun 1, 2021
View reviewed changes
Copy link
Copy Markdown
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
Copy Markdown
Member Author

fahedouch commented Jun 29, 2021

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