Skip to content

libpod: avoid polling container status#3933

Merged
openshift-merge-robot merged 1 commit into
containers:masterfrom
giuseppe:skip-polling-on-run
Sep 4, 2019
Merged

libpod: avoid polling container status#3933
openshift-merge-robot merged 1 commit into
containers:masterfrom
giuseppe:skip-polling-on-run

Conversation

@giuseppe
Copy link
Copy Markdown
Member

@giuseppe giuseppe commented Sep 4, 2019

use the inotify backend to be notified on the container exit instead
of polling continuosly the runtime. Polling the runtime slowns
significantly down the podman execution time for short lived
processes:

$ time bin/podman run --rm -ti fedora true

real 0m0.324s
user 0m0.088s
sys 0m0.064s

from:

$ time podman run --rm -ti fedora true

real 0m4.199s
user 0m5.339s
sys 0m0.344s

Signed-off-by: Giuseppe Scrivano gscrivan@redhat.com

@openshift-ci-robot
Copy link
Copy Markdown
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: giuseppe

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci-robot openshift-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/S labels Sep 4, 2019
Comment thread libpod/container_api.go Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we still need this, if we're doing the waiting for the file above? I'd just as soon drop the polling wait and convert entirely to WaitForFile()

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tests are failing as they get stuck in WaitForFile. I was just rewriting it to use both (in a select block). What do you think?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't WaitForFile already do a poll alongside the inotify? I don't think we need to keep another one here. Might want to let you set the interval on the polling in WaitForFile though.

Failures are probably the file never appearing due to --rm - we'll need a timeout on this.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed in the last version I've pushed

@giuseppe giuseppe force-pushed the skip-polling-on-run branch from 00064d0 to 64152a4 Compare September 4, 2019 13:29
@giuseppe giuseppe force-pushed the skip-polling-on-run branch 2 times, most recently from 8367617 to 354b762 Compare September 4, 2019 15:10
@giuseppe giuseppe force-pushed the skip-polling-on-run branch 2 times, most recently from a387224 to d96c3af Compare September 4, 2019 17:16
use the inotify backend to be notified on the container exit instead
of polling continuosly the runtime.  Polling the runtime slowns
significantly down the podman execution time for short lived
processes:

$ time bin/podman run --rm -ti fedora true

real	0m0.324s
user	0m0.088s
sys	0m0.064s

from:

$ time podman run --rm -ti fedora true

real	0m4.199s
user	0m5.339s
sys	0m0.344s

Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
@giuseppe giuseppe force-pushed the skip-polling-on-run branch from d96c3af to 8e337af Compare September 4, 2019 17:56
@mheon
Copy link
Copy Markdown
Member

mheon commented Sep 4, 2019

LGTM on my end - nice work @giuseppe

@rhatdan
Copy link
Copy Markdown
Member

rhatdan commented Sep 4, 2019

/lgtm
Awesome job @giuseppe

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 4, 2019
@openshift-merge-robot openshift-merge-robot merged commit 52f2454 into containers:master Sep 4, 2019
@github-actions github-actions Bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 26, 2023
@github-actions github-actions Bot locked as resolved and limited conversation to collaborators Sep 26, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants