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

resource-hwloc: put HostName in by_rank directory #892

Merged
merged 2 commits into from Nov 3, 2016

Conversation

Projects
None yet
6 participants
@grondo
Copy link
Contributor

grondo commented Nov 3, 2016

Had this commit sitting on a branch for a while, getting stale. It might be useful so I'm proposing it as a PR for discussion.

Adds a resource.hwloc.by_rank..HostName key containing the hwloc-provided hostname for the current rank.

resource-hwloc: put HostName in by_rank directory
Add a resource.hwloc.by_rank.<id>.HostName key containing the
hwloc-provided hostname for the current rank.

This key will provide an easy way to translate a rank to a
hostname if needed.

@grondo grondo added the review label Nov 3, 2016

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Nov 3, 2016

Current coverage is 72.21% (diff: 80.00%)

Merging #892 into master will increase coverage by <.01%

@@             master       #892   diff @@
==========================================
  Files           157        157          
  Lines         27007      27012     +5   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits          19500      19506     +6   
+ Misses         7507       7506     -1   
  Partials          0          0          
Diff Coverage File Path
•••••••• 80% src/modules/resource-hwloc/resource.c

Powered by Codecov. Last update a06cf00...b096018

@coveralls

This comment has been minimized.

Copy link

coveralls commented Nov 3, 2016

Coverage Status

Coverage increased (+0.03%) to 75.839% when pulling 4a28f45 on grondo:hwloc-byrank-hostname into a06cf00 on flux-framework:master.

@lipari

This comment has been minimized.

Copy link
Contributor

lipari commented Nov 3, 2016

Seems like a fine addition to me!

@garlick

This comment has been minimized.

Copy link
Member

garlick commented Nov 3, 2016

Referencing #866 and #855 where the motivation for this was discussed.

One thought: do we want to support the reverse lookup, hostname to nodeset? I guess that would greatly complicate this simple PR so I dumped a couple thoughts into #893.

This seems good to me, and I did give it a test and it worked.

@garlick

This comment has been minimized.

Copy link
Member

garlick commented Nov 3, 2016

Maybe a little test would be good actually, like

diff --git a/t/t2005-hwloc-basic.t b/t/t2005-hwloc-basic.t
index 15577f6..ee14d59 100755
--- a/t/t2005-hwloc-basic.t
+++ b/t/t2005-hwloc-basic.t
@@ -116,4 +116,10 @@ test_expect_success 'hwloc: reload fails on invalid rank' '
     grep "No route to host" stderr
 '

+test_expect_success 'hwloc: HostName is populated in by_rank' '
+    HW_HOST=$(flux kvs get resource.hwloc.by_rank.0.HostName) &&
+    REAL_HOST=$(hostname) &&
+    test x"$HW_HOST" = x"$REAL_HOST"
+'
+
 test_done
@chu11

This comment has been minimized.

Copy link
Contributor

chu11 commented Nov 3, 2016

Seems like a good idea to me. Minimally, getting hostnames out of the kvs with this is better than the xml parsing hack I originally did in #864.

@grondo

This comment has been minimized.

Copy link
Contributor Author

grondo commented Nov 3, 2016

Maybe a little test would be good actually, like

Great idea, I'll add your enhancement to this PR or you could just push it

test/hwloc: verify by_rank.0.HostName
Add a simple check that resource.hwloc.by_rank.0.HostName
is populated in the KVS and matches the actual hostname.
@garlick

This comment has been minimized.

Copy link
Member

garlick commented Nov 3, 2016

Cool, that worked!

@coveralls

This comment has been minimized.

Copy link

coveralls commented Nov 3, 2016

Coverage Status

Coverage increased (+0.008%) to 75.813% when pulling b096018 on grondo:hwloc-byrank-hostname into a06cf00 on flux-framework:master.

@garlick

This comment has been minimized.

Copy link
Member

garlick commented Nov 3, 2016

Here's where I would push the button, but I should probably recuse myself because I pushed a commit to this PR. @chu11 want to do the honors?

@grondo

This comment has been minimized.

Copy link
Contributor Author

grondo commented Nov 3, 2016

Thanks @garlick! (At home with a sick kid so not getting much work done
today)

On Nov 3, 2016 12:49 PM, "Jim Garlick" notifications@github.com wrote:

Cool, that worked!


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#892 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAtSUtpZk_GtGTx_IFUmKxMfiLRIq4iBks5q6jq6gaJpZM4KoomP
.

@chu11

This comment has been minimized.

Copy link
Contributor

chu11 commented Nov 3, 2016

@garlick sure thing. everything LGTM too.

@chu11 chu11 merged commit e8f8968 into flux-framework:master Nov 3, 2016

4 checks passed

codecov/patch 80.00% of diff hit (target 72.20%)
Details
codecov/project 72.21% (+<.01%) compared to a06cf00
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.008%) to 75.813%
Details

@garlick garlick removed the review label Nov 3, 2016

@grondo grondo referenced this pull request Nov 28, 2016

Closed

Create 0.6.0 release notes #916

@grondo grondo deleted the grondo:hwloc-byrank-hostname branch Apr 5, 2017

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.