feat(ci): run e2e tests from the runner via Holodeck remoteAccess - #2718
Open
abrarshivani wants to merge 14 commits into
Open
feat(ci): run e2e tests from the runner via Holodeck remoteAccess#2718abrarshivani wants to merge 14 commits into
abrarshivani wants to merge 14 commits into
Conversation
Enable kubernetes.remoteAccess in tests/holodeck.yaml so Holodeck hands the GitHub Actions runner a kubeconfig for the test cluster, and rework the e2e workflow to use it. The case scripts now run on the runner instead of being rsynced to the EC2 instance and driven over SSH, which removes the scp of the values override file, the ci-run-e2e.sh/local.sh/push.sh/pull.sh chain and the key.pem written into the workspace. SSH is still needed for the two host-mutating operations, so both jobs write the key under RUNNER_TEMP, export NODE_SSH_HOST/NODE_SSH_KEY/ NODE_SSH_KNOWN_HOSTS for tests/scripts/node-exec.sh, and delete the key directory at the end of the job. helm, kubectl and jq are now installed on the runner at pinned versions. helm was previously installed on the instance by tests/scripts/prerequisites.sh from the get-helm-3 master script, so it was whatever release happened to be current; pinning it is a deliberate change. kubectl is pinned to the Kubernetes version in tests/holodeck.yaml. Also add a preflight step that fails the job early if the kubeconfig is unusable, and an always() step that dumps nodes, pods, events and helm releases into the log directory so the existing artifacts are more useful. tests/local.sh and friends are unchanged and remain the documented developer path. Signed-off-by: Abrar Shivani <ashivani@nvidia.com>
The e2e tests are moving to run against the cluster from the GitHub Actions runner instead of over SSH on the node. Two operations still have to run on the node itself: loading the i2c_core and ipmi_msghandler kernel modules, and killing the gpu-operator container in the restart test. Add node-operations.sh, which implements both operations and is self-contained so that it can be streamed to the node over SSH stdin, and node-exec.sh, which dispatches an operation over SSH when NODE_SSH_HOST is set and runs it locally otherwise so that the existing developer workflow keeps working. test_restart_operator now calls node-exec.sh instead of running crictl or docker inline. The container selection logic is unchanged, except that an empty container ID is now reported as an error instead of being handed to the removal command. Signed-off-by: Abrar Shivani <ashivani@nvidia.com>
node-exec.sh deliberately falls back to running the operation locally when NODE_SSH_HOST is empty, which is what the developer path relies on. In CI that fallback would run modprobe and crictl against the shared self-hosted runner instead of the test instance, so guard both call sites with a non-empty check on NODE_SSH_HOST and fail the job loudly. Also from review: - verify the downloaded jq binary against the sha256 published in the jq 1.7.1 release checksum file before putting it on PATH - fail early if the public-dns-name lookup came back empty, rather than building NODE_SSH_HOST=ubuntu@ and getting an opaque ssh error later - add || true to the mkdir in the diagnostics step so a green job cannot be turned red by diagnostics - remove the kubeconfig alongside the SSH key in the always() cleanup, since it holds cluster-admin credentials - use GITHUB_WORKSPACE instead of interpolating github.workspace into a run block, matching how the rest of the job passes values through env Signed-off-by: Abrar Shivani <ashivani@nvidia.com>
The polling loops in checks.sh bounded themselves with a counter that was incremented by 5 on every iteration, on the assumption that an iteration costs only the 5 second sleep. That has been close enough while the tests ran on the node itself, but each iteration also issues a number of kubectl calls, and once those calls cross a network the iteration takes considerably longer than 5 seconds. The counter then runs slower than the clock and the nominal 45 minute bound stretches to several hours, which is long enough for the job timeout to cancel the run before any of the loops give up on their own. Measure elapsed time with the SECONDS builtin against a baseline taken when the loop starts, so the bound means what it says regardless of how long an iteration takes. The 45 minute budget itself is unchanged. wait_for_driver_upgrade_done printed its debug dump when the counter was divisible by 30. Elapsed time no longer advances in fixed steps, so that test can step over every multiple and the dump would never be printed. Track the time at which the next dump is due instead. Also pass --tail to the per-pod log fetch in check_gpu_pod_ready. It runs inside the poll loop for every pod in every namespace and refetches each complete log every five seconds, which is a lot of traffic to repeat for up to 45 minutes. The log collection on failure is left untouched. Signed-off-by: Abrar Shivani <ashivani@nvidia.com>
…ng them check_gpu_pod_ready regenerates a describe and a log file for every pod in every namespace on each pass of its five second poll loop. Passing --tail to bound that traffic was the wrong call: the log file is overwritten rather than appended, so the file left behind when the loop gives up is the one that gets uploaded as the failure artifact, and truncating it drops exactly the output that explains a failed driver build. Fetch the whole log again and instead regenerate on a thirty second cadence, which cuts the traffic by the same order without shortening anything. The readiness check keeps running every five seconds so success is still noticed promptly. Both the timeout and the success path collect once more on the way out so the files on disk are current rather than up to thirty seconds old. update-nvidiadriver.sh has seven loops with the same counter-based timeout that checks.sh had, guarding the nvidiadriver test that runs in the same job under the same job timeout. Convert them the same way. wait_for_nvidiadriver_owner is the worst of them: its counter needs 181 iterations to reach a fifteen minute bound, which is half an hour of wall clock once each iteration waits on a round trip. Signed-off-by: Abrar Shivani <ashivani@nvidia.com>
The archive step was gated on failure(), but a job stopped by timeout-minutes is cancelled rather than failed, so failure() evaluates false and the upload is skipped. That loses the logs on exactly the runs that are hardest to diagnose. Now that the e2e tests run from the runner, the polling loops in tests/scripts talk to the API server over the internet rather than over loopback, so a job is more likely to reach the 90 minute cap than it was when everything ran on the node. Switch both jobs to always() so a cancelled run still produces artifacts. The step still runs before the credential cleanup and still uploads only ./logs/, so neither the kubeconfig nor the SSH key can end up in the artifact. Signed-off-by: Abrar Shivani <ashivani@nvidia.com>
Both e2e jobs died in wait_for_driver_upgrade_done. The driver upgrade restarts the container runtime on the node, and now that the tests drive the cluster from the runner rather than from the node itself, the API server is briefly unreachable across the public address instead of on localhost. The first casualty was the opening kubectl in print_driver_upgrade_debug, which unlike its three siblings had no guard, so a debug dump ended the run. Guard it, and make the wait itself tolerate the outage: the node count and the per-node upgrade state are now read through checks that treat a failure as "not upgraded yet" and retry until the existing wall-clock deadline expires. A count that could not be read stays empty rather than defaulting to zero, so an unreachable API can never be mistaken for a finished upgrade. Failures are announced with a timestamp so the next run shows how long such an outage lasts, which this one died too quickly to reveal. The calls in these two functions also carry an explicit request timeout, since the default behaviour was to spend thirty seconds per call discovering that the address was black-holed. The same shape exists elsewhere: a kubectl whose output only exists for a human to read, usually just before exit 1, sitting unguarded next to siblings that already end in || true. Guard those too, in the readiness and log collection loops in checks.sh and in the timeout dumps in update-clusterpolicy.sh, migrate-clusterpolicy-to-nvidiadriver.sh and update-nvidiadriver.sh. Assertions are left alone: a check that cannot reach the API still fails the test. Signed-off-by: Abrar Shivani <ashivani@nvidia.com>
Store a deadline rather than a start time in the polling loops. Comparing SECONDS against a deadline computed once says what the loop means without the subtraction, and it removes the comment each loop was carrying to explain that SECONDS counts from shell start rather than from the loop. migrate-clusterpolicy-to-nvidiadriver.sh had six loops still counting sleeps instead of measuring time, the same bug already fixed in checks.sh and update-nvidiadriver.sh. It runs on the containerd path, so convert those too. check_gpu_pod_ready listed every pod in the cluster as json on every pass of a five second loop, but only used the result when it regenerated the log files every thirty seconds. Fetch it where it is used, ask for two custom columns instead of the whole object, and read it with the shell rather than a jq invocation per pod. The same loop printed the pod table once for the console and fetched it again for the log file, which tee does in one call. The request timeout default now lives in .definitions.sh with the other defaults instead of being repeated at each use. Also guard five more diagnostic calls that the earlier pass missed, replace the runtime comparison in node-operations.sh with a case statement, and drop the argument and readability checks in node-exec.sh that only repeat what node-operations.sh and the redirect already report. Signed-off-by: Abrar Shivani <ashivani@nvidia.com>
abrarshivani
force-pushed
the
ci-e2e-remote-access
branch
from
August 24, 2026 18:36
8213db1 to
e5eddb3
Compare
Now that the tests drive the cluster from the runner, a kubectl call can fail at any point, and several checks turned that failure into a value that happens to satisfy what they were waiting for. Counting through a pipe reports zero when kubectl writes nothing, so a deletion check saw the resource as gone and an ownership check saw every node as owned. Iterating over a command substitution runs the body no times, so the label check in test_custom_labels_override could report success without reading a single label. Testing a bare kubectl for non-zero treats unreachable the same as NotFound. Ask whether the query succeeded before reading what it said. A query that failed reports through api_unreachable and falls through to the retry, and the loops still stop on their existing deadlines. Deletion checks use -o name with --ignore-not-found so an absent object is a successful query returning nothing, which is a real answer, rather than an error. Comparisons against a node count also require the count to be above zero. This environment always has a GPU node, so zero means the count never arrived, and without that guard a zero on both sides of the comparison reads as done. The one-shot assertions fail when their query fails rather than retrying, since there is no loop around them and a check that could not run has not passed. Signed-off-by: Abrar Shivani <ashivani@nvidia.com>
Keeps the licence headers and the handful of notes that stop a reader undoing something deliberate: that node-operations.sh has to stay self-contained because it is streamed over SSH stdin, that the log files are the uploaded artifact and so must not be tailed, that the GPU node count is resolved once because the upgrade churns node labels, and that node-exec.sh runs locally when NODE_SSH_HOST is empty. The rest described what the line below it already said, or recorded design rationale that belongs in the commit history. Signed-off-by: Abrar Shivani <ashivani@nvidia.com>
SCRIPT_DIR and NODE_OPERATIONS are fixed once at startup and nothing should reassign them. Declared separately from the assignment because readonly with a command substitution swallows its exit status. Signed-off-by: Abrar Shivani <ashivani@nvidia.com>
actionlint's shellcheck pass flagged the three consecutive appends (SC2129). Grouping them also makes it clear they are one unit of work. Signed-off-by: Abrar Shivani <ashivani@nvidia.com>
The count of GPU nodes is resolved once and kept, because the upgrade churns node labels and a per-iteration count could sample a transient value and match early. The guard for that tested the cached value for emptiness, which only holds when the query failed. A successful query returning zero, which is what happens when no node carries the label yet, was cached like any other answer, and the loop then compared against a frozen zero for the rest of its budget. Cache only a positive count, so an API error and a zero both retry. Give every kubectl call that a polling loop depends on an explicit request timeout. kubectl defaults to no per-request timeout, and a wall clock deadline is only tested between commands, so one hung request could run past it for as long as the connection stayed open. Drop the remaining explanatory comments. Signed-off-by: Abrar Shivani <ashivani@nvidia.com>
A diagnostic that hangs stalls its loop exactly as a decision-driving query would, and kubectl has no per-request timeout by default, so the wall clock deadline never gets a chance to be tested. Bound the progress prints, the log and describe collection, and the state dumps on the timeout paths. The status poll in test_restart_operator is bounded for the same reason, even though that loop counts iterations rather than time; the container selection and kill are untouched. The existing 15s default suits a listing or a status field, not a fetch that carries a whole object or a container log. The driver build log runs to megabytes and now crosses the WAN, and bounding it at 15s would leave a truncated artifact, which is the failure the log collection exists to prevent. Add KUBECTL_LOG_TIMEOUT at 120s and use it for logs, describe, and full object dumps, keeping the short timeout for the cheap queries. Signed-off-by: Abrar Shivani <ashivani@nvidia.com>
abrarshivani
marked this pull request as ready for review
August 25, 2026 01:55
abrarshivani
requested review from
cdesiniotis,
karthikvetrivel,
rahulait,
rajathagasthya,
shivamerla and
tariq1890
as code owners
August 25, 2026 01:55
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.
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.