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

sankalp-12 · 2023-04-18T14:36:04Z

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 · 2023-04-18T14:59:42Z

Codecov Report

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

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

❗ 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

@@            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	Coverage Δ
checkpointctl.go	`82.95% <88.00%> (+0.60%)`	⬆️

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

adrianreber · 2023-04-18T15:25:59Z

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 · 2023-04-18T15:28:19Z

@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 · 2023-04-18T15:35:35Z

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 · 2023-04-18T15:37:43Z

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 · 2023-04-18T15:42:21Z

@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.

This patch adds an alias '--stats' for the '--print-stats' option, of the checkpointctl show command. Signed-off-by: Sankalp Acharya <[email protected]>

github-actions · 2023-04-18T18:55:52Z

Test Results

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

Results for commit 4866094. ± Comparison against base commit 15c560d.

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

♻️ This comment has been updated with latest results.

sankalp-12 · 2023-04-19T16:49:02Z

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

checkpointctl.go

test/checkpointctl.bats

checkpointctl.go

This patch adds tests for the '--all' option for checkpointctl show. Signed-off-by: Sankalp Acharya <[email protected]>

snprajwal

LGTM

adrianreber · 2023-04-26T18:04:12Z

@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 · 2023-04-26T18:05:54Z

Looks good, indeed.

adrianreber · 2023-04-26T18:11:16Z

Happy to see so good test coverage. Nice.

sankalp-12 force-pushed the add-all-option branch from 995d6ab to 6fd6177 Compare April 18, 2023 14:56

sankalp-12 force-pushed the add-all-option branch from 05d7be2 to dd8f7aa Compare April 18, 2023 15:20

sankalp-12 force-pushed the add-all-option branch 3 times, most recently from 97bdb5e to fb8eaa6 Compare April 18, 2023 17:12

sankalp-12 added 2 commits April 19, 2023 00:22

Add alias for --print-stats

7130b89

This patch adds an alias '--stats' for the '--print-stats' option, of the checkpointctl show command. Signed-off-by: Sankalp Acharya <[email protected]>

sankalp-12 force-pushed the add-all-option branch from fb8eaa6 to a4c85d7 Compare April 18, 2023 18:53

snprajwal reviewed Apr 26, 2023

View reviewed changes

checkpointctl.go Outdated Show resolved Hide resolved

test/checkpointctl.bats Outdated Show resolved Hide resolved

test/checkpointctl.bats Outdated Show resolved Hide resolved

sankalp-12 force-pushed the add-all-option branch from 5b080af to 181f656 Compare April 26, 2023 16:20

sankalp-12 requested a review from snprajwal April 26, 2023 16:23

snprajwal reviewed Apr 26, 2023

View reviewed changes

checkpointctl.go Outdated Show resolved Hide resolved

Add test for --all

4866094

This patch adds tests for the '--all' option for checkpointctl show. Signed-off-by: Sankalp Acharya <[email protected]>

sankalp-12 force-pushed the add-all-option branch from 62e89c7 to 4866094 Compare April 26, 2023 17:44

sankalp-12 requested a review from snprajwal April 26, 2023 17:47

snprajwal approved these changes Apr 26, 2023

View reviewed changes

adrianreber merged commit 739d863 into checkpoint-restore:main Apr 26, 2023

sankalp-12 deleted the add-all-option branch April 26, 2023 20:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

sankalp-12 commented Apr 18, 2023 •

edited

Loading

codecov-commenter commented Apr 18, 2023 •

edited

Loading

adrianreber commented Apr 18, 2023

sankalp-12 commented Apr 18, 2023

sankalp-12 commented Apr 18, 2023

adrianreber commented Apr 18, 2023 •

edited

Loading

adrianreber commented Apr 18, 2023

github-actions bot commented Apr 18, 2023 •

edited

Loading

sankalp-12 commented Apr 19, 2023 •

edited

Loading

snprajwal left a comment

adrianreber commented Apr 26, 2023

adrianreber commented Apr 26, 2023

adrianreber commented Apr 26, 2023

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

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

Conversation

sankalp-12 commented Apr 18, 2023 • edited Loading

codecov-commenter commented Apr 18, 2023 • edited Loading

Codecov Report

adrianreber commented Apr 18, 2023

sankalp-12 commented Apr 18, 2023

sankalp-12 commented Apr 18, 2023

adrianreber commented Apr 18, 2023 • edited Loading

adrianreber commented Apr 18, 2023

github-actions bot commented Apr 18, 2023 • edited Loading

Test Results

sankalp-12 commented Apr 19, 2023 • edited Loading

snprajwal left a comment

Choose a reason for hiding this comment

adrianreber commented Apr 26, 2023

adrianreber commented Apr 26, 2023

adrianreber commented Apr 26, 2023

sankalp-12 commented Apr 18, 2023 •

edited

Loading

codecov-commenter commented Apr 18, 2023 •

edited

Loading

adrianreber commented Apr 18, 2023 •

edited

Loading

github-actions bot commented Apr 18, 2023 •

edited

Loading

sankalp-12 commented Apr 19, 2023 •

edited

Loading