Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cmd/run: Optimize 'enter' and 'run' for already running containers, and turn IsToolboxContainer() into Container.IsToolbx() #1491

Conversation

debarshiray
Copy link
Member

@debarshiray debarshiray requested a review from HarryMichal as a code owner May 16, 2024 15:23
@debarshiray debarshiray changed the title cmd/run: Optimize 'enter' and 'run' for already running containers, and turn IsToolboxContainer() into Container.IsToolbx() [WIP] cmd/run: Optimize 'enter' and 'run' for already running containers, and turn IsToolboxContainer() into Container.IsToolbx() May 16, 2024
@debarshiray debarshiray marked this pull request as draft May 16, 2024 15:24
debarshiray added a commit to debarshiray/toolbox that referenced this pull request May 16, 2024
@debarshiray debarshiray force-pushed the wip/rishi/cmd-pkg-podman-optimize-enter-run-is-toolbx branch from a33e656 to 47be32d Compare May 16, 2024 15:24
Copy link

Build failed.
https://softwarefactory-project.io/zuul/t/local/buildset/71dc5cb75937401896b684d41b607aa4

✔️ unit-test SUCCESS in 6m 55s
unit-test-migration-path-for-coreos-toolbox FAILURE in 3m 39s
✔️ unit-test-restricted SUCCESS in 5m 41s
✔️ system-test-fedora-rawhide SUCCESS in 42m 21s
✔️ system-test-fedora-40 SUCCESS in 40m 34s
✔️ system-test-fedora-39 SUCCESS in 34m 17s
✔️ system-test-fedora-38 SUCCESS in 40m 40s

debarshiray added a commit to debarshiray/toolbox that referenced this pull request May 16, 2024
debarshiray added a commit to debarshiray/toolbox that referenced this pull request May 16, 2024
This makes it possible to confine the details of detecting a Toolbx
container within the podman package, because it was not possible to use
podman.IsToolboxContainer() when listing all the Toolbx containers.

containers#1491
@debarshiray debarshiray force-pushed the wip/rishi/cmd-pkg-podman-optimize-enter-run-is-toolbx branch from 47be32d to 4ed3c72 Compare May 16, 2024 18:17
Copy link

Build failed.
https://softwarefactory-project.io/zuul/t/local/buildset/a4431f5aa76b4d488edea5b135479c95

✔️ unit-test SUCCESS in 7m 06s
✔️ unit-test-migration-path-for-coreos-toolbox SUCCESS in 3m 27s
✔️ unit-test-restricted SUCCESS in 5m 58s
system-test-fedora-rawhide FAILURE in 39m 33s
system-test-fedora-40 FAILURE in 37m 26s
system-test-fedora-39 FAILURE in 37m 22s
system-test-fedora-38 FAILURE in 34m 09s

debarshiray added a commit to debarshiray/toolbox that referenced this pull request May 17, 2024
This makes it possible to confine the details of detecting a Toolbx
container within the podman package, because it was not possible to use
podman.IsToolboxContainer() when listing all the Toolbx containers.

containers#1491
@debarshiray debarshiray force-pushed the wip/rishi/cmd-pkg-podman-optimize-enter-run-is-toolbx branch from 4ed3c72 to 13627cc Compare May 17, 2024 09:43
@debarshiray
Copy link
Member Author

Build failed. https://softwarefactory-project.io/zuul/t/local/buildset/a4431f5aa76b4d488edea5b135479c95

system-test-fedora-rawhide FAILURE in 39m 33s ❌ system-test-fedora-40 FAILURE in 37m 26s ❌ system-test-fedora-39 FAILURE in 37m 22s ❌ system-test-fedora-38 FAILURE in 34m 09s

TASK [Run system tests]
...
fedora-rawhide | 1..343
fedora-rawhide | # test suite: Set up
...
fedora-rawhide | not ok 179 rm: Try to remove a non-existent container in 462ms
fedora-rawhide | # (from function `assert_output' in file test/system/libs/bats-assert/src/assert.bash, line 255,
fedora-rawhide | #  in test file test/system/106-rm.bats, line 37)
fedora-rawhide | #   `assert_output "Error: failed to inspect container $container_name"' failed
fedora-rawhide | #
fedora-rawhide | # -- output differs --
fedora-rawhide | # expected : Error: failed to inspect container nonexistentcontainer
fedora-rawhide | # actual   : Error: failed to invoke podman(1)
fedora-rawhide | # --
fedora-rawhide | #

Copy link

Build succeeded.
https://softwarefactory-project.io/zuul/t/local/buildset/dda1492433f7453d8757bac23d9af918

✔️ unit-test SUCCESS in 7m 05s
✔️ unit-test-migration-path-for-coreos-toolbox SUCCESS in 3m 30s
✔️ unit-test-restricted SUCCESS in 6m 04s
✔️ system-test-fedora-rawhide SUCCESS in 36m 45s
✔️ system-test-fedora-40 SUCCESS in 34m 25s
✔️ system-test-fedora-39 SUCCESS in 34m 41s
✔️ system-test-fedora-38 SUCCESS in 35m 21s

@debarshiray debarshiray changed the title [WIP] cmd/run: Optimize 'enter' and 'run' for already running containers, and turn IsToolboxContainer() into Container.IsToolbx() cmd/run: Optimize 'enter' and 'run' for already running containers, and turn IsToolboxContainer() into Container.IsToolbx() May 17, 2024
@debarshiray debarshiray marked this pull request as ready for review May 17, 2024 10:28
Copy link

Build succeeded.
https://softwarefactory-project.io/zuul/t/local/buildset/9ba8497b17ed4da9892a9b40ea77b228

✔️ unit-test SUCCESS in 6m 42s
✔️ unit-test-migration-path-for-coreos-toolbox SUCCESS in 3m 31s
✔️ unit-test-restricted SUCCESS in 6m 02s
✔️ system-test-fedora-rawhide SUCCESS in 36m 20s
✔️ system-test-fedora-40 SUCCESS in 35m 59s
✔️ system-test-fedora-39 SUCCESS in 37m 07s
✔️ system-test-fedora-38 SUCCESS in 36m 24s

This makes it possible to confine the details of detecting a Toolbx
container within the podman package, because it was not possible to use
podman.IsToolboxContainer() when listing all the Toolbx containers.

containers#1491
Currently, the 'enter' and 'run' commands always invoke 'podman start'
even if the Toolbx container's entry point is already running.  There's
no need for that.  The commands already invoke 'podman inspect' to find
out if the org.freedesktop.Flatpak.SessionHelper D-Bus service needs to
be started.  Thus, they already have what is needed to find out if the
container is stopped and 'podman start' is necessary before it can be
used with 'podman exec', or if it's already running.

The unconditional 'podman start' invocation was followed by a second
'podman inspect' invocation to find out if the 'podman start' managed to
start the container's entry point.  There's no need for this second
'podman inspect' either, just like the 'podman start', when it's already
known from the first 'podman inspect' that the container is running.

The extra 'podman start' and 'podman inspect' invocations are
sufficiently expensive to add a noticeable overhead to the 'enter' and
'run' commands.  It's common to use a container that's already running,
just like having multiple terminals within the same working directory,
and terminal emulation applications like Ptyxis try to make it easier to
do so [1].  Therefore, it's worth optimizing this code path.

[1] https://gitlab.gnome.org/chergert/ptyxis
    https://flathub.org/apps/app.devsuite.Ptyxis

containers#1070
@debarshiray debarshiray force-pushed the wip/rishi/cmd-pkg-podman-optimize-enter-run-is-toolbx branch from a120b68 to c1d30f4 Compare May 19, 2024 21:35
Copy link

Build succeeded.
https://softwarefactory-project.io/zuul/t/local/buildset/d98ee29bf25744b08623c2ef997771c6

✔️ unit-test SUCCESS in 6m 50s
✔️ unit-test-migration-path-for-coreos-toolbox SUCCESS in 3m 22s
✔️ unit-test-restricted SUCCESS in 4m 46s
✔️ system-test-fedora-rawhide SUCCESS in 37m 35s
✔️ system-test-fedora-40 SUCCESS in 35m 46s
✔️ system-test-fedora-39 SUCCESS in 34m 38s
✔️ system-test-fedora-38 SUCCESS in 36m 02s

Copy link

Build succeeded.
https://softwarefactory-project.io/zuul/t/local/buildset/8935764a9ede4fd8a17d41a1f7a8be81

✔️ unit-test SUCCESS in 6m 28s
✔️ unit-test-migration-path-for-coreos-toolbox SUCCESS in 4m 06s
✔️ unit-test-restricted SUCCESS in 5m 54s
✔️ system-test-fedora-rawhide SUCCESS in 41m 28s
✔️ system-test-fedora-40 SUCCESS in 36m 03s
✔️ system-test-fedora-39 SUCCESS in 36m 25s
✔️ system-test-fedora-38 SUCCESS in 35m 34s

Copy link

Build succeeded.
https://softwarefactory-project.io/zuul/t/local/buildset/dc12f0bb935340f9a0c7aec0d2dc1159

✔️ unit-test SUCCESS in 7m 02s
✔️ unit-test-migration-path-for-coreos-toolbox SUCCESS in 3m 23s
✔️ unit-test-restricted SUCCESS in 5m 47s
✔️ system-test-fedora-rawhide SUCCESS in 36m 12s
✔️ system-test-fedora-40 SUCCESS in 33m 54s
✔️ system-test-fedora-39 SUCCESS in 34m 38s
✔️ system-test-fedora-38 SUCCESS in 34m 22s

@debarshiray debarshiray merged commit 74d4fcf into containers:main May 20, 2024
3 checks passed
@debarshiray debarshiray deleted the wip/rishi/cmd-pkg-podman-optimize-enter-run-is-toolbx branch May 20, 2024 09:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant