search

checkpoint-restore / checkpointctl

A tool for in-depth analysis of container checkpoints
Apache License 2.0
95 stars 15 forks source link

Add '--mounts' option #43

Closed sankalp-12 closed 1 year ago

sankalp-12 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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.

codecov-commenter commented 1 year ago

Codecov Report

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

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

:exclamation: 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 ```diff @@ 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](https://codecov.io/gh/checkpoint-restore/checkpointctl/pull/43?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None) | Coverage Δ | | |---|---|---| | [container.go](https://codecov.io/gh/checkpoint-restore/checkpointctl/pull/43?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None#diff-Y29udGFpbmVyLmdv) | `77.96% <81.66%> (-1.37%)` | :arrow_down: | | [checkpointctl.go](https://codecov.io/gh/checkpoint-restore/checkpointctl/pull/43?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None#diff-Y2hlY2twb2ludGN0bC5nbw==) | `81.53% <100.00%> (+4.17%)` | :arrow_up: | Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None)

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

sankalp-12 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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

adrianreber commented 1 year ago

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

sankalp-12 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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?