diff --git a/scripts/refresh-flux-ghcr-auth.sh b/scripts/refresh-flux-ghcr-auth.sh index e296abeb8b..f6db7d92f9 100755 --- a/scripts/refresh-flux-ghcr-auth.sh +++ b/scripts/refresh-flux-ghcr-auth.sh @@ -2196,12 +2196,15 @@ claim_node_cordon_ownership() { --output json \ >"${state_file}" 2>"${reread_error_file}"; then # A failed re-read is now the actionable cause, so it replaces the claim - # conflict in the emitted output. The redirection truncates result_file - # before cat runs, so a failed copy would leave it EMPTY -- and - # emit_safe_operation_output skips an empty file entirely, which is the - # very silence this block exists to prevent. Fall back to a deterministic - # non-empty line instead of discarding the failure. - if ! cat "${reread_error_file}" >"${result_file}" 2>/dev/null; then + # conflict in the emitted output. Two things can leave that output empty: + # the copy fails, or kubectl exits non-zero having written no stderr, so the + # diagnostic file is zero bytes and copying it SUCCEEDS while the redirection + # truncates result_file. emit_safe_operation_output skips an empty file + # entirely, which is the very silence this block exists to prevent, so an + # empty diagnostic counts as a failed copy and falls back to a deterministic + # non-empty line. + if [[ ! -s "${reread_error_file}" ]] || + ! cat "${reread_error_file}" >"${result_file}" 2>/dev/null; then echo "node re-read failed; its diagnostic could not be read" \ >"${result_file}" fi diff --git a/scripts/tests/refresh-flux-ghcr-auth/fake_kubectl_test.go b/scripts/tests/refresh-flux-ghcr-auth/fake_kubectl_test.go index 3da09b12e0..29e8ca6708 100644 --- a/scripts/tests/refresh-flux-ghcr-auth/fake_kubectl_test.go +++ b/scripts/tests/refresh-flux-ghcr-auth/fake_kubectl_test.go @@ -1721,6 +1721,19 @@ func fakeKubectlGetNode(args []string) int { } return 0 } + // The re-read that follows a refused cordon claim fails, once. The silent + // variant exits non-zero without writing stderr, so the caller's diagnostic + // file is zero bytes and copying it succeeds while carrying nothing; the + // diagnostic variant writes real stderr, whose content must survive. + if nodeName == os.Getenv("FAKE_NODE_REREAD_FAILURE_NODE") && + markerExists("claim-failed-"+nodeName) && + !markerExists("node-reread-failed-"+nodeName) { + touchMarker("node-reread-failed-" + nodeName) + if diagnostic := os.Getenv("FAKE_NODE_REREAD_FAILURE_DIAGNOSTIC"); diagnostic != "" { + return commandFailure(92, "%s", diagnostic) + } + return 92 + } if nodeName == os.Getenv("FAKE_RECOVERY_ADVANCES_BEFORE_RELEASE_NODE") && !markerExists("recovery-advanced-before-release-"+nodeName) { var recoveryRecord map[string]any @@ -2196,6 +2209,7 @@ func fakeKubectlPatchNode(args []string, patchFile string) int { // and the claim must still refuse rather than drain. if nodeName == os.Getenv("FAKE_CLAIM_FAIL_NODE") { setMarkerContent("resource-version-"+nodeName, incrementDecimal(currentResourceVersion)) + touchMarker("claim-failed-" + nodeName) return commandFailure(56, "resourceVersion test failed") } // An unrelated writer (a kubelet status heartbeat, the cloud diff --git a/scripts/tests/refresh-flux-ghcr-auth/rollout_core_test.go b/scripts/tests/refresh-flux-ghcr-auth/rollout_core_test.go index 0745d8f7bb..9f0164cfe7 100644 --- a/scripts/tests/refresh-flux-ghcr-auth/rollout_core_test.go +++ b/scripts/tests/refresh-flux-ghcr-auth/rollout_core_test.go @@ -409,6 +409,52 @@ func TestConcurrentCordonBeforeAtomicClaimStopsTheRoll(t *testing.T) { } } +// A node re-read can fail without writing anything to stderr. The diagnostic +// file is then zero bytes rather than absent, so copying it succeeds while +// carrying nothing -- and emit_safe_operation_output skips an empty file. The +// operator must still be told why the node claim could not be re-verified. +func TestSilentNodeRereadFailureStillEmitsADiagnostic(t *testing.T) { + t.Parallel() + f := newFixture(t) + result := f.runHelper(validConfig(), nil, map[string]string{ + "FAKE_CLAIM_FAIL_NODE": "prod-worker-1", + "FAKE_NODE_REREAD_FAILURE_NODE": "prod-worker-1", + }) + requireFailureResult(t, result) + + if !pathExists(filepath.Join(f.syncStateDir, "node-reread-failed-prod-worker-1")) { + t.Fatal("fixture did not fail the node re-read after the refused claim") + } + output := result.stdout + result.stderr + requireContains(t, output, "Could not atomically claim and cordon") + requireContains(t, output, "cordon-claim: node re-read failed; its diagnostic could not be read") + requireNoLine(t, readLines(f.operationLog), "node-drain:prod-worker-1") +} + +// The empty-diagnostic guard must not swallow a real one: when the failed +// re-read does write stderr, that text is what the operator needs, not the +// deterministic fallback. +func TestNodeRereadFailureDiagnosticSurvives(t *testing.T) { + t.Parallel() + + const diagnostic = "Error from server (Forbidden): nodes \"prod-worker-1\" is forbidden" + + f := newFixture(t) + result := f.runHelper(validConfig(), nil, map[string]string{ + "FAKE_CLAIM_FAIL_NODE": "prod-worker-1", + "FAKE_NODE_REREAD_FAILURE_NODE": "prod-worker-1", + "FAKE_NODE_REREAD_FAILURE_DIAGNOSTIC": diagnostic, + }) + requireFailureResult(t, result) + + if !pathExists(filepath.Join(f.syncStateDir, "node-reread-failed-prod-worker-1")) { + t.Fatal("fixture did not fail the node re-read after the refused claim") + } + output := result.stdout + result.stderr + requireContains(t, output, "cordon-claim: "+diagnostic) + requireNotContains(t, output, "its diagnostic could not be read") +} + func TestPDBBlockedDrainRestoresOriginalSchedulability(t *testing.T) { t.Parallel() f := newFixture(t)