test/system: use a random port for the local Docker registry - #1822
test/system: use a random port for the local Docker registry#1822Rolv-Apneseth wants to merge 1 commit into
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the BATS helper script to dynamically assign and persist the Docker registry URI using a randomly assigned host port, rather than using a hardcoded port. This prevents port conflicts during parallel test execution. The review feedback points out that using run followed by assert_success inside _setup_docker_registry is a BATS anti-pattern and can lead to undefined behavior, suggesting executing the commands directly instead.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| run mkdir -p "$HOME"/.config/containers/certs.d/"${DOCKER_REG_URI}" | ||
| assert_success | ||
| run cp "${DOCKER_REG_CERTS_DIR}"/domain.crt "$HOME"/.config/containers/certs.d/"${DOCKER_REG_URI}"/domain.crt | ||
| assert_success |
There was a problem hiding this comment.
According to the repository's general rules, using run followed immediately by assert_success is an anti-pattern in BATS. Additionally, run should not be used inside setup_suite() or functions called by it (such as _setup_docker_registry()), because BATS variables like BATS_TEST_TMPDIR are not defined in that context, which can lead to undefined behavior. These commands should be executed directly instead.
mkdir -p "$HOME"/.config/containers/certs.d/"${DOCKER_REG_URI}"
cp "${DOCKER_REG_CERTS_DIR}"/domain.crt "$HOME"/.config/containers/certs.d/"${DOCKER_REG_URI}"/domain.crt
References
- In BATS (Bash Automated Testing System), avoid using 'run' followed immediately by 'assert_success' as it is considered an anti-pattern. Additionally, avoid using 'run' inside 'setup_suite()' or functions called by it, because BATS variables like 'BATS_TEST_TMPDIR' are not defined in that context, which can lead to undefined behavior.
There was a problem hiding this comment.
That's pre-existing code that I just moved, but that makes sense, I could make that change
There was a problem hiding this comment.
Yes, @Rolv-Apneseth is right. These uses of run already existed in the code. I took the liberty to remove some of these in #1825
I couldn't remove all of them because I was running out of time testing all the error paths. Feel free to submit a pull request removing the rest. :)
82653ee to
d051389
Compare
|
Rebased after #1824 |
debarshiray
left a comment
debarshiray
left a comment
There was a problem hiding this comment.
Thanks for working on this, @Rolv-Apneseth ! I had never used podman port before - cool. :)
Hard coding the port to 50000 can cause intermittent failures when the port happens to already be in use, and prevents running the test suite more than once at the same time. Let Podman pick a random available port and persist it to a file so test processes in the same run can find it. containers#1822 Signed-off-by: Rolv Apneseth <rolv.apneseth@gmail.com>
|
To keep things moving, I took the liberty to rebase this against |
|
Network flake? |
|
recheck |
It can't be. The
I saw this once recently. Without looking at the code, the |
Ah, probably a good idea yes |
62611bc to
b55b407
Compare
The 60 second timeouts might not be quite enough for local 'skopeo copy'
because they sometimes fail with:
not ok 187 run: Smoke test with 'exit 2'
# tags: commands-options
# (from function `assert_success' in file
test/system/libs/bats-assert/src/assert.bash, line 114,
# from function `pull_distro_image' in file
test/system/libs/helpers.bash, line 391,
# from function `pull_default_image' in file
test/system/libs/helpers.bash, line 402,
# from function `create_default_container' in file
test/system/libs/helpers.bash, line 470,
# in test file test/system/104-run.bats, line 806)
# `create_default_container' failed
# ~ /home/zuul-worker/src/github.com/containers/toolbox
# Failed to load image registry.fedoraproject.org/fedora-toolbox:45
from cache
/var/tmp/bats-run-yokdp3/suite/image-cache/fedora-toolbox-45
#
# -- command failed --
# status : 1
# output (3 lines):
# Getting image source signatures
# Copying blob sha256:8e0fe36f464ab46d2ce4ea3a1f2779c3921598ebf824e7
# time="2026-07-28T00:08:31Z" level=fatal msg="copying config:
context deadline exceeded"
# --
#
So, double the timeouts to 120 seconds.
containers#1822
containers#1826
Signed-off-by: Rolv Apneseth <rolv.apneseth@gmail.com>
debarshiray
left a comment
debarshiray
left a comment
There was a problem hiding this comment.
Thanks for bumping the timeouts for local skopeo copy! I merged it through #1826 so that we can wait a day or two to see if the Bats folks respond to bats-core/bats-core#1230
Hard coding the port to 50000 can cause intermittent failures when the port happens to already be in use, and prevents running the test suite more than once at the same time. Let Podman pick a random available port and persist it to a file so test processes in the same run can find it. containers#1822 Signed-off-by: Rolv Apneseth <rolv.apneseth@gmail.com>
|
I took the liberty to rebase this PR on top of |
|
When setting up caching for images, if an iteration of the retry loop to
download images to the cache fails, it leaves behind a stale cache which
will cause the next attempt to also fail with:
not ok 1 setup_suite
# (from function `_pull_and_cache_distro_image' in file
test/system/libs/helpers.bash, line 139,
# from function `setup_suite' in test file
test/system/setup_suite.bash, line 59)
# `_pull_and_cache_distro_image ubuntu 20.04' failed
# Failed to cache image quay.io/toolbx/ubuntu-toolbox:20.04 to
/tmp/bats-run-3FdzJ3/suite/image-cache/ubuntu-toolbox-20.04
# time="2026-07-28T22:40:16Z" level=fatal msg="initializing
destination
dir:/tmp/bats-run-3FdzJ3/suite/image-cache/ubuntu-toolbox-20.04:
not a containers image directory, don't want to overwrite
important data"
This is because skopeo(1) will refuse to write to the directory saying
it's not a valid containers image directory. Specifically, this is the
ErrNotContainerImageDir error from the go.podman.io/image/v5/directory
package [1].
Add a cleanup step to remove the directory before the next attempt to
avoid the issue.
[1] https://pkg.go.dev/go.podman.io/image/v5/directory
containers#1822
containers#1828
Signed-off-by: Rolv Apneseth <rolv.apneseth@gmail.com>
Oops! You keep finding some cool corner cases. It's ErrNotContainerImageDir from the |
39c3d55 to
a520372
Compare
When setting up caching for images, if an iteration of the retry loop to
download images to the cache fails, it leaves behind a stale cache which
will cause the next attempt to also fail with:
not ok 1 setup_suite
# (from function `_pull_and_cache_distro_image' in file
test/system/libs/helpers.bash, line 139,
# from function `setup_suite' in test file
test/system/setup_suite.bash, line 59)
# `_pull_and_cache_distro_image ubuntu 20.04' failed
# Failed to cache image quay.io/toolbx/ubuntu-toolbox:20.04 to
/tmp/bats-run-3FdzJ3/suite/image-cache/ubuntu-toolbox-20.04
# time="2026-07-28T22:40:16Z" level=fatal msg="initializing
destination
dir:/tmp/bats-run-3FdzJ3/suite/image-cache/ubuntu-toolbox-20.04:
not a containers image directory, don't want to overwrite
important data"
This is because skopeo(1) will refuse to write to the directory saying
it's not a valid containers image directory. Specifically, this is the
ErrNotContainerImageDir error from the go.podman.io/image/v5/directory
package [1].
Add a clean-up step to remove the directory before the next attempt to
avoid the issue.
Shell parameter expansion [2] is used to ensure that rm(1) doesn't
recursively delete everything in the host operating system's root
directory [3] due to future programmer errors.
[1] https://pkg.go.dev/go.podman.io/image/v5/directory
[2] https://www.gnu.org/software/bash/manual/html_node/Shell-Parameter-Expansion.html
[3] https://www.shellcheck.net/wiki/SC2115
containers#1822
containers#1828
Signed-off-by: Rolv Apneseth <rolv.apneseth@gmail.com>
Hard coding the port to 50000 can cause intermittent failures when the port happens to already be in use, and prevents running the test suite more than once at the same time. Let Podman pick a random available port and persist it to a file so test processes in the same run can find it. containers#1822 Signed-off-by: Rolv Apneseth <rolv.apneseth@gmail.com>
Merged the patch for this through #1828 |
2 participants
See #1817 (comment). I believe this should do the trick @debarshiray