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

better user interface for launching flux under flux/slurm #658

Merged
merged 9 commits into from May 2, 2016

Conversation

Projects
None yet
3 participants
@garlick
Copy link
Member

garlick commented May 1, 2016

As discussed in #643, the current usage for starting Flux under slurm is a bit awkward:

srun -N8 flux broker initial-program

If flux-start simply execs the broker whenever size=1 (the default size), and the broker attempts to bootstrap first with PMI, then with "local", we should be able to support

srun -N8 flux start initial-program

without needing to probe PMI from flux-start.

This PR takes that approach. Apparently something in it has made the sharness runlevel tests sad as they seem to hang now, but I wanted to post this to get a little feedback as to whether this seems like the right approach.

@garlick garlick added the review label May 1, 2016

@coveralls

This comment has been minimized.

Copy link

coveralls commented May 2, 2016

Coverage Status

Coverage increased (+0.02%) to 74.42% when pulling 4c00bd4 on garlick:start_exec into dd91879 on flux-framework:master.

@garlick garlick force-pushed the garlick:start_exec branch from 4c00bd4 to ad306b7 May 2, 2016

@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented May 2, 2016

This may be ready for review?

test/popen2: drop open/write/close test
This test writes a short string on a pipe to /bin/true.
Since /bin/true exits quickly, the write intermittently fails
with EPIPE.  Drop the test, which was a bit silly to begin with.

@garlick garlick referenced this pull request May 2, 2016

Merged

flux-cron: cron-like service #659

9 of 9 tasks complete
@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented May 2, 2016

This seems like a great approach to me. I'll poke at it just a bit but I don't have any objections.

I didn't think about it in detail yet, but if flux start is used instead of flux broker in all cases can we remove the path to the broker from general.exec_path?

@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented May 2, 2016

I was thinking the same thing about removing flux-broker from public view, but removing it is a bit of a nuisance, i.e. if not general.exec_path, I guess we would need a special config for the broker location? And the flux-broker(1) man page contains important docs, yet it would seem sort of misplaced if flux-broker isn't a "command"...

All things considered, I thought perhaps it was best left where it is, though I could be talked out of that.

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented May 2, 2016

Ah, no, like I said I hadn't thought about it and you have enough good points above that I think it is actually a good idea to keep flux broker command.

@coveralls

This comment has been minimized.

Copy link

coveralls commented May 2, 2016

Coverage Status

Coverage decreased (-0.03%) to 74.371% when pulling ad306b7 on garlick:start_exec into dd91879 on flux-framework:master.

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented May 2, 2016

I'm inclined to merge this. We should then be sure to update any other docs that call out use of flux broker (though I guess at this point it doesn't hurt). Particularly I'm thinking of our Quickstart on flux-framework.github.io

@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented May 2, 2016

Merge away! Wheee!

@grondo grondo merged commit 39f2ccf into flux-framework:master May 2, 2016

1 of 2 checks passed

coverage/coveralls Coverage pending from Coveralls.io
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@grondo grondo removed the review label May 2, 2016

@coveralls

This comment has been minimized.

Copy link

coveralls commented May 2, 2016

Coverage Status

Coverage increased (+0.06%) to 74.461% when pulling 9da305e on garlick:start_exec into dd91879 on flux-framework:master.

@lipari lipari referenced this pull request May 23, 2016

Closed

Update Quickstart #675

@garlick garlick deleted the garlick:start_exec branch Jun 6, 2016

@garlick garlick referenced this pull request Aug 2, 2016

Closed

0.4.0 release notes #739

@garlick garlick referenced this pull request Aug 11, 2016

Closed

quickstart needs update pass #9

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.