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

Change flux start to only exec broker if --size is not specified #951

Merged
merged 4 commits into from Jan 16, 2017

Conversation

Projects
None yet
4 participants
@chu11
Copy link
Contributor

chu11 commented Jan 13, 2017

Per issue #834

chu11 added some commits Jan 13, 2017

src/broker/broker.c: Change usage of --size
Instead of executing a single broker directly if --size=1 or if the
--size option is not specified, only execute the broker if --size
is not specified.

If --size is specified, regardless of the size count, start an
instance of that size locally.

Fixes #834
doc/flux-start(1): Update --size usage
Update documentation for changes with how --size is used.

@garlick garlick added the review label Jan 13, 2017

@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Jan 13, 2017

For discussion: Since --size only launches a local instance, should the option name be changed to make that more clear? Looking at flux-start's history, the --size option was brought along from a long time ago when slurm stuff was originally inside flux-start.

This would be a pretty sizable change, probably effecting massive amount of documentation and scripts.

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Jan 13, 2017

Current coverage is 76.04% (diff: 80.00%)

Merging #951 into master will decrease coverage by 0.01%

@@             master       #951   diff @@
==========================================
  Files           149        149          
  Lines         25962      25962          
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
- Hits          19748      19744     -4   
- Misses         6214       6218     +4   
  Partials          0          0          
Diff Coverage File Path
•••••••• 80% src/cmd/flux-start.c

Powered by Codecov. Last update c55822f...6ef1c5b

@coveralls

This comment has been minimized.

Copy link

coveralls commented Jan 13, 2017

Coverage Status

Coverage decreased (-0.02%) to 76.327% when pulling 70bca3d on chu11:issue834 into c55822f on flux-framework:master.

@garlick

This comment has been minimized.

Copy link
Member

garlick commented Jan 13, 2017

Looks good.

One idea: maybe these tests should be run in PMI mode too?

test_expect_success 'flux-start passes through errors from command' "
	test_must_fail flux start /bin/false
"
test_expect_success 'flux-start passes exit code due to signal' "
	test_expect_code 130 flux start 'kill -INT \$\$'
"

This would be a pretty sizable change, probably effecting massive amount of documentation and scripts.

Yeah, I'm thinking probably this PR should close #834 and we should open another issue to cover your and @morrone's usability observations with flux-start.

@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Jan 13, 2017

I wanted to limit "permutation" of exec vs pmi tests as we could probably do it everywhere, but I think some additional tests where you indicated are warranted. I'll update and re-push. I'll open a new ticket on the --size option too.

test/basic: Update for flux start changes
Adjust tests appropriately for change to --size argument in
flux start.  Add additional tests to test specifically for
flux start --size=1 vs flux start

@chu11 chu11 force-pushed the chu11:issue834 branch from 70bca3d to 6ef1c5b Jan 13, 2017

@coveralls

This comment has been minimized.

Copy link

coveralls commented Jan 13, 2017

Coverage Status

Coverage decreased (-0.02%) to 76.33% when pulling 6ef1c5b on chu11:issue834 into c55822f on flux-framework:master.

@garlick

This comment has been minimized.

Copy link
Member

garlick commented Jan 16, 2017

Thanks! Merging.

@garlick garlick merged commit 15709a9 into flux-framework:master Jan 16, 2017

4 checks passed

codecov/patch 80.00% of diff hit (target 76.06%)
Details
codecov/project Absolute coverage decreased by -0.01% but relative coverage increased by +3.93% compared to c55822f
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.02%) to 76.33%
Details

@grondo grondo referenced this pull request Mar 28, 2017

Closed

0.7.0 Release Notes #1019

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.