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

Allow dynamic service registration and ability for local processes to register service names #1753

Merged
merged 21 commits into from Oct 30, 2018

Conversation

Projects
None yet
6 participants
@grondo
Copy link
Contributor

grondo commented Oct 23, 2018

This PR adds basic support for dynamic service registration via service.add and service.remove RPCs as suggested in #1689. Support for service.add/remove is also added to the local connector which allows locally connected clients to register services with the broker.

The more ambitious work to allow agents to register service names that shadow existing services was parked for now in the interest of completing required functionality.

Still need to add tests, but basic support is here for comments.

@grondo

This comment has been minimized.

Copy link
Contributor Author

grondo commented Oct 23, 2018

example:

#!/usr/bin/lua
local flux = require 'flux'
local f = assert (flux.new())
local inspect = require 'inspect'

local function printf (...)
    io.stdout:write (string.format (...))
end

local mh, err = f:msghandler {
    pattern = "foo.ping",
    msgtypes = { flux.MSGTYPE_REQUEST },
    handler = function (f, msg, mh) msg:respond { seq = msg.data.seq } end
}
if not mh then error (err) end

local mh, err = f:msghandler {
    pattern = "foo.*",
    msgtypes = { flux.MSGTYPE_RESPONSE },
    handler = function (f, msg, mh)
        local resp = msg.data
        if resp.errnum then
            error (msg.tag .. ": "..resp.errnum)
        end
        local seq = resp.seq
        printf ("%s seq=%d\n", msg.tag, seq)
        if seq == 10 then
            f:reactor_stop()
            return
        end
        f:send ("foo.ping", { seq = seq + 1 })
    end
}
if not mh then error (err) end

-- Register with local connector for the "foo" service:
local rc, err = f:rpc ("service.add", { service = "foo" })
if not rc then error (err) end
printf ("Added service %s\n", "foo")

-- Send the first ping and enter reactor
assert (f:send ("foo.ping", { seq = 1 }))
f:reactor()

-- Deregister service.foo (we could also let disconnect deregister us)
local rc, err = f:rpc ("service.remove", {service = "foo"})
if not rc then error (err) end
$ src/cmd/flux start lua t.lua 
Added service foo
foo.ping seq=1
foo.ping seq=2
foo.ping seq=3
foo.ping seq=4
foo.ping seq=5
foo.ping seq=6
foo.ping seq=7
foo.ping seq=8
foo.ping seq=9
foo.ping seq=10
@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Oct 23, 2018

Codecov Report

Merging #1753 into master will increase coverage by 0.13%.
The diff coverage is 71.37%.

@@            Coverage Diff             @@
##           master    #1753      +/-   ##
==========================================
+ Coverage   79.54%   79.67%   +0.13%     
==========================================
  Files         186      187       +1     
  Lines       34483    34724     +241     
==========================================
+ Hits        27429    27666     +237     
- Misses       7054     7058       +4
Impacted Files Coverage Δ
src/broker/service.c 87.65% <100%> (+0.64%) ⬆️
src/broker/module.c 83.56% <100%> (-0.23%) ⬇️
src/common/libflux/service.c 60% <60%> (ø)
src/broker/broker.c 77.05% <60.97%> (-0.82%) ⬇️
src/modules/pymod/py_mod.c 65.06% <61.11%> (+65.06%) ⬆️
src/modules/connector-local/local.c 74.96% <73.91%> (+1.79%) ⬆️
src/modules/content-sqlite/content-sqlite.c 57.09% <80%> (+0.76%) ⬆️
src/common/libflux/message.c 81.02% <0%> (+0.12%) ⬆️
src/modules/kvs/kvs.c 65.71% <0%> (+0.15%) ⬆️
... and 6 more
@garlick

This comment has been minimized.

Copy link
Member

garlick commented Oct 23, 2018

Nice!

Couple of quick comments:

  • we should drop the service_name thing from RFC 6 like it never happened! :-)
  • could the service_ message handlers live in service.c? e.g. with a service_switch_set_flux() function called from the broker to register them once the local flux_t handle is available?
@dongahn

This comment has been minimized.

Copy link
Contributor

dongahn commented Oct 23, 2018

Cool! I think I can use this in DYAD immediately.

@grondo

This comment has been minimized.

Copy link
Contributor Author

grondo commented Oct 23, 2018

we should drop the service_name thing from RFC 6 like it never happened! :-)

Good point! I didn't think to check the RFC.

