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

make the loop:// connector built-in rather than a DSO #5486

Merged
merged 8 commits into from
Oct 9, 2023

Conversation

garlick
Copy link
Member

@garlick garlick commented Oct 5, 2023

This adds the capability for connectors to be built-in instead of requiring them all to be built as DSOs, and converts loop:// to built-in, which allowed the duplicate implementation in libtestutil to be removed.

I started this while looking into replacing shmem:// with a non-0MQ equivalent as a way to avoid the zmq_ctx_term(3) hangs when futures are leaked (#5484) . That would need to be built-in, at least for the simple design I had in mind where named channels would be stored in a libflux-core.so global protected by mutexes, but even if that turns out to be a bad idea, this seems like a nice cleanup.

Copy link
Contributor

@grondo grondo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow, nice cleanup!

From the coverage reporting at least, it appears that setting testing userid/rolemask for the loop:// connector from the environment variables FLUX_HANDLE_USERID and FLUX_HANDLE_ROLEMASK is not covered (likely wasn't before either though). Could that be dropped? If not maybe it should have a couple sanity tests? Meh, just a suggestion though.

{
loop_ctx_t *c = impl;

if (option && streq (option, FLUX_OPT_TESTING_USERID)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just curious if there's a case for this function to be called with a NULL option, and if not should it just have the following at the top?

if (option == NULL) {
    errno = EINVAL;
    return -1;
}

Oh I see this just matches the usage in the local connector, but I'm still unsure of the use case for a NULL option.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just copied over what was there in the loopback_create() implementation, but I'll have a look and see if there's a bit more cleanup we can do.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems fine to me for now if you want to defer cleanup for later. Just something that caught my eye is all..

@grondo
Copy link
Contributor

grondo commented Oct 5, 2023

Looks like codeql is failing (hopefully temporarily) due to an error fetching some ubuntu packages.

@codecov
Copy link

codecov bot commented Oct 5, 2023

Codecov Report

Merging #5486 (db53159) into master (10abbf4) will increase coverage by 0.01%.
The diff coverage is 90.90%.

❗ Current head db53159 differs from pull request most recent head 0531272. Consider uploading reports for the commit 0531272 to get more accurate results

@@            Coverage Diff             @@
##           master    #5486      +/-   ##
==========================================
+ Coverage   83.64%   83.65%   +0.01%     
==========================================
  Files         484      484              
  Lines       81443    81480      +37     
==========================================
+ Hits        68120    68166      +46     
+ Misses      13323    13314       -9     
Files Coverage Δ
src/common/libflux/handle.c 86.53% <100.00%> (+0.15%) ⬆️
src/common/libflux/connector_loop.c 80.20% <87.50%> (ø)

... and 11 files with indirect coverage changes

@garlick
Copy link
Member Author

garlick commented Oct 5, 2023

From the coverage reporting at least, it appears that setting testing userid/rolemask for the loop:// connector from the environment variables FLUX_HANDLE_USERID and FLUX_HANDLE_ROLEMASK is not covered (likely wasn't before either though). Could that be dropped? If not maybe it should have a couple sanity tests?

The environment variables are interpreted inside flux_open() and translated into flux_opt_set () calls of FLUX_OPT_TESTING_USERID and FLUX_OPT_TESTING_ROLEMASK. I know there are tests that use the options direcly for the loop connector (see libflux/test/rpc_security.c for example) and sharness tests that use the environment variables with the local connector. So I think all the code should be covered? I'll have a closer look at the coverage report.

@grondo
Copy link
Contributor

grondo commented Oct 5, 2023

Ah, ok. I pointed it out just in case, if the code is already used and tested then it could be coverage hiccup (or more likely human error on my part) so safe to ignore it!

Problem: flux_t is typedefed to 'struct flux_handle_struct'
but name_type style names are forbidden per RFC 7.

Rename to 'struct flux_handle'.
Problem: some code formatting in libflux is not consistent with modern
project practices.

Make long parameter lists one per line.
Problem: some connector code formatting is not consistent with modern
project practices.

Fix the following
- make long parameter lists one per line
- break long && chains to one per line with one indent
Problem: all connector plugins must be built as DSOs which
can be inconvenient.

The loop:// plugin is rarely used becuase the majority of unit tests
that would use it are in libflux and other code in src/common, which
is built before src/connectors.

Other connectors might be able to take advantage of global state in
libflux-core.so to their betterment, were they implemented as builtins.

Add an array of builtin connector init functions, currently empty,
and search it before we go off looking for DSOs in the file system.
Problem: the loop connector is not useful for testing libflux
when built as a connector, because connector dsos are built after
src/common.

Relocate loop connector to libflux and build it in.
The only change to loop.c is renaming the generic init function.
Problem: rpc unit tests test relies on connector level accessors for
credentials but loop:// doesn't implement them.

Add support for FLUX_OPT_TESTING_USERID and FLUX_OPT_TESTING_ROLEMASK
options, as implemented in loopback_create().
Problem: NULL parameters to flux_op_set() or flux_op_get()
could cause a segfault.

Fail with EINVAL If 'h' or 'option' are NULL.
Adjust checks in local and loop connectors to fit this interface.

Add unit tests that cover both the generic checks at entry
and the specific checks in connector_loop.
Problem: the libtestutil loopback_create() function duplicates
loop:// for unit tests that could not rely on loop.so being built,
but now that loop:// is built in, it is no longer required.

Convert unit tests to use the real loop:// connector and remove
the duplicate implementation.
@garlick
Copy link
Member Author

garlick commented Oct 9, 2023

Thanks for the review! I rebased on current master and forced a push and will set MWP.

@mergify mergify bot merged commit e7e40ba into flux-framework:master Oct 9, 2023
30 checks passed
@garlick garlick deleted the loop_builtin branch October 13, 2023 16:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants