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

Add '--mounts' option #43

Merged
merged 2 commits into from
Apr 18, 2023
Merged

Add '--mounts' option #43

merged 2 commits into from
Apr 18, 2023

Conversation

sankalp-12
Copy link
Contributor

@sankalp-12 sankalp-12 commented Apr 13, 2023 •
edited
Loading

The checkpointctl tool could be extended to display an overview of mounts used by the checkpoints.
This pull request adds --mounts option for the checkpointctl show command, which would extend the out to look like this:

checkpointctl show checkpoint.tar.gz --mounts

Displaying container checkpoint data from /tmp/checkpointctl1209302006

+----------------+--------------------------------+--------------+---------+---------------------------+--------+------------+-------------------+
|   CONTAINER    |             IMAGE              |      ID      | RUNTIME |          CREATED          | ENGINE | CHKPT SIZE | ROOT FS DIFF SIZE |
+----------------+--------------------------------+--------------+---------+---------------------------+--------+------------+-------------------+
| cool_heyrovsky | docker.io/library/httpd:latest | 7fb253cbd7c0 | crun    | 2023-04-14T04:46:00+05:30 | Podman | 5.3 MiB    | 2.0 KiB           |
+----------------+--------------------------------+--------------+---------+---------------------------+--------+------------+-------------------+

Overview of Mounts
+--------------------+--------+---------------------------+
|    DESTINATION     |  TYPE  |          SOURCE           |
+--------------------+--------+---------------------------+
| /proc              | proc   | proc                      |
| /dev               | tmpfs  | tmpfs                     |
| /sys               | sysfs  | sysfs                     |
| /dev/pts           | devpts | devpts                    |
| /dev/mqueue        | mqueue | mqueue                    |
| /etc/hostname      | bind   | ../userdata/hostname      |
| /run/.containerenv | bind   | ../userdata/.containerenv |
| /etc/resolv.conf   | bind   | ../userdata/resolv.conf   |
| /etc/hosts         | bind   | ../userdata/hosts         |
| /dev/shm           | bind   | ../userdata/shm           |
| /sys/fs/cgroup     | cgroup | cgroup                    |
+--------------------+--------+---------------------------+

Also, a --full-paths option is added to display the mounts with the full paths. The output would look like this:

checkpointctl show checkpoint.tar.gz --mounts --full-paths

Displaying container checkpoint data from /tmp/checkpointctl2681949126

+----------------+--------------------------------+--------------+---------+---------------------------+--------+------------+-------------------+
|   CONTAINER    |             IMAGE              |      ID      | RUNTIME |          CREATED          | ENGINE | CHKPT SIZE | ROOT FS DIFF SIZE |
+----------------+--------------------------------+--------------+---------+---------------------------+--------+------------+-------------------+
| cool_heyrovsky | docker.io/library/httpd:latest | 7fb253cbd7c0 | crun    | 2023-04-14T04:46:00+05:30 | Podman | 5.3 MiB    | 2.0 KiB           |
+----------------+--------------------------------+--------------+---------+---------------------------+--------+------------+-------------------+

Overview of Mounts
+--------------------+--------+------------------------------------------------------------------------------------------------------------------------------------+
|    DESTINATION     |  TYPE  |                                                               SOURCE                                                               |
+--------------------+--------+------------------------------------------------------------------------------------------------------------------------------------+
| /proc              | proc   | proc                                                                                                                               |
| /dev               | tmpfs  | tmpfs                                                                                                                              |
| /sys               | sysfs  | sysfs                                                                                                                              |
| /dev/pts           | devpts | devpts                                                                                                                             |
| /dev/mqueue        | mqueue | mqueue                                                                                                                             |
| /etc/hostname      | bind   | /run/containers/storage/overlay-containers/7fb253cbd7c0c30dd2cc26adbefc2817fd3eb2ce48f8c3110ded8dac19a6bcfa/userdata/hostname      |
| /run/.containerenv | bind   | /run/containers/storage/overlay-containers/7fb253cbd7c0c30dd2cc26adbefc2817fd3eb2ce48f8c3110ded8dac19a6bcfa/userdata/.containerenv |
| /etc/resolv.conf   | bind   | /run/containers/storage/overlay-containers/7fb253cbd7c0c30dd2cc26adbefc2817fd3eb2ce48f8c3110ded8dac19a6bcfa/userdata/resolv.conf   |
| /etc/hosts         | bind   | /run/containers/storage/overlay-containers/7fb253cbd7c0c30dd2cc26adbefc2817fd3eb2ce48f8c3110ded8dac19a6bcfa/userdata/hosts         |
| /dev/shm           | bind   | /var/lib/containers/storage/overlay-containers/7fb253cbd7c0c30dd2cc26adbefc2817fd3eb2ce48f8c3110ded8dac19a6bcfa/userdata/shm       |
| /sys/fs/cgroup     | cgroup | cgroup                                                                                                                             |
+--------------------+--------+------------------------------------------------------------------------------------------------------------------------------------+

