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

small valgrind test fixes #1941

Merged
merged 3 commits into from Jan 22, 2019

Conversation

@grondo
Copy link
Contributor

commented Jan 21, 2019

This PR makes 2 small improvements to the valgrind sharness test:

  • Run valgrind on an instance of size=2 instead of just a single broker. Some code in Flux is only active on rank > 0, so this will help find leaks and errors in those code paths.
  • The valgrind-workload.sh ignored errors from the underlying workload scripts. Log an error on non-zero exit code from any one of these scripts and cause a test failure.

This PR will fail until #1940 is addressed.

Problem: The valgrind workoad script does not exit with error
if one of the workload scripts gets an error during execution
or fails to execute.

Emit an error on non-zero exit code from the workload scripts,
and ensure that the valgrind test itself fails on error from any
script, instead of exiting with success.
@grondo grondo force-pushed the grondo:valgrind-improve branch 2 times, most recently from 1c1f24a to 717ddef Jan 22, 2019
@codecov-io

This comment has been minimized.

Copy link

commented Jan 22, 2019

Codecov Report

Merging #1941 into master will increase coverage by 0.02%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #1941      +/-   ##
==========================================
+ Coverage   80.05%   80.08%   +0.02%     
==========================================
  Files         195      195              
  Lines       35078    35078              
==========================================
+ Hits        28082    28092      +10     
+ Misses       6996     6986      -10
Impacted Files Coverage Δ
src/modules/connector-local/local.c 73.77% <0%> (-0.3%) ⬇️
src/cmd/flux-module.c 83.96% <0%> (+0.23%) ⬆️
src/modules/kvs-watch/kvs-watch.c 79.77% <0%> (+0.88%) ⬆️
src/broker/modservice.c 79.8% <0%> (+0.96%) ⬆️
src/common/libflux/mrpc.c 88.75% <0%> (+1.2%) ⬆️
src/modules/barrier/barrier.c 78.62% <0%> (+2.06%) ⬆️
grondo added 2 commits Jan 21, 2019
Ensure at least some code for rank > 0 is run in the valgrind test
by executing multiple brokers instead of just a single broker in
t5000-valgrind.t.

Default size of test is 2 brokers, may be overidden by
VALGRIND_NBROKERS.

In the case of running many brokers, the shutdown grace period may
need to be increased as well, so allow a VALGRIND_SHUTDOWN_GRACE
environment variable to do that (default=16s).
Now that the valgrind test runs more than a single broker, wreck
jobs take significantly longer to run. 10 jobs is probably overkill
anyway, so decrease it to 5 by default.
@grondo grondo force-pushed the grondo:valgrind-improve branch from 717ddef to 0e9d2b6 Jan 22, 2019
@grondo

This comment has been minimized.

Copy link
Contributor Author

commented Jan 22, 2019

Rebased. I also made a couple other changes & cleanup:

  • change test string to reflect the real number of brokers in the instance being tested
  • decrease the number of wreck jobs run from 5 to 10
  • allow the size of the instance and grace time to be modified via environment variables ($VALGRIND_NBROKERS and $VALGRIND_SHUTDOWN_GRACE)
@garlick

This comment has been minimized.

Copy link
Member

commented Jan 22, 2019

Very nice!

@garlick garlick merged commit 546f945 into flux-framework:master Jan 22, 2019
3 checks passed
3 checks passed
codecov/patch Coverage not affected when comparing c0ad955...0e9d2b6
Details
codecov/project 80.08% (+0.02%) compared to c0ad955
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@grondo

This comment has been minimized.

Copy link
Contributor Author

commented Jan 22, 2019

Thanks!

@grondo grondo deleted the grondo:valgrind-improve branch Feb 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.