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

Sync flux-sched with removed interfaces in flux-core #440

Merged
merged 10 commits into from Feb 13, 2019

Conversation

@grondo
Copy link
Contributor

grondo commented Feb 6, 2019

This PR synchronizes flux-sched with the removal of wreck and related interfaces in flux-core. The following were removed in whole:

  • rdl
  • resrc
  • sched
  • libutil/log
  • liblsd

All related testsuite scripts under t/ were also removed.

The current t5000-valgrind.t from flux-core (slightly adapted) was added back to prepare for easily adding new flux-sched valgrind workloads (just add an executable script or scripts under t/valgrind/workload.d). I attempted to add a workload that simply loaded/unloaded the resource module, but this resulted in valgrind errors so I left that out for now.

Closes #437

grondo added 8 commits Feb 5, 2019
planner tests do not use Lua so the LUA_PATH and LUA_CPATH settings
in Makefile.am are unnecessary. Remove them.
Remove code and interfaces dependent on upstream flux-core wreck
and Lua interfaces that have since been removed. Includes removal
of rdl, sched, resrc and associated tests and scripts.

Fixes #437
There are no more users of liblsd/list, so remove it.
There are no users of src/common/libutil/log, remove it.
Remove unused or unsupportable test scripts under t/scripts:
 - R_lite.lua: unused; and the flux.affinity module no longer exists
 - waitfile.lua: unused
 - kvs-watch-until.lua: unused.
Remove many of the functions exported from sched-sharness.sh
since they use removed commands such as flux-submit, flux-wreckrun,
and flux-waitjob.
Add newer valgrind test script from flux-core with flux-core
suppressions file. No scripts are added to the valgrind/workload.d/
directory yet, so the test doesn't do much of anything, but serves
as a start for future addition of workloads.
@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Feb 6, 2019

Codecov Report

Merging #440 into master will decrease coverage by 0.59%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##           master     #440     +/-   ##
=========================================
- Coverage   75.68%   75.08%   -0.6%     
=========================================
  Files          67       43     -24     
  Lines       11028     4988   -6040     
=========================================
- Hits         8346     3745   -4601     
+ Misses       2682     1243   -1439
Impacted Files Coverage Δ
src/common/libutil/xzmalloc.c 81.81% <ø> (+30.3%) ⬆️
src/common/libutil/shortjansson.h 100% <0%> (+12.28%) ⬆️

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 458c345...33e39d7. Read the comment docs.

grondo added 2 commits Feb 6, 2019
Remove functions that are unused from xzmalloc.c, since use of
these interfaces should be discouraged anyway.
@grondo

This comment has been minimized.

Copy link
Contributor Author

grondo commented Feb 7, 2019

I pushed some further changes earlier to remove unused code from xzmalloc.c (these should be avoided in production code anyway) and updated the codecov.yml (to ignore libtap in code coverage stats).

@grondo

This comment has been minimized.

Copy link
Contributor Author

grondo commented Feb 8, 2019

BTW, I don't think there is anything more to do here. There was large changes in the codebase so the code coverage change could perhaps be ignored.

@dongahn

This comment has been minimized.

Copy link
Contributor

dongahn commented Feb 9, 2019

Thanks @grondo!

@dongahn

This comment has been minimized.

Copy link
Contributor

dongahn commented Feb 13, 2019

One of my next priorities: Sorry @grondo, it is taking rather long to get to this.

@dongahn

This comment has been minimized.

Copy link
Contributor

dongahn commented Feb 13, 2019

Thanks @grondo.

The deletion looks good, but I have a mixed feeling about the removal of the scripts in t/scripts.

As part of our near future work of adding more runtime tests for resource module after hooking that up with the rest of the subsystems (execution+schedule loop module), I have to think we will need these scripts to control synchronization needed for testing and to fetch the R_lite key from the new R format. Any compelling reasons to remove those scripts now?

@grondo

This comment has been minimized.

Copy link
Contributor Author

grondo commented Feb 13, 2019

Thanks @dongahn. The scripts as written will no longer work with flux-core after #flux-framework/flux-core#1988 is merged.

Also, removing unused code is good practice in general, because it keeps cruft from accumulating in the project.

It will be simple to go back in history and pull out any scripts (with updates) to use in future testing once they are needed. Similary, you may want to adapt or refer to all the previous sharness tests for the resrc scheduler as we move forward.

However, if you feel strongly that some of the scripts should remain, I'm find we leaving them for now.

@dongahn

This comment has been minimized.

Copy link
Contributor

dongahn commented Feb 13, 2019

@grondo: I don't feel strongly about it at all in particular since these scripts will stop working with the latest flux-core PR. Just curious, what is the main reason that those scripts will stop working? API changes or Lua? How does flux-core deal with testing synchronization etc going forward?

@dongahn

This comment has been minimized.

Copy link
Contributor

dongahn commented Feb 13, 2019

In any case, from my perspective, this PR looks good me. Will need to go in after flux-framework/flux-core#1988 goes in though.

@grondo

This comment has been minimized.

Copy link
Contributor Author

grondo commented Feb 13, 2019

Oh, sorry waitfile.lua will still work. However:

  • R_lite.lua uses the flux.affinity Lua module which will be removed. We'll want to update the script to the Rv1 format anyway.
  • kvs-watch-until.lua uses the kvs Lua bindings which will be removed.

I guess there were no tests in flux-core that used the kvs-watch-until.lua script for synchronization, which is how I was able to remove it. I guess synchronization there depends on the exact use case.

@grondo

This comment has been minimized.

Copy link
Contributor Author

grondo commented Feb 13, 2019

Will need to go in after flux-framework/flux-core#1988 goes in though.

Since flux-sched is dependent on flux-core, I actually think we should merge this PR first, since this PR won't break flux-core, but merging flux-core PR first will break current flux-sched. (Unless I'm missing something)

In the grand scheme of things the order doesn't matter too much though.

@dongahn

This comment has been minimized.

Copy link
Contributor

dongahn commented Feb 13, 2019

When is flux-framework/flux-core#1988 expected to land? I can merge sched before that goes in. But some coordination seems a bit beneficial. I do agree this wouldn't matter too much from the grand scheme of things :-)

@grondo

This comment has been minimized.

Copy link
Contributor Author

grondo commented Feb 13, 2019

When is flux-framework/flux-core#1988 expected to land?

I'm not sure. It has been ready for a week or so, but we certainly didn't want to merge it before we had a plan for flux-sched.

I'll force a push there and maybe ask what else reviewers would like.

@dongahn

This comment has been minimized.

Copy link
Contributor

dongahn commented Feb 13, 2019

OK. Merging. Thanks @grondo!

@dongahn dongahn merged commit 999858c into flux-framework:master Feb 13, 2019
2 of 3 checks passed
2 of 3 checks passed
codecov/project 75.08% (-0.6%) compared to 458c345
Details
codecov/patch Coverage not affected when comparing 458c345...33e39d7
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@grondo

This comment has been minimized.

Copy link
Contributor Author

grondo commented Feb 13, 2019

Great thanks!

@grondo grondo deleted the grondo:kill-wreck branch Feb 13, 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.