Move ompi main to prrte d90a453d - #192
Open
hppritcha wants to merge 110 commits into
Open
Conversation
PMIX_PERSISTENCE was recorded on the data object at publish and then never consulted again. Nothing removed published data on a lifetime boundary, so PMIX_PERSIST_PROC and PMIX_PERSIST_APP both behaved as PMIX_PERSIST_INDEF: on a persistent DVM the store only ever grew, and a key published by a job that ended an hour ago still held that name against every job that came after it. The state machine already had a function for telling the data server that a namespace was done with, and it already routed correctly for the built-in store. All three of its callers were gated on an external data server being configured, so the built-in one - the usual case - was never told a job had ended. Ungate them, and give the message a PMIX_PERSISTENCE directive naming the lifetime that ended, so the server can tell a job termination from an explicit PMIx_Unpublish(NULL, ...), which takes everything the caller published however long it asked for it to be kept. That distinction also decides whether the caller's parked lookups go: cancelling the lookups a live process is waiting on is no part of taking its published data back. The persistence values are not a numeric ladder - PMIX_PERSIST_INDEF is 0 and outlives all of them - so the ordering is spelled out in prte_data_server_expires_by() rather than derived from a comparison. PMIX_PERSIST_PROC is reclaimed at job granularity rather than when its individual publisher exits, which is later than the Standard asks; a message to the store for every terminating process is not a cost the termination path can carry at scale. While here, correct the object's default persistence to PMIX_PERSIST_APP, which is the Standard's default and, since PMIx adds none of its own before handing a publish to the host, the one that governs. The open-coded copy of the notification in state_dvm.c becomes a call to the shared function. Signed-off-by: Ralph Castain <rhc@pmix.org>
The data server accepted a publish of a key it was already holding, stored it behind the existing copy, and returned PMIX_SUCCESS. prte_ds_lookup() answers a key from the first match it finds, so one of the two values was unreachable for the life of the DVM - a write that reported success and did nothing. Nor was the loser reliably the newcomer, which is what made it silent in both directions. The store is a pmix_pointer_array_t and pmix_pointer_array_add() fills the lowest free slot, so a duplicate landing in a slot that some earlier unpublish freed sits ahead of the original and displaces it instead. Which value a lookup resolved to was a function of unrelated publish and unpublish history, and neither publisher was told anything. The Standard is explicit about what should happen: duplicate keys are permitted on different ranges, and a duplicate on the same data range shall return PMIX_ERR_DUPLICATE_KEY. Do that, and store nothing when refusing. A range is a set of processes, though, not merely the pmix_data_range_t word: PMIX_RANGE_NAMESPACE used by two processes of different namespaces names two disjoint sets, and refusing the second of those would refuse a publish the Standard permits. So the collision test is the range word matching and the stored item being one this publisher could itself have looked up - which for NAMESPACE, LOCAL and PROC_LOCAL is exactly set equality, is trivially true for the ranges that do collide, and keeps two users' identically-keyed items apart because neither can see the other's. Refusing duplicates creates the need for a way to update a value you published yourself, which would otherwise require an intervening PMIx_Unpublish. The new "prte.pub.replace" directive does both in one step. It is owner-scoped on purpose: a collision with another process's key is refused whether or not it was given, so it is a republish and never a way to take a live name away from somebody else. The Standard defines no attribute for this, so it is PRRTE's own, and PMIx_Query_info now reports it among those supported for PMIx_Publish. The scan runs to completion before anything is removed, so a publish that ends up refused leaves the store exactly as it found it. Signed-off-by: Ralph Castain <rhc@pmix.org>
Every daemon runs prte_data_server_init(), not just the master: pmix_server_start() calls it unconditionally and is reached from ess/hnp and ess/base/ess_base_std_prted alike. Which store a request lands in is decided by its range, in execute() - PMIX_RANGE_LOCAL goes to PRTE_PROC_MY_NAME and everything else to the global server or the HNP. There is no local-first search and no fallback, so a prted's store holds exactly the local-range items its own local procs published. The termination purge went only to the global store. That reclaimed everything except local-range data, which sat in the publishing daemon's own store with nothing to take it - so having just taught PMIX_PERSISTENCE to mean something, it meant something everywhere but there. Send the purge to our own store as well, when that is somebody other than the global target. Not when an external data server is configured: there a daemon's prte_data_server() relays what it receives instead of serving it, so a request addressed to ourselves would never reach our store. Local-range publish is broken in that configuration for the same reason, which is a separate defect and not one to paper over from here. The master cannot show any of this, because there "my store" and "the global store" are one object - so the covering test is a multi-node one, asserting that a PERSIST_APP key published LOCAL on node2 is gone from node2's store once its job ends while a PERSIST_SESSION key beside it is not. Signed-off-by: Ralph Castain <rhc@pmix.org>
Whenever an external data server was configured, every nspace registration published that job's registration info into the store - on the stated grounds that "any subsequent connect has to be able to retrieve it". Nothing ever retrieved it. Every PMIx_Lookup caller in PRRTE, PMIx and Open MPI takes a key its caller supplied - plookup, MPI_Lookup_name, the vprotocol eventlog - and no reader keyed by a namespace appears anywhere in the history. It could not have served that purpose in any case. The key was prte_process_info.myproc.nspace: the DAEMON job's namespace, one constant string for the life of the DVM. The value was the registration info for whichever job was being registered, built per daemon from that daemon's own local view. So every job in the DVM, from every daemon holding its procs, wrote a different payload under the one key, and since a lookup answers from the first match it finds, a reader would have got an arbitrary daemon's copy of an arbitrary job. It is an ORTE-era carry-over, from when cross-mpirun accept/connect really did fetch the remote job's info by name. Under PMIx that travels through PMIx_Connect and the server's own machinery, and what crosses a DVM boundary through the data server is what an application publishes, and only that. The write also cannot survive duplicate keys being refused: the second daemon to register, and every job after the first, collide on that one key. Rather than make an unread write idempotent, remove it - along with the publish completion chain that existed to carry it. Signed-off-by: Ralph Castain <rhc@pmix.org>
The attach happened in one place: execute(), in pmix_server_pub.c, on behalf of a LOCAL client of this daemon that was publishing or looking something up. But relaying is the MASTER's job. An external data server is not addressable over the RML, only the master holds the PMIx tool connection to it, and every other daemon therefore sends its request to the master to be reissued. A master with no publishing client of its own never attached, so every relayed request failed PMIX_ERR_UNREACH - which is to say the feature worked only when the master happened to host a process that used it. Nothing had noticed because each job's nspace registration also published, which went through execute() and attached as a side effect. Removing that unread publication is what exposed this. Give the attach a name of its own, prte_pmix_server_init_pubsub(), make it idempotent, and call it from prte_ds_relay() as well as from execute(). A daemon that needs the connection now opens it, rather than depending on somebody else having wanted it first. Signed-off-by: Ralph Castain <rhc@pmix.org>
prte_data_server() dispatched a request to prte_ds_relay() and, when that failed, logged the error and returned - sending nothing. The daemon that asked stayed parked on its room number waiting for a reply that was never coming, and the process behind it hung for good. An unreachable data server is something a caller can be told about; a hang is not, and it is much harder to diagnose from the outside than the status would have been. Move the relay dispatch below the point where the answer buffer exists so a failure falls into the same error reply every other failure uses. The handler contract is unchanged: prte_ds_relay() returning PMIX_SUCCESS means it owns the request and will answer it, including when it has already sent a failure, so on that path the buffer we prepared is simply released. Also format the status this reports with PMIx_Error_string(). It is a pmix_status_t, and PRTE_ERROR_NAME knows only PRRTE's codes, so every error the data server reported came out as "Unknown error" - which is what the relay failure above looked like while it was being tracked down. Signed-off-by: Ralph Castain <rhc@pmix.org>
A request arriving on PRTE_RML_TAG_DATA_SERVER was handed to prte_ds_relay() whenever prte_data_server_uri was set - all of them, unconditionally. But a PMIX_RANGE_LOCAL item belongs to the store of the daemon that relayed it, and never leaves this DVM: execute() routes it to PRTE_PROC_MY_NAME precisely so that it does not. Forwarding it to an external server stored a publish where its own publisher could never look it up, and answered a local-range lookup out of a store that cannot hold local-range data at all. Local-range publish/lookup was therefore broken outright in any DVM pointed at an external data server. The range decides this, but by the time the dispatch runs it is buried in a payload whose shape depends on the command, and digging it back out would mean three command-specific unpackings of a buffer the handler still needs. The sender knows it already, so let the sender say which store it means: requests about our own go out on PRTE_RML_TAG_DATA_SERVER_LOCAL, the receive is registered for both tags, and the relay branch takes only the other one. Nothing about the message format changes; the tag parameter the receive already had was simply unused. That also lets the lifecycle purge of our own store stop being conditional. It was gated on there being no external data server, for exactly this reason - addressed to ourselves, it would have been forwarded to a DVM that does not hold the data - and on the local tag it now reaches the store it names in every configuration. Signed-off-by: Ralph Castain <rhc@pmix.org>
The round-4 pass over src/grpcomm read all five of its .c files at their current addresses and refreshed the directory's guide, so the entry that still described it as reviewed in another life - "the subsystem was taken out of MCA and rebuilt at a new path ... re-review this one first" - is no longer what the tree looks like. Move it into the list of subsystems whose AGENTS.md still matches their code. The one thing that pass turned up and could not fix, a fence contribution outliving the release that ended its round, was already recorded in the outstanding work above; this is only the status table catching up. Signed-off-by: Ralph Castain <rhc@pmix.org>
Almost every asynchronous operation in the PMIx server host module is
tracked by a prte_pmix_server_req_t, and three loops decide which request
an arriving answer belongs to by matching on its "tproc" - the proc whose
data was asked for. The constructor never set that field. PMIX_NEW
mallocs and does not zero, so a request that names no target proc at all -
monitor, publish/lookup, spawn, tool connection, all of which live in the
same two arrays as the ones that do - carried whatever the previous
occupant of that block had left there, which for a recycled request is a
real proc identity. Matching was therefore not merely unguarded, it was
nondeterministic: a modex reply could complete and release an unrelated
in-flight request, whose own client then waited for the life of the DVM.
Set the {"", PMIX_RANK_INVALID} sentinel in the constructor, and screen for
it in the two loops that did not. The sentinel alone is not enough: an
empty nspace is PMIx's wildcard, so an untargeted request still pairs with
a job-level fetch (rank=WILDCARD) - which is exactly the failure the fence
de-duplication loop was fixed for once already, when it was comparing the
wrong field. prte_pmix_server_clear() has the guard; the fence loop and
the "anyone else waiting for this target" sweep in pmix_server_dmdx_resp()
now have it too.
The direct-modex service has four ownership faults in the same area. The
info array unpacked off the wire was never freed - not on the three
requests that carry it (the destructor frees only what "copy" claims), not
on the paths that refuse a request outright, and not when the unpack
itself fails - so every keyed remote get leaked its qualifiers. A request
the local PMIx server refused was leaked whole. A modex payload was freed
only on the path that transmits it, and the destructor did not know about
the field at all. And a request whose job ends while somebody else holds
it kept an index into a slot that has since been handed to another
request, so its eventual reply cleared that one's slot instead.
A timed-out request was left armed and in the array. It has been answered
and the peer is free to reuse the index, so the retry cycle it leaves
running retries a dead request until the DVM ends - and a retry that
finally succeeds sends a second reply under an index that by then belongs
to some other request of theirs. Retire it instead, and drop the late
payload if the answer arrives after we gave up; that is what the
"timed_out" flag was for, which until now nothing read.
Two failures reached the wrong audience. A timeout was reported to the
peer as PMIX_ERR_TIMEOUT handed to a function that converts PRRTE codes
into PMIx ones, which does not recognize a PMIx status and answers
PMIX_ERROR - so a get that timed out was reported as a generic failure;
the same is true of a dmodex the local server refused. A relayed
scheduler command we could not parse was dropped with a message on our own
stdout, leaving the daemon that relayed it holding a request for a blocked
client; every path there has the requestor's index, so answer with it.
Two more, in the same file. The response handler unpacked a modex payload
into a NULL pointer if the allocation for it failed, and dropped the
request in silence if the payload would not unpack - both leave the client
that asked blocked in PMIx_Get. And pmix_server_finalize() released
requests without deleting the timers armed inside them, leaving libevent
holding a pointer into freed memory for prte_event_base_close() to walk.
Finally, the attribute table denied honoring PMIX_TIMEOUT on a PMIx_Get.
PMIx arms no timer on a host request, deliberately, but it does hand the
caller's directives up - and the daemon servicing the request arms the
timer itself. A tool asking what this runtime supports was told the
opposite of the truth.
Signed-off-by: Ralph Castain <rhc@pmix.org>
Blocks in this file are indented at three, five and six spaces in a dozen places, and two of them - the lost-connection handler and the request cleanup in finalize - run that way for their whole body. Nothing here changes what the code does; it is the leftovers of the review that just went through it. The modex response caddy also declared its payload size as int32_t while both the wire and the callback that consumes it use size_t. Nothing has ever sent a two-gigabyte modex blob, so this is a tidy-up rather than a fix, but a field that has to be widened later is one that was written down wrong now. Signed-off-by: Ralph Castain <rhc@pmix.org>
Every failure the scheduler relay reports to a client passed through the wrong converter. Three unpack statuses in pmix_server_alloc_request_resp() - already PMIx values - were run through prte_pmix_convert_rc(), the PRRTE-to-PMIx direction, which recognizes no PMIx status and answers with its default, so a malformed response reached the requesting client as a bare PMIX_ERROR rather than saying what failed. The reverse mistake sits in prte_server_send_request(): it is declared to answer in PMIx statuses and does so everywhere except the send at the end, which answers in PRRTE codes - and all three of its callers hand what it returns straight to a client's completion callback, so a DVM the master could not be reached from reported some unrelated PMIx status to the application. While there: a response whose info array fails to unpack zeroed the count but kept the array, and the destructor frees the count it is given, so the whole partially-unpacked result leaked. Signed-off-by: Ralph Castain <rhc@pmix.org>
Two allocations in the connected-assemblage registry were used without being checked. The union of memberships a terminating proc must notify is grown with realloc() assigned through the same pointer, so an exhausted heap both loses the block already gathered and leaves the loop below indexing NULL; and the membership arriving on a daemon's report is sized by a count that came off the wire, so the array it asks for is not certainly obtainable. Neither is likely, and neither should take the DVM master down when it happens. Also record, in the directory guide and in docs/todo.rst, the cost this registry carries: the PMIx definition owes each member an event per departing proc, and PRRTE delivers each of those as a DVM-wide broadcast - so a spawned job of N ranks, which is connected to its parent by default and does not disconnect by running to completion, ends by issuing N of them. Nothing pays it without an assemblage, and the cheaper shape is a send to the daemons holding the members rather than a batched event, which would be a PMIx interface change. Signed-off-by: Ralph Castain <rhc@pmix.org>
PMIX_INFO_LOAD with a NULL value is ordinary, legal PMIx: the value load zeroes the union, so the directive arrives as a PMIX_STRING whose string is NULL, and PMIx's own PMIX_INFO_TRUE handles that case on purpose. The spawn-directive chain has two kinds of branch, and only one of them was ready for it. A branch that hands the pointer on to prte_set_attribute or to a mapping-policy setter is safe, because every one of those tests for NULL. A branch that reads the value itself is not, and four did: PMIX_STDIN_TGT ran strcmp on it, PMIX_PARENT_ID transferred a proc ID through a NULL pointer, PMIX_WDIR handed it to pmix_path_is_absolute, which dereferences its argument as its first act, and both timeout keys handed it to PMIX_CONVERT_TIME, which indexes the split it makes without checking that there was anything to split and so faults inside PMIx. Each of those let any client segfault the daemon it was attached to with a single PMIx_Spawn call, so refuse the directive instead. The numeric directives had a quieter version of the same problem: they read a fixed member of the value union rather than asking what the caller actually stored, so a count sent as a uint16 was read as a uint32 and only the truncation back down rescued it - on a little-endian machine. PMIX_VALUE_GET_NUMBER converts from whichever integer type is there and reports a non-numeric value as an error, which is what the session-ID branch beside them already used. Two status codes were also crossing between the PRRTE and PMIx spaces unconverted. pmix_getcwd answers in PMIx statuses despite its PRRTE shape, and prte_pmix_xfer_app returned one to a caller that reads PRRTE codes and then converts in the PRRTE-to-PMIx direction, so a failure to resolve a relative working directory reached the client as a bare PMIX_ERROR. In spawn() a single variable carried a PMIx pack status on one path and PRRTE codes on the others while one exit converted them all alike; the two spaces now live in separate variables. Alongside those: connect_release never destructed the buffer PMIx_Data_embed copies the endpoint payload into, leaking it on every connect that carries one; prte_pmix_xfer_app overwrote an app->cwd that pmix_app_t.cwd had already supplied without freeing it; the launch response handed pmix_server_notify_spawn an uninitialized nspace on the path it deliberately falls through to reach the room number; and interim, alone among the shifted handlers here, was missing its PMIX_ACQUIRE_OBJECT. Signed-off-by: Ralph Castain <rhc@pmix.org>
Trailing blanks on two lines of the per-app PPR branch, "OIUTPUT" for "OUTPUT" in a section marker, and "npace" for "nspace" in a comment about where the namespace sits in a job-info array. Kept separate from the fixes that precede it so neither obscures the other. Signed-off-by: Ralph Castain <rhc@pmix.org>
Nothing in pmix_server_fence.c needed fixing, but two things in it read as bugs on every pass and are not, and the reasons live in PMIx rather than here - so they cost a re-derivation each time somebody looks. dmodex_req() calls the blocking PMIx_Get from the PRRTE progress thread. That is only safe because PMIx's get_data() refuses outright for a peer that is a server and not a tool, rather than parking the request pending a commit, and because a daemon's own peer stays server-only even on the master, where attaching to a scheduler builds a separate peer and leaves ours alone. The remaining work is a local datastore lookup on the other thread. Note what that rests on: the call is guarded by !refresh_cache because a refresh runs ahead of the server check inside PMIx_Get, so the guard is load bearing and not an optimization. The strdup of PMIX_REQUIRED_KEY has no NULL check because PMIx loads that key only when the key is non-NULL, and the second producer forwards the array the first one built. A NULL key arrives as an absent attribute, which is the case prte_pmix_server_derivable_key() already answers for. The one real change is a pair of pack checks that compared a PMIx status against PRTE_SUCCESS. Both spaces call success zero so the tests worked, but the three checks beside them say PMIX_SUCCESS and these two should too. Signed-off-by: Ralph Castain <rhc@pmix.org>
The info array on a tool connection is the connecting process's own: the PTL unpacks whatever it sent and hands it to the tool_connected upcall with only the uid, gid and version appended. So _toolconn() parses untrusted input, and two of its branches strdup'ed a string without checking it. A PMIX_STRING carrying no string survives the wire as a NULL - the packer writes a zero length and the unpacker hands back NULL - so a tool that connected with PMIX_HOSTNAME or PMIX_CMD_LINE loaded from a NULL took down the daemon it was attaching to. Both are now checked, and a tool that sends the same key twice keeps the first value rather than stranding it. The same loop read PMIX_RANK straight off a fixed member of the value union while the uid, gid and pid beside it went through PMIx_Value_get_number. It now does too, so a rank sent as any other integer width is converted rather than misread, and a non-numeric one is refused instead of becoming a rank. Below that, the relay to the master logged every pack failure and packed on regardless, sending a buffer the master cannot read. With no index unpacked from it there is nobody to answer, so the tool waited for the life of the DVM. A pack failure now releases the buffer, returns the array slot, and answers the tool. _client_abort() read an uninitialized status on one path: a non-NULL procs array with a count of zero skips the loop that assigns it, and the release tail hands whatever was on the stack to the caller's completion. Neither producer of that up-call can currently deliver that shape - the wire path allocates the array only when the count is positive - but the function reads the variable on a path that never writes it, so it now starts at PMIX_SUCCESS. The rest is consistency with the file's own conventions: three upcalls declared pmix_status_t answered with PRTE_SUCCESS, three callback pointers invoked without the NULL guard their siblings use, and three shifted handlers missing the PMIX_ACQUIRE_OBJECT that pairs with the POST_OBJECT on the other side of the hand-off. Signed-off-by: Ralph Castain <rhc@pmix.org>
Every other function in src/prted/pmix that PMIx calls captures its arguments and posts an event to prte_event_base. pmix_server_group_fn() does not: it checks the group id and calls prte_grpcomm_group() straight through. That reads as a violation of the directory's central rule on every pass, and it is not one - prte_grpcomm_group() builds the caddy and posts the event itself, so shifting here would shift twice. What makes the pass-through legal is narrow and worth writing down, because it is what a later edit would quietly break: nothing above the hand-off reads PRRTE state at all. The only globals in reach are the server verbosity handle, an int written once during init, and the name printer, whose scratch buffers are thread-specific storage. While in the file, drop the include block it inherited by copy-paste. Fourteen of the twenty headers were unused, one of them named twice, and in a file whose whole body is twelve lines they were four times the code. Signed-off-by: Ralph Castain <rhc@pmix.org>
pmix_server_dmdx_resp() finishes by sweeping local_reqs for anyone else
waiting on the proc whose data just arrived, matching on tproc. That
array does not hold only modex requests, and the guard it carried - skip
the {"", PMIX_RANK_INVALID} sentinel - screens only the operations that
name no target at all. The three scheduler relays name a real one: an
allocate, a session control, or a group context id records the process
that asked for it in tproc, at both ends of the relay.
A job-level fetch names nspace/WILDCARD, and PMIX_CHECK_PROCID covers
every rank of a namespace, so the sweep matched any relayed request from
any process of the job being fetched. It cleared the slot and released
the request without answering it, leaving the daemon that relayed it -
and the client behind that - waiting for the life of the DVM. For the
allocate and session-control cases that release was also a third one on
an object deliberately built to take two, so the RAS completing later
released freed memory.
mdxcbfunc is the honest discriminator: PRTE_DMX_REQ is the sole builder
of a direct-modex request and always sets one, nothing else sets it, and
the sweep already had to test it before it could do anything useful.
The indexed lookup above the sweep gets the same test, because the index
comes off the wire and names a slot that is reused the instant its
previous occupant retires.
That test is only sound while a recycled request cannot carry the last
occupant's callback, and PMIX_NEW does not zero - so the field's NULLing
in rqcon() is now load bearing in a way it was not when the value was
merely compared: the sweep calls it. test_request_tracker pins that,
recycling a released request rather than inspecting a fresh one.
Two defects in the job-control path proper come with it. The directives
arrive exactly as the client sent them, PMIx having forwarded the array
without inspecting any entry, so PMIX_JOB_CTRL_DEFINE_PSET reading the
pset name out of value.data.string meant handing the packer whatever
eight bytes the caller put there - and the packer calls strlen on it.
Any process attached to a daemon could fault it with one job-control
request carrying that key with a numeric value. The same directive with
no members was merely useless rather than fatal, but was still broadcast
to every daemon, refused by each of them, and reported to the requestor
as success. Both are refused up front now.
Four returns carried a PRRTE code out of a function whose type is
pmix_status_t and whose result reaches a client's completion callback -
one from terminate_procs and three from xcast. The two spaces now live
in separate variables so the compiler shows which is which.
Signed-off-by: Ralph Castain <rhc@pmix.org>
Eight of the twenty headers this file listed were unused, and one of them - plm_private.h - was named twice. What is left is what the code actually reaches for: the PMIx shim, verbose output, grpcomm's xcast, the error logger, the daemon command flag, the PLM module, the RML tag, the globals, and the name printer. Signed-off-by: Ralph Castain <rhc@pmix.org>
A monitor request fans out to every daemon, and the daemon serving one tracks it in remote_reqs at the index that array handed it - which the tracker records in local_index. remote_index is a different thing: the room number the *requesting* daemon keeps in its own local_reqs, carried along only so the reply can find its way home. mycbfn() cleared the slot named by remote_index and then released the request, so the request's real slot was left pointing at freed memory - which prte_pmix_server_clear walks on every job cleanup - while whatever unrelated peer request held the slot it did name was silently unlinked. The two indices agree while a single request is in flight, which is why this survived. A DVM of one never completed a monitor request at all. ndaemons is num_daemons - 1 because the requesting daemon ignores its own copy of the broadcast, so with one daemon the count is zero, nobody answers, and the only place the completion test runs is the response handler. PMIx has already gathered this node's contribution before up-calling - it asks the host only about participation it judges remote - so an empty success is the whole answer, and mfn() gives it before building a message. Three more, all of the same family as the first. A response's index is bounds-checked and NULL-checked but was not asked what kind of request it found, so one that crossed with a retirement could count a report against an unrelated request and merge monitor results into an info array that request never allocated; req->monitor answers the question. mycbfn() abandoned a reply it could not pack, leaving the requestor counting forever, and now sends the bare refusal instead. And it decided whether to pack results by reading status - a PRRTE code mycb never sets, so permanently PMIX_SUCCESS - while packing and reading pstatus, so the two ends of the message disagreed about whether results were there. The allocations on the receiving path are checked now as well, and the directive array is freed with the count it was created with rather than the count that was unpacked into it. Signed-off-by: Ralph Castain <rhc@pmix.org>
Twelve of the twenty headers this file listed were unused, and one of them - plm_private.h - was named twice. What remains is what the code reaches for: the PMIx shim, verbose output, grpcomm's xcast, the error logger, the RML tag, the globals, and the name printer. Signed-off-by: Ralph Castain <rhc@pmix.org>
A monitor request fans out with an xcast and then counts direct replies. Nothing in it is keyed on the routing tree, so nothing repaired it when the tree changed: a daemon that died mid-collective simply never answered, and the requesting daemon went on counting a DVM that no longer existed while its client stayed blocked in PMIx_Process_monitor_nb for the life of the session. Every other DVM-wide collective survives this, through the fault handler routed_radix.c calls on every daemon whenever the tree changes. This one was not wired into it - nothing under src/prted/pmix was - and that omission is the whole defect. What it does there is not what fence and group do. Their recovery is a restart, which works because a fence contribution is idempotent and can be re-derived from the repaired tree. A monitor reply is a sample of live state on a node that has just ceased to exist; there is nothing to re-offer. So recovery here is to stop waiting for the dead daemon and tell the caller its sample is short. It needs no epoch and has none. That means the request has to know which daemons it is waiting on rather than how many. A bare count cannot answer the one question a death poses - had that daemon already reported? - and both readings of it are wrong: decrement when it had, and nreported sails past the target so the request can never complete; decrement when it had not, and you are right only by accident. So the tracker now carries two bitmaps, the set it expects and the set it has accounted for, snapped in mfn() from the same predicate the xcast routes by. Recording identities is also what makes the accounting idempotent, which it must be, because the tree reports every death twice - once at LOCAL scope and again at GLOBAL - and a rank can be named again later by an adoption notice to a new parent. The expected set is the other half: a death this request never waited on must not count, or it completes early, before the daemons it is waiting on have answered. The same two sets make a duplicate response harmless, which the bare count did not - counting one daemon twice completed the request before the daemon whose slot it took was heard from. The status now says how much of the DVM answered. Everyone reported success, PMIX_SUCCESS; some answered and some could not, PMIX_ERR_PARTIAL_SUCCESS; nobody answered, the reason rather than a claim of partiality. Two things had to change for that to be sayable. pstatus carried the caller's own monitor code, which mfn() now clears once it is on the wire, and it kept the status of whichever daemon answered last - so one daemon's failure was erased by the next daemon's success. It keeps the first non-success now. test_monitor_accounting covers the parts that are invisible in a single-shot test: that the second notice for one death changes nothing, that an unrelated daemon's death is not counted, and that losing every daemon is not reported as partial success. Signed-off-by: Ralph Castain <rhc@pmix.org>
PMIX_GROUP_LEFT arrives at pmix_server_notify() as an ordinary event notification, so its info array is whatever the generating process passed to PMIx_Notify_event: PMIx forwards the array to the host without inspecting what any entry holds, and the host's answer is to xcast it to every daemon in the DVM. The group-departure handler read the PMIX_GROUP_ID off value.data.string and the departing proc off value.data.proc without first asking what type the value carried, so a key sent with, say, a PMIX_SIZE value handed strcmp() or PMIX_CHECK_PROCID whatever eight bytes the caller chose. Any process attached to any daemon could send that, and every daemon in the DVM faulted on it at once. The registry it walks is non-empty on every daemon from the first successful PMIx_Group_construct onwards, because grpcomm_group.c appends the group everywhere, so the window is the ordinary life of any job that uses groups rather than a corner of one. Check the type of both values, and then insist that the departure name a concrete identity: PMIX_CHECK_PROCID counts an empty nspace and a wildcard rank as matching anything, so a departure naming neither removed whichever member happened to sit first in the membership array. A proc leaves a group one at a time. test_group_left pins all of it without needing a DVM, which is why the handler is exported rather than static; run against the unguarded code it segfaults on the first case. Signed-off-by: Ralph Castain <rhc@pmix.org>
_notify_event() is the tail of the notify_event upcall, and the status it produces goes to cd->cbfunc, which is PMIx's completion callback for the client that generated the event. A failed prte_grpcomm_xcast() answers in PRRTE codes, and the handler discarded that code and reported a bare PMIX_ERROR - so a client whose event could not be broadcast because the DVM was coming apart, or because a peer was unreachable, learned only that something had gone wrong. The two spaces overlap everywhere but zero, which is why this has to be a conversion rather than a cast: PRRTE's code for the failure is some other real PMIx status, and passing it through unconverted would be a worse answer than the generic one. prte_pmix_convert_rc() is the PRRTE-to-PMIx direction, which is the one wanted here. Signed-off-by: Ralph Castain <rhc@pmix.org>
PMIx offers the host a pair of hooks through which it reports the status codes its local clients have registered for, so that a host able to use them can stop distributing codes nobody wants. PRRTE has always taken both, thread-shifted them, and answered success without recording anything, over a comment saying only "need to implement this". That comment reads as an oversight and is not one. Every notification a daemon originates is xcast to the whole DVM and handed to each daemon's own PMIx server, which filters it against its clients' registrations there - so no event is lost and none reaches a process that did not ask, and the host has nothing it needs to keep. What the registrations would buy is the broadcast itself, in the case where no process in the DVM wants the code; what they would cost is a replicated set of codes on every daemon that has to stay right across grow, shrink and daemon loss. That trade has not been judged worth making, and docs/todo.rst is where this project records such a decision - its marker table exists precisely so that an arm left deliberately empty is checkable against an entry in prose. While here, spell the success these two hand back to PMIx PMIX_SUCCESS. The value is the same, but a function reachable from an upcall answers in PMIx statuses, and the two spaces agree on nothing else. Signed-off-by: Ralph Castain <rhc@pmix.org>
pmix_server_notify.c carried the include list its neighbours were given years ago rather than the one it uses: hwloc, iof, plm, rmaps, schizo, show_help, argv and unistd are none of them referenced, and plm_private.h appeared twice. It needs string.h, which it was getting by luck through one of them. Also spell the notification receive's first unpack status in the same variable as the four below it - it was the only one held in a plain int called rc, which in this directory is the name a PRRTE code goes by - and match the file's own idiom for testing a key. Signed-off-by: Ralph Castain <rhc@pmix.org>
prte_relm_start_msg() is documented in relm.h to answer in PRRTE codes, and its early returns do - PRTE_ERR_BAD_PARAM, PRTE_ERR_NODE_DOWN. The arm that gives up because the RELM framing could not be packed returned the packer's PMIx status instead, and the success path answered PMIX_SUCCESS. The two spaces agree on zero, so the success case never showed it. The failure case did the damage twice over: every caller reaches this through PRTE_RML_RELIABLE_SEND and runs what it gets through prte_pmix_convert_rc(), the PRRTE-to-PMIx direction - so a PMIx status arriving here unconverted was converted a second time, and whatever client was behind the send was told about an error it had nothing to do with. Signed-off-by: Ralph Castain <rhc@pmix.org>
pmix_server_keyval_client() unloads the reply's byte object into a local pmix_data_buffer_t, which owns the payload from that moment - the code NULLs bo.bytes right there to say so, and every error arm below destructs the buffer before it leaves. The path that worked did not. So a successful PMIx_Lookup leaked the whole of what it returned, on every daemon that relayed one, for the life of the DVM. The pmix_info_t the loop unpacks each item into is the same shape of mistake one level down: it is destructed at the bottom of the iteration and on none of the three arms that leave early, so a reply that went bad half way through took a key and a value with it. Signed-off-by: Ralph Castain <rhc@pmix.org>
The room number is the first thing out of the reply, and the handler already knows what to do when that fails: it cannot answer anybody, so it falls to the release label and leaves. The command byte immediately below it is a different situation - the room number is in hand, which means a request tracker and a client behind it are addressable - and the handler returned instead. Nothing else retires that tracker. prte_pmix_server_clear() sweeps remote_reqs only, and the local ones belong to callbacks that are meant to still fire, so the request stayed in the array and the client stayed in PMIx_Publish or PMIx_Lookup for the life of the DVM. Answering with an unpack failure is the honest reply and is what the rest of the handler does with every later error. Signed-off-by: Ralph Castain <rhc@pmix.org>
The RML takes ownership of a buffer only by succeeding: on an error return it is still the caller's, emptied of its payload or not, and that contract is written down in src/rml/relm/AGENTS.md. execute() dropped straight into its callback label without releasing the buffer it had just built, so a publish or lookup aimed at a peer the RML would not send to - a daemon it believes down, an invalid rank - leaked its whole request. Separating the two status spaces into two variables is what showed this up. The failure reached the client's completion callback as whatever was in rc, which the pack failures above put a PMIx status into and the init_server arm put a PRRTE code into. A function whose result goes to a PMIx callback should hold that result in a pmix_status_t and convert the PRRTE codes into it as they arise, which is now what it does. Signed-off-by: Ralph Castain <rhc@pmix.org>
The rankfile branch creates the job map and dereferences it on the very next line, so the one PMIX_NEW here is worth checking. Three comments spell "hostfiles" as "hosthosts". Signed-off-by: Ralph Castain <rhc@pmix.org>
A DVM-startup failure that reported through show_help exited silently. `prte --hostfile /nonexistent` gave exit 66 and zero bytes on both streams; the same message appears under prterun, and appears in the tool when a running DVM refuses an add-hostfile, so neither the text nor the topic lookup was at fault. The path is longer than it looks. deliver_locally() does not deliver locally: it calls pmix_show_help_norender(), and PMIx routes a SERVER peer's log through IOF, where IOF honors PMIX_IOF_LOCAL_OUTPUT. We set that false for a persistent DVM and for every prted, so that a job's stdout never lands on a daemon's own terminal -- right for application output, and wrong for a daemon's own diagnostic, which is not application output and has nowhere else to go when no tool is subscribed. The routing above it was already correct and is unchanged; what was missing is that both of its callers had concluded the message was theirs to show, and then handed it to something that would not show it. So deliver_locally() takes an explicit "emit_directly", and the callers say when they know nobody else will. A prted that has no HNP to relay to, and a relay that failed to send, say yes unconditionally -- that fallback has never emitted anything. The HNP says yes only while the DVM has not finished starting: after that a tool holds the connection and IOF puts the message where the user actually is, and a running persistent DVM must not narrate on a terminal nobody is watching. That distinction needs a latch, which prte_dvm_ready is not -- it is cleared and re-set on every grow, session instantiate and teardown, so it says "is a size change in flight", not "have we started", and a grow is exactly when these messages fire. prte_dvm_started is set once at the first VM_READY and never cleared. The same latch fixes a smaller case of the same thing: VM_READY is re-entered at the end of every grow, so a persistent DVM printed "DVM ready" again each time a node was added. It is a startup announcement; it is now made once. Signed-off-by: Ralph Castain <rhc@pmix.org>
PMIx changed that macro to take the object it initializes, because an empty list is one whose sentinel points at itself and there is no other way to spell that address at compile time. Left as it was, the sentinel carried NULL next and prev -- which is not an empty list, it is an unwalkable one, so any list touched before its PMIX_CONSTRUCT took the process down instead of finding nothing. Nineteen sites here, each now naming its own object. The change is mechanical, but a wrong name would still compile while pointing a list at another list's sentinel, so every argument was checked to be exactly the object it initializes. PRRTE master requires PMIx 7.0.0 or later, so there is no version window in which both spellings have to work: this must land with the PMIx change, not before it. Built and run against a PMIx carrying it -- prterun with a four-rank PMIx job through fence, get and finalize, and a prte/prun DVM, which is the path that reaches the server-side setup calls after PMIx_tool_init alone. Signed-off-by: Ralph Castain <rhc@pmix.org>
An empty namespace is PMIx's wildcard: PMIx_Check_nspace() answers true
if either side is invalid. A prte_job_t that fails before
prte_plm_base_create_jobid() has named it therefore matches the daemon
namespace, and both of job_errors()' arms then do the wrong thing with
it.
The daemon arm is the dangerous one. NEVER_LAUNCHED is in its list, so
it disables routing and activates PRTE_JOB_STATE_DAEMONS_TERMINATED --
which state_dvm registers as prte_quit. One application job failing
before it was named takes the entire DVM down, and its requester is never
told anything.
Testing the namespace properly is not the fix on its own, because it only
moves the damage to the other arm: that one hands _terminate_job() the
same empty namespace, and the command it xcasts is
{nspace, RANK_WILDCARD}. prte_odls_base_default_kill_local_procs()
*skips* its namespace filter when the nspace is invalid, so rather than
terminating nothing it terminates every application process on every node
in the DVM.
So job_errors() screens for an invalid namespace before either arm. Such
a job has never launched: nothing is running under it and nothing
downstream can address it, so the whole of what is owed is an answer to
whoever asked, and prte_plm_base_spawn_response() delivers that by the
job's originator and room number rather than by its name.
prte_plm_base_spawn_alloc_failed() already reaches for it directly for
exactly this reason.
_terminate_job() gets the guard as well. It is the primitive that would
do the damage, its argument comes from a caller that may not have checked,
and what it builds goes to every daemon in the DVM.
prte_plm_base_setup_job() is where such a job arises: it activates
NEVER_LAUNCHED on two paths that run before the job is named. Neither is
easy to provoke -- one wants the jobid space exhausted -- but they are
live code, and the ras add-hosts unwind reached the same state through a
job that had not yet been through prte_plm.spawn(). Driving that path
through the state machine now answers the tool and leaves the DVM
running; before this it hung the tool.
The same shape survives in state_dvm.c's check_complete(), which nothing
reaches with a nameless job today. Why it is left alone, and what would
have to be decided first, is recorded in the component's AGENTS.md.
Signed-off-by: Ralph Castain <rhc@pmix.org>
PMIX_LIST_STATIC_INIT takes the list it initializes, because an empty list is one whose sentinel points at itself. Handing it a different list of the same type compiles perfectly and produces a list whose sentinel belongs to somebody else: appends land on the wrong list, walks find the wrong items, and destructing either one leaves the other pointing into freed memory. Nothing downstream catches that, so the nineteen uses this tree acquired when it started passing the object are worth a test -- a wrong name is not a build error, it is a silent aliasing of two objects. test/unit/check_static_init.pl requires every use to name the object it initializes: a member as ".field = NAME(obj.field)", a whole object as "var = NAME(var)". There is no list of macros to maintain. The set is derived from the tree: by definition where the macro is declared here, and otherwise by the shape of its argument -- which is the half that matters in PRRTE, where PMIX_LIST_STATIC_INIT comes from an installed PMIx header and nothing local declares it. This is a copy of the script in PMIx, deliberately identical so that a fix to one can be copied across rather than diverging. Verified by breaking each shape in turn and watching make check go red: one member pointed at its neighbour, and a standalone list naming another variable. Signed-off-by: Ralph Castain <rhc@pmix.org>
PMIX_ALLOC_SEQUENCE was added in openpmix cf95d9c70. Stamp each session with an acquisition number in prte_set_session_object - the one function every session is registered through - and serve it from the PMIX_QUERY_ALLOC_PROPERTIES arm. That arm now gates each property on the PMIX_ALLOC_PROPERTY qualifier, so naming one property returns that one rather than all of them. A qualifier naming a property PRRTE does not serve leaves the list empty and answers PMIX_ERR_NOT_FOUND, as before. Credits: AccelCom @ Barcelona Supercomputing Center Signed-off-by: Petter Sandås <petter.sandas@bsc.es>
Issue openpmix#2733 reports that an unread PMIX_PERSIST_FIRST_READ item is deleted when its publisher's job ends, which is a four-line fix. The discussion it opened is not: once you step away from PROC and APP, retention in a persistent DVM is unbounded. Nothing ever reclaims a SESSION or INDEF item - the only purge anyone sends carries a PMIX_PERSIST_APP horizon - and only the publisher, by then gone, is permitted to remove one. A job can publish as many INDEF keys as it likes and exit, and the next jobs in that DVM inherit a store they cannot clean and, for any name that was taken, cannot reuse. These documents settle what the runtime should do about that before any code moves. The specification states the retention criterion for each persistence, an idle timeout for the two that otherwise have none, a per-uid storage cap whose eviction never crosses a uid boundary, and a removal rule keyed on the publishing user rather than the publishing process - which is what lets a later job of the same user take back a name whose publisher died before it could unpublish. It also corrects a misreading this work uncovered: PMIX_PERSIST_APP means the application, not the job, so an MPMD job's apps do not share a horizon. Nothing in the existing set then says "until my job is over", so a new PMIX_PERSIST_NSPACE is proposed and becomes PRRTE's default, which is the lifetime unmarked publishes have actually been getting. The design records why this costs less than it looks: each store already knows what it must act on. A daemon reaps its own children and is already told when a namespace ends; the master tracks every process state and holds the job objects. No horizon needs a message that does not already exist, save one - the master does not tell the daemons when an application has terminated - and that one is deferred deliberately. Signed-off-by: Ralph Castain <rhc@pmix.org>
PMIX_PERSIST_FIRST_READ says "retain until the first access, then delete" - the criterion is the read, and nothing else. expires_by() grouped it with PMIX_PERSIST_PROC as one of the two shortest lifetimes, so every purge took it: the moment the publishing job ended, an item nobody had read yet was gone. That silently broke the one conforming way to hand a named value from one generation of a job to the next. The publisher exits before the successor starts, which is the whole shape of a checkpoint/restart handover, and every other persistence fails it for a different reason: PERSIST_APP goes with the predecessor's job, while SESSION and INDEF survive but leave the name wedged, since the successor is a new namespace and may neither publish over it nor remove it. FIRST_READ is what the constant is for - the successor reads, the read deletes, the successor publishes its own - and the purge was all that stood in the way. The publisher's departure is not the criterion the publisher asked for. PROC and APP are how a publisher says "delete this when I, or my application, go away"; FIRST_READ says something else, and now behaves that way. The cost is an unread item that no lifetime reclaims, which is exactly what INDEF already carries; a retention timeout for both is specified in docs/plans/datastore/. The unit test asserted the old behavior and now asserts the new, across every horizon. The swarm case is the issue's own reproducer: a publisher on one node exits, a later job on another reads what it left, a second lookup finds nothing, and the successor publishes its own value under the freed name. Fixes openpmix#2733 Signed-off-by: Ralph Castain <rhc@pmix.org>
A prted reaches job_teardown() when the procs of a job that IT hosted have terminated - the gate is num_terminated == num_local_procs, which says nothing about the rest of the job. From there it sent the DVM-wide purge, which goes to the master's store as well as its own. So on a job spanning nodes, the first node to finish its share took the entire namespace's PERSIST_APP and PERSIST_PROC data out of the master's store while the job was still running elsewhere. A process that had published with the default persistence could lose its data before its own application, or even its own process, had ended - and the process that caused it had nothing to do with the publisher beyond sharing a namespace. Nothing reported anything; the publisher simply stopped being findable. A daemon's share of a job finishing is not a lifetime ending, and the one store it may act on at that moment is its own, which holds only the local-range items its own clients published - every one of which has now gone. It therefore calls the new notify_local_data_server(), and the namespace horizon stays with the master, which reaches it when the job is over everywhere. That exposed why the arrangement had worked at all. The master's own send was skipped whenever prte_pmix_server_globals.server.nspace was unset - "nobody local to us has used the data server" - which is a true statement about our own store and a false one about the global store, whose publishers are anywhere in the DVM. On the master it is satisfied exactly when the publishing processes ran on other nodes, so the master reclaimed nothing in the usual arrangement and the daemons' DVM-wide purges were doing the work. The global send is now unconditional, and the local one keeps the guard, where it is right. The same reasoning fixes a second gap: a master pointed at an external data server relays the global purge away and used to purge its own local-range store not at all. The swarm case is one MPMD job across two nodes, where the node that exits at once must not take the data of the node still running. Signed-off-by: Ralph Castain <rhc@pmix.org>
Removal was keyed on the publishing PROCESS - namespace and rank both. That sounds like the strict reading of "data posted by this process", and it is, in the way that makes it useless: a process takes no data with it when it exits, so an item published by a job that has ended is removable by nobody at all. Its own user's next job can read it, cannot publish over it (that is a duplicate) and cannot remove it, so the name is wedged for the life of the DVM. A predecessor that died before it could unpublish is exactly the case a checkpoint/restart handover hits, and it is the case with no way out. An item is therefore owned by the USER that published it. prte_data_server_owns() compares the requestor's effective uid against the one recorded at publish, and the gid where both are known - either side reading UINT32_MAX degrades the test to the uid alone, the same degradation the read rule already makes. A uid we do not have matches nobody: two unknowns comparing equal would let anyone remove anything. The same predicate scopes PRTE_PUBLISH_REPLACE, and has to. Once a same-uid process may unpublish a key and then publish its own, the two-step is available anyway; refusing the one-step form would only make the same outcome take two calls. Nothing about access enters into it, in either direction. An accessor list widens who may READ an item and confers no removal, and a publisher whose own list excludes it may still remove what it cannot read, because it owns it. The predicate reads the recorded uid and gid and never the accessor lists, which is what keeps the two questions apart. That leaves the relay. A cross-DVM request arrives under the relaying daemon's tool identity, and PMIx appends THAT identity's PMIX_USERID and PMIX_GRPID to the call the relay makes - so with ownership decided by the user, the far end would have stored every relayed publish under the relaying daemon and answered every later removal about it. The relay now carries the originating process's uid and gid beside PMIX_REQUESTOR under prte.pub.ruid and prte.pub.rgid, honored under the same tool-only rule. They are PRRTE-private keys rather than a second PMIX_USERID because PMIx adds its own: two entries under one key would leave the answer depending on array order. prte_ds_check_requestor() reads all three in one pass, after the caller's own scan for the same reason. A lookup is included - the access rules have to be answered about the process actually asking. Signed-off-by: Ralph Castain <rhc@pmix.org>
PMIX_PERSISTENCE was enforced at exactly one boundary: a job ending, which the state machine reported as PMIX_PERSIST_APP. Everything else fell where that left it. PROC data waited for its whole job; APP data went at the job rather than the application; SESSION data was never reclaimed at all, because nothing ever sent that horizon; and a publisher meaning "until my job is over" had nothing to say, since APP means the publishing process's APPLICATION and an MPMD job's applications share one namespace without having to end together. Each of the four lifetimes now purges at its own boundary, and none of them costs a message. The store that has to act is in every case one the acting process already holds: a daemon reaps its own children and is told when a namespace is over, and the master tracks every process, application, namespace and session in the DVM. So state/base offers four calls - purge_proc, purge_app, purge_nspace, purge_session - each purging its own store, and only an external data server in another DVM still needs the message, since it is reached over a PMIx tool connection only the master holds. That is what makes the PROC horizon affordable at all: it fires once per terminating process, and a store nothing was published into is an array of one empty slot. Two horizons need something a pmix_proc_t cannot say. An application is not a process and not a namespace either, so the purge names the whole namespace and carries the app index beside it; a session outlives the jobs that ran in it and their job objects, so its purge admits any owner and the session id selects. Each item records both at publish, because neither is derivable by the time the purge arrives - the publisher is gone. PMIX_PERSIST_NSPACE, new in PMIx, is the missing policy, and PRRTE takes it as the default rather than the Standard's PMIX_PERSIST_APP. That is deliberate: reading APP correctly, as this commit does, would otherwise shorten the retention every unmarked publish has been getting, as a side effect of a bug fix. NSPACE is the lifetime those publishes have actually had. A publisher that wants application lifetime asks for APP and gets it. One gap is left deliberately and is recorded in docs/todo.rst: no message tells a daemon that an application has ended, so a local-range item published with an explicit APP persistence is held until its namespace ends - later than asked for, never shorter, and not the default. Signed-off-by: Ralph Castain <rhc@pmix.org>
Two persistences state a criterion that a running system may never reach. PMIX_PERSIST_INDEF is "retain until specifically deleted", and only the publisher may delete it - so in a DVM that outlives the publisher it is a permanent allocation made by a process that no longer exists. PMIX_PERSIST_FIRST_READ is the same shape whenever the read it waits for never comes, which is exactly what the fix for openpmix#2733 leaves behind: an item published for a successor the user then decides not to run. Neither is reclaimed by any lifetime ending, so without something else a long-running DVM accumulates them monotonically, and no surviving process is permitted to remove them. prte_data_server_timeout, five minutes by default, is that something. It is an IDLE timeout rather than a lifetime, and the distinction is the point: a rendezvous name that every job in the DVM reads must not be pulled out from under its readers merely for having been published a while ago. Each item carries the time it was last of use to anybody - stamped at publish, restamped by every lookup that returns one of its keys - and a key in active use never ages. Nothing else is swept: a persistence that names a lifetime has a criterion the runtime does reach, and cutting it short would break the retention its publisher was promised while it is still alive to rely on it. One sweep serves the whole store, armed only while the store holds something the timeout applies to and disarmed when it no longer does, so an idle store costs nothing. Its interval is a quarter of the timeout, bounded to a minute, which is what makes "removed no earlier than the timeout, and normally within a sweep interval after it" a bound worth stating. A timer per item would be exact, at the cost of an armed libevent timer per published item and a re-arm on every read. The swarm case sets the timeout with an ordinary MCA parameter - no debug-only knob - and asserts both halves: a key read every few seconds survives well past the timeout, and the same key goes once nobody is reading it, while a PERSIST_SESSION key sitting idle beside them is left alone. Signed-off-by: Ralph Castain <rhc@pmix.org>
Retention rules say when data goes; they say nothing about how much of it there may be. A job can publish as many PMIX_PERSIST_SESSION keys as it likes and exit, and every one of them outlives it by contract - so a DVM that outlives many jobs accumulates published data monotonically, with no mechanism, not even an administrative one, to reclaim it. Nothing about that requires malice: a crash loop that republishes under fresh namespaces gets there on its own. prte_data_server_max_size, 16 MiB by default, caps it - PER PUBLISHING UID, with eviction confined to that uid's own items. The scoping is the substance of the change, not a refinement of it: a blanket "replace the oldest when full" is an abuse primitive in its own right, since publishing junk in bulk then becomes a way to push somebody else's rendezvous name out of the store. What a user floods, that user loses. Eviction takes the uid's least recently used item, by the same clock the retention timeout reads, and ignores persistence - a user's own data is that user's own to budget. A publish larger than the whole cap is refused outright with PMIX_ERR_OUT_OF_RESOURCE rather than emptying the user's store and failing anyway, and the cap gate runs last, after the duplicate and directive scans, because it is the only gate that MODIFIES the store: a publish that is going to be refused must not have cost anybody their data on the way. The first eviction for a uid says so with a show_help warning naming the limit and the parameter that raises it. Per-uid accounting has one hazard, and it is the reason removal is now a single function. Seven paths take an item out of the store, and one that forgets to uncharge leaves a uid unable to publish anything ever again; prte_ds_drop() is all of them, and prte_ds_charge() covers the shrink an item undergoes when a FIRST_READ lookup takes one of its keys. Writing that down found a defect. ds_lookup removed the info item a FIRST_READ read had consumed but never removed the object when that was its last key, so a FIRST_READ item read by an ordinary lookup stayed in the store as an empty shell - matching nothing, answering nothing, and removed only by a purge. The publish side had always done this correctly where it satisfies a parked request. Signed-off-by: Ralph Castain <rhc@pmix.org>
The page a user reads about publish/lookup described the rules as they were before this work, and most of what it said about retention and removal is now wrong: it named PMIX_PERSIST_APP as the default, described PMIX_PERSIST_SESSION as lasting until the DVM terminates, said PROC and APP data were both reclaimed when the publishing job ended, and told the reader that only the publishing process may remove an item - the rule whose consequences this branch set out to fix. It now describes the four lifetimes at the granularity each is honored, including the distinction that makes the new one necessary: an application is not a job, an MPMD job's apps share its namespace without having to end together, and MPI hides that by requiring its own to. It says plainly that the default is PMIX_PERSIST_NSPACE rather than the Standard's PMIX_PERSIST_APP, and why - reading APP correctly would otherwise shorten the retention every unmarked publish has been getting. Removal is described as what it is, an ownership question answered about the publishing user, with the reason a user rather than a process: an item published by a job that has ended would otherwise be readable by its own user's next job, unusable by it, and removable by nobody. The two directions that are easy to conflate are stated - an accessor list grants reading and never removal, and an owner its own list excludes may still remove what it cannot read. Finally, the two MCA parameters, which no user-facing page mentioned at all: the idle timeout that bounds the persistences naming no lifetime, and the per-uid cap that bounds the store, each with the reason its shape is what it is. Signed-off-by: Ralph Castain <rhc@pmix.org>
PMIX_PERSIST_SESSION was the one lifetime with no multi-node case. It cannot be shown by a job or by a DVM: it needs a reservation that is torn down while the DVM lives on, and nothing else reclaims such an item - no job-end horizon takes it, the expiry sweep does not touch it, and its publisher is gone. The case instantiates a reservation with the publisher running inside it, which is what gives the data a session to belong to: that job carries PRTE_JOB_SESSION_ID, and the datastore records it against each item at publish. The key is read back while the session stands, and is gone once the session is terminated. Its control is a key of the SAME persistence published outside any reservation, which must survive the teardown - so what separates the two is the session id on each item and nothing else. Without that half the case would pass just as well against a purge that wiped the store. Signed-off-by: Ralph Castain <rhc@pmix.org>
Fork Sync: Update from parent repository
prte_ras_base_teardown_reservation gives a reservation's nodes back and removes the session from prte_sessions, but never released the prte_session_t itself. Deregistering is what made that permanent: prte_finalize's sweep over that array was the only thing left that would have reclaimed the object, and deregistering puts it beyond that sweep. It goes per reservation ever torn down, which an elastic DVM does once per grow, and it is more than the struct: the two pointer arrays, the allocation and user reference strings, and - for a scheduler-instantiated session - session->results, which accumulates one info entry for every job that ran in the session and is freed only by the destructor. For the same reason the ras module's release_allocation hook was never reached for a torn-down reservation, only for one that survived to finalize. Releasing it was unsafe because prte_job_t::session and ::target_sessions were borrowed pointers, and a reservation is routinely torn down while jobs are still running in it - the inheritance disposition fires when the OWNING namespace terminates, and jobs another namespace spawned into the reservation can outlive that. Such a job would have been left holding a dangling pointer. So make the job side count. Every assignment to jdata->session now goes through prte_set_job_session(), which maintains the reference on both the outgoing and the incoming session, resolve_spawn_targets() retains each entry of the target set, and prte_job_destruct gives them all back. There is no cycle to close: session->jobs[] borrows in the other direction, as ras/slurm's session stack already demonstrates for its own handle. Teardown can then release, conditioned on the session still being registered so that a second teardown drops no second reference. The two unwind paths that used to release a half-built session no longer do so themselves, and reclaim_session holds its own reference across the teardown because it still has to read the session to report completion to the scheduler. Clearing the back-pointers instead was the other option and does not work: a spawned job is given its session in plm_base_receive before prte_plm_base_setup_job puts it in prte_job_data, so a job parked in prte_cache is invisible to any scan of the job pool. prte_finalize now sweeps the job pool before the sessions. With counted references the old order silently stopped being correct - the daemon job names prte_default_session, so releasing that session would no longer be what carries the node pool away, and the pool teardown would land after the ras framework had closed. Signed-off-by: Ralph Castain <rhc@pmix.org>
The unit test pins the reference counting at the object level; this is the same question asked of a live DVM, and it is the scenario the counted references exist for. The elastic client grows a reservation and holds it open with a short job of its own, a second job from a different namespace is spawned into that reservation by --alloc-id, and then the client exits. Its exit terminates the owning namespace, which fires the reservation's inheritance disposition - the teardown - while the second job is still running. The instructive part is where the failure lands. Teardown itself survives a borrowed job pointer perfectly well; what does not is the moment that job RETIRES, because retiring is where the master reads jdata->session again to remove the job from session->jobs and hand the session to prte_pmix_server_session_job_terminated. Built with the pointer borrowed rather than counted, the case reports the teardown as fine and then the HNP dies with the job, taking the DVM with it - so the assertions are ordered to say which of the two moments went wrong rather than only that something did. Gated on PMIX_CAP_TOOL_FINALIZED like the departing-tool case above it: without that capability the host is never told the tool went, the disposition never runs, no teardown happens at all, and every assertion would pass having tested nothing. Signed-off-by: Ralph Castain <rhc@pmix.org>
A process that is asked for data only the DVM master keeps has no way to say so. Returning PRTE_ERR_NOT_FOUND says the DVM looked and there is no such thing, which is a different claim and a wrong one; returning PRTE_ERROR says nothing at all. The distinction matters wherever the caller can do something about it - forward the request to whoever does hold the data - because that is only possible if "not mine" is distinguishable from "does not exist". Append PRTE_ERR_NOT_AUTHORITATIVE at the next free offset, its string, and move PRTE_ERR_MAX past it. The two bound checks in test/unit/include name the last assigned code, so they move with it. Signed-off-by: Ralph Castain <rhc@pmix.org>
A prted carries the identity half of the DVM's node table and nothing else. The nidmap ships node names, aliases, daemon vpids and pool slots; it has never shipped slots, slots_max, slots_inuse or node state, and every writer of those runs only on the master - the ras components, the hostfile and dash_host parsers, plm_base_setup_virtual_machine(). prte_sessions on a prted likewise holds the default session and nothing more, because every other session is created by the master's allocation paths. So a daemon reading either gets a default-constructed zero. That is not a failure anything notices: it is a plausible number, returned as success, and indistinguishable from a real answer to whoever asked. Add prte_get_allocated_nodes(), prte_get_allocation_session() and prte_get_allocation_sessions() as the only sanctioned way in. On the master they hand back exactly what a direct read would have, so nothing changes there. Anywhere else they return PRTE_ERR_NOT_AUTHORITATIVE having touched nothing, which gives a caller the one thing it could not have before: the knowledge that it must ask someone else. Signed-off-by: Ralph Castain <rhc@pmix.org>
_query() is answered by whichever daemon the client is connected to, and five of its arms read state that only the master holds. On a prted they therefore returned zeros and reported PMIX_SUCCESS: PMIX_NUM_SLOTS and PMIX_QUERY_AVAILABLE_SLOTS summed a node pool whose slot counts are all still at their constructed defaults, PMIX_QUERY_ALLOCATION emitted a node list saying the same, and PMIX_QUERY_ALLOC_IDS and PMIX_QUERY_ALLOC_PROPERTIES answered out of a session table holding only the default session. A rank that landed on the master got the truth and every other rank got nothing, silently - so the answer to a question about the DVM depended on which daemon happened to be asked. Route those arms through the allocation accessors and, when one reports PRTE_ERR_NOT_AUTHORITATIVE, defer its key to the master on the new PRTE_RML_TAG_QUERY. The master answers with this same _query(), where nothing defers, and its results are merged into the ones gathered locally so the client sees one answer covering every key it asked for. The decision is deliberately made by the read rather than by a list of keys that need the master. Such a list cannot be complete - it is a snapshot of today's arms - and it goes stale silently, because a key someone forgets to add to it returns a plausible zero rather than failing. Which reads cannot be satisfied locally is exactly the three accessors, and enforcing it there means a key added later that reads capacity is relayed with no edit, and one that reads only what the nidmap and the launch message already deliver stays local with no edit. Deferral is per key rather than per query because the things a daemon must answer for itself can arrive in the same PMIx_Query_info as a key only the master can answer: PMIX_HWLOC_XML_V1 and _V2 export this node's topology, an unqualified PMIX_SERVER_URI is this daemon's own URI, and PMIX_QUERY_LOCAL_PROC_TABLE means the procs this daemon is hosting. Relaying a whole query would answer those about the master. The relay carries the original requestor so the master defaults the query to the right job, and uses the tracker pattern the scheduler relay already uses. A tag of its own rather than PRTE_RML_TAG_SCHED: that channel demultiplexes on a command byte a query has no value for. Signed-off-by: Ralph Castain <rhc@pmix.org>
The rule the relay depends on is that pmix_server_queries.c reaches allocation state only through the accessors. Nothing in the compiler can enforce it: slots, slots_max, slots_inuse and node state are ordinary members of a struct the file legitimately uses for node identity, so an arm that reads them directly compiles cleanly and returns a zero that looks like an answer. Scan for it instead. prte_sessions has no legitimate direct use in that file at all, and a capacity field may be read only in a query arm that has already established authority - so split the file on the key tests of the if/else-if chain and require any arm reading capacity to name an accessor. Deliberately not a list of query keys, for the same reason the relay does not use one. Signed-off-by: Ralph Castain <rhc@pmix.org>
The defect this covers cannot be seen on one node: every rank is on the master there, so every rank gets the right answer whether or not the relay exists. It takes a DVM whose ranks are on daemons other than the master, which is what the swarm is for. slotinfo asks its own daemon for PMIX_NUM_SLOTS and PMIX_QUERY_AVAILABLE_SLOTS - both in one query, because a request mixing keys is exactly what the relay has to keep whole - and prints the answer tagged with the node it ran on. The harness runs one rank per node and cross-checks rather than asserting a spot value, because the failure is not a missing answer but a plausible wrong one: it asserts the run really spanned nodes, that no rank was told zero, and that every rank was given the same number. Verified in both directions: with the authority check disabled, the master reports the true count and the other three nodes report zero. Signed-off-by: Ralph Castain <rhc@pmix.org>
The rule now has three moving parts in three places - the accessors, the defer label, and the scan that fails the build - and none of them says why the shape is what it is. Record it beside the description of _query(), where someone adding an arm will be reading: what a prted actually holds, that the wrong answer is a zero returned as success rather than a failure, why the decision is made by the read instead of by a list of keys, and why deferral is per key rather than per query. Signed-off-by: Ralph Castain <rhc@pmix.org>
register_nspace() calls PMIx_server_register_client for each proc the daemon is about to host and, on an error, logged it and carried on. The proc was then forked into a PMIx server that will refuse its PMIx_Init, so what the user saw was an obscure failure some way downstream of a launch that had appeared to succeed. Every status this call can return means the same thing about the proc: we cannot run it. Abandon the registration and return the error instead. That exposed a second gap on the path the error now takes. Both failure arms of job_reg_join() activated PRTE_JOB_STATE_NEVER_LAUNCHED without first marking any proc as having failed, so failed_start() found nothing to retire and the HNP read the report as "nothing wrong here" - the job never completed and the tool never returned. The proc-marking that REPORT_ERROR already did is now a function both arms call, so a daemon that cannot register its clients tells the HNP what actually happened. PMIX_ERR_DUPLICATE_KEY is tolerated rather than fatal. A rank we already hold is the state we wanted, and it is reachable: this new error exit abandons the loop with the ranks ahead of it already registered. Signed-off-by: Ralph Castain <rhc@pmix.org>
PRTE_JOB_NSPACE_REGISTERED was written by both registration paths and read by nothing. Meanwhile dmodex_req()'s wildcard arm re-ran the whole of register_nspace() every time PMIx surfaced a job-level key its own store could not answer - assembling and registering identical data from the same job object, so a key absent the first time was absent again and the client waited through all of it to be told NOT_FOUND anyway. Read the attribute there and answer NOT_FOUND directly, which is what it was always for. Note the arm is not reached only by daemons hosting none of the job's procs, as the comment there claimed: PMIx sends a wildcard direct_modex either when it holds nothing for the namespace or when it holds the namespace and a reserved key came back empty, and the second reaches the daemon that forked the asking client. A PMIx_Get of PMIX_NUM_SLOTS from an ordinary app is enough to produce it, and on a one-node prterun -n 2 it re-registered on the HNP with both of its own ranks coming back PMIX_ERR_DUPLICATE_KEY. The flag is now set when the registration completes rather than while it is still being assembled, so that it means "PMIx has our answer" instead of "we have finished describing it"; a request arriving mid-flight would otherwise be refused about data that was seconds from landing. The registration caddy therefore carries a reference on the job. Signed-off-by: Ralph Castain <rhc@pmix.org>
Device distances are computed against the topology of the node the proc runs on, and a daemon has one only for its own: PRRTE collects topologies at the HNP, and prte_util_decode_nidmap() hands every node in a daemon's pool a retained reference to that daemon's own topology, defaulting to homogeneous. So a daemon computing them for a proc it does not host measures its own hardware and labels the answer with somebody else's rank - correct by accident on a homogeneous cluster and silently wrong anywhere else. Publish them only for the procs we host, and refuse a request for anyone else's with PMIX_ERR_NOT_SUPPORTED. Letting it fall through would not help: the hosting daemon publishes the key to its own PMIx server but cannot hand it back, because a direct modex reply carries only what the process itself PUT, so the asker would pay a round trip to be told NOT_FOUND - or park until the proc commits something it may never commit. The prte_pmix_lazy_procdata parameter goes with it. It gated only the requesting side, so "off" meant fetching the same answer over the wire; deriving what we can and asking for the rest is simply what the daemon does. peerinfo now asks each of its peers for the key and reports the status, so the refusal is asserted from both ends - every off-node peer must come back NOT_SUPPORTED, and the daemon must be seen refusing. Nothing is asserted for a peer on the same node: whether any device of the configured types exists at all is a property of the machine. Signed-off-by: Ralph Castain <rhc@pmix.org>
Fork Sync: Update from parent repository
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
racing ahead to upstream prrte sha d90a453. this gets open mpi jobs on perlmutter to not hang.