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

CI: mount tmpfs for container storage #22831

Merged
merged 2 commits into from
Jul 4, 2024

Conversation

Luap99
Copy link
Member

@Luap99 Luap99 commented May 29, 2024

Try to speed up the CI test by using tmpfs as container storage.

No idea if this works or will make a measurable difference but let's find out.

Does this PR introduce a user-facing change?

None

@openshift-ci openshift-ci bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. release-note-none labels May 29, 2024
Copy link
Contributor

openshift-ci bot commented May 29, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 29, 2024
Copy link

Ephemeral COPR build failed. @containers/packit-build please check.

@Luap99
Copy link
Member Author

Luap99 commented May 29, 2024

type distro user DB local remote container
sys rawhide root 34:27 23:09
sys rawhide rootless 35:13
sys fedora-40-aarch64 root 26:51 18:40
sys fedora-40 root 32:56 21:39
sys fedora-40 rootless 36:15 22:52
sys fedora-39 root boltdb 35:17 24:39
sys fedora-39 rootless boltdb 39:06
sys debian-13 root 35:33 25:24
sys debian-13 rootless 38:23

Given my other speed up PRs it is hard to have a solid baseline and looking at several PR's the timings seems to vary by several minutes between runs so I cannot say for certain if it is faster, if it is then not by much so not sure if it is worth it?

@edsantiago WDYT?

@edsantiago
Copy link
Member

Sorry for taking so long. Can you hold off on this for a little while? Specifically: hold off on anything that increases any use of tmpfs? I've been observing an ENOSPC flake in #17831, only in e2e, and I think it might be due to multiple disk-hog tests running at once. Haven't looked deeply enough, and am not likely to do so soon. I'd like to understand that one before we keep piling onto tmpfs.

(In case it's not obvious, the reason it only happens in #17831 is because it disables flake retries. I am confident that it is happening in real-world PRs but we don't see it because the ginkgo flake-retry mechanism hides it).

@Luap99
Copy link
Member Author

Luap99 commented May 29, 2024

Sure all I wanted to know if it makes a measurable difference, so far it does not like it so I have no good reason to push for this.

@edsantiago
Copy link
Member

Thanks. I still think this is worth pursuing (potentially).

The noise in sys tests bothers me. I understand noise in e2e tests: flake retries, parallelization load differences. But sys tests I would expect to be more consistent.

Copy link

A friendly reminder that this PR had no activity for 30 days.

@Luap99
Copy link
Member Author

Luap99 commented Jul 1, 2024

@edsantiago Are you fine with me pushing this forward?
It doesn't improve a lot alone but with the parallel work in #23048 I see a measurable speed up and I think it is easer to review this change here alone rather than in the big parallel PR.

@rhatdan
Copy link
Member

rhatdan commented Jul 1, 2024

LGTM

@edsantiago
Copy link
Member

Yes, let's try it. Please rebase and remove the WIP.

Luap99 added 2 commits July 1, 2024 12:45
Try to speed up the CI tests by using tmpfs as container storage.
This is important for system tests, other tests setup their own --root
already on tmpfs so it should not effect them.

Signed-off-by: Paul Holzinger <pholzing@redhat.com>
The test check the the default volume is not on tmpfs, however what it
should really check that the volume is on our container storage fs. It
is possible that users run the storage on top of tmpfs so this test
always failed there.

The better check is to compare the fs from the graphroot and the volume.
Unfortunately, for unknown reasons stat -f -c %T returns UNKNOWN and not
the actual fs. I have no idea why, to work around that we now parse
/proc/mounts manually for the fs. Not nice but at least it works
correctly.

Signed-off-by: Paul Holzinger <pholzing@redhat.com>
@Luap99 Luap99 changed the title WIP: CI: mount tmpfs for container storage CI: mount tmpfs for container storage Jul 1, 2024
@Luap99 Luap99 marked this pull request as ready for review July 1, 2024 10:52
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 1, 2024
@github-actions github-actions bot removed the stale-pr label Jul 2, 2024
@Luap99
Copy link
Member Author

Luap99 commented Jul 4, 2024

@edsantiago Good to merge like this?

@edsantiago
Copy link
Member

Sure... worth a try

/lgtm

(may need to be merged manually)

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jul 4, 2024
@openshift-merge-bot openshift-merge-bot bot merged commit d1a258b into containers:main Jul 4, 2024
89 of 90 checks passed
@Luap99 Luap99 deleted the system-tmpfs branch July 4, 2024 13:05
@stale-locking-app stale-locking-app bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Oct 3, 2024
@stale-locking-app stale-locking-app bot locked as resolved and limited conversation to collaborators Oct 3, 2024
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. release-note-none
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants