feat(ci): run e2e tests from the runner via Holodeck remoteAccess - #2718
feat(ci): run e2e tests from the runner via Holodeck remoteAccess#2718abrarshivani wants to merge 2 commits into
Conversation
8213db1 to
e5eddb3
Compare
rahulait
left a comment
There was a problem hiding this comment.
few nits, sorry forgot to submit review before
7ff0f9d to
18444e9
Compare
📝 WalkthroughWalkthroughThe E2E workflows now run case scripts on the GitHub Actions runner with runner-side kubeconfig access. Helm, kubectl, and jq versions are pinned. Node operations use a local-or-SSH dispatcher. Kubernetes checks use request timeouts, deadline-based polling, API failure handling, and periodic diagnostics. Driver update and migration checks dynamically query GPU nodes and tolerate diagnostic command failures. Test logs and cluster diagnostics are archived after each run. Merge Risk: 🔵 Low · up to The workflow change is mergeable with owner awareness that diagnostic collection can fail when the log directory path contains whitespace; quoting that path should prevent the bounded artifact-collection issue. Comment |
There was a problem hiding this comment.
Note
Quiet mode is enabled, so only the most important comments were posted inline. Other review comments are grouped below.
🟡 Other comments (1)
tests/scripts/checks.sh-135-135 (1)
135-135: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winQuote
log_dirwhen creating the log directory.When
log_dircontains whitespace, this command creates separate paths. Later artifact writes use the intended quoted path and fail. The|| truehandlers then hide the missing diagnostics.Proposed fix
- mkdir -p ${log_dir} + mkdir -p "${log_dir}"Source: Linters/SAST tools
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: QUIET
Plan: Team
Run ID: 8e7f88bc-3165-4ffa-9c15-35e5f16d86fc
📒 Files selected for processing (10)
.github/workflows/e2e-tests.yamltests/README.mdtests/holodeck.yamltests/scripts/.definitions.shtests/scripts/checks.shtests/scripts/migrate-clusterpolicy-to-nvidiadriver.shtests/scripts/node-exec.shtests/scripts/node-operations.shtests/scripts/update-clusterpolicy.shtests/scripts/update-nvidiadriver.sh
Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review.
18444e9 to
f8a374c
Compare
Holodeck's kubernetes.remoteAccess hands the GitHub Actions runner a
usable kubeconfig, so helm, kubectl, the case scripts and log collection
all run there. The repository rsync, the scp of the values override, the
in-VM tooling installs and the pull.sh log retrieval are gone.
Two operations mutate the host and stay on the node: the modprobe of
i2c_core and ipmi_msghandler, and the operator container kill used by the
restart test. Both go through a new tests/scripts/node-exec.sh, which
streams the self-contained node-operations.sh over SSH stdin. With
NODE_SSH_HOST unset it runs the operation locally, which keeps the
developer path documented in tests/README.md working; CI asserts the
variable is set so a node operation can never land on the shared runner.
Moving off the node changes what a kubectl call costs and how it fails,
which exposed several long-standing assumptions in the shell suite:
- The polling loops bounded themselves by counting sleeps rather than
elapsed time. On the node an iteration cost about 5s so the two
agreed; across the network they do not, and a nominal 45 minute wait
could outlive the 90 minute job. Eighteen loops now measure elapsed
time against a deadline.
- A failed query was being read as a satisfied condition. kubectl piped
into wc -l or jq reports 0 when the API is unreachable, which read as
"deleted" or "all owned", and a for loop over a failed query iterated
zero times and reported success without checking anything. Queries
now separate "the query failed" from "the condition is not met" and
retry instead of passing.
- kubectl applies no per-request timeout by default, so a hung request
could run past a deadline that is only tested between commands.
Polling and diagnostic calls are bounded now, with a larger budget
for log and object dumps so the artifacts are not truncated.
- The artifact upload moved from failure() to always(), since a job
stopped by timeout-minutes is cancelled rather than failed and would
otherwise upload nothing.
helm, kubectl and jq are installed on the runner at pinned versions, with
jq checksummed against its release manifest. The SSH key is written under
RUNNER_TEMP rather than the workspace and is removed along with the
kubeconfig in an always() step.
tests/local.sh, ci-run-e2e.sh, push.sh, pull.sh, sync.sh, remote.sh and
prerequisites.sh are kept because they are the documented developer path.
CI simply stops calling them.
Signed-off-by: Abrar Shivani <ashivani@nvidia.com>
f8a374c to
cc1cfb0
Compare
A containerd run lost the API server twenty seconds after the driver pod began reloading host kernel modules, and never got it back: 45 minutes of retries, and the post-run diagnostics still could not connect. Two very different causes fit that evidence equally well. Either the node lost its network during the module reload, or the runner's egress address changed and no longer matches the security group rule Holodeck opened for it. The logs cannot separate them because nothing touches the node after the kernel modules are loaded, so a silent API server is the only symptom we ever see. Record the egress address while things still work, then on every run report it again alongside a direct readyz check, an ssh probe of the node on port 22, and the instance state from EC2. Whichever of those still answers tells us which side broke. It runs on always() so a passing run leaves a baseline to compare a failing one against, and every probe is bounded and swallowed so it cannot change the result of the job. Signed-off-by: Abrar Shivani <ashivani@nvidia.com>
Runs the Kubernetes-facing parts of the e2e suite on the GitHub Actions runner instead of inside the Holodeck EC2 node. Draft until CI has run against it.
What changes
kubernetes.remoteAccess(NVIDIA/holodeck#818) gives the runner a usable kubeconfig, so helm, kubectl, the case scripts and log collection all run there. The repo rsync, thescpof the values override, the in-VM tooling installs andpull.share gone.Two host-mutating operations stay on the node:
sudo modprobe -a i2c_core ipmi_msghandler, and the operator container kill intest_restart_operator. Both go through a newtests/scripts/node-exec.sh, which streams the self-containednode-operations.shover SSH. WithNODE_SSH_HOSTunset it runs locally, so the developer path still works. Container-selection logic is moved verbatim.This is not a revert. Unlike NVIDIA/k8s-device-plugin#1948, the rsync + ssh model here is the original 2021 design for laptop-driven testing, and CI was already using it on Holodeck v0.2.18, before the kubeconfig permission change.
local.sh,ci-run-e2e.sh,push.sh,pull.sh,sync.sh,remote.shandprerequisites.share kept, since they are the documented developer workflow intests/README.md. CI just stops calling them.Polling loops, worth a close look
The loops in
checks.shandupdate-nvidiadriver.shbounded themselves by counting sleeps rather than elapsed time. That held when an iteration cost about 5s on the node. From the runner it does not: the 45 minute bound stretches past the job's 90 minute cap, and a cancelled job skipped thefailure()-gated artifact upload, so a hung run produced no logs at all.Twelve loops now measure elapsed time with
SECONDSagainst a baseline taken at loop entry. Bounds, messages and exit paths are unchanged. Inwait_for_driver_upgrade_donethecurrent_time % 30debug cadence became a due-time check, since irregular elapsed jumps can miss a modulo window entirely.check_gpu_pod_readywas re-fetching every container's full log for every pod every 5s; collection now runs on a 30s cadence, forced before both exit paths, while readiness polling stays at 5s. The artifact upload moves toalways().Still not enforceable: a job runs roughly 31 of these loops at 45 minutes each against a 90 minute cap, so two slow ones back to back can exhaust it. That needs shorter budgets or a shared deadline, which felt like a separate discussion.
Testing
Both e2e jobs pass against real Holodeck infrastructure. The run exercised the
whole new path end to end: the OCI chart published and installed, the runner
read the remoteAccess kubeconfig and reached the API server,
node-exec.shloaded the kernel modules over SSH, and the operator restart test killed the
container on the node with
crictlwhile the runner watched it come back.Diagnostics, artifact upload and credential cleanup all ran. Artifacts came out
at roughly 735 KB and 336 KB.
Not exercised yet:
use_values_override: true, which is the one changed branchthe run skipped, and the timeout/cancellation path that
always()exists for.Locally, shellcheck reports no new findings on any modified file, and the
wall-clock conversions were checked against stubs — the old code overran a 10s
budget by 3.8x where the new code exits on time.