broker: add support for PMIx bootstrap #3537
Conversation
There was a problem hiding this comment.
Hi @ggouaillardet - thanks for contributing to Flux and welcome!
A quick bit of early feedback since this seems to be a work in progress -
- would it be possible to localize the PMIx specific changes to
pmiutil.crather than duplicating that code inpmixutil.c? - It seems like some of the code from the the PMIx's
pmi1.c(e.g. noticed artpol comment) is duplicated inpmixutil.c. If we are pulling in code from there, should we just grab the whole thing and retain its copyright+license header? (perhaps in src/common/libpmix as a vendored library)? - Note that when
flux startstarts flux, it provides the PMI environment. So it should not tell the broker to bootstrap with PMIx unless it can provide a PMIx environment. - We will want to be careful about passing PMIX_ environment variables through flux to a job that is launched by flux, so that it doesn't try to bootstrap in flux's job environment (although now that I think of it, we must be handling that now on our Sierra system - perhaps @SteVwonder can provide some feedback on that)
- The autoconf integration probably needs some work, and my first inclination would be to point you to the s3 example right above the m4 that you added to
configure.ac, but it might require a bit more thought. - We'll need to do something in CI. I know some of pmix is packaged in Ubuntu, but I'm not sure how old it is, or whether there is something like a standalone pmix launcher available that we could use to test-launch flux from a sharness test in our
tdirectory.
A more general question is to ask is why can't we use PMIx's pmiv1 library externally? I know we had a number of problems with it on our Sierra systems and were building it on the side for a while. I had naively thought this would be the preferred method to bootstrap flux with PMIx, but reality seems to disagree with me :-) Perhaps we could understand why it's not working for @ggouaillardet in his environment, and @SteVwonder could comment on the current state of affaiirs on Sierra?
I just realized that what you are probably doing there is just trying to get
On ubuntu 20.04 LTS, I ran |
That's a good point. We do strip these variables out at the job shell plugin level when you pass
Bootstrapping Flux on Summit, Lassen, and Sierra works currently as you describe - with an external shim. So this isn't a show stopper for LC users right now. The major downside of the shim is that the semantics of There is also a limitation in the default OpenPMIx datastore that causes a hang when using the PMI1 shim (openpmix/pmi-shim#3). So we have to provide |
FYI the key name hardwired in the shim changed as of f1b4cd5. It's just the rank now.
I'm still slightly confused why the |
|
Thanks for the feedback! I pushed some more commits to address some of the comments
The Open MPIx master does not provide The |
|
Thanks for those changes. We're hitting the following in CI which probably is caused somehow by the cppflags set in configure when pmix is not found. I'll play around with this a bit today. |
|
Yikes! Pressed the close button by accident. Sorry about that! I'll see if I can get the automake stuff straightened out today and post a suggested change for you here, since it will be nice to have CI running as this PR evolves. If it's possible to make changes only to pmiutil.c and not boot_pmi.c, I think that would be preferable to the function pointer redirection to pmixutil.c. |
|
@garlick thanks for the pointer! I just push an other commit to better handle |
|
I think you are on the right track, but @pmix_cppflags@ seems to be defined as just "-I" when pmix is not available. |
|
@garlick indeed, I overlooked the lack of |
|
Good job! Yes - unfortunately the scheduler test failures don't seem to have left behind any clues in the raw log, unless I'm missing something. I suspect it may be only the tests that use There are a couple of things I might propose to change in the autotools support:
|
|
EDIT: I added the path to libpmix.so to my LD_LIBRARY_PATH as noted above and it worked under prrte. It is still failing on Lassen under JSM. I compiled this on our Lassen system, and it just appears to hang. It doesn't hang under PRRTE (on an x86 machine) but it doesn't bootstrap properly either: @ggouaillardet any suggestions on what I might be doing wrong? Side-note: it might also be helpful if we enable the same debug tracing for PMIx that we have for PMI |
Good data point. The code here always calls
I think if we push the pmix support into pmiutil.c then we will get the tracing for free. My idea there was that we would just add a new mode PMI_MODE_PMIX that is conditionally compiled, and probably just have it call If we can do that, then we could change |
|
@SteVwonder quick note: you currently need to set |
|
@SteVwonder second quick note: Here is my output |
Correct - we chose to not do so many years ago because of the difficulty of knowing which envars can be safely propagated. Some environments allow it by configuration - I have no problem doing something similar if it is desirable. Just don't want to do it everywhere as that can get us into trouble. |
ggouaillardet
force-pushed
the
topic/pmix_bootstrap
branch
from
e2f118a
to
2703b6c
Compare
|
@garlick I pushed some more commits to address the latest comments.
@SteVwonder a consequence is the @rhc54 at this stage, I do not think changes in |
Got it. It might be nice to add support for flux in the opposite scenario where However, that's a different subject - if someone can point us to where we might find info on those matters (here or off-list), it would be appreciated. |
|
Nice progress! All the CI tests are passing now. One issue I noticed when trying this out is that If I turn on FLUX_PMI_DEBUG=1, it shows that we are finding a It seems that openpmix installs a Other random stuff:
|
Yeah, we removed that in v4.0 to avoid that very problem. Meantime, you can tell older versions not to build/install that lib by configuring with |
|
Ralph - could you please open a separate flux-core issue for that and maybe
begin by describing the flux use case you have in mind to give us some
context? (Thanks)
…On Thu, Feb 25, 2021, 7:10 AM Ralph Castain ***@***.***> wrote:
at this stage, I do not think changes in prte are necessary.
Got it. It might be nice to add support for flux in the opposite scenario
where prte executes inside of a flux environment. We'd need to know how
to pickup the flux allocation and, if possible, use flux (something like
the equivalent to srun as opposed to ssh) to start the daemons.
However, that's a different subject - if someone can point us to where we
might find info on those matters (here or off-list), it would be
appreciated.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#3537 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AABJPWY6RTEMSTDRDEE6DHLTAZR6TANCNFSM4YB46W6Q>
.
|
Maybe we can just assume that we won't see PMIx's libpmi.so in the wild then. (We can leave things as is for now, and work around later if we need to) The current automake macro doesn't find Oh! I just noticed that pmix-3.2.3 comes with a AC_DEFUN([X_AC_PMIX], [
AC_ARG_ENABLE([pmix-bootstrap],
AS_HELP_STRING([--enable-pmix-bootstrap], [Enable PMIx bootstrap]))
AS_IF([test "x$enable_pmix_bootstrap" = "xyes"], [
PKG_CHECK_MODULES([PMIX], [pmix])
AC_DEFINE([HAVE_LIBPMIX], [1], [Enable PMIx bootstrap])
])
])Then if pmix is installed to a non-standard location, one just sets PKG_CONFIG_PATH to point to the location of |
AFAIK, they all do - it was a downstream packager that contributed it |
|
@garlick what is your The intent was not to use Meanwhile, I will see if I can spot an issue with the current logic. |
|
@garlick there was indeed a bug (the lack of I have no issue using The two relatively minor advantages of
That's by no means a strong opinion, and I will be happy to replace |
|
Sorry @ggouaillardet for the delay responding! I had installed pmix to a prefix of If the pkgconfig files are on your system and @SteVwonder can confirm that this works for us on ours, then I prefer that approach since less M4 in the world makes the world better IMHO :-) Your points about dlopen are good. I'm not sure what is the right approach. I think the main argument for not-dlopen is that it's simpler, and starting simple is good. I did play around with converting it just to see how it would look and the result is on this branch if you would like to have a look and/or try it and see if there are problems. There is also a commit on there to illustrate what I think @SteVwonder needs for our sierra system to avoid the hangs. (Feel free to ignore everything on that branch if it is not right - I was just playing around to understand better) |
ggouaillardet
force-pushed
the
topic/pmix_bootstrap
branch
from
fafaa40
to
71f2a7e
Compare
|
I seem not to be allowed to push to your branch (not sure why - maybe because github considers force pushing to anothers branch to be not nice?) Anyway, my Edit: @grondo pointed out that I might be pushing to the |
ggouaillardet
force-pushed
the
topic/pmix_bootstrap
branch
from
5479410
to
f290d1a
Compare
|
Thanks. Just pushed a couple of fixups (to be squashed before we merge):
Finally, I'm noticing a memory leak when I run the sharness test under valgrind. Not sure if this is an expected leak in libpmix or if we are doing something wrong (any thoughts @rhc54?) |
|
Just glancing at the code, I see where you do the I believe you are using the head of OpenPMIx master branch? If so, we do need to leakcheck it as we have had significant change in recent months - something likely slipped thru the cracks. |
|
Actually, with dstore involved, this must be the v3.x or v4.0 branch - which are you using? |
|
I believe this is the released version packaged on ubuntu 20.04 that is a
prereq of openmpi package, but I will double check when I get back to my
test environment in a couple of hours. Thanks for verifying that our code
is not creating the leak. That was my main concern.
…On Wed, Mar 10, 2021, 8:36 AM Ralph Castain ***@***.***> wrote:
Actually, with dstore involved, this must be the v3.x or v4.0 branch -
which are you using?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#3537 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AABJPWZ3NMF6IEXXOSRNASTTC6GXVANCNFSM4YB46W6Q>
.
|
|
The memory leak was observed with pmix-3.1.5. |
|
Pushed a couple more minor fixups after a final review pass. I think this is ready for testing on the big systems. |
ggouaillardet
force-pushed
the
topic/pmix_bootstrap
branch
from
f22113e
to
0206c04
Compare
|
@garlick: just a heads up that I'm pulling this down to test on Lassen now. Will report back with results. |
|
Looks to have worked on Lassen for both 2 nodes and 8 nodes. Thanks @ggouaillardet and @garlick! 2 nodes: 8 nodes: For future me / @dongahn, when compiling this on Lassen you need a I put that in a file called |
Excellent! Thank you for doing that. Would you mind adding an approving review? Then we can just set merge-when-passing once @ggouaillardet says we're good to go. |
src/cmd/builtin/version.c
Outdated
| @@ -57,6 +62,12 @@ static int cmd_version (optparse_t *p, int ac, char *av[]) | |||
| #endif | |||
| #if HAVE_CALIPER | |||
| printf ("+caliper"); | |||
| #endif | |||
| #if HAVE_LIBPMIX | |||
| printf ("+pmix==%ld.%ld.%ld", | |||
There was a problem hiding this comment.
Do we want to be explicit that this is the version of the client-side that we are using? I'm afraid users might think this is the version of pmix that Flux provides as a server.
Maybe +pmix -> +pmix-bootstrap?
There was a problem hiding this comment.
That works for me if it helps you!
Problem: on some platforms PMIx is the preferred mechanism to use for bootstrapping Flux. Prepare to add PMIx support to the Flux broker by adding an "opt in" configure option: --enable-pmix-bootstrap. If specified, pkg-config is used to locate a suitable pmix package. Configure fails if pmix is requested but not found. Add PMIX_LIBS and PMIX_CFLAGS to the broker Makefile.am. Co-authored-by: Jim Garlick <[email protected]>
Problem: on some platforms PMIx is the preferred mechanism to use for bootstrapping Flux. Add support to the broker's pmiutil.c to use PMIx if the PMIx server environment variables are set. For now, keep the PMIx integration as simple as possible, and use the PMIx_*() functions directly. We can consider other options such as indirection through dlopen() later, if we run into problems. This implementation was guided by the PMI-1 compatibility code here: https://github.com/openpmix/pmi-shim Since Flux does not require all of PMI-1, our code is much simpler. In addition, some PMIx differences from PMI-1 with respect to key scope could be dealt with directly, compared to the shim: - add a 'from_rank' to broker_pmi_kvs_get() so that PMIx_Get() can set proc.rank to this instead of PMIX_RANK_UNDEF. This avoids a hang with the dstore gds component, as described in openpmix/pmi-shim#3 - if 'from_rank' is set to -1, then set proc.rank to PMIX_RANK_UNDEF, and set the PMIX_OPTIONAL attribute to 1 so PMIx_Get() fails immediately if the key is not set. This is used when the broker tries to fetch the 'flux.instance-level' key, which the flux shell places in the KVS, and is not expected to exist when Flux is launched by a foreign resource manager. Note to future implementor of flux shell PMIx plugin (flux-framework#3536): this assumes that 'flux.instance-level' would be set using PMIx_server_register_nspace() or equivalent, which would push the key to the client at initialization. Add some PMIX well known environment variables to the blocklist in runat.c, so they do not propagate to the initial program when Flux is launched by a PMIx process manager. Co-authored-by: Jim Garlick <[email protected]>
Problem: tests have no way to determine whether or not Flux was built with --enable-pmix-bootstrap. Add the pmix version to the output of 'flux version'.
|
I'll go ahead and force push a copy of this branch that has been rebased on current master and has a few minor fixups including the one requested by @SteVwonder shortly. |
garlick
force-pushed
the
topic/pmix_bootstrap
branch
from
0206c04
to
d5368bc
Compare
FWIW, an alternative to using an adhoc |
I had assumed the package check would fail without |
|
@ggouaillardet - I think this can be merged if you can confirm it works for you. |
Problem: need to test --enable-pmix-bootstrap in CI and obtain code coverage report for related code. Add --enable-pmix-bootstrap to bionic/coverage in the CI build matrix generator. Add openpmix and prrte to bionic docker image: - prrte: export a recent commit from the prrte git repo, since the most recent release (1.0.0) doesn't include prterun, which is needed by our pmix sharness test. - openpmix: export a recent git commit from the openpmix git repo, needed to compile prrte above as it won't work with openpmix-3.2.3. - add build requirements of flex and libevent-dev - add prrte runtime requirement of ssh
garlick
force-pushed
the
topic/pmix_bootstrap
branch
from
d5368bc
to
723ad13
Compare
|
I force pushed one more time to correct a problem in the build matrix that was preventing bionic results from being posted. |
Codecov Report
@@ Coverage Diff @@
## master #3537 +/- ##
==========================================
- Coverage 82.55% 82.40% -0.15%
==========================================
Files 323 322 -1
Lines 48981 48829 -152
==========================================
- Hits 40434 40239 -195
- Misses 8547 8590 +43
|
|
@garlick I am happy to confirm this PR works for me! |
|
Excellent! Thanks! |
PMIx support for bootstrapping flux is added by running
configure --with-pmix[=PATH]
When specified, PATH must contain the pmix.h header file.
PMIx is used at bootstrap by running
flux start --bootstrap=pmix
Limitations: