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

Add flux-hostlist command and initialize resource.hosts in rc1 #1499

Merged
merged 7 commits into from
May 3, 2018

Conversation

grondo
Copy link
Contributor

@grondo grondo commented May 2, 2018

This is somewhat experimental, but I'm posting for feedback on the general appraoch. This PR adds a simple flux hostlist command that prints the list of hostnames, one per line in rank order, for the instance.

If an optional jobid is provided, it will print the list of hostnames for that job (again one per node, not per task).

The script will look for resource.hosts and, if set, will assume this is the list of hostnames for the instance (one per rank). resource.hosts is kept in "hostlist" format.

If resource.hosts is not set, flux hostlist will fall back to flux exec hostname

For jobs, flux hostlist will attempt to read the hostnames directly out of R_lite if the node field exists, otherwise it will just lookup the hostname from resource.hosts by rank.

A new rc1 script is added to set the initial resource.hosts via FLUX_URI=$(parent_uri) flux hostlist ${FLUX_JOB_ID}. In order for this to be possible, the unsetenv ("FLUX_JOB_ID") done in the broker had to be moved until after runlevel initialization was complete. (@garlick, need your ok on that one)

Fixes #1489

@coveralls
Copy link

coveralls commented May 2, 2018

Coverage Status

Coverage decreased (-0.1%) to 78.965% when pulling e09e775 on grondo:flux-hostlist into 333c283 on flux-framework:master.

@grondo grondo force-pushed the flux-hostlist branch 2 times, most recently from 6c55f7e to 24448f1 Compare May 2, 2018 17:39
@codecov-io
Copy link

codecov-io commented May 2, 2018

Codecov Report

Merging #1499 into master will decrease coverage by 0.09%.
The diff coverage is 100%.

@@            Coverage Diff            @@
##           master    #1499     +/-   ##
=========================================
- Coverage   78.76%   78.67%   -0.1%     
=========================================
  Files         164      164             
  Lines       30673    30673             
=========================================
- Hits        24160    24131     -29     
- Misses       6513     6542     +29
Impacted Files Coverage Δ
src/broker/broker.c 76.97% <100%> (ø) ⬆️
src/bindings/lua/lua-hostlist/hostlist.c 59.89% <0%> (-3.07%) ⬇️
src/common/libflux/mrpc.c 85.49% <0%> (-1.18%) ⬇️
src/common/libkvs/kvs_watch.c 89.69% <0%> (-0.43%) ⬇️
src/broker/module.c 83.79% <0%> (-0.28%) ⬇️
src/common/libflux/message.c 81.25% <0%> (-0.12%) ⬇️
src/common/libflux/handle.c 83.91% <0%> (+0.24%) ⬆️
src/cmd/flux-module.c 85.36% <0%> (+0.3%) ⬆️
src/broker/overlay.c 74.44% <0%> (+0.31%) ⬆️
src/broker/modservice.c 80.58% <0%> (+0.97%) ⬆️
... and 1 more

@garlick
Copy link
Member

garlick commented May 2, 2018

Here are references for openpmi and mpich/hydra host file formats:

https://www.open-mpi.org/faq/?category=running#mpirun-hostfile

https://wiki.mpich.org/mpich/index.php/Using_the_Hydra_Process_Manager

would it make sense to try to emit in one of these formats, selectable by option?

@grondo
Copy link
Contributor Author

grondo commented May 2, 2018

