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

Add support for checkpoint images with 'podman run' #16569

Merged

Conversation

rst0git
Copy link
Contributor

@rst0git rst0git commented Nov 21, 2022

This pull request is a follow up on #15868, #16068 and #16078. It extends the podman run command with support for checkpoint images. When podman run is invoked with an image that contains checkpoint annotation, it would invoke restore.

Example:

podman run -d --name looper busybox /bin/sh -c 'i=0; while true; do echo $i; i=$(expr $i + 1); sleep 1; done'
podman container checkpoint --create-image checkpoint-image-1 looper  
podman run checkpoint-image-1

Does this PR introduce a user-facing change?

The `podman run` command has been extended with support for checkpoint images.

@rst0git rst0git changed the title Run checkpoint image v2 Add support for checkpoint images with 'podman run' Nov 21, 2022
@TomSweeneyRedHat
Copy link
Member

quick glance and the changs look reasonable, but the test are very red and angry....

@rst0git rst0git force-pushed the run-checkpoint-image-v2 branch 6 times, most recently from e2b0ff2 to d37eca3 Compare November 29, 2022 07:34
@rst0git rst0git force-pushed the run-checkpoint-image-v2 branch from d37eca3 to 3b160a8 Compare November 29, 2022 11:19
@rst0git
Copy link
Contributor Author

rst0git commented Nov 29, 2022

The following CI failures look unrelated to the changes in this pull request.

Summarizing 4 Failures:

[Fail] Systemd activate [It] stop podman.service 
/var/tmp/go/src/github.com/containers/podman/test/e2e/systemd_activate_test.go:104

[Fail] Podman Info [It] Podman info: check desired network backend 
/var/tmp/go/src/github.com/containers/podman/test/e2e/info_test.go:177

[Fail] Podman Info [It] Podman info: check desired network backend 
/var/tmp/go/src/github.com/containers/podman/test/e2e/info_test.go:177

[Fail] Podman Info [It] Podman info: check desired network backend 
/var/tmp/go/src/github.com/containers/podman/test/e2e/info_test.go:177

Ran 1681 of 1868 Specs in 1258.135 seconds
FAIL! -- 1680 Passed | 1 Failed | 1 Flaked | 0 Pending | 187 Skipped

@edsantiago would you be able to confirm this?

@edsantiago
Copy link
Member

