CI: test overlay and vfs #20161
Merged
CI: test overlay and vfs #20161
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
openshift-ci
bot
added
release-note-none
approved
edsantiago
force-pushed
the
ci_desired_storage
branch
2 times, most recently
from
00ab112 to
1246120
Compare
openshift-ci
bot
added
the
do-not-merge/work-in-progress
|
Draft because I'm pretty sure this is going to fail due to missing settings in .cirrus.yml |
|
Awright, who wants to speculate on why overlay make (only) podman-remote slower? |
|
@edsantiago can you share a link to the timing results? |
|
Durrr. Sorry.
|
|
Are you sure they're slower and did not hang+timeout? |
|
Good question. No, I'm not sure, but my reasoning is that all the test logs end at about 2900s, which is ~48m, which is close to the 50m limit. And the tail of each log is different (different tests). Granted, buffering means that we probably don't get to see the very very tail ... but in non-overlay tests, we never get to 2900s, so I still think something is slowing down. If I were more talented/hardworking/capable I'd write a minitool to check timestamps on each line and look for big jumps; or to compare against a regular vfs run. Maybe I'll do so today. |
edsantiago
force-pushed
the
ci_desired_storage
branch
from
1246120 to
9903e2f
Compare
|
Here we go. This is a comparison of a good (standard) CI run against a bad one (this PR). Some tests run faster with overlay, but the huge majority run slower. I'm still trying to chase this down, but ITM would anyone like to look at the table below and see if they can figure out a common factor among the slow tests?
Methodology: I grabbed timing results from |
edsantiago
force-pushed
the
ci_desired_storage
branch
3 times, most recently
from
bb006e9 to
f0f3b0b
Compare
|
Cockpit tests failed for commit f0f3b0be736b96993c2d720ebc30ccad9a03fa32. @martinpitt, @jelly, @mvollmer please check. |
edsantiago
force-pushed
the
ci_desired_storage
branch
from
f0f3b0b to
bb7634b
Compare
|
I need help with this permissions-check failure: I cannot reproduce on 1minutetip, because on a default 1minutetip the permissions are correct: Somehow, under Cirrus, the There's also a |
edsantiago
commented
Oct 2, 2023
test/e2e/common_test.go
Outdated
Comment on lines
976
to
997
| // When using overlay, podman leaves a stray mount behind. This | ||
| // leak causes remote tests to take a loooooong time, and time out. | ||
| overlayPath := path + "/root/overlay" | ||
| if _, err := os.Stat(overlayPath); err == nil { | ||
| umount := exec.Command("umount", overlayPath) | ||
| umount.Stdout = GinkgoWriter | ||
| umount.Stderr = GinkgoWriter | ||
| if err = umount.Run(); err != nil { | ||
| GinkgoWriter.Printf("Error umounting %s: %v\n", overlayPath, err) | ||
| } | ||
| } |
There was a problem hiding this comment.
This fixes the test hang.
We actually have a long history with this issue of leaked mounts: https://github.com/containers/buildah/blob/0d717cd0a52376c80cb527a344a1d7446b0d56e7/tests/helpers.bash#L116-L123
That dates to 2020. See containers/buildah#1991 .
Is there really no way to fix this leak?
edsantiago
force-pushed
the
ci_desired_storage
branch
2 times, most recently
from
8e93f67 to
6d2e4b6
Compare
|
@giuseppe PTAL |
|
Eyeballs welcome, but this is not ready to merge. I'm seeing a LOT of new flakes, like this new container-cp one. |
edsantiago
force-pushed
the
ci_desired_storage
branch
3 times, most recently
from
e33f221 to
3a2dea7
Compare
edsantiago
force-pushed
the
ci_desired_storage
branch
from
3a2dea7 to
e7b22f0
Compare
edsantiago
force-pushed
the
ci_desired_storage
branch
from
2bbf478 to
254b9e5
Compare
|
Ephemeral COPR build failed. @containers/packit-build please check. |
|
Cockpit tests failed for commit 254b9e59427822d4b4f040931a585329f332563b. @martinpitt, @jelly, @mvollmer please check. |
edsantiago
force-pushed
the
ci_desired_storage
branch
from
254b9e5 to
fe0b71a
Compare
|
Ephemeral COPR build failed. @containers/packit-build please check. |
|
Cockpit tests failed for commit fe0b71afabe599b17ebfbeba5fb7473d899b7ade. @martinpitt, @jelly, @mvollmer please check. |
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
Luap99
reviewed
Nov 7, 2023
edsantiago
force-pushed
the
ci_desired_storage
branch
from
fe0b71a to
2df168e
Compare
|
Ephemeral COPR build failed. @containers/packit-build please check. |
edsantiago
force-pushed
the
ci_desired_storage
branch
4 times, most recently
from
14247ae to
082d582
Compare
|
This is as ready as I can make it. Any feedback on my flake question?
|
Luap99
approved these changes
Nov 14, 2023
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: edsantiago, Luap99 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 |
We're only testing vfs in CI. That's bad. containers#18822 tried to remedy that but that only worked on system tests, not e2e. Here we introduce CI_DESIRED_STORAGE, to be set in .cirrus.yml in the same vein as all the other CI_DESIRED_X. Since it's 2023 we default to overlay, testing vfs only in priorfedora. Fixes required: - e2e tests: - in cleanup, umount ROOT/overlay to avoid leaking mounts - system tests: - fix a few badly-written tests that assumed/hardcoded overlay - buildx test: add weird exception to device-number test - mount tests: add special case code for vfs - unprivileged test: disable one section that is N/A on vfs Signed-off-by: Ed Santiago <[email protected]>
edsantiago
force-pushed
the
ci_desired_storage
branch
from
082d582 to
a10b88c
Compare
|
/lgtm |
github-actions
bot
added
the
locked - please file new issue/PR
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
We're only testing vfs in CI. That's bad. #18822 tried to
remedy that but that only worked for system tests, not e2e.
Here we introduce CI_DESIRED_STORAGE, to be set in .cirrus.yml
in the same vein as all the other CI_DESIRED_X. Since it's 2023
we default to overlay, testing vfs only in priorfedora.
Skip the "split imagestore" test under overlay (#19748)
Signed-off-by: Ed Santiago [email protected]