Fix runtime check during restore #14717
Conversation
ZeyadYasser
force-pushed
the
fix-restore-runtime-check
branch
4 times, most recently
from
6375949 to
8df3c60
Compare
|
Runtime verification test for container checkpoint with export used the default runtime for test which causes test to always pass. Problem rises when using non-default runtime, then doing a restore. |
|
@edsantiago PTAL |
ZeyadYasser
force-pushed
the
fix-restore-runtime-check
branch
from
8df3c60 to
a82ae61
Compare
|
Looks like |
ZeyadYasser
force-pushed
the
fix-restore-runtime-check
branch
3 times, most recently
from
f3c14d1 to
9663b20
Compare
UPDATE: I created a new test to address this problem. |
There was a problem hiding this comment.
Test LGTM overall, but needs some work. Thanks @ZeyadYasser
(and OBTW I suspect the team will ask you to squash your commits)
ZeyadYasser
force-pushed
the
fix-restore-runtime-check
branch
from
9663b20 to
b305389
Compare
Thanks @edsantiago for the feedback, I updated the test accordingly. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: edsantiago, ZeyadYasser 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 |
openshift-ci
bot
added
the
approved
|
Test LGTM, thanks again. I still think you're going to get some encouragement to squash, or at the very very least reorder the commits because in this order (tests first, then code) there is a nonfunctional commit. But I'll step back and defer to the team. |
|
LGTM but reverse your PRs. |
cfg.RuntimePath was set to default runtime, so the empty string check fails. Instead we could check if the flag was changed. Signed-off-by: Zeyad Yasser <[email protected]>
Runtime verification test for container checkpoint with export used the default runtime for test which causes test to always pass. Problem rises when using non-default runtime, then doing a restore. This test forcse using a non-default runtime during container creation. Edge case: 1. Default runtime is crun 2. Container is created with runc 3. Checkpoint without setting --runtime into archive 4. Restore without setting --runtime from archive It should be expected that podman identifies runtime from the checkpoint archive. Signed-off-by: Zeyad Yasser <[email protected]>
ZeyadYasser
force-pushed
the
fix-restore-runtime-check
branch
from
b305389 to
79a38a2
Compare
|
@mheon here we go again with the timeout. I've restarted anyway. |
|
@vrothberg Looks like we're seeing timeouts in |
|
At least seem to be significantly rarer than the other issues. I don't see any obvious cause, unfortunately. |
|
@mheon @vrothberg I'll file a bug as soon as this PR merges (at which time my flake collector will save the logs). CI is green, but I don't feel comfortable merging (tests LGTM but the code change needs someone more familiar with that code). |
|
/lgtm |
github-actions
bot
added
the
locked - please file new issue/PR
cfg.RuntimePath was set to default runtime, so the empty string check fails. Instead we could check if the flag was changed.
It is a minor bug that was already mentioned in the issue below, so I didn't think I should create a new issue. Please let me know if this is a requirement.
Fixes: #11945
Signed-off-by: Zeyad Yasser [email protected]
Does this PR introduce a user-facing change?