Yeah, that's a question for @trws (though I will note that the default flux hostlist output does work for both those formats, doesn't it?). As I understood the requirement (as detailed in #1489), we just needed a way to generate the "hostfile" for an instance, 1 host per rank (not sure how we'd determine "slots" or anything like that for a instance hostlist).

The flux hostlist JOBID support was needed to push the hostlist for child instance from the parent to the child's resource.hosts. I don't think it is useful for mpirun because you can't wreckrun or submit a single script to run on a multi-task instance, so we can't support mpirun on Flux jobs right now unless in the initial program of a child instance.

@grondo
Copy link
Contributor Author

grondo commented May 2, 2018

Sorry, I don't think my comment above is clear. The open question in my mind is whether it is useful/necessary to add an option to generate hostfiles that contain extra information supported by the hostfile formats @garlick mentioned above (e.g. "slots" "max-slots" for OpenMPI).

(not sure how we'd determine "slots" or anything like that for a instance hostlist).

It occurred to me that this information could be provided from R_lite information in the job, and stashed with the resource.hosts kvs entry (or another kvs key). However, flux launched under flux start -s N or other resource managers would require different/extra steps to initialize the slot information.

@garlick
Copy link
Member

garlick commented May 2, 2018

Sorry for the delay responding - if this works for @trws I'm certainly good! Just throwing out ideas.

@trws
Copy link
Member

trws commented May 3, 2018

It might be handy to emit things with slots or replicate a hostname once per slot under some circumstances, but even just having a way to get one hostname per line is enough to get the job done in the majority of cases.

@grondo
Copy link
Contributor Author

grondo commented May 3, 2018

It might be handy to emit things with slots or replicate a hostname once per slot under some circumstances,

This could be done now for sub-instances by propagating the job R_lite to a well known key in the child kvs. The slots could be assumed to be one per core of the R_lite core list for each rank.

Eventually of course R would certainly be available in an instances...

@garlick also had the idea to emit rank list instead of host list, which could then be passed to flux-exec, e.g. flux exec -r $(flux hostlist --ranks 123) COMMAND

@trws
Copy link
Member

trws commented May 3, 2018 via email

@grondo
Copy link
Contributor Author

grondo commented May 3, 2018

Ok, the --ranks option would be trivial to add. I kind of feel like a flux hostlist command may be the wrong abstraction though. Really this is starting to morph into some kind of tool for querying information about the assigned resources to jobs (with the instance itself being the degenerate "job") -- lists of assigned hosts, ranks, slots, etc. Is this command the wrong approach? (The initial proposal was just to "get a list of hosts for an instance" i.e. wrap flux exec hostname)

grondo added 7 commits May 3, 2018 14:10
Move unsetenv() of FLUX_JOB_ID, FLUX_JOB_SIZE, and FLUX_JOB_NNODES
until after runlevel initialization so that these variables are
available to rc1 scripts.
Problem: wreckrun generates fake 'R_lite' for testing purposes when
run in an instance with no sched module loaded, however because of
Lua array indexing, rank 0 is always placed in the last position of
the 'R_lite' array.

This doesn't cause a problem, but may cause confusion for tools or
testing workloads examining 'R_lite'. Fix by indexing the assigned
resource array in wreckrun from 1 origin, and calculate rank as
index - 1.
Add new flux-hostlist command to print list of hosts for the current
instance or optionally a set of wreck job(s).
Add a new rc1 script to populate resource.hosts with a list of
hosts (in hostlist format for now), 1 per rank, initialized either
via the job entry of an enclosing instance if FLUX_JOB_ID is set,
or via flux-hostlist fallback to `flux exec hostname` when not
running in a sub-instance.
Add a small suite of tests to check flux-hostlist and correspoding
rc1 script functionality.
Add hostlist HOSTLIST and hostnames to valid dictionary words for the
spellcheck tests.
@grondo
Copy link
Contributor Author

grondo commented May 3, 2018

FWIW, rebased and added support for -r, --ranks

@garlick
Copy link
Member

garlick commented May 3, 2018

Shall we go ahead and merge this so we can move on, and save any future enhancements for another time?

@grondo
Copy link
Contributor Author

grondo commented May 3, 2018

I'm not sure what enhancements, if any, are possible or required so we can close #1489. I'm willing to close and rework this PR if the wrong approach was taken.

@grondo grondo requested a review from trws May 3, 2018 17:05
@grondo
Copy link
Contributor Author

grondo commented May 3, 2018

Added explicit request for @trws to ACK this PR as satisfying the requirement in #1489. If so then we can merge this.

@grondo
Copy link
Contributor Author

grondo commented May 3, 2018

@trws implicitly approved this as "fine for now" so I say we merge and move on if that is ok @garlick

@garlick
Copy link
Member

garlick commented May 3, 2018

Sounds good!

@garlick garlick merged commit b148196 into flux-framework:master May 3, 2018
@grondo
Copy link
Contributor Author

grondo commented May 3, 2018

Thanks!

@grondo grondo deleted the flux-hostlist branch May 3, 2018 19:55
@trws
Copy link
Member

trws commented May 4, 2018

Works for me, sorry for the long reply latency and thanks for getting this together!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants