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

feat: add flag to view process tree #56

Merged
merged 3 commits into from
Jun 16, 2023

Conversation

snprajwal
Copy link
Member

@snprajwal snprajwal commented May 28, 2023

CRIT already allows us to explore the process tree, file descriptors, memory map, and RSS map for a checkpoint. These features can be integrated with checkpointctl to allow more info to be displayed with checkpointctl show. This PR introduces checkpointctl show --ps-tree for viewing the process tree.

Here's a sample output of the tree displayed for a single process:

Process tree

Container
└── [1]  piggie

@github-actions
Copy link

github-actions bot commented May 28, 2023

Test Results

24 tests  +1   24 ✔️ +1   0s ⏱️ ±0s
  1 suites ±0     0 💤 ±0 
  1 files   ±0     0 ±0 

Results for commit 4a28ac1. ± Comparison against base commit 34bab99.

♻️ This comment has been updated with latest results.

@codecov-commenter
Copy link

codecov-commenter commented May 28, 2023

Codecov Report

Patch coverage: 71.79% and project coverage change: -0.72 ⚠️

Comparison is base (34bab99) 80.56% compared to head (296b668) 79.85%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #56      +/-   ##
==========================================
- Coverage   80.56%   79.85%   -0.72%     
==========================================
  Files           3        3              
  Lines         391      407      +16     
==========================================
+ Hits          315      325      +10     
- Misses         55       59       +4     
- Partials       21       23       +2     
Impacted Files Coverage Δ
container.go 80.95% <66.66%> (-1.84%) ⬇️
checkpointctl.go 85.43% <88.88%> (+1.22%) ⬆️

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

go.mod Outdated Show resolved Hide resolved
@snprajwal snprajwal changed the title feat: add existing features that CRIT provides feat: add flag to view file descriptors May 28, 2023
@snprajwal snprajwal force-pushed the add-crit-feats branch 2 times, most recently from aece90d to e352f25 Compare May 28, 2023 19:09
@snprajwal snprajwal marked this pull request as ready for review May 28, 2023 19:09
@snprajwal
Copy link
Member Author

CRIU dump and restore are failing in the CI, I'm not sure why. Is it because of insufficient permissions or something? I based the workflow directly off of the one we have in go-criu.

@adrianreber
Copy link
Member

CRIU dump and restore are failing in the CI, I'm not sure why. Is it because of insufficient permissions or something? I based the workflow directly off of the one we have in go-criu.

We run in a unprivileged container. GitHub actions supports privileged containers. Please try that.

@snprajwal
Copy link
Member Author

We run in a unprivileged container. GitHub actions supports privileged containers. Please try that.

Looks like that helped - now both workflows fail on restore 😆

@adrianreber
Copy link
Member

We run in a unprivileged container. GitHub actions supports privileged containers. Please try that.

Looks like that helped - now both workflows fail on restore 😆

You either need to print the logs in an error case or log to stdout.

It could be necessary to mount /lib/modules from the host into the container so that necessary module can be loaded.

But without logs it is difficult to tell.

@snprajwal
Copy link
Member Author

I don't want to create too much noise on this PR. Let me try it out on a separate branch, and push the changes here once I get it working

@adrianreber
Copy link
Member

I think we might need to wait a bit with this PR. My favourite approach right now would be to make everything PID tree based. Maybe the table view is only useful for the quick overview. I am curious in how far the tree based approach would work for checkpointctl overall.

@snprajwal
Copy link
Member Author

I think we might need to wait a bit with this PR. My favourite approach right now would be to make everything PID tree based. Maybe the table view is only useful for the quick overview. I am curious in how far the tree based approach would work for checkpointctl overall.

Personally, I think the issue with the tree view is that it will restrict the amount of information we can provide. If we put too much info in the tree, it will become unreadable, and hard to track the parent/children. I was thinking we'll use the tree for the barebones process tree (PID + process name), and use tables for more information. If we display both, then the user can have a quick glance at the tree to understand the process structure and explore the individual process info through the tables.

@snprajwal snprajwal force-pushed the add-crit-feats branch 2 times, most recently from 625fcfd to 48b6c18 Compare May 30, 2023 16:04
@adrianreber
Copy link
Member

We talked about binary sizes before. I think we should include a CI check about the binary size like Podman has. Build each PR without the PR and record the binary size, build the PR with the changes and record the binary size. Compare both sizes and fail if +10%.

@snprajwal snprajwal changed the title feat: add flag to view file descriptors feat: add flag to view process tree Jun 2, 2023
@snprajwal
Copy link
Member Author

snprajwal commented Jun 2, 2023

I've added the process tree logic here. I didn't want to create a separate branch and PR for it as of now since I had important changes in the first two commits, and did not want to duplicate them. If this looks good, we can discuss how file descriptors, memory maps, etc. integrate into this and add them accordingly. I still have the commit and code for the file descriptors feature saved on a different branch.

@adrianreber
Copy link
Member

There are a couple of things that should be done differently. This PR mixes too many things. There is some renaming going on which should be a separate commit. The changes to the testing and the new features should be different PRs.

For the new feature I would like to see some example output in the PR description.

Instead of the loop.c example, let's try to use piggie.c from go-criu, which also creates a couple of namespaces as this will be helpful at some point during testing.

Also the binary size comparison PR should be done before any more includes from go-criu.

@snprajwal
Copy link
Member Author

Got it, I'll split it up and open separate PRs.

checkpointctl.go Outdated Show resolved Hide resolved
@snprajwal snprajwal force-pushed the add-crit-feats branch 3 times, most recently from 527b5b9 to 742e495 Compare June 12, 2023 08:28
@snprajwal
Copy link
Member Author

@adrianreber I've updated the PR description with the tree we currently display during tests (with piggie).

@adrianreber
Copy link
Member

Looks good for a first step. I like it.

container.go Outdated Show resolved Hide resolved
checkpointctl.go Outdated Show resolved Hide resolved
container.go Outdated Show resolved Hide resolved
container.go Show resolved Hide resolved
Using `ExcludePatterns` to unpack files prevents fine-grained control
over extracting specific image file from the checkpoint directory. By
iterating the archive, we can now unpack specific files from
subdirectories without unpacking the entire directory.

Signed-off-by: Prajwal S N <[email protected]>
The process tree in the checkpoint can now be viewed with the `--ps-tree` flag.

Signed-off-by: Prajwal S N <[email protected]>
Signed-off-by: Prajwal S N <[email protected]>
@adrianreber adrianreber merged commit 5bb5902 into checkpoint-restore:main Jun 16, 2023
@snprajwal snprajwal deleted the add-crit-feats branch June 16, 2023 18:38
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.

5 participants