From 6cd543f78fd23c7c991e7cdd5098aaed839a55e7 Mon Sep 17 00:00:00 2001 From: Riley Rice Date: Fri, 11 Sep 2026 12:25:22 -0700 Subject: [PATCH 1/6] ci(agent): run the operator-agent suite against the Go agent too The operator-agent chainsaw suite is the operator-side contract for agent behaviour -- log format, flag file paths, history shape, interrupt idempotence, log retention, dont_write_logs. Unit tests cannot catch drift in any of them, so the suite is the parity safety net for the Python-to-Go cutover (#221). Run it against the agent-go image in agent-go-ci.yaml, mirroring agent-ci.yaml's existing operator-agent-tests job. The scenarios are shared rather than forked: a scenario is behaviour the operator depends on, not something each agent implementation gets its own copy of. Which agent a PR exercises follows the paths it touches -- agent/** runs Python, agent/go/** runs Go, and a change to k8s-tests/operator-agent/** runs both, since it edits the shared contract. Package pods are created with ImagePullPolicy: PullAlways (job_builder.go), so the image has to be pullable rather than kind-loaded. agent-go-ci therefore gains the push and create-manifest steps agent-ci already has. Signing and provenance attestation are deliberately not copied: those are gated on refs/tags/agent/ and agent-go has no release tags yet. Add a shared dump-operator-agent-diagnostics action wired into both jobs. It dumps agent pod logs plus the on-node /etc/skyhook and /var/log/skyhook trees through the privileged debugger pod setup.sh already creates. A parity failure is only actionable if both sides are diagnosed from the same evidence. Does not flip the operator default to the Go image; that is #222. Refs #221 Signed-off-by: Riley Rice --- .../action.yml | 115 +++++++++++ .github/workflows/agent-ci.yaml | 4 + .github/workflows/agent-go-ci.yaml | 192 ++++++++++++++++++ docs/contributing/ci-test-pools.md | 23 ++- 4 files changed, 331 insertions(+), 3 deletions(-) create mode 100644 .github/actions/dump-operator-agent-diagnostics/action.yml diff --git a/.github/actions/dump-operator-agent-diagnostics/action.yml b/.github/actions/dump-operator-agent-diagnostics/action.yml new file mode 100644 index 00000000..9ab10ee3 --- /dev/null +++ b/.github/actions/dump-operator-agent-diagnostics/action.yml @@ -0,0 +1,115 @@ +# SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +# SPDX-License-Identifier: Apache-2.0 +# +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +name: Dump operator-agent diagnostics +description: > + Dump agent pod logs and the agent's on-node state after an operator-agent + chainsaw failure. Shared by the Python and Go suites so a parity failure is + diagnosed from the same evidence on both sides. + +inputs: + node: + description: Node whose agent state is dumped, via the debugger pod setup.sh creates. + required: false + default: kind-worker + +runs: + using: composite + steps: + # Every command is `|| true`-guarded and the step never fails: this runs + # under `if: failure()`, so a missing path here would replace the real + # chainsaw failure with a confusing one from the diagnostics themselves. + - name: Dump cluster state + shell: bash + env: + NODE: ${{ inputs.node }} + run: | + echo "::group::Nodes" + kubectl get nodes -o wide || true + echo "::endgroup::" + + echo "::group::Pods (all namespaces)" + kubectl get pods -A -o wide || true + echo "::endgroup::" + + echo "::group::NodeWright custom resources" + kubectl get nodewrights.nodewright.nvidia.com -o yaml || true + echo "::endgroup::" + + echo "::group::Node annotations (nodeState lives here, not on the CR)" + kubectl get node "$NODE" -o jsonpath='{.metadata.annotations}' | tr ',' '\n' || true + echo "::endgroup::" + + echo "::group::Recent events" + kubectl get events -A --sort-by=.lastTimestamp | tail -100 || true + echo "::endgroup::" + + - name: Dump agent pod logs + shell: bash + run: | + # Package pods are created per node/package; dump every container of each, + # including terminated ones, since the failing stage has usually exited. + for ns in $(kubectl get ns -o jsonpath='{.items[*].metadata.name}' || true); do + for pod in $(kubectl get pods -n "$ns" -o jsonpath='{.items[*].metadata.name}' 2>/dev/null || true); do + case "$pod" in + *-debugger) continue ;; + esac + echo "::group::logs ${ns}/${pod}" + kubectl logs -n "$ns" "$pod" --all-containers --prefix --timestamps --tail=-1 || true + echo "--- previous ---" + kubectl logs -n "$ns" "$pod" --all-containers --prefix --timestamps --previous --tail=-1 || true + echo "::endgroup::" + done + done + + - name: Dump agent on-node state + shell: bash + env: + NODE: ${{ inputs.node }} + run: | + # setup.sh leaves a privileged debugger pod on the node with the host + # root bind-mounted at /host, which is the only way to read the agent's + # flag, history and log files from inside the job. + DEBUGGER="${NODE}-debugger" + if ! kubectl get pod -n default "$DEBUGGER" >/dev/null 2>&1; then + echo "No ${DEBUGGER} pod; skipping on-node dump." + exit 0 + fi + + for dir in /host/etc/skyhook /host/var/log/skyhook; do + echo "::group::tree ${dir#/host}" + kubectl exec -n default "$DEBUGGER" -- ls -laR "$dir" || true + echo "::endgroup::" + done + + echo "::group::file contents under /etc/skyhook (flags + history)" + kubectl exec -n default "$DEBUGGER" -- \ + find /host/etc/skyhook -type f -exec sh -c 'echo "===== $1 ====="; cat "$1"' _ {} \; || true + echo "::endgroup::" + + echo "::group::agent log files under /var/log/skyhook" + kubectl exec -n default "$DEBUGGER" -- \ + find /host/var/log/skyhook -type f -exec sh -c 'echo "===== $1 ====="; cat "$1"' _ {} \; || true + echo "::endgroup::" + + - name: Dump operator logs + shell: bash + run: | + # The suite runs the operator via `make run` as a background process on + # the runner, not as a pod, so its log is a file rather than kubectl logs. + echo "::group::operator manager stdout" + cat operator/reporting/int/std.out || true + echo "::endgroup::" diff --git a/.github/workflows/agent-ci.yaml b/.github/workflows/agent-ci.yaml index 43abd950..f9e2608e 100644 --- a/.github/workflows/agent-ci.yaml +++ b/.github/workflows/agent-ci.yaml @@ -392,6 +392,10 @@ jobs: make build-cli make setup-kind-cluster operator-agent-tests + - name: Dump diagnostics on failure + if: failure() + uses: ./.github/actions/dump-operator-agent-diagnostics + # See operator-ci.yaml's ci-gate for rationale. operator-ci, agent-ci, # and lint-ci all publish a check named `ci-gate` — GitHub composes # same-named required checks, so every ci-gate that posts must pass. diff --git a/.github/workflows/agent-go-ci.yaml b/.github/workflows/agent-go-ci.yaml index bc357be3..d7ba8eb4 100644 --- a/.github/workflows/agent-go-ci.yaml +++ b/.github/workflows/agent-go-ci.yaml @@ -22,7 +22,10 @@ on: paths: - agent/go/** - containers/agent-go.Dockerfile + - k8s-tests/operator-agent/** - .github/actions/** + - operator/versions.yaml + - operator/versions.sh - scripts/latest-distroless.sh - .github/workflows/agent-go-ci.yaml push: @@ -31,7 +34,10 @@ on: paths: - agent/go/** - containers/agent-go.Dockerfile + - k8s-tests/operator-agent/** - .github/actions/** + - operator/versions.yaml + - operator/versions.sh - scripts/latest-distroless.sh - .github/workflows/agent-go-ci.yaml @@ -39,7 +45,10 @@ permissions: contents: read env: + REGISTRY: ghcr.io + IMAGE_NAME: ${{ github.repository }} DEBIAN_VERSION: trixie + PUSH_TO_REGISTRY: ${{ github.event_name != 'pull_request' || github.event.pull_request.head.repo.full_name == github.repository }} # Opt all JS actions into Node 24 ahead of GitHub's Node 20 phase-out. FORCE_JAVASCRIPT_ACTIONS_TO_NODE24: true @@ -72,6 +81,8 @@ jobs: outputs: git-sha: ${{ steps.meta.outputs.git-sha }} agent-version: ${{ steps.meta.outputs.agent-version }} + agent-image-tag: ${{ steps.meta.outputs.agent-image-tag }} + tags: ${{ steps.meta.outputs.tags }} steps: - name: Checkout repository uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 @@ -86,9 +97,16 @@ jobs: echo "git-sha=${GIT_SHA}" >> "$GITHUB_OUTPUT" AGENT_VERSION=$(git tag --list 'agent/*' --sort=-v:refname | head -n 1 | cut -d/ -f2)+${GIT_SHA} + # Convert + to - for docker tag compliance + AGENT_IMAGE_TAG=$(echo "${AGENT_VERSION}" | tr + -) + TAGS="${GIT_SHA} ${AGENT_IMAGE_TAG}" echo "agent-version=${AGENT_VERSION}" >> "$GITHUB_OUTPUT" + echo "agent-image-tag=${AGENT_IMAGE_TAG}" >> "$GITHUB_OUTPUT" + echo "tags=${TAGS}" >> "$GITHUB_OUTPUT" echo "📦 Agent Version: ${AGENT_VERSION}" + echo "🏷️ Image Tag: ${AGENT_IMAGE_TAG}" + echo "🏷️ All Tags: ${TAGS}" test: name: Agent Go Unit Tests @@ -156,11 +174,18 @@ jobs: runner: ubuntu-24.04-arm permissions: contents: read + packages: write steps: - name: Checkout repository uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 with: persist-credentials: false + - name: Log in to the Container registry + uses: docker/login-action@v4 + with: + registry: ${{ env.REGISTRY }} + username: ${{ github.actor }} + password: ${{ secrets.GITHUB_TOKEN }} - name: Set up Docker Buildx uses: docker/setup-buildx-action@37fe631027851001ddb9b187196cc803df7f5f0e # v4.3.0 - name: Build and smoke-test the agent-go container image @@ -174,6 +199,10 @@ jobs: IMAGE="nodewright-agent-go:${GIT_SHA}" set -x + # Unlike agent-ci's build-agent, this builds with --load rather than + # pushing directly: the --version smoke test below needs the image + # present locally, and buildx rejects --load together with --push. The + # platform tags are pushed afterwards, so GHCR ends up identical. docker buildx build \ --build-arg GIT_SHA=${GIT_SHA} \ --build-arg AGENT_VERSION=${AGENT_VERSION} \ @@ -193,6 +222,169 @@ jobs: exit 1 fi + - name: Push platform tags + env: + GIT_SHA: ${{ needs.compute-metadata.outputs.git-sha }} + PLATFORM: ${{ matrix.platform }} + run: | + if [ "${PUSH_TO_REGISTRY}" != "true" ]; then + echo "Fork PR build: image built and smoke-tested without pushing to registry" + exit 0 + fi + + PLATFORM_TAG=$(echo "$PLATFORM" | tr '/' '-') + IMAGE_NAME=$(echo "${{ env.IMAGE_NAME }}" | tr '[:upper:]' '[:lower:]') + REGISTRY=$(echo "${{ env.REGISTRY }}" | tr '[:upper:]' '[:lower:]') + + set -x + for TAG in ${{ needs.compute-metadata.outputs.tags }}; do + FULL_TAG="${REGISTRY}/${IMAGE_NAME}/agent-go:${TAG}-${PLATFORM_TAG}" + docker tag "nodewright-agent-go:${GIT_SHA}" "${FULL_TAG}" + docker push "${FULL_TAG}" + done + + # Create multi-platform manifest from individual architecture builds + create-manifest: + if: github.event_name != 'pull_request' || github.event.pull_request.head.repo.full_name == github.repository + name: Create Agent Go Manifest + runs-on: ubuntu-latest + needs: [compute-metadata, build-agent-go] + permissions: + contents: read + packages: write + steps: + - name: Checkout repository + uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + with: + persist-credentials: false + + - name: Log in to the Container registry + uses: docker/login-action@v4 + with: + registry: ${{ env.REGISTRY }} + username: ${{ github.actor }} + password: ${{ secrets.GITHUB_TOKEN }} + + - name: Set up Docker Buildx + uses: docker/setup-buildx-action@37fe631027851001ddb9b187196cc803df7f5f0e # v4.3.0 + + # No cosign signing or provenance attestation here, unlike agent-ci's + # create-manifest: those steps are gated on refs/tags/agent/ and agent-go + # has no release tags yet. Add them when it is released, not before. + - name: Create manifests + run: | + IMAGE_NAME=$(echo "${{ env.IMAGE_NAME }}" | tr '[:upper:]' '[:lower:]') + REGISTRY=$(echo "${{ env.REGISTRY }}" | tr '[:upper:]' '[:lower:]') + + for TAG in ${{ needs.compute-metadata.outputs.tags }}; do + FULL_TAG="${REGISTRY}/${IMAGE_NAME}/agent-go:${TAG}" + echo "📦 Creating manifest for $FULL_TAG" + docker manifest create $FULL_TAG \ + ${FULL_TAG}-linux-amd64 \ + ${FULL_TAG}-linux-arm64 + docker manifest push $FULL_TAG + echo "✅ Pushed $FULL_TAG" + done + + echo "✅ Multi-platform manifests created successfully" + + # Parity safety net for the Go cutover: the same chainsaw suite the Python + # agent runs in agent-ci.yaml, pointed at the agent-go image. The suite is the + # operator-side contract, so it is deliberately shared rather than forked -- + # a change under k8s-tests/operator-agent/ triggers both workflows. + operator-agent-go-tests: + if: github.event_name != 'pull_request' || github.event.pull_request.head.repo.full_name == github.repository + name: Operator Agent Go Integration Tests + runs-on: ubuntu-latest + needs: [compute-metadata, create-manifest] + permissions: + contents: read + packages: read + steps: + - name: Checkout repository + uses: actions/checkout@v7 + with: + fetch-tags: true + fetch-depth: 0 + + - name: Set up Go + uses: actions/setup-go@v7 + with: + go-version-file: operator/go.mod + cache-dependency-path: operator/go.sum + + - name: Log in to the Container registry + uses: docker/login-action@v4 + with: + registry: ${{ env.REGISTRY }} + username: ${{ github.actor }} + password: ${{ secrets.GITHUB_TOKEN }} + + - name: Load Kubernetes test versions + id: k8s-versions + run: | + cd operator + make yq + YQ="$PWD/bin/yq" ./versions.sh --print >> "$GITHUB_OUTPUT" + + - name: Validate KinD node image + env: + K8S_VERSION: ${{ steps.k8s-versions.outputs.kind-nodeimage }} + run: | + cd operator + make validate-kind-node-image KIND_NODE_IMAGE_VERSION="$K8S_VERSION" + + - name: Create Kubernetes KinD Cluster + uses: helm/kind-action@v1.15.0 + with: + version: ${{ steps.k8s-versions.outputs.kind-binary }} + node_image: kindest/node:v${{ steps.k8s-versions.outputs.kind-nodeimage }} + config: operator/config/local-dev/kind-config.yaml + cluster_name: kind + + - name: Restore cached Binaries + id: cached-binaries + uses: actions/cache/restore@v6 + with: + key: ${{ runner.os }}-${{ runner.arch }}-bin-${{ hashFiles('operator/go.mod', 'operator/deps.mk', 'operator/versions.yaml', 'operator/versions.sh') }} + restore-keys: ${{ runner.os }}-${{ runner.arch }}-bin- + path: | + ${{ github.workspace }}/operator/bin + ~/.cache/go-build + + - name: Install dependencies + if: steps.cached-binaries.outputs.cache-hit != 'true' + run: | + cd operator + make install-deps + + - name: Save cached Binaries + if: steps.cached-binaries.outputs.cache-hit != 'true' + uses: actions/cache/save@v6 + with: + key: ${{ runner.os }}-${{ runner.arch }}-bin-${{ hashFiles('operator/go.mod', 'operator/deps.mk', 'operator/versions.yaml', 'operator/versions.sh') }} + path: | + ${{ github.workspace }}/operator/bin + ~/.cache/go-build + + - name: Run operator-agent tests + env: + AGENT_IMAGE: ${{ format('{0}/{1}/agent-go:{2}', env.REGISTRY, github.repository, needs.compute-metadata.outputs.agent-image-tag) }} + run: | + cd operator + export AGENT_IMAGE="${AGENT_IMAGE,,}" + echo "Testing with agent image: ${AGENT_IMAGE}" + make build-cli + make setup-kind-cluster operator-agent-tests + + - name: Dump diagnostics on failure + if: failure() + uses: ./.github/actions/dump-operator-agent-diagnostics + + # See operator-ci.yaml's ci-gate for rationale. needs: only includes jobs that + # ALWAYS run. create-manifest and operator-agent-go-tests skip on fork PRs + # (they need GHCR write) -- including either would make the gate fail red on a + # legitimate skip. ci-gate: name: ci-gate needs: [fetch-distroless-versions, compute-metadata, test, lint, build-agent-go] diff --git a/docs/contributing/ci-test-pools.md b/docs/contributing/ci-test-pools.md index 823a6d51..3d9d13eb 100644 --- a/docs/contributing/ci-test-pools.md +++ b/docs/contributing/ci-test-pools.md @@ -58,6 +58,23 @@ covers the other direction: the operator is what builds the pod the agent runs i mounts, copy dir and `config.json` — and until this row existed, an operator change could break that contract without any suite noticing. +During the Python-to-Go agent rewrite the suite is the **parity contract**, so it runs against +both agents and the scenarios are shared, never forked — a scenario is the behaviour the operator +depends on, not something either implementation gets its own copy of. Which agents run is decided +by the paths a PR touches, because each workflow owns one image: + +| Paths changed | `agent-ci.yaml` (Python) | `agent-go-ci.yaml` (Go) | +|---|---|---| +| `agent/**` excluding `agent/go/**` | ✅ | — | +| `agent/go/**` | — | ✅ | +| `k8s-tests/operator-agent/**` | ✅ | ✅ | + +Both workflows dump agent pod logs and the agent's on-node state under `/etc/skyhook` and +`/var/log/skyhook` on failure, via `.github/actions/dump-operator-agent-diagnostics`. A parity +failure is only useful if both sides are diagnosed from the same evidence. + +Once the cutover (#222) removes the Python agent, the Python row and its path filter go with it. + It resolves `AGENT_IMAGE` from `chart/values.yaml` rather than pinning a version in the workflow, so bumping the agent in one place cannot leave this row testing an older one. The suite refuses to run without an explicit `AGENT_IMAGE`, because `operator/Makefile`'s global default is the @@ -124,9 +141,9 @@ Workflows publishing `ci-gate`: - `lint-ci.yaml` — runs on every PR (no path filter). Guarantees a `ci-gate` is always posted, even on doc-only changes. -- `operator-ci.yaml`, `agent-ci.yaml` — wrapper job that depends on the - matrix and image-build jobs. Posts `ci-gate` only when the workflow's - paths trigger. +- `operator-ci.yaml`, `agent-ci.yaml`, `agent-go-ci.yaml` — wrapper job + that depends on the matrix and image-build jobs. Posts `ci-gate` only + when the workflow's paths trigger. - `commit-linting.yaml`, `security-checkov.yaml`, `agentless-container.yaml` — single-job workflows wrap their worker in a `ci-gate` job for uniformity. From d8473489f7791bd28cc61a8d723d093ab44fdd46 Mon Sep 17 00:00:00 2001 From: Riley Rice Date: Fri, 11 Sep 2026 13:14:11 -0700 Subject: [PATCH 2/6] test(agent): cover uninstall, post-interrupt and check summaries in the operator-agent suite The suite is the parity contract for the Python-to-Go cutover, but it only exercised apply, config and interrupt. Three of the surfaces the Go agent reimplements had no end-to-end coverage on either agent: - uninstall / uninstall-check ran nowhere, despite being the one transition the cutover cannot walk back: a package uninstalled by a divergent agent leaves host state no later stage repairs. The new scenario also pins the post- uninstall flag sweep (Python remove_flags, Go removeStepFlags) and the "uninstalled" sentinel in the history ledger, neither of which was asserted. - post-interrupt / post-interrupt-check were scheduled by the interrupt scenario but ran as no-ops, since it supplies no post_interrupt script. They now execute real agent code. - check_results and _ALL_CHECKED were asserted nowhere. check_results records " " with Python's capitalised bool repr, which the Go agent hardcodes rather than formatting a Go bool; nothing caught a regression to lowercase. Scenarios are added rather than existing ones edited: they are the operator-side contract and both agents run them unchanged. upgrade / upgrade-check remain uncovered. The shellscript package these scenarios use declares no upgrade mode in any published version (1.0.0, 1.1.0, 1.1.1), and its 1.1.0 tag ships a config.json declaring version 1.0.0, so a version-bump test would assert on a package packaging bug rather than on agent parity. Covering upgrade needs a package that supports it. Refs #221 Signed-off-by: Riley Rice --- docs/contributing/ci-test-pools.md | 6 ++ .../operator-agent/check_results/assert.yaml | 43 ++++++++++ .../check_results/chainsaw-test.yaml | 69 ++++++++++++++++ .../check_results/nodewright.yaml | 48 +++++++++++ .../operator-agent/post_interrupt/assert.yaml | 43 ++++++++++ .../post_interrupt/chainsaw-test.yaml | 63 +++++++++++++++ .../post_interrupt/nodewright.yaml | 45 +++++++++++ .../operator-agent/uninstall/assert.yaml | 43 ++++++++++ .../uninstall/chainsaw-test.yaml | 81 +++++++++++++++++++ .../operator-agent/uninstall/nodewright.yaml | 46 +++++++++++ .../uninstall/update-trigger-uninstall.yaml | 46 +++++++++++ 11 files changed, 533 insertions(+) create mode 100644 k8s-tests/operator-agent/check_results/assert.yaml create mode 100644 k8s-tests/operator-agent/check_results/chainsaw-test.yaml create mode 100644 k8s-tests/operator-agent/check_results/nodewright.yaml create mode 100644 k8s-tests/operator-agent/post_interrupt/assert.yaml create mode 100644 k8s-tests/operator-agent/post_interrupt/chainsaw-test.yaml create mode 100644 k8s-tests/operator-agent/post_interrupt/nodewright.yaml create mode 100644 k8s-tests/operator-agent/uninstall/assert.yaml create mode 100644 k8s-tests/operator-agent/uninstall/chainsaw-test.yaml create mode 100644 k8s-tests/operator-agent/uninstall/nodewright.yaml create mode 100644 k8s-tests/operator-agent/uninstall/update-trigger-uninstall.yaml diff --git a/docs/contributing/ci-test-pools.md b/docs/contributing/ci-test-pools.md index 3d9d13eb..1db3bd30 100644 --- a/docs/contributing/ci-test-pools.md +++ b/docs/contributing/ci-test-pools.md @@ -53,6 +53,12 @@ newest release rather than a pinned one. Runs `k8s-tests/operator-agent/` — the only suite that exercises the **real agent** rather than the `agentless` package image, so the only one that proves a package's scripts run on the host. +It covers `apply`, `config`, `interrupt`, `post-interrupt` and `uninstall`, each with its `-check` +counterpart, plus log retention, `SKYHOOK_AGENT_WRITE_LOGS=false`, and the `check_results` / +`_ALL_CHECKED` summary artifacts. `upgrade` is **not** covered: the `shellscript` package +these scenarios use declares no `upgrade` mode in any published version, so an upgrade scenario +needs a package that supports one. + `agent-ci.yaml` already runs it when the *agent* changes, against a freshly built agent. This row covers the other direction: the operator is what builds the pod the agent runs in — its args, mounts, copy dir and `config.json` — and until this row existed, an operator change could break diff --git a/k8s-tests/operator-agent/check_results/assert.yaml b/k8s-tests/operator-agent/check_results/assert.yaml new file mode 100644 index 00000000..902ecaf6 --- /dev/null +++ b/k8s-tests/operator-agent/check_results/assert.yaml @@ -0,0 +1,43 @@ +# SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +# SPDX-License-Identifier: Apache-2.0 +# +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + + +apiVersion: v1 +kind: Node +metadata: + labels: + nodewright.nvidia.com/test-node: skyhooke2e + nodewright.nvidia.com/status_check-results-agent-operator: complete + annotations: + nodewright.nvidia.com/status_check-results-agent-operator: complete +status: + (conditions[?type == 'nodewright.nvidia.com/check-results-agent-operator/NotReady']): + - reason: "Complete" + status: "False" + (conditions[?type == 'nodewright.nvidia.com/check-results-agent-operator/Erroring']): + - reason: "Not Erroring" + status: "False" +--- +apiVersion: nodewright.nvidia.com/v1alpha1 +kind: NodeWright +metadata: + name: check-results-agent-operator +status: + status: complete + nodeStatus: + # grab values should be one and is complete + (values(@)): + - complete diff --git a/k8s-tests/operator-agent/check_results/chainsaw-test.yaml b/k8s-tests/operator-agent/check_results/chainsaw-test.yaml new file mode 100644 index 00000000..1f7827a8 --- /dev/null +++ b/k8s-tests/operator-agent/check_results/chainsaw-test.yaml @@ -0,0 +1,69 @@ +# SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +# SPDX-License-Identifier: Apache-2.0 +# +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +# yaml-language-server: $schema=https://raw.githubusercontent.com/kyverno/chainsaw/main/.schemas/json/test-chainsaw-v1alpha1.json +apiVersion: chainsaw.kyverno.io/v1alpha1 +kind: Test +metadata: + name: check-results-agent-operator +spec: + timeouts: + assert: 240s + exec: 90s + steps: + - try: + - script: + content: | + ## remove annotation from last run + ../../../operator/bin/nodewright reset check-results-agent-operator --confirm 2>/dev/null || true + - script: + content: | + ## reinstall the debug pod in case it was deleted + ../setup.sh kind-worker setup + - script: + content: | + ## clean up from prior runs + ../check_node.sh kind-worker "rm -rf /var/log/skyhook/check-results-agent-operator || true" ".*" 2 + ../check_node.sh kind-worker "rm -rf /var/lib/skyhook/check-results-agent-operator || true" ".*" 2 + - apply: + file: nodewright.yaml + - assert: + file: assert.yaml + - script: + content: | + ## The check-summary artifacts are what a check stage leaves behind for the next + ## invocation to read, and nothing else in this suite asserts them. They live in the + ## flag root rather than under /, unlike step flags. + flags=/var/lib/skyhook/check-results-agent-operator/flags + + ## A check stage that passed writes _ALL_CHECKED. Both stages are asserted + ## because the file is per-stage, so covering only one would miss a stage that never + ## summarized its results. + ../check_node.sh kind-worker "ls $flags" "apply-check_ALL_CHECKED" + ../check_node.sh kind-worker "ls $flags" "config-check_ALL_CHECKED" + + ## check_results is rewritten by each check stage as " ", where + ## the boolean reports FAILURE, so a passing run records False. The capitalisation is + ## Python's bool repr and is load-bearing: the Go agent hardcodes "True"/"False" to + ## match rather than formatting a Go bool, which would emit lowercase. + ../check_node.sh kind-worker "cat $flags/check_results" "^shellscript_run.sh False$" + + ## Guard against the inverse failure: a stage that recorded a failure but still let the + ## package report complete would leave True here and is worth failing loudly on. + ../check_node.sh kind-worker "cat $flags/check_results" "True" 2 true + - finally: + - delete: + file: nodewright.yaml diff --git a/k8s-tests/operator-agent/check_results/nodewright.yaml b/k8s-tests/operator-agent/check_results/nodewright.yaml new file mode 100644 index 00000000..fb8f6d24 --- /dev/null +++ b/k8s-tests/operator-agent/check_results/nodewright.yaml @@ -0,0 +1,48 @@ +# SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +# SPDX-License-Identifier: Apache-2.0 +# +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +apiVersion: nodewright.nvidia.com/v1alpha1 +kind: NodeWright +metadata: + labels: + app.kubernetes.io/part-of: skyhook-operator + app.kubernetes.io/created-by: skyhook-operator + name: check-results-agent-operator +spec: + nodeSelectors: + matchLabels: + nodewright.nvidia.com/test-node: skyhooke2e + packages: + check-results-agent-operator: + version: "1.1.1" + image: ghcr.io/nvidia/skyhook-packages/shellscript + configMap: + # Both check stages are given a real script so each one records a result. + # shellscript_run.sh sources configmaps/.sh and exits 0 when the file + # is absent, so omitting a script would leave the stage a silent no-op and + # the assertions would pass without the check ever having run. + apply.sh: | + #!/bin/bash + echo "applying" + apply_check.sh: | + #!/bin/bash + echo "apply checked" + config.sh: | + #!/bin/bash + echo "configuring" + config_check.sh: | + #!/bin/bash + echo "config checked" diff --git a/k8s-tests/operator-agent/post_interrupt/assert.yaml b/k8s-tests/operator-agent/post_interrupt/assert.yaml new file mode 100644 index 00000000..b44be792 --- /dev/null +++ b/k8s-tests/operator-agent/post_interrupt/assert.yaml @@ -0,0 +1,43 @@ +# SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +# SPDX-License-Identifier: Apache-2.0 +# +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + + +apiVersion: v1 +kind: Node +metadata: + labels: + nodewright.nvidia.com/test-node: skyhooke2e + nodewright.nvidia.com/status_post-interrupt-agent-operator: complete + annotations: + nodewright.nvidia.com/status_post-interrupt-agent-operator: complete +status: + (conditions[?type == 'nodewright.nvidia.com/post-interrupt-agent-operator/NotReady']): + - reason: "Complete" + status: "False" + (conditions[?type == 'nodewright.nvidia.com/post-interrupt-agent-operator/Erroring']): + - reason: "Not Erroring" + status: "False" +--- +apiVersion: nodewright.nvidia.com/v1alpha1 +kind: NodeWright +metadata: + name: post-interrupt-agent-operator +status: + status: complete + nodeStatus: + # grab values should be one and is complete + (values(@)): + - complete diff --git a/k8s-tests/operator-agent/post_interrupt/chainsaw-test.yaml b/k8s-tests/operator-agent/post_interrupt/chainsaw-test.yaml new file mode 100644 index 00000000..46677d97 --- /dev/null +++ b/k8s-tests/operator-agent/post_interrupt/chainsaw-test.yaml @@ -0,0 +1,63 @@ +# SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +# SPDX-License-Identifier: Apache-2.0 +# +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +# yaml-language-server: $schema=https://raw.githubusercontent.com/kyverno/chainsaw/main/.schemas/json/test-chainsaw-v1alpha1.json +apiVersion: chainsaw.kyverno.io/v1alpha1 +kind: Test +metadata: + name: post-interrupt-agent-operator +spec: + timeouts: + assert: 240s + exec: 90s + steps: + - try: + - script: + content: | + ## remove annotation from last run + ../../../operator/bin/nodewright reset post-interrupt-agent-operator --confirm 2>/dev/null || true + - script: + content: | + ## reinstall the debug pod in case it was deleted + ../setup.sh kind-worker setup + - script: + content: | + ## clean up from prior runs + ../check_node.sh kind-worker "rm -rf /var/log/skyhook/post-interrupt-agent-operator || true" ".*" 2 + ../check_node.sh kind-worker "rm -rf /var/lib/skyhook/post-interrupt-agent-operator || true" ".*" 2 + - apply: + file: nodewright.yaml + - assert: + file: assert.yaml + - script: + content: | + ## The existing interrupt test supplies no post_interrupt script, so its post-interrupt + ## stages run as no-ops and prove only that the operator scheduled them. These two + ## assertions are what make the stages actually execute agent code. + log=/var/log/skyhook/post-interrupt-agent-operator/shellscript/1.1.1 + ../check_node.sh kind-worker "cat $log/*.log" "post interrupt ran" 60 + ../check_node.sh kind-worker "cat $log/*.log" "post interrupt checked" 60 + - script: + content: | + ## post-interrupt is a checked stage, so it must leave the same summary artifacts the + ## other checked stages do. This is the only place the suite asserts them for a stage + ## that runs after the interrupt rather than before it. + flags=/var/lib/skyhook/post-interrupt-agent-operator/flags + ../check_node.sh kind-worker "ls $flags" "post-interrupt-check_ALL_CHECKED" 60 + ../check_node.sh kind-worker "cat $flags/check_results" "^shellscript_run.sh False$" 60 + - finally: + - delete: + file: nodewright.yaml diff --git a/k8s-tests/operator-agent/post_interrupt/nodewright.yaml b/k8s-tests/operator-agent/post_interrupt/nodewright.yaml new file mode 100644 index 00000000..fe72d9b9 --- /dev/null +++ b/k8s-tests/operator-agent/post_interrupt/nodewright.yaml @@ -0,0 +1,45 @@ +# SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +# SPDX-License-Identifier: Apache-2.0 +# +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +apiVersion: nodewright.nvidia.com/v1alpha1 +kind: NodeWright +metadata: + labels: + app.kubernetes.io/part-of: skyhook-operator + app.kubernetes.io/created-by: skyhook-operator + name: post-interrupt-agent-operator +spec: + nodeSelectors: + matchLabels: + nodewright.nvidia.com/test-node: skyhooke2e + packages: + post-interrupt-agent-operator: + version: "1.1.1" + image: ghcr.io/nvidia/skyhook-packages/shellscript + # noop keeps the interrupt itself inert -- this test is about the stages that + # run *after* the interrupt, and a reboot here would take the kind node down. + interrupt: + type: noop + configMap: + apply.sh: | + #!/bin/bash + echo "Hello, world!" + post_interrupt.sh: | + #!/bin/bash + echo "post interrupt ran" + post_interrupt_check.sh: | + #!/bin/bash + echo "post interrupt checked" diff --git a/k8s-tests/operator-agent/uninstall/assert.yaml b/k8s-tests/operator-agent/uninstall/assert.yaml new file mode 100644 index 00000000..98a71cb7 --- /dev/null +++ b/k8s-tests/operator-agent/uninstall/assert.yaml @@ -0,0 +1,43 @@ +# SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +# SPDX-License-Identifier: Apache-2.0 +# +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + + +apiVersion: v1 +kind: Node +metadata: + labels: + nodewright.nvidia.com/test-node: skyhooke2e + nodewright.nvidia.com/status_uninstall-agent-operator: complete + annotations: + nodewright.nvidia.com/status_uninstall-agent-operator: complete +status: + (conditions[?type == 'nodewright.nvidia.com/uninstall-agent-operator/NotReady']): + - reason: "Complete" + status: "False" + (conditions[?type == 'nodewright.nvidia.com/uninstall-agent-operator/Erroring']): + - reason: "Not Erroring" + status: "False" +--- +apiVersion: nodewright.nvidia.com/v1alpha1 +kind: NodeWright +metadata: + name: uninstall-agent-operator +status: + status: complete + nodeStatus: + # grab values should be one and is complete + (values(@)): + - complete diff --git a/k8s-tests/operator-agent/uninstall/chainsaw-test.yaml b/k8s-tests/operator-agent/uninstall/chainsaw-test.yaml new file mode 100644 index 00000000..471e0579 --- /dev/null +++ b/k8s-tests/operator-agent/uninstall/chainsaw-test.yaml @@ -0,0 +1,81 @@ +# SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +# SPDX-License-Identifier: Apache-2.0 +# +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +# yaml-language-server: $schema=https://raw.githubusercontent.com/kyverno/chainsaw/main/.schemas/json/test-chainsaw-v1alpha1.json +apiVersion: chainsaw.kyverno.io/v1alpha1 +kind: Test +metadata: + name: uninstall-agent-operator +spec: + timeouts: + assert: 240s + exec: 90s + steps: + - name: install + try: + - script: + content: | + ## remove annotation from last run + ../../../operator/bin/nodewright reset uninstall-agent-operator --confirm 2>/dev/null || true + - script: + content: | + ## reinstall the debug pod in case it was deleted + ../setup.sh kind-worker setup + - script: + content: | + ## clean up from prior runs + ../check_node.sh kind-worker "rm -rf /var/log/skyhook/uninstall-agent-operator || true" ".*" 2 + ../check_node.sh kind-worker "rm -rf /var/lib/skyhook/uninstall-agent-operator || true" ".*" 2 + - apply: + file: nodewright.yaml + - assert: + file: assert.yaml + - script: + content: | + ## Pin that the package really installed before uninstalling it. Without this the + ## uninstall assertions below would also pass against a package that never ran, + ## since both amount to "the step flag is absent". + ../check_node.sh kind-worker "ls /var/lib/skyhook/uninstall-agent-operator/flags/shellscript/1.1.1/" "shellscript_run.sh.*" + + - name: uninstall + try: + - apply: + file: update-trigger-uninstall.yaml + - script: + content: | + ## The uninstall and uninstall-check stages are the only stages in this suite with no + ## coverage at all before this test, and they are the ones the cutover cannot walk + ## back: a package uninstalled by a divergent agent leaves the host in a state no + ## later stage repairs. + log=/var/log/skyhook/uninstall-agent-operator/shellscript/1.1.1 + ../check_node.sh kind-worker "cat $log/*.log" "uninstall ran" 60 + ../check_node.sh kind-worker "cat $log/*.log" "uninstall checked" 60 + - script: + content: | + ## After a successful uninstall-check both agents delete every step flag for the + ## package, so a later reinstall re-runs its steps instead of finding stale flags and + ## skipping them. Python does this in remove_flags(); Go does it in removeStepFlags(). + ## Asserting absence is what makes a divergence here visible. + ../check_node.sh kind-worker "ls /var/lib/skyhook/uninstall-agent-operator/flags/shellscript/1.1.1/ 2>/dev/null || true" "shellscript_run.sh" 60 true + - script: + content: | + ## The history ledger is the agent's own record of what is installed, and uninstall is + ## the one transition that writes a sentinel rather than a version. Nothing else in + ## this suite reads the ledger's contents. + ../check_node.sh kind-worker "cat /var/lib/skyhook/uninstall-agent-operator/history/shellscript.json" "uninstalled" 60 + - finally: + - delete: + file: update-trigger-uninstall.yaml diff --git a/k8s-tests/operator-agent/uninstall/nodewright.yaml b/k8s-tests/operator-agent/uninstall/nodewright.yaml new file mode 100644 index 00000000..79ed7ea7 --- /dev/null +++ b/k8s-tests/operator-agent/uninstall/nodewright.yaml @@ -0,0 +1,46 @@ +# SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +# SPDX-License-Identifier: Apache-2.0 +# +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +apiVersion: nodewright.nvidia.com/v1alpha1 +kind: NodeWright +metadata: + labels: + app.kubernetes.io/part-of: skyhook-operator + app.kubernetes.io/created-by: skyhook-operator + name: uninstall-agent-operator +spec: + nodeSelectors: + matchLabels: + nodewright.nvidia.com/test-node: skyhooke2e + packages: + uninstall-agent-operator: + version: "1.1.1" + image: ghcr.io/nvidia/skyhook-packages/shellscript + uninstall: + enabled: true + # apply stays false here so this document installs; the paired + # update-trigger-uninstall.yaml flips it to true to run the uninstall. + apply: false + configMap: + apply.sh: | + #!/bin/bash + echo "installing uninstall-agent-operator" + uninstall.sh: | + #!/bin/bash + echo "uninstall ran" + uninstall_check.sh: | + #!/bin/bash + echo "uninstall checked" diff --git a/k8s-tests/operator-agent/uninstall/update-trigger-uninstall.yaml b/k8s-tests/operator-agent/uninstall/update-trigger-uninstall.yaml new file mode 100644 index 00000000..76bff84e --- /dev/null +++ b/k8s-tests/operator-agent/uninstall/update-trigger-uninstall.yaml @@ -0,0 +1,46 @@ +# SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +# SPDX-License-Identifier: Apache-2.0 +# +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +apiVersion: nodewright.nvidia.com/v1alpha1 +kind: NodeWright +metadata: + labels: + app.kubernetes.io/part-of: skyhook-operator + app.kubernetes.io/created-by: skyhook-operator + name: uninstall-agent-operator +spec: + nodeSelectors: + matchLabels: + nodewright.nvidia.com/test-node: skyhooke2e + packages: + uninstall-agent-operator: + version: "1.1.1" + image: ghcr.io/nvidia/skyhook-packages/shellscript + uninstall: + enabled: true + # apply is true here: this document triggers the uninstall workflow on + # every target node. Otherwise identical to nodewright.yaml. + apply: true + configMap: + apply.sh: | + #!/bin/bash + echo "installing uninstall-agent-operator" + uninstall.sh: | + #!/bin/bash + echo "uninstall ran" + uninstall_check.sh: | + #!/bin/bash + echo "uninstall checked" From 080a3382093cfebc4deae73fa10418fea2ff0472 Mon Sep 17 00:00:00 2001 From: Riley Rice Date: Fri, 11 Sep 2026 13:17:55 -0700 Subject: [PATCH 3/6] ci(agent): make ci-gate actually gate on the Go parity suite Review feedback on #605. ci-gate omitted create-manifest and operator-agent-go-tests from needs:, which follows the sibling gates' convention of listing only jobs that always run. That convention is right when a conditionally-skipped job is incidental to the merge decision, but it is wrong here: excluding a job does not just stop a skip from failing the gate, it stops the job from affecting the gate at all, including when it genuinely fails. operator-agent-go-tests is the parity check this workflow exists for, so the gate was gating on nothing -- a red Go suite still produced a green ci-gate. Both jobs are now in needs:, with their expected state asserted in both directions: success when PUSH_TO_REGISTRY is true, skipped when it is false. Asserting the skip rather than tolerating it means a run that should have happened and silently did not also fails the gate. Verified against simulated needs payloads for all eight combinations of the two jobs, PUSH_TO_REGISTRY, and an unrelated failing job. Also pin docker/login-action to a commit SHA. Two of the three jobs using it hold packages: write and pass GITHUB_TOKEN, so a moved tag there could act on repository packages. This file already pins checkout, upload-artifact and setup-buildx-action by SHA; agent-ci.yaml's unpinned tag was copied without re-examining it. Refs #221 Signed-off-by: Riley Rice --- .github/workflows/agent-go-ci.yaml | 59 ++++++++++++++++++++++++------ docs/contributing/ci-test-pools.md | 13 +++++++ 2 files changed, 61 insertions(+), 11 deletions(-) diff --git a/.github/workflows/agent-go-ci.yaml b/.github/workflows/agent-go-ci.yaml index d7ba8eb4..f65e15bf 100644 --- a/.github/workflows/agent-go-ci.yaml +++ b/.github/workflows/agent-go-ci.yaml @@ -181,7 +181,7 @@ jobs: with: persist-credentials: false - name: Log in to the Container registry - uses: docker/login-action@v4 + uses: docker/login-action@dbcb813823bdd20940b903addbd779551569679f # v4.6.0 with: registry: ${{ env.REGISTRY }} username: ${{ github.actor }} @@ -259,7 +259,7 @@ jobs: persist-credentials: false - name: Log in to the Container registry - uses: docker/login-action@v4 + uses: docker/login-action@dbcb813823bdd20940b903addbd779551569679f # v4.6.0 with: registry: ${{ env.REGISTRY }} username: ${{ github.actor }} @@ -314,7 +314,7 @@ jobs: cache-dependency-path: operator/go.sum - name: Log in to the Container registry - uses: docker/login-action@v4 + uses: docker/login-action@dbcb813823bdd20940b903addbd779551569679f # v4.6.0 with: registry: ${{ env.REGISTRY }} username: ${{ github.actor }} @@ -381,18 +381,55 @@ jobs: if: failure() uses: ./.github/actions/dump-operator-agent-diagnostics - # See operator-ci.yaml's ci-gate for rationale. needs: only includes jobs that - # ALWAYS run. create-manifest and operator-agent-go-tests skip on fork PRs - # (they need GHCR write) -- including either would make the gate fail red on a - # legitimate skip. + # See operator-ci.yaml's ci-gate for the "skipped == green" rationale, but note + # this gate cannot use the usual shape. The sibling gates exclude conditionally + # skipped jobs from needs:, which is safe when those jobs are incidental -- + # here operator-agent-go-tests is the whole point of the workflow, and a job + # absent from needs: cannot fail the gate at all. Both registry-dependent jobs + # are therefore included and split out below: required to succeed when they can + # run, required to be skipped when they cannot, so neither a real failure nor a + # silently-missing run reads as green. ci-gate: name: ci-gate - needs: [fetch-distroless-versions, compute-metadata, test, lint, build-agent-go] + needs: + - fetch-distroless-versions + - compute-metadata + - test + - lint + - build-agent-go + - create-manifest + - operator-agent-go-tests if: always() runs-on: ubuntu-latest steps: - name: Verify all required jobs passed + env: + # Passed through the environment rather than interpolated into the jq + # program: a needs result is a fixed enum, but keeping the payload out + # of the script text avoids building a program from workflow context. + # PUSH_TO_REGISTRY is already a workflow-level env var. + RESULTS: ${{ toJSON(needs) }} run: | - results='${{ toJSON(needs) }}' - echo "$results" - echo "$results" | jq -e 'to_entries | all(.value.result == "success")' + set -euo pipefail + echo "$RESULTS" + + # Jobs with no `if:` run on every event, including fork PRs. + echo "$RESULTS" | jq -e ' + to_entries + | map(select(.key != "create-manifest" and .key != "operator-agent-go-tests")) + | all(.value.result == "success")' + + # create-manifest and operator-agent-go-tests need GHCR write, so they + # skip on fork PRs. Asserting the expected state in both directions + # keeps a skip from hiding a run that should have happened. + if [ "$PUSH_TO_REGISTRY" = "true" ]; then + echo "$RESULTS" | jq -e ' + to_entries + | map(select(.key == "create-manifest" or .key == "operator-agent-go-tests")) + | all(.value.result == "success")' + else + echo "$RESULTS" | jq -e ' + to_entries + | map(select(.key == "create-manifest" or .key == "operator-agent-go-tests")) + | all(.value.result == "skipped")' + fi diff --git a/docs/contributing/ci-test-pools.md b/docs/contributing/ci-test-pools.md index 1db3bd30..86b409ea 100644 --- a/docs/contributing/ci-test-pools.md +++ b/docs/contributing/ci-test-pools.md @@ -159,3 +159,16 @@ standard fix for GitHub Actions' "skipped == green" pitfall. Each gate's `needs:` only lists jobs that always run — conditionally-skipped jobs (`create-manifest`/`upload-coverage` on fork PRs / tag builds) are deliberately excluded so legitimate skips don't fail the gate. + +`agent-go-ci.yaml` is the one exception, and the reason is worth +understanding before copying either shape. Excluding a job from `needs:` +does not merely stop a skip from failing the gate — it stops that job +from affecting the gate *at all*, including when it genuinely fails. That +is acceptable for jobs whose failure is incidental to the merge decision, +but `operator-agent-go-tests` is the parity check the whole workflow +exists for, so excluding it would gate on nothing. It and +`create-manifest` are therefore in `needs:`, and the gate asserts their +expected state in both directions: `success` when `PUSH_TO_REGISTRY` is +true, `skipped` when it is false. Asserting the skip rather than merely +tolerating it means a run that should have happened and silently didn't +also fails the gate. From e2c8a81277f914077c4c8598ef6a7feef16719f8 Mon Sep 17 00:00:00 2001 From: Riley Rice Date: Tue, 22 Sep 2026 13:23:06 -0700 Subject: [PATCH 4/6] test(agent): make the new operator-agent scenarios actually gate Review feedback on #605. The check_results scenario asserted nothing. Chainsaw runs script.content as `sh -c` with no `set -e`, so only the last command's exit status reaches the test, and that last command was an inverted check -- which passes when the file is absent, because `cat` on a missing file produces no output. An agent writing none of check_results, apply-check_ALL_CHECKED or config-check_ALL_CHECKED turned the scenario green. `set -e` now gates every assertion block in the three new scenarios, matching interrupt/ and dont_write_logs/. The diagnostics action dumped a directory that does not exist under the operator. /etc/skyhook is the agent's standalone SKYHOOK_ROOT_DIR default, but the operator overrides it to / == /var/lib/skyhook/ (skyhook_controller.go getAgentConfigEnvVars). That is where flags, check_results, *_ALL_CHECKED, history and interrupts/flags live, so the action collected none of the state these scenarios fail on. Also from review: - Assert each stage's script actually ran. shellscript_run.sh prints "Could not find file ..." and exits 0 on a missing configmap key, so a typo leaves a stage a silent no-op that still produces check_results and *_ALL_CHECKED. All three scenarios now grep the stage's own output and invert-grep that message. - Drop the check_results assertion from post_interrupt. Every stage of this package writes identical content and each check stage overwrites the file, so it passed on the copy apply-check left behind whether or not post-interrupt-check ran. Only the _ALL_CHECKED name attributes a stage. - Cut retry budgets from 60 to 10 where assert.yaml has already waited for complete. Two 60-retry polls in one script op exceed the 90s exec timeout and the op is killed before check_node.sh prints its diagnostic. - Match the history ledger on the current-version key rather than the bare word, which also appears in the history[] entry. Kept whitespace-agnostic because Python's json.dump emits a space after the colon and Go's encoding/json does not. - Assert the operator converged via an emptied nodeState. Every other assertion after the uninstall trigger reads the node filesystem, so an operator that ran the uninstall and then wedged still passed. It also keeps the finalizer's delete-time uninstall off the cleanup critical path, with cleanup/delete timeouts raised past chainsaw's 15s default for the same reason. - Use update: rather than apply: for the trigger, matching the existing uninstall scenarios. Refs #221 Signed-off-by: Riley Rice --- .../action.yml | 12 ++++-- docs/contributing/ci-test-pools.md | 2 +- .../check_results/chainsaw-test.yaml | 25 +++++++++-- .../post_interrupt/chainsaw-test.yaml | 30 ++++++++++--- .../uninstall/chainsaw-test.yaml | 42 +++++++++++++++++-- 5 files changed, 95 insertions(+), 16 deletions(-) diff --git a/.github/actions/dump-operator-agent-diagnostics/action.yml b/.github/actions/dump-operator-agent-diagnostics/action.yml index 9ab10ee3..30d2eb95 100644 --- a/.github/actions/dump-operator-agent-diagnostics/action.yml +++ b/.github/actions/dump-operator-agent-diagnostics/action.yml @@ -83,21 +83,27 @@ runs: # setup.sh leaves a privileged debugger pod on the node with the host # root bind-mounted at /host, which is the only way to read the agent's # flag, history and log files from inside the job. + # + # /var/lib/skyhook, not the agent's standalone /etc/skyhook default: under + # the operator SKYHOOK_ROOT_DIR is / + # (skyhook_controller.go getAgentConfigEnvVars, CopyDirRoot default + # /var/lib/skyhook), and that is where flags/, check_results, + # *_ALL_CHECKED, history/ and interrupts/flags/ actually land. DEBUGGER="${NODE}-debugger" if ! kubectl get pod -n default "$DEBUGGER" >/dev/null 2>&1; then echo "No ${DEBUGGER} pod; skipping on-node dump." exit 0 fi - for dir in /host/etc/skyhook /host/var/log/skyhook; do + for dir in /host/var/lib/skyhook /host/var/log/skyhook; do echo "::group::tree ${dir#/host}" kubectl exec -n default "$DEBUGGER" -- ls -laR "$dir" || true echo "::endgroup::" done - echo "::group::file contents under /etc/skyhook (flags + history)" + echo "::group::file contents under /var/lib/skyhook (flags + history)" kubectl exec -n default "$DEBUGGER" -- \ - find /host/etc/skyhook -type f -exec sh -c 'echo "===== $1 ====="; cat "$1"' _ {} \; || true + find /host/var/lib/skyhook -type f -exec sh -c 'echo "===== $1 ====="; cat "$1"' _ {} \; || true echo "::endgroup::" echo "::group::agent log files under /var/log/skyhook" diff --git a/docs/contributing/ci-test-pools.md b/docs/contributing/ci-test-pools.md index 86b409ea..0f559cc9 100644 --- a/docs/contributing/ci-test-pools.md +++ b/docs/contributing/ci-test-pools.md @@ -75,7 +75,7 @@ by the paths a PR touches, because each workflow owns one image: | `agent/go/**` | — | ✅ | | `k8s-tests/operator-agent/**` | ✅ | ✅ | -Both workflows dump agent pod logs and the agent's on-node state under `/etc/skyhook` and +Both workflows dump agent pod logs and the agent's on-node state under `/var/lib/skyhook` and `/var/log/skyhook` on failure, via `.github/actions/dump-operator-agent-diagnostics`. A parity failure is only useful if both sides are diagnosed from the same evidence. diff --git a/k8s-tests/operator-agent/check_results/chainsaw-test.yaml b/k8s-tests/operator-agent/check_results/chainsaw-test.yaml index 1f7827a8..2879417b 100644 --- a/k8s-tests/operator-agent/check_results/chainsaw-test.yaml +++ b/k8s-tests/operator-agent/check_results/chainsaw-test.yaml @@ -44,6 +44,21 @@ spec: file: assert.yaml - script: content: | + ## set -e because chainsaw runs script content as `sh -c`, so without it only the + ## last command's exit status reaches the test and every assertion above it is + ## decorative. interrupt/ and dont_write_logs/ set it for the same reason. + set -e + + ## Nothing else here proves a stage's script ran at all: shellscript_run.sh prints + ## "Could not find file ..." and exits 0 when a configmap key is missing, so a typo + ## leaves a stage a silent no-op that still produces check_results and *_ALL_CHECKED. + log=/var/log/skyhook/check-results-agent-operator/shellscript/1.1.1 + ../check_node.sh kind-worker "cat $log/*.log" "apply checked" + ../check_node.sh kind-worker "cat $log/*.log" "config checked" + ../check_node.sh kind-worker "cat $log/*.log" "Could not find file" 2 true + - script: + content: | + set -e ## The check-summary artifacts are what a check stage leaves behind for the next ## invocation to read, and nothing else in this suite asserts them. They live in the ## flag root rather than under /, unlike step flags. @@ -56,13 +71,15 @@ spec: ../check_node.sh kind-worker "ls $flags" "config-check_ALL_CHECKED" ## check_results is rewritten by each check stage as " ", where - ## the boolean reports FAILURE, so a passing run records False. The capitalisation is - ## Python's bool repr and is load-bearing: the Go agent hardcodes "True"/"False" to - ## match rather than formatting a Go bool, which would emit lowercase. + ## the boolean reports FAILURE, so a passing run records False. The capitalisation + ## comes from Python's bool repr, and the Go agent hardcodes "True"/"False" to match + ## rather than formatting a Go bool, which would emit lowercase. ../check_node.sh kind-worker "cat $flags/check_results" "^shellscript_run.sh False$" ## Guard against the inverse failure: a stage that recorded a failure but still let the - ## package report complete would leave True here and is worth failing loudly on. + ## package report complete would leave True here and is worth failing loudly on. This + ## inverted check only means anything because the positive check above gates first -- + ## on its own it would also pass when check_results does not exist. ../check_node.sh kind-worker "cat $flags/check_results" "True" 2 true - finally: - delete: diff --git a/k8s-tests/operator-agent/post_interrupt/chainsaw-test.yaml b/k8s-tests/operator-agent/post_interrupt/chainsaw-test.yaml index 46677d97..882d2f63 100644 --- a/k8s-tests/operator-agent/post_interrupt/chainsaw-test.yaml +++ b/k8s-tests/operator-agent/post_interrupt/chainsaw-test.yaml @@ -44,20 +44,40 @@ spec: file: assert.yaml - script: content: | + ## set -e: chainsaw runs script content as `sh -c`, so without it only the last + ## command decides the step and everything above it is decorative. + set -e + ## The existing interrupt test supplies no post_interrupt script, so its post-interrupt - ## stages run as no-ops and prove only that the operator scheduled them. These two + ## stages run as no-ops and prove only that the operator scheduled them. These ## assertions are what make the stages actually execute agent code. + ## + ## The inverted grep catches the failure mode those two cannot: shellscript_run.sh + ## prints "Could not find file ..." and exits 0 on a missing configmap key, so a typo + ## would otherwise leave a stage a silent no-op. + ## + ## The assert on assert.yaml above already waited for the package to reach complete, + ## so these files exist by now and a long retry budget buys nothing. It costs on the + ## failure path: two 60-retry polls in one script op exceed the 90s exec timeout and + ## the op is killed before check_node.sh prints its Data:/Check: diagnostic. log=/var/log/skyhook/post-interrupt-agent-operator/shellscript/1.1.1 - ../check_node.sh kind-worker "cat $log/*.log" "post interrupt ran" 60 - ../check_node.sh kind-worker "cat $log/*.log" "post interrupt checked" 60 + ../check_node.sh kind-worker "cat $log/*.log" "post interrupt ran" 10 + ../check_node.sh kind-worker "cat $log/*.log" "post interrupt checked" 10 + ../check_node.sh kind-worker "cat $log/*.log" "Could not find file" 2 true - script: content: | + set -e ## post-interrupt is a checked stage, so it must leave the same summary artifacts the ## other checked stages do. This is the only place the suite asserts them for a stage ## that runs after the interrupt rather than before it. + ## + ## Only the _ALL_CHECKED filename is attributable to this stage. check_results is + ## rewritten by every check stage with identical content for this package, so an + ## assertion on it here would pass on the copy apply-check or config-check left + ## behind even if post-interrupt-check never ran. It is deliberately not asserted; + ## check_results content is covered by the check_results scenario instead. flags=/var/lib/skyhook/post-interrupt-agent-operator/flags - ../check_node.sh kind-worker "ls $flags" "post-interrupt-check_ALL_CHECKED" 60 - ../check_node.sh kind-worker "cat $flags/check_results" "^shellscript_run.sh False$" 60 + ../check_node.sh kind-worker "ls $flags" "post-interrupt-check_ALL_CHECKED" 10 - finally: - delete: file: nodewright.yaml diff --git a/k8s-tests/operator-agent/uninstall/chainsaw-test.yaml b/k8s-tests/operator-agent/uninstall/chainsaw-test.yaml index 471e0579..5f0283c6 100644 --- a/k8s-tests/operator-agent/uninstall/chainsaw-test.yaml +++ b/k8s-tests/operator-agent/uninstall/chainsaw-test.yaml @@ -23,6 +23,11 @@ spec: timeouts: assert: 240s exec: 90s + # This is the only scenario in the suite whose CR has uninstall.enabled, so + # deleting it goes through the finalizer-driven uninstall flow rather than a + # plain delete. Chainsaw's 15s default is not enough for that round trip. + cleanup: 120s + delete: 120s steps: - name: install try: @@ -45,6 +50,7 @@ spec: file: assert.yaml - script: content: | + set -e ## Pin that the package really installed before uninstalling it. Without this the ## uninstall assertions below would also pass against a package that never ran, ## since both amount to "the step flag is absent". @@ -52,30 +58,60 @@ spec: - name: uninstall try: - - apply: + - update: file: update-trigger-uninstall.yaml - script: content: | + ## set -e: chainsaw runs script content as `sh -c`, so without it only the last + ## command decides the step. + set -e + ## The uninstall and uninstall-check stages are the only stages in this suite with no ## coverage at all before this test, and they are the ones the cutover cannot walk ## back: a package uninstalled by a divergent agent leaves the host in a state no ## later stage repairs. + ## + ## The inverted grep catches a missing configmap key, which shellscript_run.sh + ## reports on stdout while still exiting 0. log=/var/log/skyhook/uninstall-agent-operator/shellscript/1.1.1 ../check_node.sh kind-worker "cat $log/*.log" "uninstall ran" 60 ../check_node.sh kind-worker "cat $log/*.log" "uninstall checked" 60 + ../check_node.sh kind-worker "cat $log/*.log" "Could not find file" 2 true - script: content: | + set -e ## After a successful uninstall-check both agents delete every step flag for the ## package, so a later reinstall re-runs its steps instead of finding stale flags and ## skipping them. Python does this in remove_flags(); Go does it in removeStepFlags(). - ## Asserting absence is what makes a divergence here visible. + ## Asserting absence is what makes a divergence here visible. The positive ls on this + ## exact path in the install step is what stops this passing vacuously. ../check_node.sh kind-worker "ls /var/lib/skyhook/uninstall-agent-operator/flags/shellscript/1.1.1/ 2>/dev/null || true" "shellscript_run.sh" 60 true - script: content: | + set -e ## The history ledger is the agent's own record of what is installed, and uninstall is ## the one transition that writes a sentinel rather than a version. Nothing else in ## this suite reads the ledger's contents. - ../check_node.sh kind-worker "cat /var/lib/skyhook/uninstall-agent-operator/history/shellscript.json" "uninstalled" 60 + ## + ## Matched against the current-version key rather than the bare word, which also + ## appears in the history[] entry for this transition. The ` *` keeps it agnostic to + ## separator spacing: Python's json.dump emits `"current-version": "uninstalled"` + ## and Go's encoding/json emits no space, so an exact match would pass on one agent + ## and fail on the other. + ../check_node.sh kind-worker "cat /var/lib/skyhook/uninstall-agent-operator/history/shellscript.json" '"current-version": *"uninstalled"' 60 + - assert: + ## Everything above this point reads the node filesystem, so an operator that ran the + ## uninstall and then failed to converge would still pass. An emptied nodeState is the + ## established signal that it finished, and asserting it here also keeps the + ## finalizer-driven delete-time uninstall off the cleanup critical path. + resource: + apiVersion: v1 + kind: Node + metadata: + labels: + nodewright.nvidia.com/test-node: skyhooke2e + annotations: + nodewright.nvidia.com/nodeState_uninstall-agent-operator: '{}' - finally: - delete: file: update-trigger-uninstall.yaml From b20f3acf57ce5d116ad8dfd461eca806208adc57 Mon Sep 17 00:00:00 2001 From: Riley Rice Date: Tue, 22 Sep 2026 13:36:50 -0700 Subject: [PATCH 5/6] test(agent): give every traversed stage a script in the new scenarios The "Could not find file" guard added in the previous commit did its job on its first run and failed both agents identically, on uninstall/ and post_interrupt/. The message was real: shellscript_run.sh prints it and exits 0 for a missing configmap key, and both scenarios only supplied scripts for the stages they were about, so apply-check, config and config-check were silent no-ops. The guard greps the whole log directory, so it saw them. Supplying a script for every stage the package actually traverses fixes the false positive and makes those stages execute rather than no-op. uninstall/ declares no interrupt, so post-interrupt never runs there and needs no script; post_interrupt/ gets the four install-path scripts alongside its own. Both agents failed on exactly the same two scenarios and passed the same five, which is the parity signal working as intended -- the defect was in the test, not in either agent. Also stop the diagnostics action dumping cluster-infrastructure namespaces. On the first real failure it caught, the chainsaw error was buried under thousands of kube-system reflector lines; a dump nobody can read is the same as no dump. Refs #221 Signed-off-by: Riley Rice --- .../dump-operator-agent-diagnostics/action.yml | 8 ++++++++ .../operator-agent/post_interrupt/nodewright.yaml | 14 ++++++++++++++ .../operator-agent/uninstall/nodewright.yaml | 15 +++++++++++++++ .../uninstall/update-trigger-uninstall.yaml | 15 +++++++++++++++ 4 files changed, 52 insertions(+) diff --git a/.github/actions/dump-operator-agent-diagnostics/action.yml b/.github/actions/dump-operator-agent-diagnostics/action.yml index 30d2eb95..7a92da37 100644 --- a/.github/actions/dump-operator-agent-diagnostics/action.yml +++ b/.github/actions/dump-operator-agent-diagnostics/action.yml @@ -62,7 +62,15 @@ runs: run: | # Package pods are created per node/package; dump every container of each, # including terminated ones, since the failing stage has usually exited. + # + # Cluster-infrastructure namespaces are skipped. They contribute thousands + # of unrelated reflector errors on a kind cluster, and a dump nobody can + # read is the same as no dump: on the first real failure this action + # caught, the chainsaw error was buried under kube-system scheduler noise. for ns in $(kubectl get ns -o jsonpath='{.items[*].metadata.name}' || true); do + case "$ns" in + kube-system|kube-public|kube-node-lease|local-path-storage) continue ;; + esac for pod in $(kubectl get pods -n "$ns" -o jsonpath='{.items[*].metadata.name}' 2>/dev/null || true); do case "$pod" in *-debugger) continue ;; diff --git a/k8s-tests/operator-agent/post_interrupt/nodewright.yaml b/k8s-tests/operator-agent/post_interrupt/nodewright.yaml index fe72d9b9..eae1b907 100644 --- a/k8s-tests/operator-agent/post_interrupt/nodewright.yaml +++ b/k8s-tests/operator-agent/post_interrupt/nodewright.yaml @@ -33,10 +33,24 @@ spec: # run *after* the interrupt, and a reboot here would take the kind node down. interrupt: type: noop + # Every stage this package traverses gets a script, including the ones this + # scenario is not about. shellscript_run.sh prints "Could not find file ..." + # and exits 0 for a missing key, so a stage without one is a silent no-op -- + # and the test greps the whole log directory for that message, which would + # otherwise trip on apply-check and config rather than on a real defect. configMap: apply.sh: | #!/bin/bash echo "Hello, world!" + apply_check.sh: | + #!/bin/bash + echo "apply checked" + config.sh: | + #!/bin/bash + echo "configuring" + config_check.sh: | + #!/bin/bash + echo "config checked" post_interrupt.sh: | #!/bin/bash echo "post interrupt ran" diff --git a/k8s-tests/operator-agent/uninstall/nodewright.yaml b/k8s-tests/operator-agent/uninstall/nodewright.yaml index 79ed7ea7..f7c5173c 100644 --- a/k8s-tests/operator-agent/uninstall/nodewright.yaml +++ b/k8s-tests/operator-agent/uninstall/nodewright.yaml @@ -34,10 +34,25 @@ spec: # apply stays false here so this document installs; the paired # update-trigger-uninstall.yaml flips it to true to run the uninstall. apply: false + # Every stage this package traverses gets a script, including the ones this + # scenario is not about. shellscript_run.sh prints "Could not find file ..." + # and exits 0 for a missing key, so a stage without one is a silent no-op -- + # and the test greps the whole log directory for that message, which would + # otherwise trip on apply-check and config rather than on a real defect. + # No interrupt is declared, so post-interrupt never runs and needs no script. configMap: apply.sh: | #!/bin/bash echo "installing uninstall-agent-operator" + apply_check.sh: | + #!/bin/bash + echo "apply checked" + config.sh: | + #!/bin/bash + echo "configuring" + config_check.sh: | + #!/bin/bash + echo "config checked" uninstall.sh: | #!/bin/bash echo "uninstall ran" diff --git a/k8s-tests/operator-agent/uninstall/update-trigger-uninstall.yaml b/k8s-tests/operator-agent/uninstall/update-trigger-uninstall.yaml index 76bff84e..45335cf1 100644 --- a/k8s-tests/operator-agent/uninstall/update-trigger-uninstall.yaml +++ b/k8s-tests/operator-agent/uninstall/update-trigger-uninstall.yaml @@ -34,10 +34,25 @@ spec: # apply is true here: this document triggers the uninstall workflow on # every target node. Otherwise identical to nodewright.yaml. apply: true + # Every stage this package traverses gets a script, including the ones this + # scenario is not about. shellscript_run.sh prints "Could not find file ..." + # and exits 0 for a missing key, so a stage without one is a silent no-op -- + # and the test greps the whole log directory for that message, which would + # otherwise trip on apply-check and config rather than on a real defect. + # No interrupt is declared, so post-interrupt never runs and needs no script. configMap: apply.sh: | #!/bin/bash echo "installing uninstall-agent-operator" + apply_check.sh: | + #!/bin/bash + echo "apply checked" + config.sh: | + #!/bin/bash + echo "configuring" + config_check.sh: | + #!/bin/bash + echo "config checked" uninstall.sh: | #!/bin/bash echo "uninstall ran" From 7886add8ca1c128242e31eebef10f7266db8560a Mon Sep 17 00:00:00 2001 From: Riley Rice Date: Wed, 23 Sep 2026 11:21:35 -0700 Subject: [PATCH 6/6] test(agent): settle the node before the uninstall filesystem checks Move the nodeState '{}' assert to directly after the uninstall trigger so it does the waiting, then cut the filesystem polls from 60 retries to 10. Two 60-retry check_node.sh loops in one script op ran past the 90s exec timeout on the failure path, killing the op before its Data:/Check: diagnostic printed. The flag-absence and history reads no longer race uninstall-check either. Note on the "Could not find file" guard in post_interrupt/ and uninstall/ that it only sees the 5 retained shellscript_run.sh logs, so with six stages the apply log has already been rotated away before it runs. Pin the four tag-referenced actions in operator-agent-go-tests to commit SHAs like the rest of the workflow, and set persist-credentials: false on its checkout to match the other jobs. Signed-off-by: Riley Rice --- .github/workflows/agent-go-ci.yaml | 11 ++--- .../post_interrupt/chainsaw-test.yaml | 6 ++- .../uninstall/chainsaw-test.yaml | 44 +++++++++++-------- 3 files changed, 37 insertions(+), 24 deletions(-) diff --git a/.github/workflows/agent-go-ci.yaml b/.github/workflows/agent-go-ci.yaml index f65e15bf..56097768 100644 --- a/.github/workflows/agent-go-ci.yaml +++ b/.github/workflows/agent-go-ci.yaml @@ -302,13 +302,14 @@ jobs: packages: read steps: - name: Checkout repository - uses: actions/checkout@v7 + uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 with: fetch-tags: true fetch-depth: 0 + persist-credentials: false - name: Set up Go - uses: actions/setup-go@v7 + uses: actions/setup-go@b7ad1dad31e06c5925ef5d2fc7ad053ef454303e # v7.0.0 with: go-version-file: operator/go.mod cache-dependency-path: operator/go.sum @@ -335,7 +336,7 @@ jobs: make validate-kind-node-image KIND_NODE_IMAGE_VERSION="$K8S_VERSION" - name: Create Kubernetes KinD Cluster - uses: helm/kind-action@v1.15.0 + uses: helm/kind-action@06c1ae10762d3b9c1644e7fe69596ae519e015a2 # v1.15.0 with: version: ${{ steps.k8s-versions.outputs.kind-binary }} node_image: kindest/node:v${{ steps.k8s-versions.outputs.kind-nodeimage }} @@ -344,7 +345,7 @@ jobs: - name: Restore cached Binaries id: cached-binaries - uses: actions/cache/restore@v6 + uses: actions/cache/restore@55cc8345863c7cc4c66a329aec7e433d2d1c52a9 # v6.1.0 with: key: ${{ runner.os }}-${{ runner.arch }}-bin-${{ hashFiles('operator/go.mod', 'operator/deps.mk', 'operator/versions.yaml', 'operator/versions.sh') }} restore-keys: ${{ runner.os }}-${{ runner.arch }}-bin- @@ -360,7 +361,7 @@ jobs: - name: Save cached Binaries if: steps.cached-binaries.outputs.cache-hit != 'true' - uses: actions/cache/save@v6 + uses: actions/cache/save@55cc8345863c7cc4c66a329aec7e433d2d1c52a9 # v6.1.0 with: key: ${{ runner.os }}-${{ runner.arch }}-bin-${{ hashFiles('operator/go.mod', 'operator/deps.mk', 'operator/versions.yaml', 'operator/versions.sh') }} path: | diff --git a/k8s-tests/operator-agent/post_interrupt/chainsaw-test.yaml b/k8s-tests/operator-agent/post_interrupt/chainsaw-test.yaml index 882d2f63..655a79a6 100644 --- a/k8s-tests/operator-agent/post_interrupt/chainsaw-test.yaml +++ b/k8s-tests/operator-agent/post_interrupt/chainsaw-test.yaml @@ -54,7 +54,11 @@ spec: ## ## The inverted grep catches the failure mode those two cannot: shellscript_run.sh ## prints "Could not find file ..." and exits 0 on a missing configmap key, so a typo - ## would otherwise leave a stage a silent no-op. + ## would otherwise leave a stage a silent no-op. It only sees the logs still on disk: + ## both agents keep the 5 newest shellscript_run.sh logs, and this package runs six + ## stages through it, so the apply log has already been rotated away by now and a + ## missing apply.sh would get past this guard. Every other scenario exercises apply, + ## so that gap is accepted rather than raced. ## ## The assert on assert.yaml above already waited for the package to reach complete, ## so these files exist by now and a long retry budget buys nothing. It costs on the diff --git a/k8s-tests/operator-agent/uninstall/chainsaw-test.yaml b/k8s-tests/operator-agent/uninstall/chainsaw-test.yaml index 5f0283c6..bb5ffcca 100644 --- a/k8s-tests/operator-agent/uninstall/chainsaw-test.yaml +++ b/k8s-tests/operator-agent/uninstall/chainsaw-test.yaml @@ -60,6 +60,23 @@ spec: try: - update: file: update-trigger-uninstall.yaml + - assert: + ## This is the wait for the uninstall to finish, and it does more than wait. An emptied + ## nodeState is the established signal that the operator converged, so an operator + ## that ran the uninstall and then wedged fails here rather than passing on filesystem + ## checks alone. It also settles the node before those checks run: without it the + ## flag-absence and history reads below would race uninstall-check and need a retry + ## budget that, on the failure path, runs past the 90s exec timeout and kills the op + ## before check_node.sh prints its Data:/Check: diagnostic. Asserting it before finally + ## also keeps the finalizer-driven delete-time uninstall off the cleanup critical path. + resource: + apiVersion: v1 + kind: Node + metadata: + labels: + nodewright.nvidia.com/test-node: skyhooke2e + annotations: + nodewright.nvidia.com/nodeState_uninstall-agent-operator: '{}' - script: content: | ## set -e: chainsaw runs script content as `sh -c`, so without it only the last @@ -72,10 +89,14 @@ spec: ## later stage repairs. ## ## The inverted grep catches a missing configmap key, which shellscript_run.sh - ## reports on stdout while still exiting 0. + ## reports on stdout while still exiting 0. It only sees the logs still on disk: + ## both agents keep the 5 newest shellscript_run.sh logs, and this package runs six + ## stages through it, so the apply log has already been rotated away by now and a + ## missing apply.sh would get past this guard. Every other scenario exercises apply, + ## so that gap is accepted rather than raced. log=/var/log/skyhook/uninstall-agent-operator/shellscript/1.1.1 - ../check_node.sh kind-worker "cat $log/*.log" "uninstall ran" 60 - ../check_node.sh kind-worker "cat $log/*.log" "uninstall checked" 60 + ../check_node.sh kind-worker "cat $log/*.log" "uninstall ran" 10 + ../check_node.sh kind-worker "cat $log/*.log" "uninstall checked" 10 ../check_node.sh kind-worker "cat $log/*.log" "Could not find file" 2 true - script: content: | @@ -85,7 +106,7 @@ spec: ## skipping them. Python does this in remove_flags(); Go does it in removeStepFlags(). ## Asserting absence is what makes a divergence here visible. The positive ls on this ## exact path in the install step is what stops this passing vacuously. - ../check_node.sh kind-worker "ls /var/lib/skyhook/uninstall-agent-operator/flags/shellscript/1.1.1/ 2>/dev/null || true" "shellscript_run.sh" 60 true + ../check_node.sh kind-worker "ls /var/lib/skyhook/uninstall-agent-operator/flags/shellscript/1.1.1/ 2>/dev/null || true" "shellscript_run.sh" 10 true - script: content: | set -e @@ -98,20 +119,7 @@ spec: ## separator spacing: Python's json.dump emits `"current-version": "uninstalled"` ## and Go's encoding/json emits no space, so an exact match would pass on one agent ## and fail on the other. - ../check_node.sh kind-worker "cat /var/lib/skyhook/uninstall-agent-operator/history/shellscript.json" '"current-version": *"uninstalled"' 60 - - assert: - ## Everything above this point reads the node filesystem, so an operator that ran the - ## uninstall and then failed to converge would still pass. An emptied nodeState is the - ## established signal that it finished, and asserting it here also keeps the - ## finalizer-driven delete-time uninstall off the cleanup critical path. - resource: - apiVersion: v1 - kind: Node - metadata: - labels: - nodewright.nvidia.com/test-node: skyhooke2e - annotations: - nodewright.nvidia.com/nodeState_uninstall-agent-operator: '{}' + ../check_node.sh kind-worker "cat /var/lib/skyhook/uninstall-agent-operator/history/shellscript.json" '"current-version": *"uninstalled"' 10 - finally: - delete: file: update-trigger-uninstall.yaml