Re-enable CRIU tests by not using overlayfs snapshotter#4708
Re-enable CRIU tests by not using overlayfs snapshotter#4708crosbymichael merged 2 commits intocontainerd:masterfrom
Conversation
|
Hi @kzys. Thanks for your PR. I'm waiting for a containerd member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
Hmm, let me check the kernel version on the GitHub Actions' Ubuntu. |
|
Ubuntu 14.04 uses |
|
I don't think this will work based on ongoing commentary in checkpoint-restore/criu#860 (see this comment). The upstream bug is confusing as it has been marked fixed at least once or twice, but is clearly still an issue. The recommendation in checkpoint-restore/criu#1239 (comment) might be worth considering--should we run just the checkpoint/restore tests to make sure our code is still working by running a quick test with a different graphdriver? |
|
Thanks @estesp. How about running the tests with multiple different snapshotters? criu may need devmappper, but I'm bit concerned to move containerd's tests from overlay to let's say devmapper entirely because of criu. |
|
Yes, I think we can build a small action that only runs the criu tests with a native/devicemapper snapshotter and let the rest of the testsuite continue using overlay. |
kzys
force-pushed
the
enable-criu
branch
6 times, most recently
from
6bace7f to
4213735
Compare
kzys
changed the title
kzys
force-pushed
the
enable-criu
branch
2 times, most recently
from
a8a3388 to
fd8bc31
Compare
|
containerd/runtime/v2/runc/container.go Line 53 in f1a469a |
There was a problem hiding this comment.
I'm unsure this is the right place to fix this issue. Shouldn't it be handled by typeurl.UnmarshalAny()?
There was a problem hiding this comment.
Ya, i think this is fine. It should be handled by the caller because of the nil check. I'll have to think a bit on a better way to fix this in the lib but it shouldn't be common.
|
@estesp Could you take a look? Seems like CRIU has been broken. |
|
Build succeeded.
|
|
Sorry @kzys this needs a rebase due to the PR which moved *_test.go from the root being merged. Also, thinking about the time cost of having yet another pass through |
|
@estesp Sounds good to me. Let me update the PR. |
kzys
force-pushed
the
enable-criu
branch
2 times, most recently
from
706a99c to
36afcb7
Compare
|
Build succeeded.
|
|
Rebased! Not so sure why "CGroupsV2 and SELinux Integration" is failing here. |
429 Too Many Requests. ... we're trying to work on this.. it's the new docker.io pull rate limits |
|
@estesp Can you take a look? |
- process.Init#io could be nil - Make sure CreateTaskRequest#Options is not empty before unmarshaling Signed-off-by: Kazuyoshi Kato <[email protected]>
While the issue hasn't been fixed in the kernel yet, we can workaround the issue by not using overlayfs snapshotter. The newly added step runs all tests that match /TestCheckpoint/. So, TestCRWithImagePath has been renamed to match the regexp. Fixes containerd#3930. Signed-off-by: Kazuyoshi Kato <[email protected]>
|
Build succeeded.
|
estesp
left a comment
estesp
left a comment
There was a problem hiding this comment.
LGTM; would love @crosbymichael to confirm the options typeurl change
|
/ok-to-test |
5 participants
While the issue hasn't been fixed in the kernel yet, we can workaround
the issue by not using overlayfs snapshotter.
Fixes #3930.
Signed-off-by: Kazuyoshi Kato [email protected]