Skip to content

TMT: include podman checkpoint system tests #1756

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

Closed
wants to merge 3 commits into from

Conversation

lsm5
Copy link
Member

@lsm5 lsm5 commented May 21, 2025

podman checkpoint... tests depend on crun as well so they should be included in the TMT system test runs. These are currently failing on podman upstream PRs, for example: 0, 1 and 2.

Summary by Sourcery

Tests:

  • Add invocation of the 520-checkpoint.bats script to enable checkpoint tests during TMT runs

Copy link

sourcery-ai bot commented May 21, 2025

Reviewer's Guide

Extend the Podman TMT system test script to include the checkpoint test by invoking the new '520-checkpoint.bats' case.

File-Level Changes

Change Details Files
Integrate checkpoint test into system-test.sh
  • Add ‘bats -t /usr/share/podman/test/system/520-checkpoint.bats’ invocation
tests/tmt/podman/system-test.sh

Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it. You can also reply to a
    review comment with @sourcery-ai issue to create an issue from it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time. You can also comment
    @sourcery-ai title on the pull request to (re-)generate the title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time exactly where you
    want it. You can also comment @sourcery-ai summary on the pull request to
    (re-)generate the summary at any time.
  • Generate reviewer's guide: Comment @sourcery-ai guide on the pull
    request to (re-)generate the reviewer's guide at any time.
  • Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
    pull request to resolve all Sourcery comments. Useful if you've already
    addressed all the comments and don't want to see them anymore.
  • Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
    request to dismiss all existing Sourcery reviews. Especially useful if you
    want to start fresh with a new review - don't forget to comment
    @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

Copy link

TMT tests failed. @containers/packit-build please check.

@lsm5 lsm5 force-pushed the tmt-podman-checkpoint branch 2 times, most recently from 3d043f8 to d2aa05f Compare May 21, 2025 19:46
@giuseppe
Copy link
Member

the failure seems to be a regression introduced with e5d2489

@kolyshkin FYI

@giuseppe
Copy link
Member

maybe the bisect was not correct, the failure still happens if I revert the patch. Repeating it

@giuseppe
Copy link
Member

now I get e0b0158 and indeed there is no error if I revert it

@giuseppe
Copy link
Member

do you think we need something like?

diff --git a/src/libcrun/criu.c b/src/libcrun/criu.c
index 2fbcf552..6039be87 100644
--- a/src/libcrun/criu.c
+++ b/src/libcrun/criu.c
@@ -587,9 +587,10 @@ libcrun_container_checkpoint_linux_criu (libcrun_container_status_t *status, lib
           const char *dest_in_root;
 
           dest_in_root = chroot_realpath (rootfs, def->mounts[i]->destination, buf);
-          if (UNLIKELY (dest_in_root == NULL))
-            return crun_make_error (err, errno, "unable to resolve external bind mount `%s` under rootfs", def->mounts[i]->destination);
-          dest_in_root += strlen (rootfs);
+          if (LIKELY (dest_in_root != NULL))
+            dest_in_root += strlen (rootfs);
+          else
+            dest_in_root = def->mounts[i]->destination;
 
           ret = libcriu_wrapper->criu_add_ext_mount (dest_in_root, dest_in_root);
           if (UNLIKELY (ret < 0))
@@ -916,9 +917,10 @@ libcrun_container_restore_linux_criu (libcrun_container_status_t *status, libcru
           const char *dest_in_root;
 
           dest_in_root = chroot_realpath (rootfs, def->mounts[i]->destination, buf);
-          if (UNLIKELY (dest_in_root == NULL))
-            return crun_make_error (err, errno, "unable to resolve external bind mount `%s` under rootfs", def->mounts[i]->destination);
-          dest_in_root += strlen (rootfs);
+          if (LIKELY (dest_in_root != NULL))
+            dest_in_root += strlen (rootfs);
+          else
+            dest_in_root = def->mounts[i]->destination;
 
           ret = libcriu_wrapper->criu_add_ext_mount (dest_in_root, def->mounts[i]->source);
           if (UNLIKELY (ret < 0))

@giuseppe
Copy link
Member

proposed patch: #1757

kolyshkin and others added 3 commits May 21, 2025 20:00
Commit e0b0158 "criu: support mounts where dest is a symlink" brought
in a few issues.

First, the concatenation of status->bundle and status->rootfs do not
make any sense (and I'm not yet sure why it is used as CRIU root).
Revert to using status->rootfs directly.

Second, if chroot_resolve returns ENOENT, it is OK (not all paths are
visible from the host -- some are only inside the mount namespace).
In this case, use the destination as is.

This should fix podman checkpoint failures.

Signed-off-by: Kir Kolyshkin <[email protected]>
This reverts commit 55498c1.

Signed-off-by: Kir Kolyshkin <[email protected]>
`podman checkpoint...` tests depend on crun as well so they should be
included in the TMT system test runs. These are currently failing on
podman upstream PRs, for example: [0], [1] and [2].

[0]: https://artifacts.dev.testing-farm.io/fd5c08eb-2b0d-4ea8-9c33-ac9bf3447bd8/
[1]: https://artifacts.dev.testing-farm.io/e227ef00-c5c4-43e6-9785-0b5b5fcd2908/
[2]: https://artifacts.dev.testing-farm.io/c4ef6e7a-3b5d-4afc-91b9-dd477ef699e3/

Signed-off-by: Lokesh Mandvekar <[email protected]>
@giuseppe
Copy link
Member

@lsm5 are you fine to close this one in favor of #1758?

@kolyshkin FYI

@lsm5
Copy link
Member Author

lsm5 commented May 22, 2025

ah i didn't notice this change was included in #1758. Closing..

@lsm5 lsm5 closed this May 22, 2025
@lsm5 lsm5 deleted the tmt-podman-checkpoint branch May 22, 2025 12:32
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.

3 participants