Fix slow IsCheckpointImage making podman run slow #16068
Conversation
|
@alexlarsson: Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it. Instructions 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. |
openshift-ci
bot
added
the
do-not-merge/release-note-label-needed
alexlarsson
force-pushed
the
fix-slow-ischeckpointimage
branch
from
5f8c5d8 to
99fc9af
Compare
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: alexlarsson, giuseppe 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
rhatdan
removed
the
do-not-merge/release-note-label-needed
|
I think this is correct, although I am worried about podman-remote versus podman. Since I think this is only getting the OCI Runtime from the client. I am not sure if this runtime is sent to the server side. Calling info for this information does seem to be way overkill though. @mheon PTAL |
|
I now believe this function should be called on the server side not on the client side anyways. |
|
@rst0git PTAL |
alexlarsson
force-pushed
the
fix-slow-ischeckpointimage
branch
from
99fc9af to
7c12081
Compare
IsCheckpointImage() currently calls registry.ContainerEngine().Info(ctx) in order to get the name of the oci runtime. However, this call is very slow, as it collects all sort of other (unnecessary) information. In addition, this returns the *default* oci runtime, and really it seems to be more correct here to use the *currently-in-use* oci runtime. So, we switch to getting the oci runtime name via: registry.PodmanConfig().RuntimePath This seems to return the same value, and is much faster. The actual runtime is highly variable, so I'm taking the fastest run of 20 as a measure. Before this change: $ time ./bin/podman run fedora true real 0m0,556s user 0m0,155s sys 0m0,090s After the change: $ time ./bin/podman run fedora true real 0m0,456s user 0m0,101s sys 0m0,042s So, this seems to be about a 100msec performance improvement. Signed-off-by: Alexander Larsson <[email protected]>
alexlarsson
force-pushed
the
fix-slow-ischeckpointimage
branch
from
7c12081 to
e0d8d3b
Compare
|
You need to add |
| if err != nil { | ||
| return false, err | ||
| } | ||
| runtimeName := filepath.Base(registry.PodmanConfig().RuntimePath) |
There was a problem hiding this comment.
RuntimePath is legacy at this point - you want OCIRuntime. It should already be the name of the runtime as used by Podman, not a path that needs to be trimmed.
There was a problem hiding this comment.
Would OCIRuntime be set to the value specified with --runtime when it is used?
There was a problem hiding this comment.
I'm not sure what you mean. How would i get the OCIRuntime.
For the name, here is what we currently get by default:
hostinfo.OCIRuntime.Name: crun
registry.PodmanConfig().RuntimePath: crun
And with --runtime runc:
hostinfo.OCIRuntime.Name: runc
registry.PodmanConfig().RuntimePath: runc
And with --runtime /usr/bin/crun
hostinfo.OCIRuntime.Name: /usr/bin/crun
registry.PodmanConfig().RuntimePath: /usr/bin/crun
Given the above it seems like the current/old code (using hostinfo) doesn't apply filepath.Base(). Does that mean it is wrong?
There was a problem hiding this comment.
PodmanConfig should be https://github.com/containers/common/blob/main/pkg/config/config.go#L68
In there, you can see that RuntimePath is deprecated (https://github.com/containers/common/blob/main/pkg/config/config.go#L388-L392) while OCIRuntime is the replacement (https://github.com/containers/common/blob/main/pkg/config/config.go#L355-L356)
There was a problem hiding this comment.
That doesn't seem to give the right thing though.
If I use registry.PodmanConfig().Engine.OCIRuntime, then by default that contains crun which is fine.
However if i run podman run --runtime=runc, it still contains crun, which strikes me as wrong.
There was a problem hiding this comment.
You can't get that from the config... You'd need to grab Libpod's DefaultOCIRuntime to know that.
If RuntimePath in the config is actually reflecting that, that's a bug, not intended behavior.
There was a problem hiding this comment.
Also, this probably doesn't work properly for podman-remote as implemented, now that I think about it... You'll get the config from the client, not the server.
There was a problem hiding this comment.
That is what I said above, this needs to be checked on the server side not the client.
|
I believe the original patch for checkpoint needs to be reverted, I opened a PR to do this and will close this PR. |
github-actions
bot
added
the
locked - please file new issue/PR
IsCheckpointImage() currently calls registry.ContainerEngine().Info(ctx) in order to get the name of the oci runtime. However, this call is very slow, as it collects all sort of other (unnecessary) information.
In addition, this returns the default oci runtime, and really it seems to be more correct here to use the currently-in-use oci runtime.
So, we switch to getting the oci runtime name via:
registry.PodmanConfig().RuntimePath
This seems to return the same value, and is much faster. The actual runtime is highly variable, so I'm taking the fastest run of 20 as a measure.
Before this change:
$ time ./bin/podman run fedora true
real 0m0,556s
user 0m0,155s
sys 0m0,090s
After the change:
$ time ./bin/podman run fedora true
real 0m0,456s
user 0m0,101s
sys 0m0,042s
So, this seems to be about a 100msec performance improvement.