test(e2e): expand native-host and k8s coverage for untested subsystems - #579
Open
biluriuday wants to merge 6 commits into
Open
test(e2e): expand native-host and k8s coverage for untested subsystems#579biluriuday wants to merge 6 commits into
biluriuday wants to merge 6 commits into
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #579 +/- ##
==========================================
+ Coverage 76.67% 76.69% +0.02%
==========================================
Files 169 169
Lines 68228 68490 +262
==========================================
+ Hits 52307 52524 +217
- Misses 15921 15966 +45 🚀 New features to boost your workflow:
|
Contributor
There was a problem hiding this comment.
Pull request overview
This PR substantially expands Spur’s end-to-end test coverage across both the native-host and Kubernetes fixtures, filling previously uncovered subsystems (REST, metrics, FFI, SPANK, quota, multi-controller Raft HA, images, WireGuard, topology, burst buffer, etc.) and enhancing the e2e harness to support those scenarios.
Changes:
- Switch pytest import mode to
importlibto avoid same-basename module collisions between native-host and k8s suites. - Add a broad set of new native-host e2e tests covering scheduling/placement, HA Raft behavior, REST/metrics, resource limits, plugins/FFI, and multiple CLI behaviors.
- Extend the k8s e2e fixture and manifests (ports/RBAC/Postgres) and add new k8s e2e suites for spec coverage, failure modes, quota projection, and operator health.
Reviewed changes
Copilot reviewed 38 out of 38 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/pytest.ini | Use --import-mode=importlib to prevent test module name collisions across suites. |
| tests/native_host/e2e/test_topology.py | New topology-aware placement e2e coverage. |
| tests/native_host/e2e/test_task_layout.py | New coverage for task defaults and node/task capping behavior. |
| tests/native_host/e2e/test_spank.py | New SPANK plugin loading/hook/env semantics coverage. |
| tests/native_host/e2e/test_scheduling_placement.py | New coverage for partitions OR-lists, nodelist/nodefile selection, spread, pending reasons. |
| tests/native_host/e2e/test_rest_api.py | New REST API route coverage (read/write, filters, follower semantics). |
| tests/native_host/e2e/test_resource_limits.py | New coverage for cgroup limits, OOM detection, CPU env limits, memlock rlimit. |
| tests/native_host/e2e/test_readonly_cli.py | New smoke coverage for read-only Slurm-compatible CLIs. |
| tests/native_host/e2e/test_raft_ha.py | New multi-controller Raft HA e2e coverage (elections, forwarding, failover, quorum loss). |
| tests/native_host/e2e/test_pty.py | New interactive/PTY and attach/overlap e2e coverage. |
| tests/native_host/e2e/test_net_mesh.py | New spur net (WireGuard mesh) coverage with mutating tests gated by env var. |
| tests/native_host/e2e/test_metrics.py | New controller OpenMetrics endpoint coverage + high-cardinality gating coverage. |
| tests/native_host/e2e/test_licenses.py | New coverage for license resource contention and request forms. |
| tests/native_host/e2e/test_job_ownership.py | New coverage for interactive ownership enforcement (spur exec, sattach, streaming). |
| tests/native_host/e2e/test_image.py | New spur image local directory management + job integration coverage. |
| tests/native_host/e2e/test_hetjob.py | New coverage for heterogeneous component submission semantics and non-dropping invariant. |
| tests/native_host/e2e/test_ffi.py | New Slurm-compatible FFI e2e coverage using a compiled C smoke driver. |
| tests/native_host/e2e/test_dispatch_resilience.py | New multi-node dispatch/launch failure resilience coverage (requeue, GPU release, etc.). |
| tests/native_host/e2e/test_controller_failover.py | Update docs to reference the new Raft HA module. |
| tests/native_host/e2e/test_cli_queries.py | New read-side CLI formatting/filter/sorting coverage + filter-based scancel. |
| tests/native_host/e2e/test_cli_env_defaults.py | New coverage for submit-time env-var defaults for sbatch/salloc and QoS. |
| tests/native_host/e2e/test_burst_buffer.py | New burst buffer capacity + staging wrapper semantics coverage. |
| tests/native_host/e2e/fixtures/spank_test.c | New C SPANK fixture used by e2e suite. |
| tests/native_host/e2e/fixtures/ffi_smoke.c | New C FFI smoke driver compiled/used by e2e suite. |
| tests/native_host/e2e/conftest.py | Add new native-host fixtures (metrics_cluster, raft_cluster, spank_cluster). |
| tests/k8s/e2e/test_spurjob.py | Add additional SpurJob lifecycle and multinode behaviors + cleanup assertions. |
| tests/k8s/e2e/test_spurjob_spec.py | New SpurJob CRD spec→PodSpec field propagation coverage. |
| tests/k8s/e2e/test_spurjob_failure_modes.py | New k8s failure-mode coverage (OOMKilled, image pull failures, restart/orphan cleanup). |
| tests/k8s/e2e/test_quota.py | New quota projection e2e coverage (Namespace/Quota/LimitRange/RBAC + drift correction). |
| tests/k8s/e2e/test_operator_health.py | New operator health/readiness/metrics coverage via apiserver service proxy. |
| tests/k8s/e2e/test_node_watcher.py | New node watcher coverage (registration + opt-in destructive health/removal tests). |
| tests/k8s/e2e/manifests/spurctld.yaml | Expose REST and metrics ports in the k8s controller fixture. |
| tests/k8s/e2e/manifests/rbac.yaml | Add RBAC needed for quota projection (namespaces, quotas, RBAC escalate/bind, etc.). |
| tests/k8s/e2e/manifests/postgres.yaml | Add ephemeral Postgres deployment/service for accounting-backed suites (quota). |
| tests/k8s/e2e/k8s_cluster.py | Extend fixture config (quota + postgres), apply manifests with mutation hooks, add helpers. |
| tests/k8s/e2e/conftest.py | Add quota_cluster fixture + include it in per-test cleanup. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
yansun1996
requested changes
Aug 7, 2026
yansun1996
left a comment
yansun1996
left a comment
Member
There was a problem hiding this comment.
- there are many CI test targets failure PTAL
- could you please also benchmarking the e2e total time change after this PR ?
biluriuday
force-pushed
the
e2es
branch
2 times, most recently
from
August 7, 2026 12:59
6c93af2 to
fc4506d
Compare
biluriuday
requested review from
powderluv,
sajmera-pensando and
sgopinath1
as code owners
August 7, 2026 12:59
biluriuday
force-pushed
the
e2es
branch
6 times, most recently
from
August 10, 2026 07:02
606943b to
badac55
Compare
Two gaps drove this: 65 of the last 120 feat/fix commits shipped without touching tests/, and whole subsystems (REST, metrics, FFI, SPANK, quota, multi-controller Raft, licenses, burst buffer, topology, images, WireGuard) had never had any e2e coverage at all. Harness lands first since everything depends on it. SpurCluster gains salloc, sattach, spur_exec, the read-only CLI wrappers, cli_with_env for submit-side env vars, curl-over-SSH HTTP helpers, plugstack and C fixture compilation, and per-node controller control so a three-controller Raft cluster can be driven. The k8s fixture gains assert_pod_spec, spec builders, operator restart and log access, and a quota variant. Three k8s subsystems turned out to be unreachable from the suite rather than merely untested: the operator manifest never passed --enable-quota, the ClusterRole lacked every resource the quota controller touches along with escalate/bind, nothing provided the Postgres that SlurmAccounting needs to serve accounts, and spurctld exposed neither the REST nor the metrics port. Import mode moves to importlib because the default prepend mode keys modules by basename, so the new native test_raft_ha.py collided with the k8s module of the same name and broke collection of the full suite. 218 -> 585 collected tests. Co-authored-by: Cursor <cursoragent@cursor.com>
Both bugs were found by the expanded e2e suite running against a real cluster. spurd read SPUR_BURST_BUFFER via std::env::var, which is the daemon's own environment, while the agent sets it in the job's environment. The two never met, so every stage_in:/stage_out: directive was silently dropped: a job with "stage_in:exit 7" completed with exit 0. Read it from cfg.environment instead. scancel declared --partition and --account, documented them, and passed them to get_jobs, but the guard deciding whether the caller named anything to cancel only looked at job IDs, --user and --name. Both flags therefore always failed with "no job IDs or filters specified". The predicate is now has_selection(), covered by unit tests. --state stays out: it narrows a selection rather than making one. Harness corrections: - Verify spurctld owns the controller port after start. A bind failure was silent, so a foreign process holding 6817 answered instead and tests failed far from the cause with "not the Raft leader". - Address Raft peers by IP with an explicit per-node node_id, so the suite no longer depends on every node resolving its peers' hostnames. Deriving the id from a hostname peer list keeps its own module, gated on a resolvability probe. - job_state() now knows DL and OOM. An unlisted code read as "no state yet" and turned correct behaviour into a polling timeout. - Add sinfo_node_names(), which uses -N so assertions survive the hostlist compression that renders three nodes as spur-node[1-3]. Tests realigned with behaviour the code actually promises: ownership denial runs as an unprivileged account rather than root (an admin override in check_job_owner), images are stored under the canonical OCI stem that remove resolves, srun --input is asserted to be ignored in step mode as it warns, and burst buffer failure cases use failing commands rather than `exit`, which would kill the wrapper shell. Drops the requeue test whose premise needs a second run attempt that scontrol requeue does not yet create. Reverts --import-mode=importlib; the colliding native module is renamed to test_controller_raft.py instead, leaving import behaviour untouched. Co-authored-by: Cursor <cursoragent@cursor.com>
The e2e suite added in the previous commit exposed three product defects alongside several harness assumptions that only held on the CI nodes. - spurd set memory.max but never capped swap, so a job that outgrew --mem was paged out rather than OOM-killed and never reached OUT_OF_MEMORY. Swap is now capped through a [cgroup] section mirroring Slurm's ConstrainSwapSpace and AllowedSwapSpace, defaulting off so deployed clusters keep the behavior they have today. - sattach and srun took the first comma-separated field of the nodelist as a hostname, which yields "node[1" once the controller compresses a multi-node allocation. Both now expand the hostlist first. - The k8s operator retried every failed submit, so a SpurJob the controller permanently rejected (unknown partition, denied account) sat without status forever. Permanent rejections are marked Failed; only transient codes requeue, matching the agent's controller-RPC rule. On the test side: expand hostlists before asserting placement, read cgroups from the node that actually ran the job, widen squeue's state column so the three-character OOM code is not clipped, capture smd's stderr summary, and skip rather than fail where the environment lacks wireguard-tools or hostname resolution between nodes. Verified on the 3-node bare-metal cluster, where the resource-limit suite passes 17/17 including the OOM cases that depend on the new knob. Co-authored-by: Cursor <cursoragent@cursor.com>
Three native-host failures in CI came from tests asserting against a node the scheduler never picked, or against a limit the host cannot apply. - The dispatch confirmation test read the first two nodes rather than the job's own NodeList, so it reported a node that was never allocated as having missed the launch. - The SPANK failing-hook test looked for a job-scoped log line on node 0 while the job ran elsewhere. Neighbouring assertions in that file pass because plugin-load lines appear on every agent at startup; only this one depends on placement. It now searches every agent's log. - The OOM tests assumed --mem is enforced whenever spurd runs as root. spurd enables the memory controller on its own cgroup root, which only takes effect if the parent delegated it, and containerised hosts commonly have not: spurd then warns and runs the job with no limit, so the hog completes and the job is never OOM-killed. A preflight now mirrors that same sequence and skips with the offending node named, matching how the file-reading tests in the class already degrade. NodeList parsing had been copied into three modules, so it moves to job_node_names/job_node_indices in the harness alongside the other job helpers. Also drops imports left unused by these changes. Verified on the 3-node bare-metal cluster: resource limits, topology and placement pass 39/39 with no skips, so the new preflight does not mask the OOM cases there. The SPANK fix was confirmed by experiment -- the test fails 5/5 with the previous code and passes 5/5 with this one. Co-authored-by: Cursor <cursoragent@cursor.com>
error_policy requeued every reconcile failure after a flat MAX_BACKOFF_SECS (60s), despite a comment claiming exponential backoff. kube-rs uses that Action verbatim, so the first transient error already cost a full minute: any controller blip or rolling restart added up to 60s of latency to an in-flight SpurJob submission. Retries now double from 1s up to the 60s cap, counted per SpurJob and reset on a clean pass. Cleanup reconciles take the same path, so entries are dropped when a SpurJob goes away. This is what broke test_state_survives_leader_failover in CI. It submits right after the preceding test deletes a controller pod, and the operator holds one long-lived channel pinned to a single pod of the headless spurctld Service, so a single Unavailable was enough to outlast the test's 60s wait. Alongside it, three e2e fixes: - metrics_cluster was defined without @pytest.fixture, so all 19 tests in test_metrics.py errored at setup while every neighbouring fixture resolved. - The OOM assertions now report the node's swap and any cgroup warnings spurd logged. spurd only warns when a limit fails to apply and runs the job unconstrained, which is invisible from the job state alone. - test_raft_ha.py grew wait_for_spur_job_id, replacing three duplicated closures and surfacing the SpurJob status when no ID ever arrives. A new e2e-fixtures CI job runs pytest --setup-plan over tests/, which resolves every fixture without starting a cluster. Removing the decorator above reproduces the same 18 errors in under a second instead of an hour into the E2E run; plain collection does not catch it. E2E gates on the CI workflow's conclusion, so this blocks both suites. Tested: cargo test -p spur-k8s (152 passed) and clippy clean; the full test_resource_limits.py and test_metrics.py pass on the bare-metal cluster (36 passed), where the OOM tests were already green. Co-authored-by: Cursor <cursoragent@cursor.com>
Native-host e2e ran as one ~60 min job; k8s as one ~40 min job. Both are now partitioned by pytest markers so CI can run them in parallel. - native-host: fanned into six suite matrix jobs (scheduling, policy, api, runtime, fabric, ha), each on its own ephemeral 4-VM cluster. RUN_DIR, the VM prefix, and the results artifact are scoped per suite so the jobs never collide on the shared run id. - k8s: five suites (core, spec, quota, ha, nodes) run concurrently on one bootstrapped cluster, each in its own namespace via tests/k8s/e2e/run_suites.sh. Safe because destructive node tests stay skipped, so no suite mutates shared cluster-scoped Nodes. - Every e2e test file carries exactly one suite_* / suite_k8s_* marker. A collection guard in each conftest and a parity check in the e2e-fixtures job fail fast if a file has zero or two markers, so none can silently escape a suite. - Documented the marker rule and the node-mutation guardrail in AGENTS.md. Co-authored-by: Cursor <cursoragent@cursor.com>
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
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.
Summary
This PR closes a long-standing e2e gap — 65 of the last 120 feat/fix commits shipped without touching
tests/, and whole subsystems had never had any e2e coverage — then fixes the product bugs that the new coverage exposed, and finally restructures the suite so it runs in parallel. Collected tests go from 218 → 585.The commits are ordered so each builds on the last: harness first, then the tests, then the fixes those tests surfaced, then the CI split.
1. Harness + coverage expansion (
52bf35e)The harness had to grow before the subsystems could be tested:
SpurClustergainssalloc,sattach,spur exec, read-only CLI wrappers,cli_with_env(submit-side env vars), curl-over-SSH HTTP helpers, plugstack + C-fixture compilation, and per-node controller control so a 3-controller Raft cluster can be driven.assert_pod_spec, spec builders, operator restart/log access, and a quota variant.New coverage for previously untested surfaces:
Three k8s subsystems turned out to be unreachable from the suite rather than merely untested, so the manifests were fixed too: the operator never passed
--enable-quota, the ClusterRole lacked the quota controller's resources plusescalate/bind, nothing provided the Postgres thatSlurmAccountingneeds, andspurctldexposed neither the REST nor the metrics port.2. Product bugs surfaced by the new coverage
Real defects the suite caught against a live cluster:
6a73ddb) —spurdreadSPUR_BURST_BUFFERfrom its own process env instead of the job env, so everystage_in:/stage_out:was a no-op (a job withstage_in:exit 7completed with exit 0). Now read fromcfg.environment.scancelfilters always errored (6a73ddb) —--partition/--accountwere declared and passed toget_jobs, but the "did the caller select anything?" guard only checked job IDs/--user/--name, so both flags failed with "no job IDs or filters specified". Replaced with ahas_selection()predicate (unit-tested).--memoverruns paged out instead of OOM-killing (760ebb5) —spurdsetmemory.maxbut never capped swap, so a job that outgrew its limit never reachedOUT_OF_MEMORY. Swap is now capped via a[cgroup]section mirroring Slurm'sConstrainSwapSpace/AllowedSwapSpace, defaulting off so deployed clusters keep today's behavior.760ebb5) —sattach/sruntook the first comma-separated field of the nodelist, yieldingnode[1once the controller compresses an allocation. Both now expand the hostlist first.760ebb5) — it retried every failed submit, so a SpurJob the controller permanently rejected (unknown partition, denied account) sat without status forever. Permanent rejections are now markedFailed; only transient codes requeue.f7b7a63) —error_policyrequeued after a flat 60s despite a comment claiming exponential backoff; kube-rs uses thatActionverbatim, so the first transient blip cost a full minute of submission latency. Now doubles 1s→60s cap, per-SpurJob, reset on a clean pass (cleanup reconciles take the same path). This is what broketest_state_survives_leader_failoverin CI.3. Test/harness robustness (
bfdd00b,f7b7a63)Several failures were the tests' fault, not the product's — they assumed placement or enforcement the environment didn't guarantee:
NodeList(extracted into sharedjob_node_names/job_node_indiceshelpers) instead of guessing the first N nodes.spurduses and skip (naming the node) on hosts where the memory controller isn't delegated — instead of failing where--memcan't be enforced.wireguard-toolsor inter-node hostname resolution.Verified on the 3-node bare-metal cluster: resource limits + topology + placement 39/39 with no skips (so the preflight doesn't mask genuine OOM cases there), and the SPANK fix flips that test from 5/5 failing to 5/5 passing.
New
e2e-fixturesCI job (f7b7a63): runspytest --setup-planovertests/, resolving every fixture without starting a cluster. It catches things like a fixture missing its@pytest.fixturedecorator (which had errored 19test_metrics.pytests at setup) in under a second instead of an hour into the E2E run. E2E gates on this workflow's conclusion.4. Parallelizing e2e (
f885805)native-host ran as one ~60 min job and k8s as one ~40 min job. Both are now partitioned by pytest markers so CI runs them in parallel:
scheduling,policy,api,runtime,fabric,ha), each on its own ephemeral 4-VM cluster.RUN_DIR, the VM prefix, and the results artifact are scoped per suite so the jobs never collide on the shared run id.core,spec,quota,ha,nodes) run concurrently on one bootstrapped cluster, each in its own namespace viatests/k8s/e2e/run_suites.sh. Safe because the destructive node tests stay skipped, so no suite mutates shared cluster-scopedNodes.suite_*/suite_k8s_*marker. A collection guard in eachconftestplus a parity check ine2e-fixturesfail fast if a file has zero or two markers, so a new test can't silently escape a suite. The marker rule and the node-mutation guardrail are documented inAGENTS.md.Addressing the review feedback
6a73ddb,760ebb5,bfdd00b, andf7b7a63.workflow_run, which GitHub always evaluates from the default branch, so the parallel split only takes effect after this PR merges. The before number is visible now (native-host ~60 min single job, k8s ~37 min single job); the after (per-suite parallel) numbers will land on the firstmainrun and I'll post them here. Target is ≤15 min per suite; if any suite overshoots we rebalance markers, which is a one-line change per file.