could the service_ message handlers live in service.c? e.g. with a service_switch_set_flux() function called from the broker to register them once the local flux_t handle is available?

This would have been my preference and was my initial approach. However, I ran into awkward trouble quickly since the module code and "service switch" code are in different abstractions. i.e. it seemed like trying to put the service.* handling code, which needs to access the module hash, was breaking these abstractions that were already set up. There is a comment to that effect on the msg handlers for those RPCs:

/* Dynamic service registration.
 * These handlers need to appear in broker.c so that they have
 *  access to broker internals like modhash
 */
static void service_add_cb (flux_t *h, flux_msg_handler_t *w,
                            const flux_msg_t *msg, void *arg)
{

At some future point, we will probably want to have the "modservice" and "service.*" code better integrated. Even now it might be nice to be able to include the service names for loaded modules, but neither the extension module RFC nor the existing code easily allows that. Fixing that seemed like more than we would want to tackle for this PR though.

@garlick

This comment has been minimized.

Copy link
Member

garlick commented Oct 23, 2018

I ran into awkward trouble quickly since the module code and "service switch" code are in different abstractions.

Uh, sorry, I should have looked more closely before speaking. That cleanup's probably for another day and your approach makes sense!

@garlick

This comment has been minimized.

Copy link
Member

garlick commented Oct 24, 2018

I think probably a couple of API functions ought to be added like:

flux_future_t *flux_service_add (flux_t *h, const char *name);
flux_future_t *flux_service_remove (flux_t *h, const char *name);

rather than having users cal flux_rpc_pack() directly.

It seems like client->services should be a zlist? Or, since that's for disconnect handling, would it maybe be possible to eliminate it entirely and just do a linear search through the ctx->services looking for a match for the client_t pointer? (presuming there will be relatively few services registered this way)

@grondo

This comment has been minimized.

Copy link
Contributor Author

grondo commented Oct 24, 2018

I think probably a couple of API functions ought to be added like:

I thought about that, but I kind of lean toward not adding a new API function for every RPC. We already have a nice rpc interface, and the API is growing very rapidly, which then requires language bindings. I figured at this point it was unnecessary to wrap the 3 lines of code to send add/remove RPCs -- an API interface could be added later when we figure out if this direction is useful.

It seems like client->services should be a zlist? Or, since that's for disconnect handling, would it maybe be possible to eliminate it entirely and just do a linear search through the ctx->services looking for a match for the client_t pointer? (presuming there will be relatively few services registered this way)

It is much easier to maintain the set of service names in use by a client as a set, not a list. What is the benefit of the extra code to do linear searches through zlists exactly? Sorry if I'm missing some obvious drawback of a hash.

@grondo

This comment has been minimized.

Copy link
Contributor Author

grondo commented Oct 24, 2018

Actually you make a good point about just dropping client->services hash and doing a linear search at disconnect. This is what the broker already does anyway, and it will remove a member from each client which would largely be unused.

@garlick

This comment has been minimized.

Copy link
Member

garlick commented Oct 24, 2018

 I kind of lean toward not adding a new API function for every RPC

Were you thinking this is for internal use only at this point?

It is much easier to maintain the set of service names in use by a client as a set, not a list. What is the benefit of the extra code to do linear searches through zlists exactly? Sorry if I'm missing some obvious drawback of a hash.

Oh sorry, maybe I misread, it looked like the hash was being used at a list, and the hash pre-allocates 128 slots (or something like that, so it's a bit heavy if it's not ever being used as a hash. I'll have another look at that.

@grondo

This comment has been minimized.

Copy link
Contributor Author

grondo commented Oct 24, 2018

Were you thinking this is for internal use only at this point?

I hadn't considered it, but in the timeframe of the current sprint, yes possibly. flux_rpc is exported via the API though, correct? It seems like we could take advantage of that sometimes instead of wrapping every simple RPC in a new API call? I'm probably missing some perspective at this point though..

@grondo grondo force-pushed the grondo:dynamic-services branch from 0f9f990 to ffb96b8 Oct 24, 2018

@grondo

This comment has been minimized.

Copy link
Contributor Author

grondo commented Oct 24, 2018

Oh sorry, maybe I misread, it looked like the hash was being used at a list, and the hash pre-allocates 128 slots (or something like that, so it's a bit heavy if it's not ever being used as a hash. I'll have another look at that.

Nah, I think you were right and already removed it.

However, it was being used as a set, so the hash was handy for removing services as they were manually deregistered by a client (instead of searching a list to delete an entry). However, that wasn't really necessary so good comment!

@grondo

This comment has been minimized.

Copy link
Contributor Author

grondo commented Oct 24, 2018

Also added here is an experimental update to pymod to dynamically register a service name based on the name of the script being loaded, instead of just pymod. (There is also commented code to remove the pymod service name, which could theoretically let multiple pymod scripts be loaded at the same time. However, this isn't safe in practice, nor is there a way to remove multiple modules with the same name presently)

The echo.py example is also updated to take advantage of the new pymod feature, and it actually works:

grondo@peep:~/git/f.git$ flux module load pymod --verbose --path=$(pwd)/src/modules/pymod echo
grondo@peep:~/git/f.git$ flux dmesg | grep pymod
2018-10-24T03:27:05.753254Z broker.debug[0]: insmod pymod
2018-10-24T03:27:05.770080Z pymod.info[0]: loading python module named: echo
grondo@peep:~/git/f.git$ cat echo.lua 
#!/usr/bin/lua
local f = assert (require 'flux'.new())
local resp = assert (f:rpc ("echo.foo", { data = "foo" }))
print (require 'inspect' (resp))

grondo@peep:~/git/f.git$ lua echo.lua 
{
  data = "foo"
}
@garlick

This comment has been minimized.

Copy link
Member

garlick commented Oct 24, 2018

The pymod change is pretty cool!

I hadn't considered it, but in the timeframe of the current sprint, yes possibly. flux_rpc is exported via the API though, correct? It seems like we could take advantage of that sometimes instead of wrapping every simple RPC in a new API call? I'm probably missing some perspective at this point though..

If you feel strongly about it, I'm fine with that for now. My general feeling is, for a new service that might be externally useful, it seems like a good compromise position is to provide the API call with header-only documentation. It makes the thing easier to figure out for users, and allows the expected payloads, etc. to change without the need to go find users and fix them.

I agree API bloat is a problem though. The zeromq people have a way of tagging interfaces as experimental, which allows them to get rid of things that don't work out over a few release cycles. Maybe something like that's a way to be more deliberate about API growth?

@grondo

This comment has been minimized.

Copy link
Contributor Author

grondo commented Oct 24, 2018

If you feel strongly about it, I'm fine with that for now. My general feeling is, for a new service that might be externally useful, it seems like a good compromise position is to provide the API call with header-only documentation. It makes the thing easier to figure out for users, and allows the expected payloads, etc. to change without the need to go find users and fix them.

Well I don't feel too strongly about it, but each time I wrap a simple RPC in an API function, I kind of wonder what the point of the nice flux_rpc_pack interface is, since no external users will have occasion to use it. Sometimes if feels like it would be better to have a nice way to document protocols and allow us the speed and flexibility of rapidly developing RPCs, rather than adding that extra level of C api that then needs to be separately maintained as the protocol evolves. With the C api you also require bindings to be maintained instead of using an existing, possibly nicer RPC interface.

However, I can see the benefit of abstracting the protocol away from (most) users, and I don't feel too strongly so I can add functions to the API (was mainly just defending my position not adding them to the initial PR)

@grondo

This comment has been minimized.

Copy link
Contributor Author

grondo commented Oct 24, 2018

Also added here is an experimental update to pymod

I forgot to mention that there is a problem with pymod on systems where _cffi_backend.so is not linked against libpython*.so. In this case, pymod will not be able to load and run a python script because libpython symbols won't have visibility in the script, so import * will fail with an undefined symbol error.

To workaround, you have to dlopen the correct libpython*.so so you can promote its flags to RTLD_GLOBAL. (This is the same issue we hit with librdl loading Lua modules when it was loaded in the context of sched.so)

On TOSS, at least the default _cffi_backend.so is linked correctly with libpython2.7.so. However, on Ubuntu 18.04 it doesn't appear to have proper linking so this workaround would be required. The tricky part, though, is finding the right name to dlopen()... Not sure there is a good way to do that.

@grondo grondo force-pushed the grondo:dynamic-services branch from 300caa6 to dfeb13e Oct 24, 2018

@grondo

This comment has been minimized.

Copy link
Contributor Author

grondo commented Oct 25, 2018

Latest push enables pymod on distros where _cffi_backend.so isn't linked with libpython by dlopening the correct libpython (as determined during configure) with RTLD_GLOBAL.

This allows a sanity test for pymod to be added to sharness testsuite.

The last bit here is to add a test for connector-local dynamic services.

@grondo grondo force-pushed the grondo:dynamic-services branch 3 times, most recently from de8689c to 3a45f69 Oct 26, 2018

@grondo

This comment has been minimized.

Copy link
Contributor Author

grondo commented Oct 26, 2018

I rebased this on top of #1772 since that PR makes it easier to add the pymod test unconditionally.

I've also added a new test under python/t1000-service-add-remove.py which test the service.add service.remove interface. I kind of stumbled along writing the test, so would appreciate a gander by @SteVwonder or @trws.

@SteVwonder
Copy link
Member

SteVwonder left a comment

Thanks @grondo! Other than the comments below, this PR LGTM!

return 2

def service_add (f, name):
future = f.flux_service_register (name)

This comment has been minimized.

@SteVwonder

SteVwonder Oct 26, 2018

Member

If you so desired, I believe this could be written as future = f.service_register (name), and it will still work the same.

This comment has been minimized.

@grondo

grondo Oct 26, 2018

Author Contributor

Ah, ok so flux_ prefix can be dropped from the automatic ffi functions?

This comment has been minimized.

@SteVwonder

SteVwonder Oct 26, 2018

Member

Yeah. Whenever you do flux_handle.method, the bindings automatically try and find a function of the form flux_method and if that fails, it tries just method.

If you do it on a different wrapper class like RPC, it will try flux_rpc_method as well.

self.f.close()

def test_001_register_service (self):
rc = service_add (self.f, "foo")

This comment has been minimized.

@SteVwonder

SteVwonder Oct 26, 2018

Member

Looks like some tabs got mixed in with the spaces.

This comment has been minimized.

@grondo

grondo Oct 26, 2018

Author Contributor

Thanks! I will remove all tabs from the file.

self.assertEqual(rc, 0, msg="flux_service_register ('foo')")

def test_002_service_add_eexist (self):
with self.assertRaises(Exception) as e:

This comment has been minimized.

@SteVwonder

SteVwonder Oct 26, 2018

Member

If I'm being nitpicky, this should be an EnvironmentError. They are the only exceptions (along with their subclasses) that contain the attribute errno. So if any other type of exception is caught, the e.exception.errno below with throw an AttributeError.

This comment has been minimized.

@grondo

grondo Oct 26, 2018

Author Contributor

ok, great point!

self.assertEqual (msg, p)
f.reactor_stop (f.get_reactor())

w2 = self.f.msg_watcher_create (cb, FLUX_MSGTYPE_RESPONSE,

This comment has been minimized.

@SteVwonder

SteVwonder Oct 26, 2018

Member

I believe this watcher needs to be started before the reactor is entered.

This comment has been minimized.

@grondo

grondo Oct 26, 2018

Author Contributor

Yes, this points at a flaw in the test (and also is why it would be nice to see an ok for each assertion that is checked.) Since I failed to start the message watchers, the reactor is returning immediately and the assertions in the callbacks are never called. Will have to add a global that gets set by the callback to ensure it actually runs I think..

def test_005_service_rpc_enosys (self):
fut = self.f.flux_rpc ("foo.echo", "{}", FLUX_NODEID_ANY, 0)
with self.assertRaises(Exception) as e:
self.f.flux_future_get (fut, ffi.NULL)

This comment has been minimized.

@SteVwonder

SteVwonder Oct 26, 2018

Member

I think here you can take advantage of the default arguments in the rpc bindings and it still work the same.

        fut = self.f.rpc_create ("foo.echo")
        with self.assertRaises(EnvironmentError) as e:
            fut.get ()
        self.assertEqual (e.exception.errno, errno.ENOSYS)

This comment has been minimized.

@grondo

grondo Oct 26, 2018

Author Contributor

great, thanks!

This comment has been minimized.

@grondo

grondo Oct 26, 2018

Author Contributor

is there a .get() method on futures in the python bindings yet? fut.get() doesn't seem to work for me:

      AttributeError: cdata 'struct flux_future *' points to an opaque type: cannot read fields

This comment has been minimized.

@trws

trws Oct 26, 2018

Member

We haven't re-worked the futures yet, there isn't a generic future at all at this point unfortunately.

This comment has been minimized.

@SteVwonder

SteVwonder Oct 26, 2018

Member

That should be calling the .get method of the RPC class. I'm not sure why that isn't working. That is a bit concerning.

This comment has been minimized.

@trws

trws Oct 26, 2018

Member

@SteVwonder is right, but I think I see the issue. If you call f.rpc_create() it returns a python wrapper that adds the get() method. The raw flux_rpc () call returns something like a cffi.ctype("flux_future_t *") which doesn't have the wrapper class to provide methods.

This comment has been minimized.

@grondo

grondo Oct 28, 2018

Author Contributor

Got it, thanks!

'

# Test can be a tad "racey" at times if grace period is too low, up to
# 5 seconds to ensure joblog can be generated properly.

This comment has been minimized.

@SteVwonder

SteVwonder Oct 26, 2018

Member

Accidental copy-paste carryover?

This comment has been minimized.

@grondo

grondo Oct 26, 2018

Author Contributor

yes. 👍

@trws

This comment has been minimized.

Copy link
Member

trws commented Oct 26, 2018

@grondo grondo force-pushed the grondo:dynamic-services branch from 3a45f69 to 08b0cfd Oct 28, 2018

@grondo

This comment has been minimized.

Copy link
Contributor Author

grondo commented Oct 28, 2018

Rebased, addressed @SteVwonder's comments I believe, and got the python/t1000-service-add-remove.py test working. However, this required the workaround discussed in #1776 (connector-local module always enables routes on requests), and I'm not sure that is acceptable. (However, I wanted to get the test working in python, and not have to fall back to Lua or C to test)

@grondo

This comment has been minimized.

Copy link
Contributor Author

grondo commented Oct 28, 2018

Hm, the distcheck builders are getting a failure in the pymod test:

Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
ImportError: No module named flux
not ok 2 - pymod echo.py function works

This might mean that PYTHONPATH is not quite correct in the VPATH builds (however, all the other python tests work, so it might be more subtle than that. Probably just affects the environment of the broker itself?)

grondo added some commits Oct 23, 2018

broker: add service_get_uuid()
Add a service_get_uuid() call to the broker's service switch
module. This will be required to allow service deregistration when
dynamic service registration is added to the broker.
broker: add function to lookup module by uuid
Add a module_lookup() function to the broker's modhash interface
to allow a lookup of modules by their uuid. This will be necessary
for dynamic service registration/deregistration.
broker: add dynamic service registration
Add new RPCs service.add and service.remove to the broker, which allow
modules to dynamically register alternate service names for which they
will accept requests.

Each of these RPCs take a single member payload with key "service" set
to the name of the requested service to register (service.add) or
deregister (service.remove).

This allows a single module to register under more than 2 service names,
and will eventually allow locally connected API clients to register
services via the local conneector.
content-sqlite: use service.add RPC
Instead of using the MOD_SERVICE macro to register the content-sqlite
module under the "content-backing" service name, use the new service.add
RPC offered by the broker.

Since the name must be registered before entering the reactor, we use
a synchronous RPC for `service.add`. This should have the equivalent
effect of the previous MOD_SERVVICE() usage.
broker/module: remove MOD_SERVICE support
Remove support for the optional "mod_service" symbol in modules.
Support for this symbol is no longer needed after introduction of
the service.add/service.remove RPCs.
connector-local: move response code out of internal_request
Abstract the client response code out of internal_request() and
create a reusable client_repond() function.

This not only makes a new reusable function, but breaks up the
internal_request() function, which was getting a bit long (and
will get longer soon.)
connector-local: allow clients to send responses
Problem: API connected clients cannot send messages of type
FLUX_MSGTYPE_RESPONSE. In order to allow these clients to register
services, we must allow responses to flow through the connector
to the broker.
connector-local: enable service registration
This commit enables service registration from connector-local connected
clients via the `service.add` RPC.

Since the broker isn't currently capable of multi-hop routing of
request messages, this feature is implemented by adding a service
lookup hash to the connector-local module, which can then register with
the broker on behalf of clients, and take care of routing requests to
the client which has requested registration for the service matching
those requests.

The connector-local module will intercept both service.add and
service.remove messages, and handle them appropriately. It will
additionally automatically deregister all services currently owned
by a client on disconnect.

Fixes #1689.
bindings/python: fix bad import for MessageWatchers
In bindings/pythoon/flux/core/handle.py MessageWatcher was imported
from flux.core.watchers when the implementation is actually in
flux.message. Fix the bad path.
modules/pymod: register service name dynamically
Add code to pymod to register a service name dynamically based on the
name of the script being loaded. This allows a python-based module
to register any service name, not just `pymod` and handle request
messages for that service.

For example, an echo server may be loaded with

  flux module load pymod echo

and process messages matching topic string "echo.*".

Code to *deregister* the automatic `pymod` service name is also
included in this commit but commented out, because, though now
theoretically possible, it is not currently safe to load a second
pymod module with a different python script (and there is no way to
unload just one of two modules with the same name)
modules/pymod: update echo.py example
Update the echo.py example pymod script given the updated pymod
module's capability to register a service name based on the
loaded script.

This version starts a simple echo service that accepts echo.* messages
and replies with the same payload.
libflux: add flux_service_register/unregister
Add flux_service_register and flux_service_unregister to the
libflux API. These functions call the broker's `service.add` and
`service.remove` methods on behalf of the caller.
configure.ac: add libpython name to AC_SUBST
Export the name of the discovered libpython DSO name to config.h
so that code that may need to know which libpython Flux was built
against doesn't have to rediscover the name.
bindings/python: fix Message payload setter
Message payload setter should use the set_string method not set_json.
bindings/python: ensure destruct is set on Message
Ensure self.destruct member of Message objects is set properly
to avoid double-free of flux_msg_t in situations where python
doesn't have ownership of the msg object.

Fixes #1777
connector-local: enable routes on all api requests
Problem: The connector-local module throws an error and drops any
request messages from connected clients which do not have the
FLUX_MSGFLAG_ROUTE flag. Currently the python flux_send() binding
does not set this flag for requests, so python scripts are unable
to use this interface.

It is arguable that the local connector *should* drop requests
without FLUX_MSG_ROUTE set, because this means that the msg being
sent via the connector is not a valid request. However, since we
know all requests will need FLUX_MSGFLAG_ROUTE set, and the connector
is accepting messages on behalf of clients, is there a harm in
ensuring the route flag is set on all requests? This reduces the
burden of "encoding" requests from the API clients to the API
itself, which may or may not be the correct approach.

Until the python bindings can encode a request, though, add code
to the connector-local module to enable routes on all request
messages.
bindings/python: fix MessageWatcher destructor
The MessageWatcher destroy() method was calling the incorrect
C function. Update the name to `flux_msg_handler_destroy` to
avoid errors when calling watcher.destroy().
modules/pymod: ensure libpython syms are global
Problem: On some distributions (e.g. Ubuntu 18.04) _cffi_backend.so
is not linked against libpython DSO, and since pymod.so is loaded
with RTLD_LOCAL, symbols from libpython may not be visble to cffi,
and thus `import cffi` fails with unresolved symbol errors. (This
may be a general problem with any python module that is not
linked with libpython in this scenario)

To fix, promote libpython symbols to global by calling dlopen(3)
on libpython (as found by configure) with RTLD_GLOBAL.
testsuite: add sanity test for pymod
Ensure pymod module can be loaded with the echo.py example,
and that the example echo server can handle requests.
testsuite: add a python test for service.add/remove
Add python/t1000-service-add-remove.py to the testsuite to test
the connector-local usage of `service.add` and `service.remove`.
testsuite: avoid relative paths in PYTHONPATH
Problem: python or one of its modules seems to clear relative
paths from PYTHONPATH, and this is preventing the t9001-pymod.t
test from working problem with a failure to import the 'flux' module.

The problem seems to be resolved using absolute paths to top_srcdir
and top_builddir, so just use this as the solution for now.

@grondo grondo force-pushed the grondo:dynamic-services branch from 1cfcb5d to 39c7f51 Oct 30, 2018

@grondo

This comment has been minimized.

Copy link
Contributor Author

grondo commented Oct 30, 2018

Thanks for the review @SteVwonder! Addressed your comments, squashed and force-pushed.

@grondo

This comment has been minimized.

Copy link
Contributor Author

grondo commented Oct 30, 2018

I actually ended up removing that code that was #ifdef'd out. It may have been instructive, but probably a bad example anyway

@garlick

This comment has been minimized.

Copy link
Member

garlick commented Oct 30, 2018

Nice work @grondo, I'm pressing the button!

@garlick garlick merged commit 0f881e9 into flux-framework:master Oct 30, 2018

2 of 3 checks passed

codecov/patch 71.37% of diff hit (target 79.54%)
Details
codecov/project 79.67% (+0.13%) compared to 8039b92
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@grondo

This comment has been minimized.

Copy link
Contributor Author

grondo commented Oct 30, 2018

Thanks!

@grondo grondo deleted the grondo:dynamic-services branch Feb 8, 2019

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.