feat(ci): run e2e tests from the runner via Holodeck remoteAccess - #2718
Draft
abrarshivani wants to merge 8 commits into
Draft
feat(ci): run e2e tests from the runner via Holodeck remoteAccess#2718abrarshivani wants to merge 8 commits into
abrarshivani wants to merge 8 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>
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
Not yet run against real infrastructure. Locally, shellcheck is clean on the new scripts with no new findings on the modified ones, and the wall-clock conversions were checked against stubs: the old code overran a 10s budget by 3.8x, the new code exits on time.