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

libidset: replace internal nodeset class #1862

Merged
merged 18 commits into from Dec 12, 2018

Conversation

Projects
None yet
4 participants
@garlick
Copy link
Member

garlick commented Dec 8, 2018

The old "nodeset" class has some deficiencies documented in #1495. We created libflux-idset as an external, standalone library to export nodeset-like functionality to framework projects, but only created the bare minimum needed by flux-sched at the time and left nodeset in place.

In the spriit of recent API cleanup efforts, this PR finishes the job, adding functionality to idset, converting users, and removing nodeset.

@garlick garlick force-pushed the garlick:idset_more branch from a8c5018 to 96d85b8 Dec 10, 2018

@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented Dec 10, 2018

Still some squashing to do here before a review. Sorry for the noise.

@garlick garlick force-pushed the garlick:idset_more branch from 96d85b8 to c535848 Dec 10, 2018

@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented Dec 10, 2018

Rebased.

One builder failed the valgrind test:

==963== 6,310,202 (224 direct, 6,309,978 indirect) bytes in 1 blocks are definitely lost in loss record 103 of 103
==963==    at 0x4C31D2F: realloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==963==    by 0x4E87BF9: ev_realloc (ev.c:1741)
==963==    by 0x4E881E4: ev_feed_event (ev.c:2005)
==963==    by 0x4E8B12F: queue_events (ev.c:2034)
==963==    by 0x4E8B12F: ev_run (ev.c:3714)
==963==    by 0x4E585C2: flux_reactor_run (reactor.c:140)
==963==    by 0xB7C910F: mod_main (job.c:938)
==963==    by 0x1142EB: module_thread (module.c:158)
==963==    by 0x55BA6DA: start_thread (pthread_create.c:463)
==963==    by 0x636188E: clone (clone.S:95)
==963== 

looks unrelated so I restarted it.

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Dec 10, 2018

Two quick comments, neither should be considered must-fix:

  • 73f7634: it would be slightly cleaner to add libidset to libflux_internal in a separate commit. Lumping it in with the mrpc update seems arbitrary and isn't symmetrical with the other code that is updated. (or maybe it is? I would also accept a comment in the commit message that describes why libidset was moved into libflux_internal in this commit)

  • e6b2e84 could use a commit message. The commit subject says the linkage was "fixed" but there isn't a hint why (which could be helpful for future developer wondering why that test cant use $(test_ldadd) like all the other tests)

@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented Dec 10, 2018

Thanks, good suggestions!

@garlick garlick force-pushed the garlick:idset_more branch from c535848 to 5b03426 Dec 11, 2018

@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented Dec 11, 2018

OK, I think I addressed @grondo's comments. I also expanded some commit messages a bit.

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Dec 11, 2018

Codecov Report

Merging #1862 into master will increase coverage by 0.16%.
The diff coverage is 89.59%.

@@            Coverage Diff             @@
##           master    #1862      +/-   ##
==========================================
+ Coverage   79.89%   80.06%   +0.16%     
==========================================
  Files         196      196              
  Lines       35228    34960     -268     
==========================================
- Hits        28147    27992     -155     
+ Misses       7081     6968     -113
Impacted Files Coverage Δ
src/broker/hello.c 79.46% <ø> (ø) ⬆️
src/common/libflux/response.c 79.62% <ø> (ø) ⬆️
src/modules/cron/datetime.c 90.62% <ø> (ø) ⬆️
src/cmd/flux-ping.c 87.05% <100%> (ø) ⬆️
src/common/libflux/mrpc.c 87.55% <100%> (+0.83%) ⬆️
src/broker/broker.c 77.41% <100%> (ø) ⬆️
src/cmd/flux-exec.c 74.41% <50%> (-1.34%) ⬇️
src/cmd/flux-module.c 83.72% <76%> (-0.97%) ⬇️
src/common/libidset/idset_encode.c 89.47% <89.47%> (ø)
src/common/libutil/cronodate.c 94.68% <90%> (-0.43%) ⬇️
... and 10 more
@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented Dec 11, 2018

