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

track bulk message handler registration API change #278

Merged
merged 4 commits into from Nov 15, 2017

Conversation

Projects
None yet
5 participants
@garlick
Copy link
Member

garlick commented Nov 11, 2017

This PR will allow sched to compile after flux-framework/flux-core#1277 is merged.

I was able to compile test this but for some reason could not get make check to pass on my desktop. I reverted back to master on both flux-core and flux-sched and couldn't get that to work either so I assume it's environmental and unrelated to this PR, so submitted it anyway.

After the above gets merged, travis will need to be manually restarted here.

garlick added some commits Nov 11, 2017

track flux_msg_handler_addvec(), _delvec() changes
Track API changes proposed in flux-framework/flux-core#1277,
namely the definition of struct flux_msg_handler_spec
and the function prototypes for flux_msg_handler_addvec()
and flux_msg_handler_delvec().
@coveralls

This comment has been minimized.

Copy link

coveralls commented Nov 14, 2017

Coverage Status

Coverage decreased (-4.2%) to 73.147% when pulling 36ef369 on garlick:msg_handlers into a55889f on flux-framework:master.

@morrone

This comment has been minimized.

Copy link
Contributor

morrone commented Nov 14, 2017

@garlick, forget about the problem I mentioned in the hallway. I cleaned out my /usr/local, reinstalled flux-core, and all the flux-sched tests pass now for me.

I don't know why we keep getting "FAIL: trdl" in travis.

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Nov 15, 2017

The rdl/test/trdl test is failing because it can't find the flux.hostlist module in LUA_CPATH as exported by the Makefile. (This is the problem I reproduced on my desktop anyway)

The problem seems to be that, up until now, compiled Lua modules have been found under $prefix/lib64/lua/5.1/. However, when I now install flux-core on my desktop they go under $prefix/lib/lua/5.1/. I'm not sure when/how that changed, but a temporary fix would be to look in both paths during make check. For example, this fixes this particular problem for me:

diff --git a/rdl/test/Makefile.am b/rdl/test/Makefile.am
index e4790ac..c970f98 100644
--- a/rdl/test/Makefile.am
+++ b/rdl/test/Makefile.am
@@ -5,7 +5,7 @@ AM_CPPFLAGS = -I$(top_srcdir) $(JANSSON_CFLAGS) $(LUA_INCLUDE)
 
 TESTS_ENVIRONMENT = \
     LUA_PATH="$(abs_top_srcdir)/rdl/?.lua;$(FLUX_PREFIX)/share/lua/5.1/?.lua;$(FLUX_PREFIX)/share/lua/5.1/fluxometer/?.lua;$(LUA_PATH);;" \
-    LUA_CPATH="$(abs_top_builddir)/rdl/?.so;$(abs_top_builddir)/rdl/test/.libs/?.so;$(FLUX_PREFIX)/lib64/lua/5.1/?.so;$(LUA_CPATH);;" \
+    LUA_CPATH="$(abs_top_builddir)/rdl/?.so;$(abs_top_builddir)/rdl/test/.libs/?.so;$(FLUX_PREFIX)/lib64/lua/5.1/?.so;$(FLUX_PREFIX)/lib/lua/5.1/?.so;$(LUA_CPATH);;" \
     TESTRDL_INPUT_FILE="$(abs_top_srcdir)/conf/hype.lua"
 
 TESTS = trdl test-jansson.lua
diff --git a/resource/planner/test/Makefile.am b/resource/planner/test/Makefile.am
index 38f4a0b..c1abf29 100644
--- a/resource/planner/test/Makefile.am
+++ b/resource/planner/test/Makefile.am
@@ -9,7 +9,7 @@ AM_CPPFLAGS = -I$(top_srcdir) $(CZMQ_CFLAGS) $(FLUX_CORE_CFLAGS)
 TESTS_ENVIRONMENT = \
     TESTRESRC_INPUT_FILE="$(abs_top_srcdir)/conf/hype.lua" \
     LUA_PATH="$(abs_top_srcdir)/rdl/?.lua;$(FLUX_PREFIX)/share/lua/5.1/?.lua;$(LUA_PATH);;" \
-    LUA_CPATH="$(abs_top_builddir)/rdl/?.so;$(FLUX_PREFIX)/lib64/lua/5.1/?.so;$(LUA_CPATH);;"
+    LUA_CPATH="$(abs_top_builddir)/rdl/?.so;$(abs_top_builddir)/rdl/test/.libs/?.so;$(FLUX_PREFIX)/lib64/lua/5.1/?.so;$(FLUX_PREFIX)/lib/lua/5.1/?.so;$(LUA_CPATH);;"
 
 TESTS = planner_test01
 
diff --git a/resrc/test/Makefile.am b/resrc/test/Makefile.am
index 1e9038a..0e9e636 100644
--- a/resrc/test/Makefile.am
+++ b/resrc/test/Makefile.am
@@ -6,7 +6,7 @@ AM_CPPFLAGS = -I$(top_srcdir) $(JANSSON_CFLAGS) $(CZMQ_CFLAGS) $(FLUX_CORE_CFLAG
 TESTS_ENVIRONMENT = \
     TESTRESRC_INPUT_FILE="$(abs_top_srcdir)/conf/hype.lua" \
     LUA_PATH="$(abs_top_srcdir)/rdl/?.lua;$(FLUX_PREFIX)/share/lua/5.1/?.lua;$(LUA_PATH);;" \
-    LUA_CPATH="$(abs_top_builddir)/rdl/?.so;$(FLUX_PREFIX)/lib64/lua/5.1/?.so;$(LUA_CPATH);;"
+    LUA_CPATH="$(abs_top_builddir)/rdl/?.so;$(abs_top_builddir)/rdl/test/.libs/?.so;$(FLUX_PREFIX)/lib64/lua/5.1/?.so;$(FLUX_PREFIX)/lib/lua/5.1/?.so;$(LUA_CPATH);;"

A better long term fix is for flux-sched to pick up the actual Lua paths from flux-core at configure time.

@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented Nov 15, 2017

Thanks @grondo, hmm, maybe fallout from

flux-framework/flux-core#1275 Update to the latest ax_lua.m4

? I'll try adding something to this PR to that effect.

I found out my problems on my desktop were that I had some parts of flux-core installed in /usr/local in unreadable directories (due to restrictive umask setting during install). After that I get further in the tests but I still get stuck here:

FAIL: t1004-module-load.t 11 - module-load: no jobs are lost

looking a little closer,

not ok 11 - module-load: no jobs are lost
#	
#	    for i in `seq $sched_start_jobid $sched_end_jobid`
#	    do
#	        state=$(flux kvs get $(job_kvs_path $i).state)
#	        if test $state != "submitted"; then
#	            return 48
#	        fi
#	    done &&
#	    flux module remove sched
#	

adding a little echo in there to see what the state key is set to, I get:

"submitted" != submitted

which sounds like fallout from earlier KVS changes, (json string vs raw data). So maybe that will be the next problem we hit here (though @morrone claims he is passing all the tests on his desktop and hit this along the way before cleaning out his /usr/local.).

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Nov 15, 2017

I forgot to mention that I was also seeing not ok 11 - module-load: no jobs are lost after fixing the Lua path issue.

build: look in lib and lib64 for flux lua lib
Problem: rdl/test/trdl is failing.

Add temporary fix described by @grondo in pr #278:

 The rdl/test/trdl test is failing because it can't find the
 flux.hostlist module in LUA_CPATH as exported by the Makefile. (This is
 the problem I reproduced on my desktop anyway)

 The problem seems to be that, up until now, compiled Lua modules have
 been found under $prefix/lib64/lua/5.1/. However, when I now install
 flux-core on my desktop they go under $prefix/lib/lua/5.1/. I'm not sure
 when/how that changed, but a temporary fix would be to look in both
 paths during make check.
@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Nov 15, 2017

Codecov Report

Merging #278 into master will decrease coverage by 0.63%.
The diff coverage is 62.96%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #278      +/-   ##
==========================================
- Coverage   75.56%   74.92%   -0.64%     
==========================================
  Files          34       34              
  Lines        7337     7359      +22     
==========================================
- Hits         5544     5514      -30     
- Misses       1793     1845      +52
Impacted Files Coverage Δ
sched/sched.c 75.37% <100%> (-0.51%) ⬇️
sched/plugin.c 69.66% <100%> (ø) ⬆️
simulator/simsrv.c 76.64% <42.85%> (-0.56%) ⬇️
simulator/sim_execsrv.c 84.77% <50%> (-0.73%) ⬇️
simulator/submitsrv.c 80.74% <50%> (-0.23%) ⬇️
src/common/libutil/log.c 28.88% <0%> (-26.67%) ⬇️
src/common/libtap/tap.c 18.18% <0%> (-11.94%) ⬇️
rdl/test/trdl.c 80% <0%> (-7.7%) ⬇️
rdl/rdl.c 72.81% <0%> (-0.47%) ⬇️

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 a55889f...9ee7e4d. Read the comment docs.

t/t1004-module-load.t: use kvs get -j for lwj status
Problem: The "module-load: no jobs are lost" subtest
of sharness t1004-module-load.t is failing.

The test is comparing "status" (with quotes) to status
(without quotes).  The value in the KVS is a JSON string,
but flux-framework/flux-core#1257 changed flux-kvs so
that it displays raw values by default, in this case the
encoded JSON string with the quotes.

Add -j to the flux kvs get command in the test, to get
the old behavior.
@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Nov 15, 2017

Thanks @grondo, hmm, maybe fallout from

flux-framework/flux-core#1275 Update to the latest ax_lua.m4

Yes, that was it. There was a single modification we'd added to ax_lua.m4 which was dropped in the update:

flux-framework/flux-core@90e333f#diff-5c5d94ac801dec27a4153e6500233409

Unfortunately, I'm not sure why we added that change, so I'd be hesitant to put it back into flux-core, since it will certainly bite us again in the future.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Nov 15, 2017

Coverage Status

Coverage decreased (-0.1%) to 77.198% when pulling 9ee7e4d on garlick:msg_handlers into a55889f on flux-framework:master.

@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented Nov 15, 2017

OK,passing travis now. I'd say let's merge this despite the poor test coverage results, since sched won't compile against the newer core without it.

@morrone morrone merged commit e068c92 into flux-framework:master Nov 15, 2017

2 of 4 checks passed

codecov/patch 62.96% of diff hit (target 75.56%)
Details
codecov/project 74.92% (-0.64%) compared to a55889f
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.1%) to 77.198%
Details

@grondo grondo referenced this pull request May 11, 2018

Closed

Need 0.5.0 Release #340

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.