fix: Clarify error message for podman restore without --tcp-established#29026
fix: Clarify error message for podman restore without --tcp-established#29026simek-m wants to merge 1 commit into
Conversation
Newer version of crun use show_criu_log() to report errors from CRIU and output multiple error lines. It lead to confusing behavior in readConmonPipeData() in Podman, because the JSON Unmarshal() failed to parse multiple JSON objects over multiple lines. When podman restore was used without --tcp-established, a confusing error message: cgroupd: recv req error: No such file or directory was output, instead of the former: crun: CRIU restoring failed -52. Parse all errors from the log and try to match "Connected TCP socket in image". If found, return a clear error message. Update tests. Fixes: https://redhat.atlassian.net/browse/RHEL-186005 Signed-off-by: Marek Simek <msimek@redhat.com>
|
Do these work with both runc and crun? |
| if strings.Contains(e.Msg, "Connected TCP socket in image") { | ||
| return fmt.Errorf("checkpoint contains established TCP connections, restore requires --tcp-established or --tcp-close: %w", define.ErrOCIRuntime) | ||
| } | ||
| } |
There was a problem hiding this comment.
that does not seem maintainable, we should not make assumptions about the error text from crun or criu, it changes often enough. Sure the tests depend on it to some extend but we should not make the code depend, i.e. this may not work for another runtime.
It would be better to work with crun and criu to produce better errors to begin with.
Yes, it should work the same with both. [root@lima-default]~# podman container restore server
Error: OCI runtime error: checkpoint contains established TCP connections, restore requires --tcp-established or --tcp-close
[root@lima-default]~# podman info --format '{{.Host.OCIRuntime.Name}}'
runcIt's a CRIU error being matched.
I agree, but that's exactly what caused this bug report. if match := regexp.MustCompile("(?i).*executable file not found in.*|.*no such file or directory.*|.*open executable.*").FindString(runtimeMsg); match != "" {
errStr := match
if includeFullOutput {
errStr = runtimeMsg
}
return fmt.Errorf("%s: %s: %w", name, strings.Trim(errStr, "\n"), define.ErrOCIRuntimeNotFound)
}Unfortunately, there's AFAIK no specific error code returned by crun or CRIU in this case that could be used and that might not be stable either. Alternatively, the whole error output from crun could be shown to the user, or there can be no change at all - it still works as documented. But in my opinion and from the perspective of a user, I'd like to see a helpful error message in this case, even though it's documented in https://docs.podman.io/en/latest/markdown/podman-container-restore.1.html#tcp-established |
Newer versions of crun use
show_criu_log()to report errors from CRIU and output multipleerror lines. It lead to a confusing behavior in
readConmonPipeData()in Podman, becausethe JSON
Unmarshal()failed to parse multiple JSON objects over multiple lines. Whenpodman restorewas used without--tcp-establishedor--tcp-close, a confusing error message:was output, instead of the former:
In this PR, I parse all errors from the log and try to match
"Connected TCP socket in image". If found,
a clear error message is returned:
For other error messages, the existing behavior is retained. Tests were updated with the changed error message.
Steps to reproduce:
Fixes: https://redhat.atlassian.net/browse/RHEL-186005
Checklist
Ensure you have completed the following checklist for your pull request to be reviewed:
commits. (
git commit -s). (If needed, usegit commit -s --amend). The author email must matchthe sign-off email address. See CONTRIBUTING.md
for more information.
Fixes: #00000in commit message (if applicable)make validatepr(format/lint checks)Noneif no user-facing changes)Does this PR introduce a user-facing change?