Let me see if I can get the idset coverage up just a bit before this gets merged.

I also feel like the way everything is being linked is a little convoluted and might be improved if the veb stuff moved to libidset (though I've tried that twice now and got myself in the weeds both times).

libidset: refactor to prepare for expansion
Make the following changes to libidset in preparation
for adding more code:

- namespace the include guard symbol with FLUX_
- create idset_private.h
- add common validate_idset_flags() function
- make idset_set() a public function
- move idset_decode() to idset_decode.c
- move idset_encode() to idset_encode.c
- drop internal magic for user-after-free
- rename ENCODE_CHUNK to IDSET_ENCODE_CHUNK
- use default size on idset_create (size=0)

Note on idset_create():

idset_create (0, IDSET_FLAG_AUTOGROW) is similar to
nodeset_create(), in that it creates an idset with a
default internal size of 1K that is grown to fit id's
>= size as they are set with idset_set ().

idset_create (N, IDSET_FLAG_AUTOGROW), where N > 0, is
similar to nodeset_create_size (N).

idset_create (0, IDSET_FLAG_AUTOGROW), followed by
idset_set () is the equivalent to nodeset_create_rank ().
Similarly, nodeset_create_range () is replaced with
idset_create () followed by idset_set_range ().
(idset_set_range() will be added in a future commit).

Note on idset_encode():

idset_encode (idset, IDSET_FLAG_BRACKETS, IDSET_FLAG_RANGE)
is similar to nodeset_string (ns), except that the returned
string must be freed by the caller, and the flags to the
encode function, rather than settings on the nodeset,
determine whether the idset is encoded with brackets or ranges.

@garlick garlick force-pushed the garlick:idset_more branch from 203347d to 3191df4 Dec 11, 2018

@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented Dec 11, 2018

Rebased on current master and squashed down the unit test addition. I think this could be ready to go in. @chu11, sounds like @grondo may be offline for a bit. Would you mind giving this a quick review?

@garlick garlick added this to the release 0.11.0 milestone Dec 12, 2018

* Return 0 on success, -1 on failure with errno set.
*/
int idset_set (struct idset *idset, unsigned int id);
int idset_range_set (struct idset *idset, unsigned int lo, unsigned int hi);

/* Remove id (or range [lo-hi] from idset.

This comment has been minimized.

@chu11

chu11 Dec 12, 2018

Contributor

stupid typo, no end paren ')'

This comment has been minimized.

@garlick

garlick Dec 12, 2018

Author Member

Doh!


if (!idset1 || !idset2)
return false;

This comment has been minimized.

@chu11

chu11 Dec 12, 2018

Contributor

minor thought, would it be a net win to do some obvious checks before iterating through everything below? I know vebsize() is costly and perhaps not worth it. T.M != T.M? I don't know the library that well, so maybe it's not a net win.

This comment has been minimized.

@garlick

garlick Dec 12, 2018

Author Member

T.M is the number of available slots in the set, not filled slots. vebsize() is the size in bytes of the data structure, not related to filled slots. So I don't think either of those is helpful here.

In looking at this, I realized there are no users of idset_compare() so hey, let's just get rid of it.

test_range_set ();
test_clear ();
test_range_clear ();
test_copy ();

This comment has been minimized.

@chu11

chu11 Dec 12, 2018

Contributor

i don't think i see a unit test for AUTOGROW?

This comment has been minimized.

@garlick

garlick Dec 12, 2018

Author Member

Thanks will add some.

@garlick garlick force-pushed the garlick:idset_more branch from 34bb3bc to 5b1fbe0 Dec 12, 2018

@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented Dec 12, 2018

LMK how this looks and I'll squash the incremental devel.

@chu11
Copy link
Contributor

chu11 left a comment

LGTM, just one stupid typo I saw.

@@ -275,7 +275,7 @@ void test_range_set (void)

ok (idset_range_set (idset, 0, 2) == 0,
"idset_range_set 0-2 worked");
ok (idset_range_set (idset, 80, 79) == 0,
ok (idset_range_set (idset, 80, 79) == 0, // revsersed

This comment has been minimized.

@chu11

chu11 Dec 12, 2018

Contributor

typo "revsersed"

This comment has been minimized.

@garlick

garlick Dec 12, 2018

Author Member

Sheesh, I blame this keyboard.

garlick added some commits May 18, 2018

libidset: add new functions
Add the following new functions:

New:				Similar to:
idset_range_set()		nodeset_add_range()
idset_clear()			nodeset_delete_rank()
idset_range_clear()		nodeset_delete_range()
idset_first()			nodeset_min()
idset_next()			nodeset_next_rank()
idset_count()			nodeset_count()
idset_test()			nodeset_test_rank()
idset_copy()			nodeset_dup()
build: add libidset.la to libflux_internal
Problem: as parts of flux in src/modules and src/cmd are
converted from nodset to libidset, they require linkage
with libidset.  However, since libidset is dependent
on libutil and parts of libutil are dependent on libidset,
merely adding libidset to LDADD creates a circular dependency.

Add libidset.la to libflux_internal, since the linker
can then resolve dependencies between objects in any
order.
build: add libidset.la to libflux $(test_ldadd)
Problem: libflux tests such as the mrpc unit test
need to be linked against libidset.

Add libidset to libflux $(test_ldadd).
libutil/cronodate: convert from nodeset to idset
Use idset instead of deprecated nodeset.
build: link cronodate test with libidset
Problem: the libutil/cronodate unit test needs to be
linked against libutil and libidset, but libidset needs
libutil/veb, so merely adding libidset to $(test_ldadd)
creates a circular dependency.

Make a special LDADD for this test program that specifies
the minimum set of objects, in the proper order to satisfy
dependencies.
testsuite: fix valgrind errors in cronodate test
Problem: valgrind on the cronodate unit test complains of:

==57352== Conditional jump or move depends on uninitialised value(s)
==57352==    at 0x4F0E3F9: isdst_differ (mktime.c:185)
==57352==    by 0x4F0E3F9: __mktime_internal (mktime.c:501)
==57352==    by 0x4F0E3F9: mktime (mktime.c:591)
==57352==    by 0x10B769: cronodate_next (cronodate.c:470)
==57352==    by 0x10A820: main (cronodate.c:265)

==57145== Conditional jump or move depends on uninitialised value(s)
==57145==    at 0x4F0E3F9: isdst_differ (mktime.c:185)
==57145==    by 0x4F0E3F9: __mktime_internal (mktime.c:501)
==57145==    by 0x4F0E3F9: mktime (mktime.c:591)
==57145==    by 0x10AC75: cronodate_check_next (cronodate.c:61)
==57145==    by 0x10A331: main (cronodate.c:223)

Shut it up by initializing 'struct tm' to all zeroes.

garlick added some commits Dec 8, 2018

cmd/flux-nodeset: drop flux-nodeset command
Problem: flux-nodeset needs to be converted to idset
but has no users.

Drop flux-nodeset command.
libutil/nodeset: drop unused class
Now that libutil/nodeset has been completely replaced
with libidset/idset, drop nodeset.

@garlick garlick force-pushed the garlick:idset_more branch from 5b1fbe0 to 527b951 Dec 12, 2018

@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented Dec 12, 2018

Fixed that typo and squashed.

@chu11

This comment has been minimized.

Copy link
Contributor

chu11 commented Dec 12, 2018

lgtm, I'll hit the button once travis passes. You'll have to rebase this or #1867, depending on what finishes first :-)

@chu11 chu11 merged commit 02fceb5 into flux-framework:master Dec 12, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@garlick garlick deleted the garlick:idset_more branch Dec 12, 2018

@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented Dec 12, 2018

Thanks!

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.