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

Use /tmp for test repositories #11126

Merged
merged 5 commits into from
Apr 19, 2020
Merged

Conversation

guillep2k
Copy link
Member

This PR modifies the build process so test repositories are created in a temporary directory by means of mktemp. This happens when the USE_REPO_TEST_DIR environment variable has a value of 1.

The goal is to avoid too much I/O overhead as observed recently, especially in ARM builds.

I've also changed .drone.yml in order to use this new setting.

Inspired in this SO answer.

@guillep2k guillep2k added topic/build PR changes how Gitea is built, i.e. regarding Docker or the Makefile skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features. labels Apr 18, 2020
@GiteaBot GiteaBot added the lgtm/need 1 This PR needs approval from one additional maintainer to be merged. label Apr 18, 2020
@6543
Copy link
Member

6543 commented Apr 18, 2020

export USE_REPO_TEST_DIR=1 found in my .bashrc ;)

@guillep2k
Copy link
Member Author

SQLite test from #11032:

image

SQLite test (also on ARM) from this PR:

image

PGSQL test (also on ARM) from this PR:

image

@guillep2k
Copy link
Member Author

It would be nice if anyone can test this in other environments (e.g. Windows, FreeBSD, etc.)

@guillep2k
Copy link
Member Author

Tests from #11032:

image

Tests from this PR:

image

Copy link
Member

@jolheiser jolheiser left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This appears to work as much in Windows as it did before, at least on my machine.

That is to say, certain tests fail, but they have never worked on my machine. So this PR didn't break anything further as far as I can tell. 🙃

Not going to explicitly approve, just so we can have others test as well before merging. (I might test it later on my Linux machine as well if I have some time)

@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Apr 19, 2020
@codecov-io
Copy link

Codecov Report

Merging #11126 into master will increase coverage by 0.05%.
The diff coverage is 58.13%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #11126      +/-   ##
==========================================
+ Coverage   43.42%   43.48%   +0.05%     
==========================================
  Files         600      600              
  Lines       85022    85108      +86     
==========================================
+ Hits        36922    37005      +83     
+ Misses      43541    43536       -5     
- Partials     4559     4567       +8     
Impacted Files Coverage Δ
routers/api/v1/repo/branch.go 51.41% <57.64%> (+0.96%) ⬆️
routers/api/v1/api.go 76.50% <100.00%> (+0.03%) ⬆️
modules/indexer/stats/db.go 40.62% <0.00%> (-28.13%) ⬇️
modules/indexer/stats/queue.go 62.50% <0.00%> (-18.75%) ⬇️
modules/git/utils.go 65.67% <0.00%> (-4.48%) ⬇️
modules/git/repo.go 49.79% <0.00%> (-0.84%) ⬇️
modules/setting/repository.go 51.42% <0.00%> (ø)
models/issue.go 51.95% <0.00%> (+0.46%) ⬆️
modules/notification/webhook/webhook.go 40.20% <0.00%> (+0.58%) ⬆️
modules/setting/setting.go 45.28% <0.00%> (+0.94%) ⬆️
... and 6 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 16f39ce...4de795a. Read the comment docs.

@6543
Copy link
Member

6543 commented Apr 19, 2020

@jolheiser have you tested?

@lafriks lafriks merged commit 23163e9 into go-gitea:master Apr 19, 2020
@mrsdizzie
Copy link
Member

I'm all for storing things in RAM here though this PR assumes /tmp is tmpfs and not on disk but that usually isn't the case unless it has been configured that way.

The two systems I have access to (Debian Jessie and MacOS) don't mount /tmp as tmpfs by default so this PR wouldn't reduce disk I/O in any way for those and /tmp is just another folder on disk.

Since the test run times here are the same as every other current PR I'm skeptical that it is doing anything different in our CI system either since @6543 claimed in a test that using tmpfs reduced the run time by 10 minutes.

Maybe @techknowlogick would know and could say if /tmp in whatever environment our CI runs is mounted as /tmpfs and if not maybe there is a better location for that (or it could be reconfigured to do so if this is even possible).

@zeripath
Copy link
Contributor

I understand what you mean as it's not guaranteed that /tmp is a tmpfs/ramdisk however, it's definitely better form for our tests to create tmp repositories in /tmp. At least now the drone-cli could be set up to use a tmpfs for /tmp and gain generally from that.

@guillep2k guillep2k deleted the use-tmp-repos branch April 19, 2020 12:55
@guillep2k
Copy link
Member Author

@mrsdizzie Usage of /tmp is completely optional (unless you depend on .drone.yml), and is controlled by the USE_REPO_TEST_DIR var; if you don't have it set to 1, the behavior is the same as before. It happens that our tests had their times reduced considerably, so I guess the images mounted by them (Alpine Linux I believe) do have /tmp as tmpfs.

This PR could be expanded, for instance by accepting a directory name, or making the Makefile mount something else.

@guillep2k
Copy link
Member Author

@mrsdizzie it seems you're right. After merging with this PR, you PR is still taking a long time. 😟

image

@mrsdizzie
Copy link
Member

@guillep2k hm where do you see a high reduced test time in our CI? The average builds for this PR and anything after it was merged don't show any consistent decrease (some show increases, but seems like normal fluctuation probably) and #11032 is still hitting the same file write limiting. Apline linux does not seem to mount /tmp as tmpfs either in a quick check

docker run alpine less /etc/fstab
/dev/cdrom	/media/cdrom	iso9660	noauto,ro 0 0
/dev/usbdisk	/media/usb	vfat	noauto,ro 0 0

docker run alpine df /tmp
Filesystem           1K-blocks      Used Available Use% Mounted on
overlay               61255492   6283644  51830524  11% /

I see how this PR works and I think this change is good and not suggesting it should be different I just don't think it is yet making any change to disk I/O in our CI builds.

@zeripath is right we should now try and make sure /tmp is mounted as tmpfs and then it should 'just work' because of changes in this PR. Not sure how involved that is if it means changing something about the default docker image used.

@guillep2k
Copy link
Member Author

So the gain in time was apparently just a fluke? ☹️
I like better my other PR #11132 if it worked.

ydelafollye pushed a commit to ydelafollye/gitea that referenced this pull request Jul 31, 2020
* Add option to use /tmp for test repositories

* Fix exit status

* Add feedback about using tmp repos

Co-authored-by: Guillermo Prandi <guillep2k@users.noreply.github.com>
Co-authored-by: Lunny Xiao <xiaolunwen@gmail.com>
@go-gitea go-gitea locked and limited conversation to collaborators Nov 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features. topic/build PR changes how Gitea is built, i.e. regarding Docker or the Makefile
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants