plm: second framework review - launch accounting, reservation ownership, and dead weight - #2614
Merged
Merged
Conversation
The daemon job's map is persistent - it accumulates the DVM's nodes for the life of the session - but two of its fields are not cumulative. num_new_daemons is what every component keys its "is there anything to launch?" test off, and daemon_vpid_start is the base vpid slurm, lsf and pals substitute into the prted command line, from which each daemon the resource manager starts computes its own name. Both describe only the launch about to happen, and every launcher reads them the moment setup_virtual_machine returns. Each branch of that function was left to clear them for itself, and two of the four did not. So a second launch that brings in a node - an --add-host job, an elastic grow - handed an RM launcher the *first* launch's start vpid, telling the new daemons to claim ranks that live daemons already own. ssh never showed it, because it substitutes each node's own vpid per node, which is why this survived so long. Clear both once, at the top, before any branch runs, so a branch cannot forget and a new branch inherits the behavior. The grow path's own reset (added when it rediscovered the need for itself) goes with them. While here, stop deriving the elastic grow campaign's target list as a run of consecutive vpids starting at daemon_vpid_start, and collect the ranks as they are actually assigned. The derivation holds only while vpids come out consecutively, which is not the rule in a bootstrapped DVM - there a daemon takes its node's canonical rank - and a campaign naming ranks it did not launch never drains its fence. The fence is raised by the campaign's own count for the same reason. test/unit/plm gains test_setup_vm, which drives three passes plus a fixed-DVM job through setup_virtual_machine and requires each to report its own daemons; it fails against the previous behavior. The dockerswarm suite gains the end-to-end shape: form a DVM, bring in a third node with --add-host, and require the second pass to add exactly one daemon. Signed-off-by: Ralph Castain <rhc@pmix.org>
A job spawned into a reservation is supposed to become an owner of it, so that it can in turn spawn - or extend - onto those nodes. The grant was made while vetting the request, in plm_base_receive, and at that point the job has no namespace at all: a spawn request arrives unnamed and the HNP names the job later, in prte_plm_base_setup_job. So what went into the owner set was an empty namespace. That is worse than a no-op. PMIx_Check_nspace answers "true" whenever either side is empty - that is its wildcard rule - so an empty entry in the owner list matches every namespace that ever asks, and the reservation's ownership gate is retired for good. What saved us in practice is the same rule applied one step earlier: the duplicate scan in prte_session_add_owner matched the existing owner, so the empty entry was usually dropped, which means the feature simply never worked. A reservation whose owner list happened to be empty would have taken the entry, and with it every namespace in the DVM. Move the grant to setup_job, immediately after the job is named, where it can record the real namespace, and refuse an empty namespace outright in prte_session_add_owner so no future caller can reopen this. Both are no-ops for the default session, which everyone may use. The vetting of the requester stays where it was, but now runs before the job is handed to the session: session->jobs borrows its entries and nothing ever removes one, so a request rejected after being added left a phantom job on the session for the rest of its life. test/unit/runtime covers the guard, on a reservation with no owners yet - the case the wildcard would otherwise hide. Signed-off-by: Ralph Castain <rhc@pmix.org>
When a daemon reports its topology, the HNP walks every topology it has already recorded looking for a match, calling hwloc_topology_diff_build against each. Only a return of 0 was handled - the match. But hwloc allocates a diff list whenever it has anything to say, and that includes the TOO_COMPLEX entry it returns 1 with when the two topologies are genuinely different. On this path "different" is the common outcome: that is what the scan is looking for. So a heterogeneous DVM leaked one diff list per daemon per recorded topology, and a daemon reporting in a second time - the bootstrap unheal path - dropped the diff its node was already holding on the floor, though prte_node_destruct is what frees that one. Destroy every list this code does not adopt, and destroy the node's previous diff before storing a new one. Also destruct the buffer the topology was unpacked from when the unpack itself fails; the error paths above it construct it and leave, which is harmless, but this one has data in it. Signed-off-by: Ralph Castain <rhc@pmix.org>
prte_plm_base_spawn_response() is the single answer to a spawn request, and it is deliberately called with failures too - errmgr/dvm and state/dvm both route a failed job through it so that a quick-failing job cannot leave its requester waiting. For a tool-requested spawn it also raises PMIX_LAUNCH_COMPLETE, and it did so whatever the status said. That event means the job is running, and a tool is entitled to read it that way. Raise it only on success. A failure already travels by the two routes that exist for it: the PMIX_ERR_JOB_FAILED_TO_LAUNCH event raised just above, and the error status carried by the response itself, which is what releases the requester from PMIx_Spawn. Signed-off-by: Ralph Castain <rhc@pmix.org>
A tool that connects through some daemon is reported to the master, which builds a job object for it and records the node it sits on. It found that node by indexing prte_node_pool with the reporting daemon's rank, which assumes vpids and pool indices run in step. They need not: the pool holds every node the allocation named, including ones no daemon was ever launched on - excluded by a -host or hostfile spec, marked DO_NOT_USE, shrunk away - while vpids go only to nodes that get a daemon. One such node ahead of the reporter names the wrong node; past the end of the pool it names nothing. Ask the daemon instead: its proc object in the daemon job carries the node it is on. Not knowing where a tool sits is also no reason to bring the DVM down, which is what the old NOT_FOUND did - it fell through to the master's CLEANUP, which activates FORCED_EXIT. Record what we do know and carry on. While here, check the return of prte_set_job_data_object rather than building out a job the registry refused. Signed-off-by: Ralph Castain <rhc@pmix.org>
plm_node_regex_threshold was registered as an MCA parameter and documented as controlling whether the node regex went onto the prted command line. Nothing has ever read it: the regex travels in the launch message, and has for years. A knob that does nothing is worse than no knob - a user who hits a too-long command line sets it, sees no change, and cannot tell a wrong setting from a wrong diagnosis. The remedy that does work, plm_ssh_pass_environ_mca_params, is the one help-plm-ssh.txt names. This is the same reasoning that retired plm_ssh_delay. The two dead fields beside it in prte_plm_globals_t - tree_spawn_cmd and daemonlaunchstart - go as well. prte_plm_base_vm_ready() and prte_plm_base_setup_job_complete() are likewise removed. The DVM state machine registers state/dvm's own vm_ready() and init_complete() on those states; these two were never registered by anything, so they sat here looking authoritative while drifting from the code that actually runs - the live vm_ready() also builds and xcasts the WIREUP message and drains the elastic grow campaigns, neither of which the copy knew about. A note in their place says where to edit instead. Signed-off-by: Ralph Castain <rhc@pmix.org>
Five small hardenings from the same review, none of which changes a working launch: The daemon callback records everything it unpacks onto daemon->node, and dereferenced it unchecked. setup_vm links the two when it creates a daemon, so a report naming a rank with no node is a report from something we did not launch - which should be a diagnostic, not a segfault in the HNP. prte_plm_base_daemon_failed likewise assumed the daemon job object exists; if it does not, fail the DVM rather than returning as though no daemon had died. prte_plm_base_daemons_reported walked jdata->session->nodes with no regard for a job that has no session. Every path that admits a job to the launch machinery gives it one, but this handler runs for the daemon job too and is reached from several states, so fall back to the default pool rather than taking the DVM down. plm/slurm parsed "srun --version" by stepping past the major number and then unconditionally over the separator. "slurm 23" with no minor leaves that step on the terminating NUL, and what it reads next is whatever the uninitialized tail of the fgets buffer holds. Step over a separator only when there is one. plm/ssh strdup'd a NULL when prte_launch_agent named nothing to run (an empty --prtemca prte_launch_agent), and leaked the MCA var system's copy of the agent string when it substituted qrsh, llspawn or pbs_tmrsh. The first now says what is wrong through a new help topic; the second frees what it replaces. Signed-off-by: Ralph Castain <rhc@pmix.org>
The server-URI reachability case asked "docker exec prte-node2" for the address it compares the served URI against. Every other global name this harness claims comes from $PRTE_SWARM precisely so two clones can run swarms side by side, and a literal here asks a different swarm - or nothing at all. Running as PRTE_SWARM=plmrev alongside another swarm, the case failed against an unrelated container's address. Signed-off-by: Ralph Castain <rhc@pmix.org>
…shed The base is where nearly all of plm's code lives - setup_virtual_machine, the daemon callback, the command processor, the elastic launch fence - and it was the one directory in the framework without a guide of its own. Add one, covering the parts a reader gets wrong: the branch structure of setup_virtual_machine and why its per-launch accounting is reset at the top rather than per branch, the ownership rules on a node's topology and its hwloc diff, the two invariants that govern an incoming launch request (reject before handing the job to a session; the job has no namespace yet), which of the three things spawn_response sends travel on failure, and where the VM_READY handler actually lives. The framework guide gains the same points in summary; the slurm, lsf and pals guides note that they are the consumers that make daemon_vpid_start matter, and that the swarm cannot exercise it because there is no fake srun - fake-slurm.py stands in for the SLURM control plane, not its launcher. Signed-off-by: Ralph Castain <rhc@pmix.org>
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
1 participant
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.
A second deep review of
src/mca/plm, following #2548. Six review+fix rounds; the last found nothing of consequence. Each commit stands alone.The two that matter
The per-launch daemon accounting was never reset. The daemon job's map is persistent, but
num_new_daemonsanddaemon_vpid_startdescribe only the launch being computed — and every launcher reads them the momentsetup_virtual_machinereturns. Each branch was left to clear them and two of the four did not, so a second launch that brings in a node (--add-host, an elastic grow) handed slurm/lsf/pals the first launch's base vpid: the new daemons are told to claim ranks that live daemons already own.sshis immune — it substitutes each node's own vpid per node — which is why this survived so long. The reset is now done once, at the top, before any branch runs.A reservation's owner was recorded before the job had a name. A spawn request arrives unnamed (the HNP names the job at
INIT), so the grant wrote an empty namespace into the owner list.PMIx_Check_nspacetreats an empty side as a wildcard, so such an entry matches every namespace and retires the reservation's ownership gate. What saved us in practice is the same rule one step earlier — the duplicate scan matched the existing owner and dropped the entry — which means the documented feature (a spawned job becoming an owner so it can spawn or extend onto those nodes) simply never worked. The grant moved tosetup_job;prte_session_add_ownernow refuses an empty namespace.The rest
TOO_COMPLEXentry it returns 1 with — the common outcome while scanning recorded topologies for a match. Only0 ==was handled, so a heterogeneous DVM leaked one diff per node per recorded topology.PMIX_LAUNCH_COMPLETEwas raised for failed launches (spawn_responseis deliberately called with failures). Success only now.session->jobs, which borrows its entries and never removes one.TOOL_ATTACHEDindexed the node pool by the reporting daemon's rank — only right while vpids and pool indices run in step — and a miss took the whole DVM down viaFORCED_EXIT.prte_plm_base_vm_ready()andprte_plm_base_setup_job_complete()removed: unregistered duplicates ofstate/dvm's live handlers, which the copies had drifted from (the realvm_readyalso xcasts WIREUP and drains the grow campaigns).plm_node_regex_thresholdretired — registered and documented, read by nothing; the regex has travelled in the launch message for years. Same reasoning asplm_ssh_delay.srun --versionparse walking past the NUL, an ssh agent-string leak, and an emptyprte_launch_agentreachingstrdup(NULL).Tests
test/unit/plmgainstest_setup_vm(three launch passes plus a fixed-DVM job throughsetup_virtual_machine); it fails against the previous behavior.test/unit/runtimecovers the empty-owner guard on a reservation with no owners yet — the case the wildcard otherwise hides.contrib/dockerswarmgains the end-to-end shape: form a DVM, bring in a third node with--add-host, require the second pass to add exactly one daemon. Also fixes a hardcoded container name that ignored$PRTE_SWARM.Note what the swarm cannot reach:
daemon_vpid_start's only consumers are the RM launchers, and there is no fakesrun—fake-slurm.pystands in for the SLURM control plane, not its launcher. That invariant is pinned by the unit test.Verification
Warning-free
--enable-debugbuild;make check21/21;make -C test/offline check-offline1180/1180; live DVM cycle; dockerswarm 539 passed / 1 failed / 2 skipped. The one failure ("N of 3 ranks survived a recoverable failure") is a harness output race, not a regression — the same build passes it 3/3 on re-run, and in the failing instance all three survivors had already printed their notification line; only one finalSURVIVEDprint was lost at teardown.Left alone
#2546 (plm/slurm comparing a count against a pointer-array capacity) is untouched: benign today, and fixing it changes which nodes
srunpicks, so it wants a real SLURM allocation to re-test.