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: Pass down $MAKEJOBS to test_runner.py, other improvements #16739

Merged
merged 2 commits into from Aug 29, 2019

Conversation

maflcko
Copy link
Member

@maflcko maflcko commented Aug 27, 2019

Some changes to the ci system:

  • Pass down MAKEJOBS to the test_runner, instead of falling back to the default of 4. Passing it down avoids OOM on weak machines and allows better use of resources on beefy machines.
  • Move CCACHE_DIR to ./ci/scratch/ subfolder: ccache is executed with root permissions inside the docker, so the cache files are created with root as owner. So it might be wise to not put them in the $HOME of the host
  • Use the scratch dir as prefix for the test runner, as opposed to /tmp/, which is often a ramdisk and thus leads to OOM on the host when either a lot of tests are run in parallel or when a lot of tests fail and the datadirs are not cleaned.

@maflcko maflcko added the Tests label Aug 27, 2019
@DrahtBot
Copy link
Contributor

DrahtBot commented Aug 27, 2019

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #16367 (Multiprocess build support by ryanofsky)
  • #12134 (Build previous releases and run functional tests by Sjors)
  • #10102 ([experimental] Multiprocess bitcoin by ryanofsky)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

@@ -6,6 +6,7 @@

export LC_ALL=C.UTF-8

mkdir -p "${BASE_SCRATCH_DIR}"
Copy link
Member

Choose a reason for hiding this comment

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

Is this really necessary? On my system:

$ ls /tmp/this
ls: /tmp/this: No such file or directory
$ CCACHE_DIR=/tmp/this/doesnt/exits ccache find /tmp/this
/tmp/this
/tmp/this/doesnt
/tmp/this/doesnt/exits
/tmp/this/doesnt/exits/9
/tmp/this/doesnt/exits/9/stats
/tmp/this/doesnt/exits/ccache.conf

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, this is needed when the ci runner sets the ccache dir to not reside in the scratch dir. E.g. our cirrus ci:

CCACHE_DIR: "/tmp/ccache_dir"

Copy link
Member

Choose a reason for hiding this comment

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

Oh right, when CCACHE_DIR is unrelated to BASE_SCRATCH_DIR, thanks.

Copy link
Member

@fanquake fanquake left a comment

Choose a reason for hiding this comment

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

ACK fa27372

Copy link
Member

@laanwj laanwj left a comment

Choose a reason for hiding this comment

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

ACK fa27372

laanwj added a commit that referenced this pull request Aug 29, 2019
…ements

fa27372 ci: Move CCACHE_DIR and test_runner tmp dir into ./ci/scratch/ (MarcoFalke)
fa60583 ci: Pass down $MAKEJOBS to test_runner.py (MarcoFalke)

Pull request description:

  Some changes to the ci system:

  * Pass down MAKEJOBS to the test_runner, instead of falling back to the default of 4. Passing it down avoids OOM on weak machines and allows better use of resources on beefy machines.
  * Move CCACHE_DIR to ./ci/scratch/ subfolder: `ccache` is executed with root permissions inside the docker, so the cache files are created with root as owner. So it might be wise to not put them in the $HOME of the host
  * Use the scratch dir as prefix for the test runner, as opposed to `/tmp/`, which is often a ramdisk and thus leads to OOM on the host when either a lot of tests are run in parallel or when a lot of tests fail and the datadirs are not cleaned.

ACKs for top commit:
  fanquake:
    ACK fa27372
  laanwj:
    ACK fa27372

Tree-SHA512: 67834fbab282051ec81c319d460528b32870507e53df2b8a1ce9a1f3f6a685aaf8eb8ba03f5406918ca4a33adf736e6a4adad7134c54cf3a9e47a26c64a13442
@laanwj laanwj merged commit fa27372 into bitcoin:master Aug 29, 2019
@maflcko maflcko deleted the 1908-ciScratch branch August 29, 2019 14:01
kwvg added a commit to kwvg/dash that referenced this pull request Dec 13, 2021
kwvg added a commit to kwvg/dash that referenced this pull request Dec 13, 2021
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants