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

Fix --rank issues, add NODESET documentation, and minor cleanup #860

Merged
merged 13 commits into from Oct 20, 2016

Conversation

Projects
None yet
4 participants
@chu11
Copy link
Contributor

chu11 commented Oct 20, 2016

Well, what began as noticing a doc error in the ping manpage lead to a lot more. I should maybe split this up into multiple PRs, but I'll leave it all in as it is generally a large batch of small things. LMK if it'd be better to split up.

  • Per discussion in #854 , support --rank="all" across all tools and add tests appropriately. Nothing too fancy here.
  • Support tests for invalid ranks in tools. Found bugs in flux-ps and flux-ping as a result. I'd appreciate review on the bug fixes commits 31b6c26 and
    1bd8f93, particularly the latter. Perhaps there is something nifty that can be done in the flux api that I don't see at the moment.
  • Added misc tests I thought should be there, invalid ping targets, invalid modules, module info tests.
  • Per discussion in #854, made --rank=NODESET the consistent documentation across all tools and manpages and added NODESET FORMAT section to all appropriate manpages. Would appreciate review on the text to see if I'm missing anything. Commit 0205fb4.

chu11 added some commits Oct 19, 2016

t/t0005-exec.t: Add rank "all" tests
Add new rank="all" tests for flux-exec and flux-ps
Add tests for invalid ranks
Add tests for invalid ranks for flux-ps, flux-ping,
flux-module, and flux-hwloc.  Tests include basic invalid rank
tests and also inputs that include both valid and invalid ranks.

Update prior invalid rank tests to use calculated invalid rank.
src/cmd/flux-ps: Fix count/size logic bug
flux ps could hang on errors because the size variable was
decremented at the same time the count was incremented, leading
to reactor_stop() never getting called.  Remove size decrement
as it is not logically used.
src/cmd/flux-ping.c: Fix corner case in ping output
If a rpc response contains an error and it is the last response
in a set, ping output will not be output as code follows an error
path in the last rpc.  Add ping_data struct to manage history of
ping responses and output ping info appropriately regardless the
order of rpc responses.
Give consistent help with --rank option
Globally in tools and documentation, document that --rank option
takes a NODESET as input.

@garlick garlick added the review label Oct 20, 2016

@coveralls

This comment has been minimized.

Copy link

coveralls commented Oct 20, 2016

Coverage Status

Coverage increased (+0.2%) to 75.354% when pulling 0205fb4 on chu11:fixrankall into e4a3eac on flux-framework:master.

@garlick

This comment has been minimized.

Copy link
Member

garlick commented Oct 20, 2016

I'll have a closer look tomorrow but this looks like really nice work @chu11!

Btw the gcc T_INSTALL travis test failed here:

Found ./t/lua-t0002-rpc.broker.log
2016-10-20T01:39:04.929990Z broker.info[0]: pmi: bootstrap time 0.0s
2016-10-20T01:39:04.937804Z broker.info[0]: wireup: 2/2 (complete) 0.0s
2016-10-20T01:39:04.937824Z broker.info[0]: Run level 1 starting
2016-10-20T01:39:05.444550Z broker.err[0]: rc1: flux-module: flux_open: No such file or directory
2016-10-20T01:39:05.444909Z broker.err[0]: Run level 1 Exited with non-zero status (rc=1) 0.5s
2016-10-20T01:39:05.445074Z broker.info[0]: Run level 3 starting
2016-10-20T01:39:05.445737Z broker.info[0]: shutdown in 0.500s: run level 1 Exited with non-zero status

I'm restarting - looks like possibly flux_open didn't wait long enough for broker socket to appear?

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Oct 20, 2016

Nice work, LGTM as far as that's worth.

My only comment is it might be an interesting exercise to rewrite flux-ping from scratch using the most modern flux APIs, to see how much more it could be simplified (especially flux_rpc_getf might be helpful here). Even if nothing comes of it, you might uncover issues or have comments on the APIs and their documentation as you seem to be good at that kind of thing @chu11!

@garlick

This comment has been minimized.

Copy link
Member

garlick commented Oct 20, 2016

That's a good idea re: reworking ping especially the JSON manipulation.

@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Oct 20, 2016

Seems like a good idea, I need to learn the flux rpc api anyways. We'll add it to the TODO list.

@garlick

This comment has been minimized.

Copy link
Member

garlick commented Oct 20, 2016

Sorry, forgot to get back to this. Looks great - merging!

@garlick garlick merged commit 0eb8cdc into flux-framework:master Oct 20, 2016

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.2%) to 75.354%
Details

@garlick garlick removed the review label Oct 20, 2016

@chu11 chu11 referenced this pull request Oct 20, 2016

Closed

Flux ping manpage fixups? #854

@garlick garlick referenced this pull request Oct 26, 2016

Closed

0.5.0 release notes #879

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.