checkpoint-restore / checkpointctl

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

Add alias and '--all' option to checkpointctl show #46

Closed sankalp-12 closed 1 year ago

sankalp-12 commented 1 year ago

This pull request adds an alias for the --print-stats option, to be changed to --stats to maintain consistency among option names of checkpointctl show.

Also, an -all option is added to display all additional information about the available checkpoints, which would extend the out to look like:

checkpointctl show checkpoint.tar.gz --all

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                    |
+--------------------+--------+---------------------------+

CRIU dump statistics
+---------------+-------------+--------------+---------------+---------------+---------------+
| FREEZING TIME | FROZEN TIME | MEMDUMP TIME | MEMWRITE TIME | PAGES SCANNED | PAGES WRITTEN |
+---------------+-------------+--------------+---------------+---------------+---------------+
| 5333 us       | 200318 us   | 111941 us    | 75612 us      |        239777 |         45512 |
+---------------+-------------+--------------+---------------+---------------+---------------+
codecov-commenter commented 1 year ago

Codecov Report

Patch coverage: 88.00% and project coverage change: +0.46 :tada:

Comparison is base (15c560d) 77.73% compared to head (5b080af) 78.20%.

:exclamation: Current head 5b080af differs from pull request most recent head 181f656. Consider uploading reports for the commit 181f656 to get more accurate results

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #46 +/- ## ========================================== + Coverage 77.73% 78.20% +0.46% ========================================== Files 3 3 Lines 292 312 +20 ========================================== + Hits 227 244 +17 - Misses 51 53 +2 - Partials 14 15 +1 ``` | [Impacted Files](https://codecov.io/gh/checkpoint-restore/checkpointctl/pull/46?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None) | Coverage Δ | | |---|---|---| | [checkpointctl.go](https://codecov.io/gh/checkpoint-restore/checkpointctl/pull/46?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None#diff-Y2hlY2twb2ludGN0bC5nbw==) | `82.95% <88.00%> (+0.60%)` | :arrow_up: |

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

adrianreber commented 1 year ago

You should not replace --print-stats but mark it as hidden. That way the old flag still works, but it is not shown in in the help output.

sankalp-12 commented 1 year ago

@adrianreber Would it be required to have tests with --all option with invalid/missing *.dump files? Also, currently the default output of the --all option is with shortened paths. The user has an option to use --full-paths along with --all. Is this fine?

sankalp-12 commented 1 year ago

You should not replace --print-stats but mark it as hidden. That way the old flag still works, but it is not shown in in the help output.

So, we mark --print-stats as hidden and introduce --stats as a new flag?

adrianreber commented 1 year ago

You should not replace --print-stats but mark it as hidden. That way the old flag still works, but it is not shown in in the help output.

So, we mark --print-stats as hidden and introduce --stats as a new flag?

Yes, that would be my recommendation.

adrianreber commented 1 year ago

@adrianreber Would it be required to have tests with --all option with invalid/missing *.dump files?

Undecided. I would look at the existing code test coverage and decide using that data. If code is covered by tests then no need to introduce new tests. If it is not covered by tests, tests are needed.

Also, currently the default output of the --all option is with shortened paths. The user has an option to use --full-paths along with --all. Is this fine?

Sounds good.

github-actions[bot] commented 1 year ago

Test Results

19 tests  +1   19 :heavy_check_mark: +1   0s :stopwatch: ±0s   1 suites ±0     0 :zzz: ±0    1 files   ±0     0 :x: ±0 

Results for commit 48660941. ± Comparison against base commit 15c560db.

This pull request removes 4 and adds 5 tests. Note that renamed tests count towards both. ``` checkpointctl.bats ‑ Run checkpointctl show with tar file and --print-stats and invalid stats-dump checkpointctl.bats ‑ Run checkpointctl show with tar file and --print-stats and missing stats-dump checkpointctl.bats ‑ Run checkpointctl show with tar file and --print-stats and valid stats-dump checkpointctl.bats ‑ Run checkpointctl show with tar file and missing --mounts and --full-paths ``` ``` checkpointctl.bats ‑ Run checkpointctl show with tar file and --all and valid spec.dump and valid stats-dump checkpointctl.bats ‑ Run checkpointctl show with tar file and --stats and invalid stats-dump checkpointctl.bats ‑ Run checkpointctl show with tar file and --stats and missing stats-dump checkpointctl.bats ‑ Run checkpointctl show with tar file and --stats and valid stats-dump checkpointctl.bats ‑ Run checkpointctl show with tar file and missing --mounts/--all and --full-paths ```

:recycle: This comment has been updated with latest results.

sankalp-12 commented 1 year ago

@adrianreber Shall I add a shorthand for all the options of checkpointctl show? Currently, only —help has one.

adrianreber commented 1 year ago

@adrianreber Shall I add a shorthand for all the options of checkpointctl show? Currently, only —help has one.

No real opinion on this. I would not add it for now. Maybe later.

adrianreber commented 1 year ago

Looks good, indeed.

adrianreber commented 1 year ago

Happy to see so good test coverage. Nice.