f37 root is a network flake, red herring. The common factor, then, is rootless, and it makes no sense. It looks like environment is being clobbered in weird ways. No other PRs are seeing this (i.e., it's likely not something that accident-merged into main), and there's one more weird commonality, a screwup in $CIRRUS_CHANGE_TITLE for all failing runs. I'm going to tentatively blame it on a Cirrus hiccup. I've restarted all failed jobs and will monitor.

@edsantiago edsantiago changed the title Add support for checkpoint images with 'podman run' Add support for checkpoint images with 'podman run' Nov 29, 2022
@edsantiago
Copy link
Member

Nope. My current hypothesis is that the trailing space in the title is getting backslash-escaped improperly in our environment passthrough. @rst0git that will take me some time to debug; ITM, to expedite this, could I ask you to re-push? I've fixed the title (removed trailing space) but it's still necessary to re-push for Cirrus to get the new title. You can git commit --amend, type a character, remove it, and save. That's enough to get a new commit hash for a push. Sorry for the inconvenience.

@rst0git rst0git force-pushed the run-checkpoint-image-v2 branch from 3b160a8 to 74ef164 Compare November 29, 2022 15:01
@rst0git
Copy link
Contributor Author

rst0git commented Nov 29, 2022

Thank you @edsantiago!

@edsantiago
Copy link
Member

Submitted PR #16678 to fix the trailing-whitespace bug. Whew! Thanks for pinging me about this, @rst0git.

@rst0git
Copy link
Contributor Author

rst0git commented Dec 1, 2022

@rhatdan @adrianreber PTAL

pkg/domain/infra/abi/containers.go Show resolved Hide resolved
pkg/domain/infra/abi/containers.go Outdated Show resolved Hide resolved
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Dec 2, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rhatdan, rst0git

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 2, 2022
These tests were unintentionally removed in commit
b47b48f (Revert "Add checkpoint image tests"). They
verify the functionality of the `--create-image` option for
`podman container checkpoint`.

Signed-off-by: Radostin Stoyanov <[email protected]>
This patch extends the podman run command with support for checkpoint
images. When `podman run` is invoked with an image that contains
a checkpoint, it would restore the container from that checkpoint.

Example:
    podman run -d --name looper busybox /bin/sh -c \
	    'i=0; while true; do echo $i; i=$(expr $i + 1); sleep 1; done'

    podman container checkpoint --create-image checkpoint-image-1 looper

    podman run checkpoint-image-1

Signed-off-by: Radostin Stoyanov <[email protected]>
@rst0git rst0git force-pushed the run-checkpoint-image-v2 branch from 74ef164 to a93a390 Compare December 2, 2022 14:39
@adrianreber
Copy link
Collaborator

Looks good to me.

@rhatdan
Copy link
Member

rhatdan commented Dec 5, 2022

My one concern with this, is it going to slow down the start of Podman. We are trying to speed up starting of containers, and if this has to inspect images on all containers before starting, this could slow it down.

@adrianreber
Copy link
Collaborator

My one concern with this, is it going to slow down the start of Podman. We are trying to speed up starting of containers, and if this has to inspect images on all containers before starting, this could slow it down.

Good point.

@rst0git If I understand the code correctly you are only looking at the annotations of the image and do not actually open the image to detect if it is a checkpoint image, right?

Could you also run a couple of timing tests with and without the tests. How much slower is run with and without your changes.

@rst0git
Copy link
Contributor Author

rst0git commented Dec 5, 2022

I used the following command to measure the performance of podman run, however, the difference in start-up time with and without the changes in this pull request appears to be negligible.

perf stat --repeat 10 podman run alpine:latest true

@rhatdan
Copy link
Member

rhatdan commented Dec 5, 2022

@alexlarsson PTAL
Potential to slow down container startup ,but also chance to speedup application start.

@adrianreber
Copy link
Collaborator

I used the following command to measure the performance of podman run, however, the difference in start-up time with and without the changes in this pull request appears to be negligible.

perf stat --repeat 10 podman run alpine:latest true

Can you share your numbers?

@rst0git
Copy link
Contributor Author

rst0git commented Dec 6, 2022

Can you share your numbers?

Yes, I've uploaded the results in https://gist.github.com/rst0git/c3ee40feaa36eb9e5c64750aa92f3b0d

@adrianreber
Copy link
Collaborator

adrianreber commented Dec 6, 2022

The numbers look the same for both cases. So just looking at an annotation as your change does, does not seem to introduce a measurable delay.

Not sure if there is some caching going in Podman when it comes to image annotations. Maybe somebody else can give an additional interpretation for your numbers.

@edsantiago
Copy link
Member

This is probably a very stupid comment, but: https://gist.github.com/rst0git/c3ee40feaa36eb9e5c64750aa92f3b0d#file-test-sh-L5

...I would feel so much more comfortable with your results if the string podman were replaced with $PODMAN, and there were two lines immediately above, like

PODMAN=$(pwd)/bin/podman
$PODMAN version

@rst0git
Copy link
Contributor Author

rst0git commented Dec 6, 2022

@edsantiago Thank you for the advice! I've updated the evaluation script:
https://gist.github.com/rst0git/c3ee40feaa36eb9e5c64750aa92f3b0d

@rhatdan
Copy link
Member

rhatdan commented Dec 7, 2022

/lgtm
If this is found to slow things down, we might need to add an option to disable/enable it, but for now I will merge.
Thanks @rst0git

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Dec 7, 2022
@openshift-merge-robot openshift-merge-robot merged commit 4096d04 into containers:main Dec 7, 2022
@rst0git rst0git deleted the run-checkpoint-image-v2 branch December 7, 2022 17:16
@github-actions github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 18, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 18, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. release-note
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants