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

Extend the PMIx_Tool support #122

Merged
merged 1 commit into from
Aug 11, 2016
Merged

Extend the PMIx_Tool support #122

merged 1 commit into from
Aug 11, 2016

Conversation

rhc54
Copy link
Contributor

@rhc54 rhc54 commented Aug 3, 2016

Initial work on the corresponding buffer ops

@rhc54 rhc54 added this to the v2.0 milestone Aug 3, 2016
@rhc54 rhc54 self-assigned this Aug 3, 2016
@rhc54
Copy link
Contributor Author

rhc54 commented Aug 5, 2016

@dsolt @artpol84 This has the required changes to support the tool extensions. It includes:

Replace enum definitions with #define to allow 3rd party extensions

Per telecon, redefine the rank as uint32 - give it a dedicated typedef in case there are any future changes.

Add new pmix_query_t data type and modify PMIx_Query API to accept it.

Add pack/unpack support for persistence, scope, range, command, info directives, data array, and query data types.

All compiles - pack/unpack support for data array is not complete, but header files appear complete

Please provide any comments/suggestions re the pmix_common.h header changes.

@rhc54 rhc54 changed the title Update pmix_common.h to support the tool extensions. Extend the PMIx_Tool support Aug 6, 2016
@rhc54
Copy link
Contributor Author

rhc54 commented Aug 7, 2016

Refs pmix/RFCs#10

@rhc54
Copy link
Contributor Author

rhc54 commented Aug 7, 2016

@artpol84 Looks like there is some cross-build contamination going on:

07:05:06 aclocal: error: config/autogen_found_items.m4:40: file 'src/mca/pdl/configure.m4' does not exist

This file is from another PR and has nothing to do with this one.

@rhc54 rhc54 force-pushed the topic/toolext branch 3 times, most recently from 49fa3a2 to 23ee156 Compare August 8, 2016 01:29
@artpol84
Copy link
Contributor

artpol84 commented Aug 8, 2016

bot:retest

@rhc54 rhc54 force-pushed the topic/toolext branch 3 times, most recently from 300b6c0 to f1d6808 Compare August 9, 2016 19:05
@rhc54
Copy link
Contributor Author

rhc54 commented Aug 9, 2016

@artpol84 @karasevb I had to update the dstor interfaces to use the new pmix_rank_t data type - please check to ensure it doesn't cause a problem.

@dsolt
Copy link
Contributor

dsolt commented Aug 10, 2016

I notice that when calling PMIx_Query_info and I want to query PMIX_QUERY_NAMESPACE, I am really querying a key, but I pass in an info because that is what PMIx_Query_Info takes. However, I do not bother to set up the value portion of this since I expect the value to get filled in by the call, however this leads to pack failures because PMIx_Query_Info tries to send my info with uninitialized value portion. Any thoughts on this? Am I just being lazy and I should initialize the value field's type at least or should pack not complain if the type is garbage? I don't think PMIx_Query_info can fill in the value for me because maybe there are some types of queries (someday) where the PMIx_Info is both an IN and and OUT parameter (mmmm... hungry for IN-n-OUT burger now)

@rhc54
Copy link
Contributor Author

rhc54 commented Aug 10, 2016

Oh, that is cruel - we don’t have an IN-n-OUT here, and I’ve sorely missed them. Positive thing is that they opened one about a 3hr drive away, and my wife an I are planning a trip just for lunch!

That was one motivation for dumping the blocking version of this call. I'd suggest transitioning to the non-blocking version to eliminate this problem.

Please review/approve this RFC so we can do so! 😄

@dsolt
Copy link
Contributor

dsolt commented Aug 10, 2016

I am using the non-blocking version of the call right now, but either way one will have this problem of packing these PMIx_Info's. Doing some more testing today and then will approve. We got in-n-out in TX a few years ago... its is awesome.

@rhc54
Copy link
Contributor Author

rhc54 commented Aug 10, 2016

The type of the returned info is set by the library - you only pass in directives and keys stipulating what should be found.

@dsolt
Copy link
Contributor

dsolt commented Aug 10, 2016

Ok, well then I think it is a problem as is because it tries to pack the Info to send up to the server and fails if the type is garbage. I don't know if we have to fix this now though.

@rhc54
Copy link
Contributor Author

rhc54 commented Aug 10, 2016

I really don't understand - are you talking about directives that modify the queries? If so, then yes, of course you have to provide the type. But if you are talking about the keys - there is no "type", it's just a char**.

@dsolt
Copy link
Contributor

dsolt commented Aug 11, 2016

Ah, nevermind. I was looking at my copy which still had the old API for query. The new Query API uses pmix_query_t's instead of PMIx_Info's. Ok, also had a chance to review this code more carefully and it looks good to me.

@dsolt
Copy link
Contributor

dsolt commented Aug 11, 2016

Oh I forgot to ask, what is the reason for going from enums to pound defines on many things? By the way, I skimmed the bufferops code pretty lightly. That buffer packing/unpacking code is pretty dull, but didn't notice anything that looked wrong.

@rhc54
Copy link
Contributor Author

rhc54 commented Aug 11, 2016

If we use enums, then 3rd party adopters cannot extend the definitions. So if someone wanted, for example, to support a scope that wasn't in our definitions, they couldn't do it without modifying the library.

Using pound defines allows someone to pass any value - the library isn't looking at it anyway. We just pass it thru. So if the RM can understand it, then it's okay. On another system, the user would just get a "not supported" error, but that's also okay.

@rhc54 rhc54 force-pushed the topic/toolext branch 2 times, most recently from 43dd6ee to 90fc19e Compare August 11, 2016 18:50
…the corresponding buffer ops

Per telecon, redefine the rank as uint32 - give it a dedicated typedef in case there are any future changes.

Add pack/unpack support for persistence, scope, range, command, info directives, data array, and query data types.

All compiles - pack/unpack support for data array is not complete, but header files appear complete

Complete basic operations - passing simple tests

Debug it all - passes tests!

Add the PMIx_Log function for logging data

Implement support for system-level tool connections, and for user-specified selection of system vs allocation level tool connections

Add PMIx_Log attributes

Add PMIx_Log keys. Correct type of some rank-related values to pmix_rank_t

Update dstor interfaces to use pmix_rank_t

Silence Coverity warnings

Remove build product

Remove build product

Fix double-free of event base

Fix duplicate event base free in client

Cleanup name conflicts with OMPI and uninit vars

Rename symbol
@rhc54 rhc54 merged commit 0dbb438 into openpmix:master Aug 11, 2016
@rhc54 rhc54 deleted the topic/toolext branch August 11, 2016 21:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants