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

Experiment: test: Disable fsync in travis tests #10220

Closed
wants to merge 7 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@laanwj
Member

laanwj commented Apr 17, 2017

Install the package "eatmydata" which disables fsync and friends, and use this while running the functional tests.

Not sure whether it applies to travis, but at least locally on a VM this made some of the tests run three times as fast.

laanwj added some commits Apr 17, 2017

test: Disable fsync in travis tests
Install the package "eatmydata" which disables fsync and friends,
and use this while running the functional tests.

Not sure whether it applies to travis, but at least locally on a VM this
made some of the tests run three times as fast.

@laanwj laanwj added the Tests label Apr 17, 2017

.travis.yml Outdated
# No wallet
- HOST=x86_64-unknown-linux-gnu PACKAGES="python3 xvfb" DEP_OPTS="NO_WALLET=1" RUN_TESTS=true GOAL="install" BITCOIN_CONFIG="--enable-glibc-back-compat --enable-reduce-exports"
- HOST=x86_64-unknown-linux-gnu PACKAGES="python3 xvfb eatmydata" DEP_OPTS="NO_WALLET=1" RUN_TESTS=true GOAL="install" BITCOIN_CONFIG="--enable-glibc-back-compat --enable-reduce-exports"

This comment has been minimized.

@MarcoFalke

MarcoFalke Apr 17, 2017

Member

functional tests are disabled for windows and no_wallet, so I don't think we need this here.

You probably did this for consistency/simpler code.

This comment has been minimized.

@laanwj

laanwj Apr 17, 2017

Member

My intent was to add it for all the lines that have RUN_TESTS=true

@MarcoFalke

This comment has been minimized.

Member

MarcoFalke commented Apr 17, 2017

Concept ACK:

*** BEFORE ***
ALL                            | ✓ Passed  | 2648 s (accumulated) 
Runtime: 713 s

*** AFTER ***
ALL                            | ✓ Passed  | 1290 s (accumulated) 
Runtime: 337 s

As we ignore the return code of fsync in our code, this should not add additional debug hassle in case the fsync fails to write everything to disk (maybe due to hardware issues)?

Edit: The comparison is wrong. I accidentally compared a CRON job against this pull request, where this pull runs a subset of the tests of a cron job.

@laanwj

This comment has been minimized.

Member

laanwj commented Apr 17, 2017

As we ignore the return code of fsync in our code, this should not add additional debug hassle in case the fsync fails to write everything to disk (maybe due to hardware issues)?

I don't think so.

It does remind me that we could use the same technique (library interposing) to inject failures for testing to test how bitcoind handles i/o failures.

@laanwj

This comment has been minimized.

Member

laanwj commented Apr 17, 2017

Argh, it fails on 32-bit linux:

ERROR: ld.so: object '/usr/lib/libeatmydata/libeatmydata.so' from LD_PRELOAD cannot be preloaded (wrong ELF class: ELFCLASS64): ignored.

laanwj added some commits Apr 17, 2017

retry (whole package as i386)
Apparently on trusty there is no separate package for the library yet.
@laanwj

This comment has been minimized.

Member

laanwj commented Apr 17, 2017

I can't get it to work on 32-bit: not with 32-bit and not with 64-bit eatmydata.

The problem, I think, is that python runs as 64-bit, and bitcoind as 32-bit. So with the 64-bit eatmydata it fails on bitcoind, with the 32-bit one it fails on python.

Not sure what would be a good way to fix this. I suppose a hack that sets the environment variable BITCOIND so that only bitcoind is started through eatmydata (and not the outer script) could work.

Edit: BITCOIND="eatmydata $PWD/src/bitcoind" doesn't work, at least. This would need an actual wrapper script.

@laanwj

This comment has been minimized.

Member

laanwj commented Apr 17, 2017

Hm another idea, instead of using the wrapper/interposer, which seems to be tricky on mixed architctures, couldn't we just link bitcoind against libeatmydata.so directly?
On Ubuntu 16.04 libeatmydata1 is a separate package that can be installed, this isn't the case on trusty/14.04.

16.04 (just for illustration):

$ dpkg -L libeatmydata1
...
/usr/lib/x86_64-linux-gnu/libeatmydata.so.1.1.2
/usr/lib/x86_64-linux-gnu/libeatmydata.so.1
/usr/lib/x86_64-linux-gnu/libeatmydata.so

14.04 (which is what travis uses):

$ dpkg -L eatmydata
...
/usr/lib/libeatmydata/libeatmydata.so
@laanwj

This comment has been minimized.

Member

laanwj commented Apr 17, 2017

With the wrapper most works, but it still fails in p2p-versionbits-warning.py, as this makes bitcoind invoke another process which apparently is 64 bit:

p2p-versionbits-warning.py failed, Duration: 8 s
stdout:
2017-04-17 17:09:13.351000 TestFramework (INFO): Initializing test directory /tmp/testmahvm5qh/275
2017-04-17 17:09:13.609000 TestFramework.mininode (INFO): Connecting to Bitcoin Node: 127.0.0.1:13200
2017-04-17 17:09:19.543000 TestFramework (INFO): Stopping nodes
2017-04-17 17:09:21.665000 TestFramework (INFO): Cleaning up
2017-04-17 17:09:21.666000 TestFramework (INFO): Tests successful
stderr:
ERROR: ld.so: object '/usr/lib/libeatmydata/libeatmydata.so' from LD_PRELOAD cannot be preloaded (wrong ELF class: ELFCLASS32): ignored.
ERROR: ld.so: object '/usr/lib/libeatmydata/libeatmydata.so' from LD_PRELOAD cannot be preloaded (wrong ELF class: ELFCLASS32): ignored.

Sigh...

@jnewbery

This comment has been minimized.

Member

jnewbery commented Apr 17, 2017

this makes bitcoind invoke another process

The only process I can think of that p2p-versionbits-warning.py specifically would invoke is echo:

        self.extra_args = [["-alertnotify=echo %s >> \"" + self.alert_filename + "\""]]

This causes bitcoind to launch a thread to run echo when it detects blocks signalling unknown versionbits.

link against libeatmydata
Get rid of the wrapper and link directly against the library instead.
@laanwj

This comment has been minimized.

Member

laanwj commented Apr 18, 2017

Hm another idea, instead of using the wrapper/interposer, which seems to be tricky on mixed architctures, couldn't we just link bitcoind against libeatmydata.so directly?

I've tried this locally with some success, but not entirely sure how to port it to travis. Trying...

@laanwj

This comment has been minimized.

Member

laanwj commented Apr 18, 2017

Oh wow that worked.

But seems @MarcoFalke 's comparison was mistaken and this doesn't actually make a difference so closing...

@laanwj laanwj closed this Apr 18, 2017

@MarcoFalke

This comment has been minimized.

Member

MarcoFalke commented Apr 18, 2017

@laanwj Sorry for my confirmation bias. On a first glance those numbers seemed perfectly plausible.

@laanwj

This comment has been minimized.

Member

laanwj commented Apr 18, 2017

Yes it would have been plausible; apparently Travis isn't using qemu/kvm or has already configured their VMs to not pass through fsync.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment