From 98268885394e3d0e9ab853615ab5104992746743 Mon Sep 17 00:00:00 2001 From: Riley Rice Date: Wed, 30 Sep 2026 13:05:16 -0700 Subject: [PATCH] test(agent): assert what gracefulShutdown guarantees in sigterm_grace sigterm_grace asserted that progress held exactly one "start" and one "end" after the package pod was deleted mid-step. That is a transient state. Deleting the pod makes the Job count the attempt as failed whatever the agent exits with, so a replacement pod runs the apply stage again, and the shellscript steps declare idempotence: true, which both agents read as "run regardless of the completion flag". apply.sh runs twice by design, and the count passed in CI only by being taken within seconds of the first "end", before the replacement pod appended its own pair. On a real cluster the settled file had two pairs. The test now lets the package converge, then checks the property gracefulShutdown actually guarantees: the attempt that received SIGTERM finished. apply.sh tags each progress line with an attempt id, and one awk on the node checks that the id on the first "start" also appears on an "end" and that every start has an end, printing the counts when it fails so check_node.sh shows why. Counts or timing alone cannot catch the #672 regression: a killed first attempt leaves "start A, start B, end B", whose only end still comes a full sleep after the first start. Two docs record what running both agents on one node during release testing showed. agent/RELEASE_NOTES.md gains the v7 log-file differences the [out]/[err] entry did not cover: stderr is merged into the step's .log at mode 0600, where the Python agent wrote a 0644 .log and a .log.err that neither agent reaps, and interrupt logs are one -.log per interrupt run, reaped to five, where the Python agent wrote one per operation and never reaped them; the directory is the same. The Breaking entry now claims unchanged log directories rather than unchanged log paths. docs/architecture/lifecycle.md said deletion cleans the state annotations off nodes and removes the runtime-required taint; the finalizer keeps nodeState_ and version_ while they record packages whose files remain on the host, and never touches the taint (#714), and the Deletion list now says so. Closes #710 Closes #711 Closes #712 Signed-off-by: Riley Rice --- agent/RELEASE_NOTES.md | 21 ++++++++++++-- docs/architecture/lifecycle.md | 11 ++++++-- .../sigterm_grace/chainsaw-test.yaml | 28 ++++++++++++------- .../sigterm_grace/nodewright.yaml | 9 ++++-- 4 files changed, 51 insertions(+), 18 deletions(-) diff --git a/agent/RELEASE_NOTES.md b/agent/RELEASE_NOTES.md index 61a3b3579..37eb75099 100644 --- a/agent/RELEASE_NOTES.md +++ b/agent/RELEASE_NOTES.md @@ -11,9 +11,11 @@ For the full commit-level log see CHANGELOG.md. removed.** The next release is `agent/v7.0.0`; `agent/v6.x` images are the Python agent and stay on GHCR. The operator-facing contract is unchanged: positional arguments, environment variables, exit codes, and the flag, - history, interrupt-marker and log paths written on the host, as verified by - the operator-agent chainsaw suite that ran against both implementations - before the cutover. Both implementations honour each other's on-host state, + history, interrupt-marker and log directories written on the host, as + verified by the operator-agent chainsaw suite that ran against both + implementations before the cutover. Two log file names inside those + directories do change; see Behavior Changes. Both implementations honour + each other's on-host state, so moving a node between `v6.x` and `v7.x` in either direction does not re-run completed steps or interrupts, with the single `node_restart` exception described under Upgrade and Rollback below. @@ -30,6 +32,19 @@ For the full commit-level log see CHANGELOG.md. timestamp.** Step and interrupt output is written to the log file as the script produced it. Anything parsing those files for the prefix must stop expecting it. +- **Step stderr lands in the same host log file as stdout, and log files are + mode 0600.** The Python agent wrote stdout to `-.log` (0644) + and stderr to a sibling `-.log.err`. Neither agent reaps the + zero-byte `.log.err` files a `v6.x` node already carries; remove them by hand + if the clutter matters. +- **Interrupt logs are one file per interrupt run, and are reaped.** The + directory is unchanged, + `///interrupts/`, + but the file is now `-.log` for the whole interrupt, kept to + the newest five like step logs. The Python agent wrote + `_-.log` per operation and never removed any; those + names do not match the new reaping pattern, so they stay on a `v6.x` node + until removed by hand. - **`SKYHOOK_AGENT_BUFFER_LIMIT` is no longer read or printed.** It only tuned the Python agent's stream reader and had no equivalent in Go. - **The CycloneDX SBOM moved from the multi-platform index digest to each platform manifest digest, and an OpenVEX document is now published alongside it.** An SBOM describes exactly one root filesystem, so one attached to a multi-platform index described neither child truthfully, and a consumer who resolved `linux/amd64` and enumerated referrers on that manifest found nothing. diff --git a/docs/architecture/lifecycle.md b/docs/architecture/lifecycle.md index c74152113..1ccb5b849 100644 --- a/docs/architecture/lifecycle.md +++ b/docs/architecture/lifecycle.md @@ -386,8 +386,15 @@ Deleting a NodeWright does not delete the changes it made to your hosts. The operator: 1. Runs the uninstall workflow for every package with `uninstall.enabled: true` -2. Cleans its metadata off the nodes — state annotations, cordons it owns, and - the runtime-required taint +2. Cleans its metadata off the nodes — the status labels, annotations and + conditions it owns, and the cordons it holds. The `nodeState_` and + `version_` annotations are kept as long as they still record packages + whose files remain on the host (a non-absent entry means "installed"; see + [CR Deletion in the uninstall guide](../user-guide/uninstall.md#cr-deletion-finalizer)), + and removed once nothing remains. + The runtime-required taint is not touched here; only the completion path in + [runtime-required](../user-guide/runtime-required.md#when-is-the-runtime-required-taint-removed-from-a-node) + removes it. 3. Releases the finalizer, at which point the object disappears Packages without `uninstall.enabled` are simply forgotten, not reversed. Their diff --git a/k8s-tests/operator-agent/sigterm_grace/chainsaw-test.yaml b/k8s-tests/operator-agent/sigterm_grace/chainsaw-test.yaml index 5dfed03cf..248903b5e 100644 --- a/k8s-tests/operator-agent/sigterm_grace/chainsaw-test.yaml +++ b/k8s-tests/operator-agent/sigterm_grace/chainsaw-test.yaml @@ -85,18 +85,26 @@ spec: ## SIGTERM writes "end". progress=/var/lib/skyhook/sigterm-grace-agent-operator/progress ../check_node.sh kind-worker "cat $progress" "^end" 75 - ## "end" alone does not prove survival: a killed step leaves no flag, so the - ## retry pod re-runs apply.sh from scratch and appends a second "start" and - ## then its own "end". Exactly one of each is what pins the first attempt as - ## the one that finished. grep -c prints the count, so match it as a whole line. - ../check_node.sh kind-worker "grep -c '^start' $progress" "^1$" 2 - ../check_node.sh kind-worker "grep -c '^end' $progress" "^1$" 2 - assert: - ## The retry pod finds apply.sh's completion flag, skips it and finishes the - ## remaining stages, so the package converges. A Go agent that had killed the - ## step would instead re-run it from scratch here, and the absence of "end" - ## above would already have failed the test. + ## Let the package converge before counting attempts. Deleting the pod makes the + ## Job count that attempt as failed whatever the agent exits with (a pod deleted + ## mid-init never reaches Succeeded), so a replacement pod runs the apply stage + ## again, and this package's steps declare idempotence: true (Disabled), which + ## both agents read as "run regardless of the completion flag". apply.sh therefore + ## runs twice by design, and a count taken right after the first "end" only ever + ## saw one pair by racing the replacement pod. file: assert.yaml + - script: + content: | + set -e + ## What gracefulShutdown guarantees is that the attempt that received SIGTERM + ## finished, so the check is on identity, not counts or timing: the attempt id + ## on the first "start" must appear on an "end", and every start must have an + ## end. In the #672 regression the killed attempt's id never gets an end. One + ## awk does both and prints the counts when it fails, since check_node.sh only + ## shows what the command printed. + progress=/var/lib/skyhook/sigterm-grace-agent-operator/progress + ../check_node.sh kind-worker "awk '\$1 == \"start\" {starts++; if (!first) first = \$2} \$1 == \"end\" {ends++; if (\$2 == first) finished = 1} END {if (finished && starts == ends) print \"first-finished\"; else print \"first=\" (finished ? \"finished\" : \"killed\") \" starts=\" starts \" ends=\" ends}' $progress" "^first-finished$" 2 - finally: - delete: file: nodewright.yaml diff --git a/k8s-tests/operator-agent/sigterm_grace/nodewright.yaml b/k8s-tests/operator-agent/sigterm_grace/nodewright.yaml index b9db8200c..f9b1ca9b9 100644 --- a/k8s-tests/operator-agent/sigterm_grace/nodewright.yaml +++ b/k8s-tests/operator-agent/sigterm_grace/nodewright.yaml @@ -36,14 +36,17 @@ spec: configMap: # Progress markers go to the package's state root rather than # $SKYHOOK_DIR: the copy dir is per-attempt, and the retry after the - # pod delete must read what the killed attempt wrote. + # pod delete must read what the killed attempt wrote. Each line carries + # an attempt id so the test can tell whose "end" it is reading; the + # timestamp is only there for reading the file by hand. apply.sh: | #!/bin/bash progress=/var/lib/skyhook/sigterm-grace-agent-operator/progress + attempt="$$-$RANDOM" mkdir -p "$(dirname "$progress")" - echo "start $(date +%s)" >> "$progress" + echo "start $attempt $(date +%s)" >> "$progress" sleep 60 - echo "end $(date +%s)" >> "$progress" + echo "end $attempt $(date +%s)" >> "$progress" apply_check.sh: | #!/bin/bash echo "apply checked"