Make remote submission over SSH work, from the host and from the container - #1001
Make remote submission over SSH work, from the host and from the container#1001calvinp0 wants to merge 9 commits into
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1001 +/- ##
==========================================
+ Coverage 64.76% 65.38% +0.61%
==========================================
Files 119 120 +1
Lines 40039 40303 +264
Branches 10350 10390 +40
==========================================
+ Hits 25932 26352 +420
+ Misses 11129 10944 -185
- Partials 2978 3007 +29
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
6198a03 to
ec9d9c7
Compare
| command = f'rm -r "{remote_path}"' | ||
| _, stderr = self._send_command_to_server(command) | ||
| if stderr: | ||
| raise ServerError( | ||
| f'Cannot remove dir for the given path ({remote_path}).\nGot: {stderr}') |
There was a problem hiding this comment.
adopted. also found seven other sites that required this change.
ec9d9c7 to
fa404f1
Compare
|
Reviewed this at First, the credit: this PR genuinely fixes host-key handling. Base fed the private key path to The one thing worth fixing first
The shipped Orca Slurm template then runs the job somewhere else entirely: WorkDir=/state/partition1/user/{un}/$SLURM_JOB_NAME-$SLURM_JOB_ID
cd $WorkDir
cp $SubmitDir/input.in . # only input.in is copiedSo Suggested fix: put bare filenames in the NEB template and add And it's unguarded — I reverted One root cause behind four findings
One fix covers all four: classify the exception once — re-raise Two more with executed repros
ARC suppresses its own host-key warning ( On an unknown host ARC connects and prints nothing — not to stdout, not to Ten more findings (click to expand) — pool coverage, dead fallback, silent-import fallbacks, pipe guard, style
Checked and found sound — so nobody re-treads these
Where this review is weak, so you can weight it accordingly: the Docker half has zero executed coverage. Happy to open a PR against your branch for the NEB path + its test if that's useful. |
fa404f1 to
e3f5078
Compare
Orca NEB refuses a non-absolute remote path rather than emitting a deck Orca will fail on. Permanent auth/host-key failures are classified and raised on the first attempt instead of feeding the 24-hour retry. The per-connection host-key report now goes through ARC's logger, since initialize_log filters paramiko's warnings.warn. The pool fallback is rebuilt on Regarding the Also, in the docs |
e3f5078 to
68cb19a
Compare
ff998ac to
641fbff
Compare
97833ae to
3e69c3f
Compare
3e69c3f to
fd7c938
Compare
…regardless of compute_thermo (#1022) ## What this fixes An Arkane **kinetics** input generated by ARC can reference species it never declares, so Arkane dies before computing anything: ``` File "arkane/input.py", line 297, in reaction reactants = sorted([species_dict[spec] for spec in reactants]) KeyError: '[O]C=O[1]' ``` This was found on a run where everything else went right: two species and a transition state all optimised and frequency-checked on a cluster, and then **no rate coefficient at all**. ## Cause `ArkaneAdapter.render_arkane_input_template` builds `species_list` under `if e0_only or spc.compute_thermo:`. A caller that sets `compute_thermo=False` — as an orchestrator does when it wants kinetics but does not want a full thermodynamics job queued — gets: - the **E0** render (`e0_only=True`) → gate passes → all species declared; - the **kinetics** render (`e0_only=False`) → gate fails → *none* declared, while `reaction(...)` still names them by label. `generate_species_files` writes the per-species statmech files either way, so ARC writes `statmech/kinetics/species/<label>.py` to disk and then writes a main input that references that species without declaring it. `compute_thermo` should govern whether a thermo job runs — not whether a species may be declared in an input that names it. The existing `test_generate_arkane_input` never caught this because `ARCSpecies.compute_thermo` defaults to `not is_ts`, i.e. `True` for ordinary species; the defect only surfaces when a caller sets it to `False` explicitly. ## The change Collect the labels named by any reaction in the render's reaction list, and let a species through the gate if it is one of them: ```python if e0_only or spc.compute_thermo or spc.label in reaction_species_labels: ``` Deliberately narrow rather than dropping the gate: dropping it would push species into thermo inputs that exclude them on purpose. For a thermo render `self.reactions` is `None`, so the new set is empty and the gate is unchanged — thermo inputs are unaffected. ## Testing - New unit test: a reactant with `compute_thermo=False` now renders as a `species('R', ...)` line in a kinetics input. - Mutation: restoring the old gate turns it red, with the rendered text showing `reaction(reactants=['R'], products=['P'])` above zero `species(` lines. - `arc/statmech/`: 50 passed. - **End to end on real converged output.** Taking the failing run's own kinetics input and adding only the two `species(...)` declarations this fix emits, Arkane exits 0 and writes the rate coefficient it previously could not produce: ``` kinetics(label = '[O]C=O(1) <=> O=[C]O(8)', kinetics = Arrhenius(A=(36.3562,'s^-1'), n=3.33173, Ea=(61.8336,'kJ/mol'), T0=(1,'K'), Tmin=(300,'K'), Tmax=(3000,'K'))) ``` No new quantum chemistry — the converged logs were already on disk. (Those numbers come from an RMG-Py checkout that is behind `main` in `arkane/`, so treat them as a plumbing result, not a physical one.) Note for anyone running these tests: `arc/statmech/` needs `-n0`. Under the default xdist config `test_generate_arkane_input` flakes, because sibling workers share the on-disk `arc/testing/arkane_input_tests_delete` directory. Pre-existing and unrelated to this change. --- ## Separately: an unrelated bug found on the same run `arc/job/ssh.py:42-43` unpacked `self.connect()`, which returns `None`. Not fixed here, and no longer needs to be: @calvinp0 pointed out it is already fixed in #1001 (`self._sftp, self._ssh = self.connect()` → `self.connect()`, with a regression test). Tracked there.
fd7c938 to
db2a20a
Compare
|
Re-verified all 18 findings against the current head One thing has to change before this merges, and it's a consequence of the NEB fix rather than something the review anticipated.
|
Every remote job opened its own paramiko Transport for upload, submission and status polling. A TS search issuing ~100 guess optimizations against one server opened ~100 connections, which is slow and trips per-user connection limits on some clusters. Add a process-global pool (arc/job/ssh_pool.py). A remote queue job leases one client for the duration of its submission and reuses it for both file upload and run; _open_or_borrow_ssh() prefers that leased client and otherwise borrows from the pool. The borrower never closes a client it does not own. The third case, a one-shot client for when the pool itself cannot lease one, is real rather than nominal. Written as `try: return pool.borrow(server)` with an `except` around it, it could never fire: borrow is a @contextmanager, so calling it merely builds the generator, and the factory runs when the caller enters the with-block -- outside the try meant to catch it. Rebuilt around contextlib.ExitStack, the fallback covers exactly the lease, i.e. entering the pool's context manager, and nothing else. In particular it does not cover the caller's own work: a job that raises inside its with-block still raises, instead of having its failure read as a broken pool and its body re-entered against a fresh client. Two pre-existing defects in the remote path that set_file_paths() builds are fixed here as well, since a remote job's files are only reachable if that path is. The server's configured 'path' was lowercased before use, which silently rewrites any path with an uppercase component -- remote file systems are case-sensitive, and /Home/Users is not /home/users. It is now used verbatim. NOTE that this changes where files land for anyone whose configured 'path' contains uppercase: their earlier runs are under the lowercased tree, and ARC will now use the path as configured. A server with no 'path' still gets a path relative to the SSH login directory, because there is no absolute path ARC can know offline -- the home directory is the server's to report, and set_file_paths() runs at job construction with no connection open. Rooting it at '~' would be worse rather than better: _send_command_to_server quotes the remote path with shlex.quote, so the remote shell would take the tilde literally, and SFTP performs no tilde expansion at all, so both would end up creating a directory actually named '~'. Relative, which the remote shell and SFTP both resolve against the login directory, stays correct. What changes is that it is no longer silent: such a server is reported once per run, naming the setting, because an adapter that has to name a path inside an input file cannot work with it. Pooled clients also need closing when ARC exits. ssh_pool.py documented that ARC.py's main() calls reset_default_pool(), but nothing did -- every caller was a test, so pooled SSHClients were left to interpreter shutdown rather than closed. Releasing them belongs to the run rather than to the command-line entry point, so ARC.execute() does it in a finally: connections are torn down on ctrl-C and on an exception as well as on a clean run, and a consumer that drives ARC in process -- a library caller, a test, a pipe worker -- releases them too, which an ARC.py-only hook could never do. ARC.py is unchanged. The borrow itself is now one function, ssh_pool.borrow_ssh_client(), rather than a method on JobAdapter. The pool's other callers are not adapters -- Scheduler.get_server_job_ids(), server troubleshooting, the ESS survey -- and each would otherwise have grown its own copy of the lease-then-fall-back dance. _open_or_borrow_ssh() keeps only what is adapter-specific, the per-execute() leased client, and delegates the rest; the shared-client branch is contextlib.nullcontext rather than a hand-rolled generator. A pooled connection is held for the whole run and sits idle between polls, so _default_factory sets a keepalive on the transport. An SSH daemon's ClientAliveInterval or a firewall's idle timeout otherwise drops it silently: the socket stays half-open, Transport.is_active() keeps reporting True, and the pool's liveness check therefore hands out a handle whose first command hangs until TCP gives up. upload_file() and download_file() were the only SSHClient methods reaching for self._sftp without @check_connections. That was harmless while every caller opened its own client and used it immediately; with a client that has been alive for hours it is not, since a dead transport surfaces as the transfer failing rather than as a reconnect. Both are decorated now. The pool is tested directly rather than only through a JobAdapter, so its own contract -- reuse, reaping a dead client, retaining ownership on context exit, idempotent close_all -- is stated by its tests instead of implied by adapter behaviour. arc/job/ssh_pool_test.py drives SSHConnectionPool with a stub factory and covers the cases adapter_test.py could not reach, namely that a raising with-body leaves the pool reusable and that reset_default_pool() closes pooled clients rather than just dropping the reference. The adapter-driven integration tests stay with the adapter, which is what they actually exercise. Two claims the pool's docstrings make were still untested, and the imports for them were sitting unused in adapter_test.py: that a remote-queue execute() with no pool injected borrows from the instance get_default_pool() returns, and that reset_default_pool() -- ARC.py's exit hook -- closes the clients those jobs opened and leaves a usable empty pool behind. Both are now asserted rather than implied. The pool tearDowns also called set_default_pool(None), which drops the reference without closing anything, so each test class leaked its stub clients and contradicted the lifecycle ssh_pool.py documents; they call reset_default_pool() instead. Also two test-only cleanups CodeQL flags: two factory lambdas that only forwarded their argument now pass the callable itself, and the "a raising with-body leaves the pool usable" test uses assertRaises' callable form. Its context-manager form made every statement after the block unreachable to a control-flow analyser, because nothing in the CFG says assertRaises.__exit__ suppresses the exception. Two of CodeQL's remaining alerts on this file are in adapter_test.py and are the same two defects already fixed in ssh_pool_test.py. The assertRaises context-manager form around a with-block whose last statement is `raise` makes everything after the block unreachable to a control-flow analyser, since nothing in the CFG says __exit__ suppresses the exception; both tests use the callable form, and still assert what they did -- that the caller's own exception reaches the caller, and that the pool is usable afterwards. And the module both imported arc.job.adapter and imported names from it; the module-alias form existed for two patch.object() calls, which are now patch('arc.job.adapter.<name>'), so the file uses one import form. Dropping the alias also removes a name that a class attribute in the same file shadowed. Absorbed from PR #1000 by @alongd, brought in here rather than merged so the two pull requests do not conflict over the same lines: set_file_paths() splits the project's remote directory out of the job's remote path as remote_project_path, which is what the remote check file cleanup is scoped to. Record the keepalive interval on the client as well as on its transport. A keepalive belongs to a paramiko Transport, and check_connections reconnects a client in place when its socket has gone half-open, so the transport the factory set the keepalive on is not the one the client ends up holding. SSHClient.connect() re-applies the recorded interval to every transport it opens, which is the case a connection held for a whole run actually meets. Also brings the module to ARC's conventions: docstrings on __init__ and _close_quietly, Args and Returns sections, f-string logging in place of %s, single-quoted strings, and no comments on code lines.
6a06d1a to
d2e5962
Compare
…scoped retry, and an absolute remote path Makes the SSH key optional so that an ssh-agent or the default key paths can authenticate, verifies host keys rather than adding them silently, and scopes the connect retry to the failures retrying can actually resolve. Classify a key file that does not exist as a permanent failure. paramiko guards only _key_from_filepath's SSHException, so an absent key_filename surfaces as FileNotFoundError, which is an OSError and so was not in PERMANENT_CONNECTION_ERRORS. At the default 1440 connection attempts that stalled the run for 24 hours over a path that will never appear, and the shipped server examples all carried the 'key': 'path_to_rsa_key' placeholder that triggers it. It is told apart from a transient network OSError by being a FileNotFoundError naming the configured key path, so a refused or reset connection is still retried. Re-apply a recorded keepalive interval to every transport connect() opens, so that a pooled client reconnected in place by check_connections does not lose idle-drop protection for the rest of the run. Report a server with no absolute remote path before any job is spawned. An adapter whose input file names a path on the server cannot run on a server that has no 'path' entry: its remote directories are then relative to the SSH login directory, which the deck cannot name, so the job cannot be built at all. orca_neb is such an adapter and is in the default ts_adapters, so this was reached only when the first reaction got to its TS search, part way into a run. check_ess_settings() already validates the adapter and server names it is given and runs before any calculation is spawned, so it now also takes the run's ts_adapters and checks the servers each path-naming adapter will run on, naming the server and the setting to fix. The check is keyed on the ESS settings entry an adapter resolves its server from rather than on the adapter's own name, because OrcaNEBAdapter is given its server while it is still an OrcaAdapter and therefore runs wherever orca runs. The shipped remote server examples gain the absolute 'path' entry they omitted, which is what made that the default configuration rather than an unusual one.
The pool was built and tested but the highest-frequency caller never used it. Scheduler.get_server_job_ids() opens a connection per server per poll cycle, for every cycle of every job's lifetime -- on a run of any length that is the dominant source of connections by an order of magnitude, and it is exactly the traffic shape a per-user connection limit is there to stop. It borrows now, so a run's polling costs one connection per server rather than one per poll. The same for the three sites in trsh_job_on_server() and for CFour's execute_queue(). CFour overrides execute_queue() rather than calling JobAdapter.legacy_queue_execution(), so it did not inherit the sharing the other adapters got; it goes through _open_or_borrow_ssh(), which means its submission also reuses the client its upload just used. One of those trsh sites leaked. `ssh = SSHClient(server)` with no `with` and no close() left a connection open for the rest of the process every time a job was troubleshooted by changing node. It never actually reached the server, since check_connections() raised TypeError on an unconnected client (fixed with the rest of the SSH work), but the leak is real for any caller that got past it. Not routed: delete_all_arc_jobs() in arc/job/ssh.py. Its only caller is arc/utils/delete.py, a standalone command-line utility that deletes jobs and exits, outside any ARC run; it opens no connection ARC would otherwise reuse and its `with` already closes what it opens, so pooling would swap a closed connection for one left open until the interpreter exits. ssh.py is also the module ssh_pool.py imports, so pooling there would have to be a function-local import to avoid a cycle -- a cost with nothing bought. Absorbed from PR #1000 by @alongd, brought in here rather than merged so the two pull requests do not conflict over the same lines: the scheduler records the remote project path of each server it spawns a job on, which is what the end of a run hands to the check file cleanup. Fix logger.denug in the unknown-cluster-software branch of trsh_job_on_server, which raised AttributeError instead of declining to troubleshoot.
arc/imports.py caught ImportError from the local settings.py, submit.py and inputs.py overlays and passed. An overlay that fails to load therefore leaves ARC running on the repository defaults with nothing said, and the defaults are a working configuration, so there is no other symptom: the cluster templates in submit.py or the server definitions in settings.py are simply not the ones the user wrote, and a run goes to the wrong place, or to ARC's dummy servers, for hours. The usual cause is an overlay that imports something not installed in the environment ARC is running in, which is easy to produce and invisible once produced. Report it instead. Control flow is unchanged -- the defaults still stand, the run still starts -- and the message names the file and the error, and is queued so it survives the log being initialized later in the run. Loudness follows what actually failed, since the two cases mean opposite things. A file that loaded but does not define the name is a partial overlay, which is the ordinary way to override one setting and leave the rest alone: a submit.py that defines submit_scripts and neither incore_commands nor pipe_submit is correct, and warning about it would put two lines in every run's log of every user who has one. That is a debug line. A file that did not load at all loses every setting in it, and is a warning, reported once per file rather than once per name imported from it. The two are told apart by whether the module is in sys.modules after the failure. A syntax error in an overlay is not covered, and cannot be: it is a SyntaxError, not an ImportError, and it propagates out of arc/imports.py and stops ARC from starting -- loudly, if confusingly, but never silently.
PipeRun.submit_to_scheduler() invokes qsub/sbatch on the machine running ARC, and the worker (python -m arc.scripts.pipe_worker) reads pipe_root from its local filesystem. When the engine's resolved server is remote, that submission errors silently and the run deadlocks waiting for results that can never arrive. Make should_use_pipe() refuse a non-local server so the planner falls back to per-job queue submission over SSH, and say in the log which engine and server triggered the refusal and what is being used instead -- that fallback is slower than a pipe run, so without the message the only symptom is an unexplained slowdown. Supporting pipe on a remote server needs it rebuilt around batch jobs staged on the remote side, which is out of scope here. The guard resolved its server with `next((s for s in server_list if s in servers_dict), None)`, which fails open in three ways: it skips an entry that names an unconfigured server and silently judges the next one instead, it compares server names case-sensitively when a server name is a settings key whose casing the user chose, and it permits the pipe when nothing resolves at all. That last one matters most, because derive_cluster_software() applies the same "skip what is not configured" rule and then falls back to guessing slurm, so an unresolvable server produced a pipe submitted with a guessed template. Resolve the first entry unconditionally, compare case-insensitively, and refuse unless the result is a configured server that is this machine. Refusing costs the run only the bundling -- the planner submits the tasks as individual queue jobs, which works for a local and a remote server alike -- so failing closed here is cheap and failing open is not. "Cannot be resolved" is not the same as "has no server", and conflating the two would have disabled TSG pipe mode outright. A TS-guess batch carries engine=<method>, and gcn, kinbot, xtb_gsm and the rest are not ESS: they are absent from ess_settings by design and run in this process, which is why _initialize_adapter resolves a server only for an engine ess_settings names and leaves every other one with server=None, and why set_file_paths gives such a job no remote path at all. In process is this machine, so those tasks pipe. The refusal is for an engine that ess_settings does name and that still does not resolve to a configured local server -- an ESS declared and available nowhere, or named on a server that is not configured. The resolution itself is not a second implementation. _initialize_adapter() already decided which server a job goes to, inline: a trsh override first, then the first entry of the ESS settings for the adapter, with a bare string read as a single server. That is now resolve_job_server() in arc/job/adapters/common.py, the module that owns the concept, called by both, so the pipe's answer is the answer the job would have got rather than a lookalike. Extracting it also fixes an IndexError on an empty server list, and drops a redundant re-check of a condition the enclosing `if` had already established. Absorbed from PR #1000 by @alongd, brought in here rather than merged so the two pull requests do not conflict over the same lines: _initialize_adapter() initializes the new remote_project_path attribute.
The NEB input template embeds absolute paths to reactant.xyz and product.xyz,
built from self.local_path. For a remote server that names a directory on the
machine running ARC, which does not exist on the cluster, so Orca cannot open
the geometries.
set_files() already uploads both files, and they land in remote_path. Choose
the path accordingly -- remote_path for a remote server, local_path otherwise
-- mirroring the choice JobAdapter already makes for the pipe payload's "pwd".
This needs no change to any submit script, since the files are staged where the
input now points.
remote_path is only absolute when the server carries a 'path' in the settings:
JobAdapter builds it as os.path.join(servers[server].get('path', '').lower(),
'runs', 'ARC_Projects', ...), which is relative when 'path' is unset, and the
shipped Orca submit scripts cd into a scratch directory before running. Refuse
to write the deck in that case, naming the server and the setting, rather than
emitting one Orca will fail on for a reason that is not visible in the input.
The relative path is pre-existing behaviour of arc/job/adapter.py shared by every
adapter. The commit adding the connection pool fixes what can be fixed there --
the lowercasing of a configured path, and a report naming any server without one
-- but a server with no 'path' configured has no absolute remote path that ARC
can know before it connects, so refusing here is the other half of that fix.
The adapter's tests only ever ran with server='local', where reverting the path
choice to local_path still passed. They now cover a remote server too: the deck
must point into the absolute remote_path, and a relative remote_path must be
refused.
Raise SettingsError rather than ValueError when the remote path is not absolute, matching the
error arc.common.check_remote_paths_of_path_naming_adapters raises for the same condition at
startup, so the two read as one settings problem. Validation now reports this before any job is
spawned for every server the adapter is configured to run on; the refusal here remains the
backstop for the paths validation does not see, such as a job moved to another server by
troubleshooting.
ARC inside the image can now reach a remote HPC cluster either through a
forwarded SSH agent socket (preferred -- keys never enter the container, and
passphrase-protected keys keep working) or through a read-only bind mount of the
user's key material, with the ~/.arc settings overlay mounted alongside it.
entrywrapper.sh:
- pass SSH_AUTH_SOCK explicitly across the `runuser -u mambauser` privilege drop,
and explicitly `env -u` it when the socket turned out to be unusable, so the SSH
client falls back to key files instead of failing against a dead socket;
- never chown/chmod a bind-mounted /home/mambauser/.ssh (detected by comparing the
device of the path with its parent), so read-only mounts neither fail nor leak
ownership changes back to the host. paramiko does not enforce 0600 on key files,
so a read-only mount is fine;
- emit actionable PUID/PGID diagnostics when a mounted socket or .ssh directory is
not accessible to the container user.
PUID/PGID remap: the base image carried a vestigial 'ubuntu' account at 1000:1000
and the entrypoint refused to remap onto any occupied ID, so `-e PUID=$(id -u) -e
PGID=$(id -g)` -- the very flags needed for bind-mount ownership -- aborted with
exit 1 for the majority of Linux desktop users, and docker-compose.yml defaults
both to 1000, so the compose path was broken by default. The Dockerfile deletes
that unused account in the final stage, and the entrypoint no longer depends on
the image being fixed: a collision with an ordinary, idle account is resolved by
sharing the ID (usermod -o), since permissions are numeric and nothing then has to
be deleted or renamed. "Unused" is established from /proc rather than assumed from
the account's name. A collision with the superuser, with a system account (<= 999),
or with an account owning running processes is still fatal, exit code unchanged,
but now names the exact flag to drop and states what ownership the mounts would
fall back to.
Agent socket: prepare_ssh_agent_socket() ran `chmod o+rw` on the forwarded socket
whenever the container user could not open it. A bind mount shares the inode, so
that mutated the user's live agent socket on the host -- 600 -> 606, verified --
leaving it readable and writable by every local user for as long as the agent runs,
never restored, and contradicting the rule this same file applies to every other
bind-mounted path. Restoring it is impossible here in any case, since the
entrypoint hands off with exec and no EXIT trap can fire. The remap above is the
mechanism instead: a container user carrying the host UID opens a 0600 socket with
nothing changed. The widening survives only behind an explicit
ARC_WIDEN_AGENT_SOCKET=1, and when used it reports what it changed on the host and
how to undo it; without the opt-in, an inaccessible socket is reported with the
PUID/PGID fix and agent forwarding is skipped.
Dockerfile: add openssh-client to the final stage (ssh-keyscan and friends for
debugging; ARC itself uses paramiko) and pre-create /home/mambauser/.ssh so a bind
mount lands with sane ownership. Final stage only.
docker-compose.yml: replace the stale definition (foreign image, non-existent
/home/rmguser/KMClass path, a CONTAINER_MODE variable the entrypoint never read)
with one matching the real entrypoint contract: /work bind mount, ~/.arc, agent
socket forwarding, PUID/PGID.
arc_preflight.py: arc/imports.py catches ImportError from the local settings.py
and passes, so a settings.py that cannot be imported leaves ARC running against
its *dummy* servers with no output at all -- verified: a settings.py whose first
line imports a missing module yields settings['servers']['server1']['address'] ==
'server1.host.edu', silently. In a container a mistyped mount path lands in exactly
that state, and the only symptom is a run that spends hours failing to reach a host
that never existed. The pre-flight imports the overlay exactly as ARC does -- as a
top-level module on sys.path, without importing ARC itself -- and exits 78
(EX_CONFIG) if it is present but unimportable; a directory with no settings.py is
reported as absent and exits 0. Everything else is a warning, deliberately: a
missing or unreadable `key` file, a server with no `key` and neither an agent nor
a default key, and strict_host_key_checking with no known_hosts. A server that is
configured but unused in a given run must not be able to abort it.
ARC_SKIP_PREFLIGHT=1 bypasses the whole check. The check runs for `arc` only, never
for `rmg`, and the overlay directory is passed as argv so the entrypoint and the
tests cannot drift apart on the path.
The overlay is mounted read-only, with PYTHONDONTWRITEBYTECODE=1 so Python does not
pointlessly attempt __pycache__ writes into it; runuser sets HOME=/home/mambauser,
hence the mount target.
test_docker_smoke.py: SSH-oriented smoke checks that need no remote server
(paramiko importable and constructible, arc.job.ssh imports, openssh-client
present, entrypoint agent-socket handling, forwarded socket usable when present,
the PUID remap and the socket mode -- each of the last three confirmed to fail
against the pre-fix entrypoint), plus an opt-in live-cluster test skipped unless
ARC_SMOKE_SSH_HOST is set. The overlay is exercised through a subprocess, never by
importing arc in the pytest process, since some ARC branches disable the ~/.arc
overlay whenever pytest is loaded, which would silently hollow out an in-process
test.
Not changed: usermod -u recursively chowns the home directory, which was raised as
a possible startup cost now that ~/.julia is 2.9 GB. Measured on the built image --
28,795 files, 0.375 s without the remap against 0.925 s with it. That does not
warrant working around usermod, so it is left alone.
Julia is pinned to 1.10.11 rather than tracking the 1.10 channel. juliacall
segfaults on import under 1.10.12 and the channel floats to the newest patch, so
an unchanged Dockerfile silently changed what it installed; RMG-Py pinned the
same version for the same reason in 62eb728c0.
docker-compose.yml also mounts known_hosts read-only at
/home/mambauser/.ssh/known_hosts, paramiko's only host-key location, so the
container gets the same treatment as the key material and the ~/.arc overlay and
so ARC's startup host-key check means something there. The source is
${ARC_KNOWN_HOSTS:-/dev/null} rather than ${HOME}/.ssh/known_hosts directly:
Docker materialises a missing bind-mount source as a directory, so naming that
path unconditionally would leave a root-owned directory at
$HOME/.ssh/known_hosts on any host that has never written the file, which then
breaks ssh on the host itself. /dev/null always exists and reads as an empty key
list.
The container and SSH halves of remote submission were in place; what was missing was the user-facing configuration around them. These docs are written against the `key` and host-key semantics this branch introduces, not against main's. docs/source/remote_submission.rst (new, in the toctree): authentication via a forwarded agent (preferred -- keys never enter the container and passphrase-protected keys keep working) or via a mounted key file; host key verification and the new per-server strict_host_key_checking, including why an unseeded known_hosts matters in a fresh container now that WarningPolicy and RejectPolicy have replaced the silent AutoAdd; the ~/.arc overlay mount, which a remote run needs as much as the SSH material since submit.py carries the cluster's PBS/Slurm templates; both `docker run` invocations and the compose equivalent; and the entrypoint's exit codes. Two limitations are documented rather than worked around: - ARC never builds a paramiko.SSHConfig, so ~/.ssh/config is not read at all and ProxyJump/bastion hosts are unsupported. This is true on bare metal too, and is called out so nobody blames the container for it. - a default-bridge container reaches an ordinary login node with no extra flags, since paramiko speaks SSH itself; the exceptions are a host VPN whose routing excludes docker0, and internal names served only by a VPN-pushed resolver. Each claim is checked against the code rather than assumed: paramiko's load_system_host_keys() reads ~/.ssh/known_hosts and nothing else, so /etc/ssh/ssh_known_hosts is not mentioned as an alternative (seeding it under strict_host_key_checking would have refused every connection); a rejected host key raises into the same 24-hour retry loop and so presents as a hang rather than a fast failure; the retry reason reaches the logger only on every tenth attempt, the others going to stdout; and Docker materialises a missing bind-mount source as a root-owned directory, which is what a stale SSH_AUTH_SOCK or an absent ~/.arc produces on the host. installation.rst and running.rst described `key` as a private key path, which was wrong on main and is right as of this branch; they now say so, present the agent route as the default, and mention strict_host_key_checking. docker.rst gains the remote-submission pointer, index.rst the toctree entry, and the stray `key` in the advanced.rst node-limits example is dropped, since that example is about cpus and memory. remote_submission.rst now also states why warning rather than rejecting is the default host-key policy -- a refused key does not fail a long-running scheduler once but starves it while the driver stays alive, and it presents as a hang rather than an error -- and how to opt into refusal per server. The Startup Checks section leads with the check ARC itself performs on every run, in a container or not, and describes the compose file's known_hosts mount and why ARC_KNOWN_HOSTS defaults to /dev/null. running.rst gains a pointer to the same startup report. Absorbed from PR #1000 by @alongd, brought in here rather than merged so the two pull requests do not conflict over the same lines: keep_checks is documented as covering the servers a project ran on, not only the local project directory. Document the server 'path' key and give every remote server example one. No example defined it, so the documented remote configuration was the one in which an input file that must name a path on the server, such as Orca NEB's, cannot be written.
arc/job/adapters/gaussian_test.py and arc/job/adapters/common_test.py both built their adapter fixtures under arc/testing/test_GaussianAdapter and both deleted that directory in tearDownClass. Under pytest-xdist the two modules run on different workers, so whichever class finished first removed the tree the other was still writing input files into, and the three tests that render an input file and read it back failed with FileNotFoundError on the input.gjf they had just written. The same collision is possible within this module alone, since the worksteal scheduler may split a class across workers and each worker runs its own setUpClass and tearDownClass. Create the project directory with tempfile.mkdtemp() in setUpClass and remove it through addClassCleanup, so every class setup owns a directory no other class or worker can name, and each removes only the directory it created. No test asserts on the directory's path; they all derive it from the adapter's local_path. Reproduced by running this module together with arc/job/adapters/common_test.py under -n 4 --dist worksteal: 8 of 8 runs failed before, 7 of 7 pass after.
d2e5962 to
bf4a202
Compare
|
NEB abort — fixed, though not the way you suggested. Both options fail silently mid-run: the user loses a TS method and only finds out from a log line, and widening the One thing that isn't visible in the diff and shaped the design: The root cause was the templates. No remote server block in the docs defines Where I might be wrong on this. If you think fixing the templates is enough, the validation is arguably insurance for a population neither of us can measure — I don't know how many existing overlays lack Missing key — a Keepalive — the client records the interval it was given and re-applies it to each new transport, so Both pre-existing ones are fixed too. Two corrections. 350 passed. Each fix reverted individually with the guarding test confirmed failing. Rebased onto current |
Makes ARC's remote-cluster path work over SSH, and then makes it work from inside the Docker image.
SSH
A server's configured
keywas handed toload_system_host_keys()rather than toconnect(), so it was never offered as an authentication identity. It is now passed askey_filename, and made optional so a forwarded ssh-agent or paramiko's default key paths can authenticate where no local key file exists.The same path stops silently trusting unknown hosts —
AutoAddPolicybecomesWarningPolicy, with an opt-in per-serverstrict_host_key_checkingselectingRejectPolicy— and narrows a bareexceptaround the connect retry that had been swallowing Ctrl-C and discarding the real authentication error.A process-global connection pool replaces one-Transport-per-job, so a TS search no longer opens ~100 connections to a single cluster; teardown is wired into ARC's exit. Pipe mode now refuses a remote server, and says why, instead of deadlocking on a submission that can never land. Orca NEB resolves its reactant/product geometry paths on the executing machine.
Reuse check: searched
mainfor existing pooling or connection reuse (pool,reuse.*connect,persistent.*connect,ControlMasteracrossarc/job/ssh.pyandarc/job/adapter.py) — none exists, andSSHClient()is constructed in exactly one place (arc/job/ssh.py:371).arc/job/ssh_pool.pyis genuinely new rather than a third copy.Docker
Adds agent-socket forwarding and read-only key /
~/.arcmounts; fixes a PUID/PGID remap that aborted the container for most Linux desktop users; stops achmod o+rwthat was widening the user's live host agent socket; and adds a pre-flight that fails fast when a mounted settings overlay is unimportable, instead of letting ARC run silently against its dummyserver1.host.edufixtures.Structure
Six commits, no file touched by more than one — the two stories partition cleanly by path (SSH owns
arc/**+ARC.py; Docker ownsDockerfile,docker-compose.yml,dockerfiles/**,docs/**), with only documentation cross-references coupling them.Verification
Rebased onto
main(d033cbbf) from 107 behind, replayed with zero conflicts. The rebase and the subsequent 13→6 restructure were both checked by content identity, not just by tests: the changed-line multiset ofold-base..old-headversusnew-base..new-headis identical in both directions (2453 lines each), and the restructure left the tree bit-for-bit unchanged. Tests cannot prove a rebase didn't drop a hunk; this can.249 passedacrossssh_test,ssh_pool_test,pipe/,adapter_test,settings. The broaderarc/job/sweep shows the same 5 pre-existingtorch_ani_testenvironmental failures before and after — zero new.🤖 Generated with Claude Code