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.

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

feat: add new inspect sub-command #76

Merged
merged 4 commits into from
Jul 3, 2023

Conversation

behouba
Copy link
Collaborator

@behouba behouba commented Jun 20, 2023

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
Copy link

codecov-commenter commented Jun 20, 2023

Codecov Report

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

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

Additional details and impacted files
@@            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 Coverage Δ
container.go 76.19% <73.17%> (-4.70%) ⬇️
tree.go 88.60% <88.60%> (ø)
checkpointctl.go 89.03% <88.75%> (+1.74%) ⬆️

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

@github-actions
Copy link

github-actions bot commented Jun 20, 2023

Test Results

33 tests  +7   33 ✔️ +7   1s ⏱️ -1s
  1 suites ±0     0 💤 ±0 
  1 files   ±0     0 ±0 

Results for commit 4c9386e. ± Comparison against base commit ba8d41b.

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
…

♻️ This comment has been updated with latest results.

table.go Outdated Show resolved Hide resolved
tree.go Show resolved Hide resolved
tree.go Outdated Show resolved Hide resolved
@behouba behouba force-pushed the show-tree-view branch 3 times, most recently from 2ae44f9 to 7fbdf36 Compare June 21, 2023 00:04
tree.go Outdated Show resolved Hide resolved
@behouba behouba marked this pull request as ready for review June 24, 2023 19:13
@behouba behouba requested review from rst0git and snprajwal June 24, 2023 19:28
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.

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

container.go Outdated Show resolved Hide resolved
checkpointctl.go Outdated Show resolved Hide resolved
@behouba
Copy link
Collaborator Author

behouba commented Jun 25, 2023

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
Copy link
Member

snprajwal commented Jun 25, 2023

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
Copy link
Collaborator Author

behouba commented Jun 25, 2023

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
Copy link
Member

rst0git commented Jun 25, 2023

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?

@behouba behouba force-pushed the show-tree-view branch 4 times, most recently from 05e8058 to 316d496 Compare June 25, 2023 18:14
checkpointctl.go Outdated Show resolved Hide resolved
@behouba behouba force-pushed the show-tree-view branch 3 times, most recently from 628fe0f to 0812b22 Compare June 25, 2023 19:40
@adrianreber
Copy link
Member

@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 behouba changed the title show: extend the output format with a tree view Add new inspect sub-command Jun 28, 2023
checkpointctl.go Outdated Show resolved Hide resolved
@snprajwal
Copy link
Member

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

  • Create a custom struct that utilises CRIT's structs for the fields, and marshal that through Go's encoding/json
  • 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 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
Copy link
Collaborator Author

behouba commented Jun 29, 2023

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

checkpointctl.go Outdated Show resolved Hide resolved
checkpointctl.go Outdated Show resolved Hide resolved
checkpointctl.go Outdated Show resolved Hide resolved
checkpointctl.go Outdated Show resolved Hide resolved
checkpointctl.go Outdated Show resolved Hide resolved
checkpointctl.go Outdated Show resolved Hide resolved
checkpointctl.go Outdated Show resolved Hide resolved
@behouba behouba force-pushed the show-tree-view branch 3 times, most recently from c87413c to 82d2814 Compare July 1, 2023 14:04
checkpointctl.go Outdated Show resolved Hide resolved
test/checkpointctl.bats Outdated Show resolved Hide resolved
@behouba behouba force-pushed the show-tree-view branch 2 times, most recently from 74d8397 to 8371eef Compare July 2, 2023 14:25
@behouba behouba requested review from rst0git and snprajwal July 2, 2023 14:33
tree.go Outdated Show resolved Hide resolved
behouba added 3 commits July 3, 2023 17:14
This commit introduces a new sub-command `inspect`. This new sub-command
provides more detailed information about checkpoints using a default tree
output format. The `inspect` sub-command inherits all the flags previously
used for the show sub-command, with the exception of --full-paths which has
been removed. This change was made because the tree and json formats now
provide more compact representations, allowing us to display the full paths
by default.

Signed-off-by: Kouame Behouba Manasse <[email protected]>
This commit adds tests for the default output  format (tree) of
the `inspect` sub-command.

Signed-off-by: Kouame Behouba Manasse <[email protected]>
This commit reduces code duplication between the table and the tree
rendering logics. The duplicated logic for retrieving checkpoint
information has been extracted into a reusable function:
`getCheckpointInfo`.

Signed-off-by: Kouame Behouba Manasse <[email protected]>
@rst0git rst0git merged commit 027219c into checkpoint-restore:main Jul 3, 2023
@behouba behouba mentioned this pull request Jul 4, 2023
@behouba behouba changed the title Add new inspect sub-command feat: add new inspect sub-command Aug 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Extend the output format with a tree view
5 participants