checkpoint-restore / checkpointctl

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

feat: add new `inspect` sub-command #76

Closed behouba closed 1 year ago

behouba commented 1 year ago

This PR adds a new inspect sub-command with a default tree output format, supplementing the existing show sub-command. The show sub-command no longer supports flags and is now used for getting a quick checkpoint overview in a table format. On the other hand, the new inspect sub-command offers a detailed view of the checkpoint, using all the flags previously used with show.

Fixes: #75

codecov-commenter commented 1 year ago

Codecov Report

Patch coverage: 85.50% and project coverage change: +0.28 :tada:

Comparison is base (ba8d41b) 80.56% compared to head (8371eef) 80.84%.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #76 +/- ## ========================================== + Coverage 80.56% 80.84% +0.28% ========================================== Files 3 4 +1 Lines 463 496 +33 ========================================== + Hits 373 401 +28 - Misses 66 70 +4 - Partials 24 25 +1 ``` | [Impacted Files](https://app.codecov.io/gh/checkpoint-restore/checkpointctl/pull/76?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None) | Coverage Δ | | |---|---|---| | [container.go](https://app.codecov.io/gh/checkpoint-restore/checkpointctl/pull/76?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None#diff-Y29udGFpbmVyLmdv) | `76.19% <73.17%> (-4.70%)` | :arrow_down: | | [tree.go](https://app.codecov.io/gh/checkpoint-restore/checkpointctl/pull/76?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None#diff-dHJlZS5nbw==) | `88.60% <88.60%> (ø)` | | | [checkpointctl.go](https://app.codecov.io/gh/checkpoint-restore/checkpointctl/pull/76?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None#diff-Y2hlY2twb2ludGN0bC5nbw==) | `89.03% <88.75%> (+1.74%)` | :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.

github-actions[bot] commented 1 year ago

Test Results

33 tests  +7   33 :heavy_check_mark: +7   1s :stopwatch: -1s   1 suites ±0     0 :zzz: ±0    1 files   ±0     0 :x: ±0 

Results for commit 4c9386e6. ± Comparison against base commit ba8d41b1.

This pull request removes 9 and adds 16 tests. Note that renamed tests count towards both. ``` checkpointctl.bats ‑ Run checkpointctl show with multiple tar files with additional flags 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 --mounts and --full-paths and valid spec.dump checkpointctl.bats ‑ Run checkpointctl show with tar file and --mounts and valid spec.dump checkpointctl.bats ‑ Run checkpointctl show with tar file and --ps-tree 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 ``` ``` checkpointctl.bats ‑ Run checkpointctl inspect with invalid format checkpointctl.bats ‑ Run checkpointctl inspect with multiple tar files checkpointctl.bats ‑ Run checkpointctl inspect with tar file and --all and valid spec.dump and valid stats-dump checkpointctl.bats ‑ Run checkpointctl inspect with tar file and --mounts and valid spec.dump checkpointctl.bats ‑ Run checkpointctl inspect with tar file and --ps-tree and missing pstree.img checkpointctl.bats ‑ Run checkpointctl inspect with tar file and --stats and invalid stats-dump checkpointctl.bats ‑ Run checkpointctl inspect with tar file and --stats and missing stats-dump checkpointctl.bats ‑ Run checkpointctl inspect with tar file and --stats and valid stats-dump checkpointctl.bats ‑ Run checkpointctl inspect with tar file and rootfs-diff tar file checkpointctl.bats ‑ Run checkpointctl inspect with tar file from containerd with valid config.dump and valid spec.dump and checkpoint directory … ```

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

behouba commented 1 year ago

Would it make sense to always use --all and --full-paths with --format tree / --format json?

It makes sense, given that the tree format with "--all" and "--full-path" won't cause the terminal screen to be overcrowded in general.

snprajwal commented 1 year ago

Given that tree view is the default, and JSON is better than the table view, do we even want to retain it? It might make sense to just get rid of it if we don't plan on using it going forward.

behouba commented 1 year ago

Given that tree view is the default, and JSON is better than the table view, do we even want to retain it? It might make sense to just get rid of it if we don't plan on using it going forward.

While I don't have a strong opinion on this, it's an interesting question.

rst0git commented 1 year ago

Given that tree view is the default, and JSON is better than the table view, do we even want to retain it? It might make sense to just get rid of it if we don't plan on using it going forward.

It would make sense to keep the table as default because it shows a brief summary of the content of a checkpoint and allows to display an overview of multiple checkpoints.

Example:

Displaying container checkpoint data from /tmp/checkpoint.tar.gz

+-----------+----------------------------------+--------------+---------+---------------------------+--------+------------+
| CONTAINER |              IMAGE               |      ID      | RUNTIME |          CREATED          | ENGINE | CHKPT SIZE |
+-----------+----------------------------------+--------------+---------+---------------------------+--------+------------+
| looper    | docker.io/library/busybox:latest | a69f3e73ec89 | crun    | 2023-06-25T11:00:18+01:00 | Podman | 154.5 KiB  |
+-----------+----------------------------------+--------------+---------+---------------------------+--------+------------+

In comparison, when displaying tree and json format with --all and --full-path (as discussed above), checkpointctl would provide a complete information about a checkpoint archive.

Example:

Displaying container checkpoint tree view from /tmp/checkpoint.tar.gz

looper
├── Image: docker.io/library/busybox:latest
├── ID: a69f3e73ec89d6d3c4999d00407a2cca96df212163da4e088107b75225821319
├── Runtime: crun
├── Created: 2023-06-25T11:00:18+01:00
├── Engine: Podman
├── CHKPT Size: 154.5 KiB
├── Overview of Mounts
│   ├── Destination: /proc
│   │   ├── Type: proc
│   │   └── Source: proc
│   ├── Destination: /dev
│   │   ├── Type: tmpfs
│   │   └── Source: tmpfs
│   ├── Destination: /sys
│   │   ├── Type: sysfs
│   │   └── Source: sysfs
│   ├── Destination: /dev/pts
│   │   ├── Type: devpts
│   │   └── Source: devpts
│   ├── Destination: /dev/mqueue
│   │   ├── Type: mqueue
│   │   └── Source: mqueue
│   ├── Destination: /etc/hosts
│   │   ├── Type: bind
│   │   └── Source: /run/containers/storage/overlay-containers/a69f3e73ec89d6d3c4999d00407a2cca96df212163da4e088107b75225821319/userdata/hosts
│   ├── Destination: /dev/shm
│   │   ├── Type: bind
│   │   └── Source: /var/lib/containers/storage/overlay-containers/a69f3e73ec89d6d3c4999d00407a2cca96df212163da4e088107b75225821319/userdata/shm
│   ├── Destination: /run/.containerenv
│   │   ├── Type: bind
│   │   └── Source: /run/containers/storage/overlay-containers/a69f3e73ec89d6d3c4999d00407a2cca96df212163da4e088107b75225821319/userdata/.containerenv
│   ├── Destination: /run/secrets
│   │   ├── Type: bind
│   │   └── Source: /run/containers/storage/overlay-containers/a69f3e73ec89d6d3c4999d00407a2cca96df212163da4e088107b75225821319/userdata/run/secrets
│   ├── Destination: /etc/hostname
│   │   ├── Type: bind
│   │   └── Source: /run/containers/storage/overlay-containers/a69f3e73ec89d6d3c4999d00407a2cca96df212163da4e088107b75225821319/userdata/hostname
│   ├── Destination: /etc/resolv.conf
│   │   ├── Type: bind
│   │   └── Source: /run/containers/storage/overlay-containers/a69f3e73ec89d6d3c4999d00407a2cca96df212163da4e088107b75225821319/userdata/resolv.conf
│   └── Destination: /sys/fs/cgroup
│       ├── Type: cgroup
│       └── Source: cgroup
├── CRIU dump statistics
│   ├── Freezing Time: 102175 us
│   ├── Frozen Time: 143805 us
│   ├── Memdump Time: 1808 us
│   ├── Memwrite Time: 184 us
│   ├── Pages Scanned: 1195 us
│   └── Pages Written: 34 us
└── Process tree
    └── [1]  sh

@adrianreber What do you think?

adrianreber commented 1 year ago

@rst0git and I talked about this a bit.

Our idea was to keep the current table as is if using the the show sub-command. The show sub-command shouldn't have any parameters and is only used for a quick overview of the checkpoints in the table. Should work with multiple checkpoints.

In addition to show there should be an inspect sub-command which defaults to the tree view and has the all the additionally introduced parameter to allow the user to select which details to include in the tree view.

Maybe we should include a hint in the description of show that inspect has many more options.

The introduction of the new sub-command is motivated that the output of show should not change based on the used parameters. With a new command like inspect the format would always be a tree as default.

behouba commented 1 year ago

With a new command like inspect the format would always be a tree as default.

@adrianreber, @rst0git should the inspect sub command support displaying table formats in addition to the default tree format upcoming JSON format.

adrianreber commented 1 year ago

With a new command like inspect the format would always be a tree as default.

@adrianreber, @rst0git should the inspect sub command support displaying table formats in addition to the default tree format upcoming JSON format.

From my point of view it should not.

snprajwal commented 1 year ago

Regarding serialising to JSON, we have two ways to do it:

I can hack the library together in a couple of days, but I would like to know if you guys think this approach is any more useful that the first one.

behouba commented 1 year ago
  • We can use a library to serialise the tree into a JSON and vice versa. The benefit of this is that we can dynamically generate the JSON, independent of the structure of the tree. There would be no struct definitions needed for us to use the representation.

I like this option as well. Because it will provide some consistency with the table and tree format.