-
Notifications
You must be signed in to change notification settings - Fork 349
Fixup "criu: support mounts where dest is a symlink" #1758
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
Conversation
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]>
Reviewer's GuideRefactor CRIU root path handling by renaming internal variables, remove redundant rootfs setup in restore, and improve external bind mount resolution to handle symlink destinations and missing paths gracefully. Sequence Diagram: Updated External Mount Path Resolution in CRIU CheckpointsequenceDiagram
participant C as libcrun_container_checkpoint_linux_criu
participant CRR as chroot_realpath
participant LCW as libcriu_wrapper
C->>CRR: Call chroot_realpath(status->rootfs, mount->destination, buf)
CRR-->>C: Return resolved_path_absolute or NULL
alt resolved_path_absolute is NULL
C->>C: Check errno
alt errno is ENOENT
C->>C: dest_in_root = mount->destination
else errno is not ENOENT
C->>C: Handle error (unable to resolve mount)
end
else resolved_path_absolute is not NULL
C->>C: dest_in_root = resolved_path_absolute + strlen(status->rootfs)
end
C->>LCW: criu_add_ext_mount(dest_in_root, dest_in_root)
Sequence Diagram: Updated External Mount Path Resolution in CRIU RestoresequenceDiagram
participant R as libcrun_container_restore_linux_criu
participant CRR as chroot_realpath
participant LCW as libcriu_wrapper
R->>CRR: Call chroot_realpath(status->rootfs, mount->destination, buf)
CRR-->>R: Return resolved_path_absolute or NULL
alt resolved_path_absolute is NULL
R->>R: Check errno
alt errno is ENOENT
R->>R: dest_in_root = mount->destination
else errno is not ENOENT
R->>R: Handle error (unable to resolve mount)
end
else resolved_path_absolute is not NULL
R->>R: dest_in_root = resolved_path_absolute + strlen(status->rootfs)
end
R->>LCW: criu_add_ext_mount(dest_in_root, mount->source)
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
`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]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @kolyshkin - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
TMT tests failed. @containers/packit-build please check. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
podman checkpoint tests pass. So, LGTM from me.
Looks like centos stream 10 has issues with criu. I remember we've had issues with criu in the past on centos targets. Maybe we can ignore it here and I'll remove centos job in another PR. |
/packit retest-failed |
Account lsm5 has no write access nor is author of PR! |
It's only x86_64 failing, so let me know how you'd prefer to deal with that. I don't have access to retry the failed test. |
does not look like a flake, I've retried it |
I suggest we get this PR in asap as that would make CI green in podman and some other places where I'm trying to get revdep testing with podman. Maybe the cs10 test failure can be done elsewhere? I'd be happy to disable it for the time being if that helps. |
Ref: containers#1758 (comment) Issue filed: containers#1759 Signed-off-by: Lokesh Mandvekar <[email protected]>
Ref: containers#1758 (comment) Issue filed: containers#1759 Signed-off-by: Lokesh Mandvekar <[email protected]>
This fixes issues brought by #1747, and also cherry-picks #1756 so that the changes can be tested.
Summary by Sourcery
Fix CRIU checkpoint and restore procedures to support bind mounts where the destination in rootfs is a symlink
Bug Fixes:
Enhancements: