run e2e test on tmpfs #22533
run e2e test on tmpfs #22533
Conversation
openshift-ci
bot
added
release-note-none
do-not-merge/work-in-progress
|
Ephemeral COPR build failed. @containers/packit-build please check. |
|
Error logs are misleading. Actual error seems to be in |
Yeah I think the first step is to remove the big images we use, the image dir is already over 1.1G so we should get this down as first step. Maybe we can get away with 4 Gb RAM then. |
|
This maybe? diff --git a/test/e2e/common_test.go b/test/e2e/common_test.go
index bc1f73b82..f617c3a58 100644
--- a/test/e2e/common_test.go
+++ b/test/e2e/common_test.go
@@ -1027,6 +1027,7 @@ func (p *PodmanTestIntegration) RestoreArtifactToCache(image string) error {
p.Root = p.ImageCacheDir
restore := p.PodmanNoEvents([]string{"load", "-q", "-i", tarball})
restore.WaitWithDefaultTimeout()
+ Expect(restore).To(ExitCleanly())
}
return nil
} |
yes |
|
Q: why did debian pass? |
Yeah... I noticed that while playing with my fedora tmpfs changes. AFAICT that is the Debian default, not something we do in VM creation, so I left it as-is. |
filled containers/automation_images#350 to track this, but first let's see how much of a difference this really makes here first |
|
I think you're onto something. Ballpark shows tests running in ~30m, down from ~40
|
|
Yeah that looks like a solid start, not sure how accurate the time report is We clearly got the system time down a lot which signals IO is a problem, also we still did not max out the CPU per the stats reporting in the cirrus UI so this surprises me a bit because locally this goes full out and I have close to 100% CPU usage on all cores. But maybe the cirrus graph is not counting everything? |
Luap99
force-pushed
the
e2e-tmp-ci
branch
2 times, most recently
from
bd1b2c0
to
59a60bf
Compare
|
@edsantiago What do you think about the |
|
I like it. |
|
Oops: containerized Don't re-push yet though; I'm really curious to see how this'll work out |
Which is perfect as it shows my check actually works, and yeah I let it run and will continue tomorrow. |
|
I can't reproduce. Doesn't seem to be AVC. Could it be that the new remount is adding |
Which of the failures are you talking about? There just to many to make sense of this. /tmp does not have noexec set, if it would be then no container would be able to run in the e2e tests. |
Seems to be a bit under 30m now, looking at other PRs they seem to be in the range of 35-40+ mins so I think it is safe to say this is a noticeable change. I will try to drop the toolbox change as I think it also helps a bit and I only want to see the tmpfs diff. |
The image is way to big (over 800MB) that slows tests down as we always have to pull this, the tests itself are also super slow due the entrypoint logic that we don't care about. We should be testing for features needed and not specific tools. I think the current changes should have a similar coverage in terms of podman features, it no longer tests toolbox but IMO this never was a task for podman CI tests. The main driver for this is to make the tests run entirely based on tmpfs and this image is just to much[1]. [1] containers#22533 Signed-off-by: Paul Holzinger <[email protected]>
The image is way to big (over 800MB) that slows tests down as we always have to pull this, the tests itself are also super slow due the entrypoint logic that we don't care about. We should be testing for features needed and not specific tools. I think the current changes should have a similar coverage in terms of podman features, it no longer tests toolbox but IMO this never was a task for podman CI tests. The main driver for this is to make the tests run entirely based on tmpfs and this image is just to much[1]. [1] containers#22533 Signed-off-by: Paul Holzinger <[email protected]>
I suspect we hit another tar bug now in debian, maybe the same as #19407?? Other issue: Seems to be the removal of the new TMPDIR I create, not seen on every run but seem to be rootless only so I guess it could be due missing |
The image is way to big (over 800MB) that slows tests down as we always have to pull this, the tests itself are also super slow due the entrypoint logic that we don't care about. We should be testing for features needed and not specific tools. I think the current changes should have a similar coverage in terms of podman features, it no longer tests toolbox but IMO this never was a task for podman CI tests. The main driver for this is to make the tests run entirely based on tmpfs and this image is just to much[1]. [1] containers#22533 Signed-off-by: Paul Holzinger <[email protected]>
Luap99
force-pushed
the
e2e-tmp-ci
branch
2 times, most recently
from
1829e7f
to
0f955f4
Compare
Yep, np. Was just concerned if maybe there were some failures due to OOM (I haven't looked). |
The image is way to big (over 800MB) that slows tests down as we always have to pull this, the tests itself are also super slow due the entrypoint logic that we don't care about. We should be testing for features needed and not specific tools. I think the current changes should have a similar coverage in terms of podman features, it no longer tests toolbox but IMO this never was a task for podman CI tests. The main driver for this is to make the tests run entirely based on tmpfs and this image is just to much[1]. [1] containers#22533 Signed-off-by: Paul Holzinger <[email protected]>
|
Cockpit tests failed for commit 385c493. @martinpitt, @jelly, @mvollmer please check. |
Follow up to commit eaf60c7, with the toolbox image removal it is possible to run all tests from tmpfs. Signed-off-by: Paul Holzinger <[email protected]>
from containers/automation_images#351 Signed-off-by: Paul Holzinger <[email protected]>
First, setup a custom TMPDIR to ensure we have no special assumptions about hard coded paths. Second, make sure it is actually on a tmpfs so we can catch regressions in the VM setup immediately. Signed-off-by: Paul Holzinger <[email protected]>
This reverts commit 02b8fd7. The new CI images should have a apparmor workaround. Fixes containers#22625 Signed-off-by: Paul Holzinger <[email protected]>
|
Ephemeral COPR build failed. @containers/packit-build please check. |
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
|
This is good to review now, I think tests are going to pass this time around |
|
Cockpit tests failed for commit 9233864. @martinpitt, @jelly, @mvollmer please check. |
| export TMPDIR | ||
| fstype=$(findmnt -n -o FSTYPE --target $TMPDIR) | ||
| if [[ "$fstype" != "tmpfs" ]]; then | ||
| die "The CI test TMPDIR is not on a tmpfs mount, we need tmpfs to make the tests faster" |
There was a problem hiding this comment.
Not worth a re-push, but: in case of failure, this will be very difficult to debug. A more helpful message might be something like
die "The CI test TMPDIR ($TMPDIR) fs type is '$fstype'; it should be 'tmpfs' (PR #22533)"
|
[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 |
|
Timing results:
Seems to shave 2-5 minutes on podman local, maybe 5-8 remote. |
|
/lgtm |
openshift-merge-bot
bot
merged commit c9808e7
into
containers:main
Follow up to commit eaf60c7, lets see how bad things are going to break.
Does this PR introduce a user-facing change?