@adrianreber
Copy link
Member

Thanks for the PR. Can you add more details to the commit message (and maybe PR description) how the output would look like for example.

This would also need some tests and a happy CI.

@adrianreber
Copy link
Member

The errors during test coverage seem to be the same errors we are currently fixing go-criu, related to go 1.20. Once the fixes in go-criu are working it would need to be ported to checkpointctl.

@sankalp-12 sankalp-12 changed the title Add mounts option to checkpointctl show Add --mounts option Apr 13, 2023
@sankalp-12 sankalp-12 changed the title Add --mounts option Add '--mounts' option Apr 13, 2023
@sankalp-12
Copy link
Contributor Author

Thanks for the PR. Can you add more details to the commit message (and maybe PR description) how the output would look like for example.

This would also need some tests and a happy CI.

I have added some details on how the output would look in the PR description.

AFAIU, the tests would be added in the test/checkpointctl.bats file. Looking through some of the previous tests written (like for --print-stats option), a populated stats-dump file is present as a CRIU image. Similarly, for testing the --mounts option, should I populate the spec.dump file?

@adrianreber
Copy link
Member

I have added some details on how the output would look in the PR description.

Looks good. Please also add it to the commit so that it can be seen without GitHub.

Thanks for the PR. Can you add more details to the commit message (and maybe PR description) how the output would look like for example.
This would also need some tests and a happy CI.

I have added some details on how the output would look in the PR description.

AFAIU, the tests would be added in the test/checkpointctl.bats file. Looking through some of the previous tests written (like for --print-stats option), a populated stats-dump file is present as a CRIU image. Similarly, for testing the --mounts option, should I populate the spec.dump file?

Yes, adding one or two mounts to our test data sounds like the right thing to do.

Code coverage errors are being fixed in #44

@rst0git what do you think about the name --mounts. We have --print-stats and now --mounts? Should we prefix all those options with a common string like --show or --print? For --print-stats we can introduce an alias or something.

@sankalp-12
Add mounts option to checkpointctl show
ef22d1e 
This patch adds `--mounts` option for the checkpointctl show command.
The extended output for the same would look like:

checkpointctl show checkpoint.tar.gz --mounts

Overview of Mounts
+--------------------+--------+---------------------------+
|    DESTINATION     |  TYPE  |          SOURCE           |
+--------------------+--------+---------------------------+
| /proc              | proc   | proc                      |
| /dev               | tmpfs  | tmpfs                     |
| /sys               | sysfs  | sysfs                     |
| /dev/pts           | devpts | devpts                    |
| /dev/mqueue        | mqueue | mqueue                    |
| /etc/hostname      | bind   | ../userdata/hostname      |
| /run/.containerenv | bind   | ../userdata/.containerenv |
| /etc/resolv.conf   | bind   | ../userdata/resolv.conf   |
| /etc/hosts         | bind   | ../userdata/hosts         |
| /dev/shm           | bind   | ../userdata/shm           |
| /sys/fs/cgroup     | cgroup | cgroup                    |
+--------------------+--------+---------------------------+

Closes #40

Signed-off-by: Sankalp Acharya <[email protected]>
@codecov-commenter
Copy link

codecov-commenter commented Apr 14, 2023 •
edited
Loading

Codecov Report

Patch coverage: 84.72% and project coverage change: +0.66 🎉

Comparison is base (86e9400) 74.50% compared to head (d2bdeb2) 75.17%.

❗ Current head d2bdeb2 differs from pull request most recent head e16e99d. Consider uploading reports for the commit e16e99d to get more accurate results

Additional details and impacted files 
@@            Coverage Diff             @@
##             main      #43      +/-   ##
==========================================
+ Coverage   74.50%   75.17%   +0.66%     
==========================================
  Files           3        3              
  Lines         255      294      +39     
==========================================
+ Hits          190      221      +31     
- Misses         51       57       +6     
- Partials       14       16       +2
Impacted Files Coverage Δ
container.go 77.96% <81.66%> (-1.37%) ⬇️
checkpointctl.go 81.53% <100.00%> (+4.17%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

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

@sankalp-12
Copy link
Contributor Author

sankalp-12 commented Apr 15, 2023 •
edited
Loading

Looks good. Please also add it to the commit so that it can be seen without GitHub.
Yes, adding one or two mounts to our test data sounds like the right thing to do.

I have added the sample output in the commit as well. Also, populated the spec.dump file with test data for mounts & added in a test case.

@rst0git
Copy link
Member

rst0git commented Apr 15, 2023

what do you think about the name --mounts. We have --print-stats and now --mounts? Should we prefix all those options with a common string like --show or --print? For --print-stats we can introduce an alias or something.

The print prefix looks unnecessary because this option is used with the show command.
IMHO, the first would be more intuitive:

sudo checkpointctl show --mounts /tmp/chkpt.tar.gz

vs

sudo checkpointctl show --print-mounts /tmp/chkpt.tar.gz

We could also add an option like --all that could show everything.

@adrianreber
Copy link
Member

Good point. Makes sense. Then we could add a alias for stats.

adrianreber
adrianreber reviewed Apr 15, 2023
View reviewed changes
test/spec.dump Outdated Show resolved Hide resolved
@adrianreber
Copy link
Member

With the test added and test coverage working the results from codecov look really good now.

@sankalp-12 sankalp-12 requested a review from adrianreber April 15, 2023 18:39
@sankalp-12
Copy link
Contributor Author

sankalp-12 commented Apr 16, 2023 •
edited
Loading

Good point. Makes sense. Then we could add a alias for stats.

Should the alias for --print-stats and an --all option suggested by @rst0git be added in this PR itself?

@adrianreber
Copy link
Member

Good point. Makes sense. Then we could add a alias for stats.

Should the alias for --print-stats and an --all option suggested by @rst0git be added in this PR itself?

No, let's do that in another PR.

adrianreber
adrianreber reviewed Apr 16, 2023
View reviewed changes
test/spec.dump Outdated Show resolved Hide resolved
adrianreber
adrianreber reviewed Apr 16, 2023
View reviewed changes
test/spec.dump Outdated Show resolved Hide resolved
adrianreber
adrianreber reviewed Apr 16, 2023
View reviewed changes
test/spec.dump Outdated Show resolved Hide resolved
adrianreber
adrianreber reviewed Apr 16, 2023
View reviewed changes
container.go Outdated Show resolved Hide resolved
@sankalp-12 sankalp-12 requested a review from adrianreber April 16, 2023 15:58
adrianreber
adrianreber reviewed Apr 17, 2023
View reviewed changes
container.go Outdated Show resolved Hide resolved
@sankalp-12 sankalp-12 requested a review from adrianreber April 17, 2023 08:12
adrianreber
adrianreber reviewed Apr 17, 2023
View reviewed changes
checkpointctl.go Outdated Show resolved Hide resolved
@sankalp-12
Add test for --mounts
d7c5e4b 
This patch adds test for the `--mounts` option for checkpointctl.
Also adds '--full-paths' option to display mounts with full paths.

Signed-off-by: Sankalp Acharya <[email protected]>
@sankalp-12 sankalp-12 requested a review from adrianreber April 17, 2023 10:49
@adrianreber
Copy link
Member

Looks good to me. (In checkpointctl.go there is one line removed which seems like an unnecessary change, but we can ignore that for now.)

@rst0git What do you think?

rst0git
rst0git reviewed Apr 17, 2023
View reviewed changes
container.go Show resolved Hide resolved
rst0git
rst0git approved these changes Apr 17, 2023
View reviewed changes
Copy link
Member

@rst0git rst0git left a comment

Choose a reason for hiding this comment

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

LGTM

@adrianreber adrianreber merged commit acd9f55 into checkpoint-restore:main Apr 18, 2023
@sankalp-12 sankalp-12 deleted the add-mounts-overview branch April 18, 2023 10:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rst0git rst0git rst0git approved these changes

@adrianreber adrianreber Awaiting requested review from adrianreber

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

Successfully merging this pull request may close these issues.
4 participants
@sankalp-12 @adrianreber @codecov-commenter @rst0git