diff --git a/client/Chart.yaml b/client/Chart.yaml index d8b3ed31..ecd2e38c 100644 --- a/client/Chart.yaml +++ b/client/Chart.yaml @@ -2,8 +2,8 @@ apiVersion: v2 name: client description: A unified Helm chart for tracebloc on AKS, EKS, bare-metal, and OpenShift type: application -version: 1.9.116 -appVersion: "1.9.116" +version: 1.9.117 +appVersion: "1.9.117" keywords: - tracebloc - kubernetes diff --git a/client/templates/image-refresh-cronjob.yaml b/client/templates/image-refresh-cronjob.yaml index 352c0921..c57cf07a 100644 --- a/client/templates/image-refresh-cronjob.yaml +++ b/client/templates/image-refresh-cronjob.yaml @@ -500,6 +500,13 @@ data: restart_needed=0 annotate_args="" + # Pass-0 diagnostic annotations -- the `stale-pin-` clears and + # finding-sets -- accumulate SEPARATELY from annotate_args and are written in + # their own bounded annotate BEFORE the restart block (#1008 item 1). + # They touch only the stale-pin keys, never `last-refreshed--digest`, + # so they are safe to land before the rollout; keeping them out of the final + # annotate is what lets them survive a latched-flap tick (see the write site). + stale_pin_args="" # `kubectl set image` argument lists, accumulated per WORKLOAD (a single # `set image` call can carry several container=ref pairs, so the # two-container jobs-manager Deployment is re-imaged in one patch and @@ -585,7 +592,7 @@ data: disabled_stale_key="tracebloc.io/stale-pin-${repo#*/}" if [ -n "$(get_annotation "$disabled_stale_key" || true)" ]; then log " clearing ${disabled_stale_key}: this image is no longer pinned by a digest" - annotate_args="$annotate_args ${disabled_stale_key}-" + stale_pin_args="$stale_pin_args ${disabled_stale_key}-" fi continue fi @@ -613,7 +620,7 @@ data: # (@saqlainsyed007 + Bugbot on client#824.) if [ -n "$(get_annotation "$stale_key" || true)" ]; then log " clearing a previous ${stale_key}: the pin is current again" - annotate_args="$annotate_args ${stale_key}-" + stale_pin_args="$stale_pin_args ${stale_key}-" fi else log " WARN: PIN IS STALE. values pin ${pin_digest}" @@ -624,7 +631,7 @@ data: log " in values (backend#2458)." # Queryable after the log ages out, on the same object the refresh # annotations use, so `kubectl describe` shows pin state beside refresh state. - annotate_args="$annotate_args ${stale_key}=${pin_latest}" + stale_pin_args="$stale_pin_args ${stale_key}=${pin_latest}" fi continue fi @@ -637,7 +644,7 @@ data: unpinned_stale_key="tracebloc.io/stale-pin-${repo#*/}" if [ -n "$(get_annotation "$unpinned_stale_key" || true)" ]; then log " clearing ${unpinned_stale_key}: this image is no longer pinned" - annotate_args="$annotate_args ${unpinned_stale_key}-" + stale_pin_args="$stale_pin_args ${unpinned_stale_key}-" fi latest="$(get_latest_digest "$repo" "$IMAGE_TAG" "$IMAGE_REGISTRY" || true)" @@ -881,6 +888,32 @@ data: esac done + # Pass-0 annotations land HERE, before the restart block (#1008 item 1). + # The restart block's #563 flap guard does `WARN + FLAP_KEY + exit 0` + # once refresh-attempt >= MAX_REFRESH_ATTEMPTS -- BEFORE the digest-record + # annotate at the end of the tick. So on a tick that is both off-digest + # (restart_needed=1) and latched, batching the stale-pin writes into that + # final annotate dropped them: a stale-pin CLEAR that never landed leaves a + # FALSE "pin is stale" finding to persist forever (a write-only annotation + # outliving its problem -- the class client#824's clear paths fixed), and it + # is dropped on the exact tick refresh is dead, when the finding matters most. + # These touch only the `stale-pin-` keys, never + # `last-refreshed--digest`, so writing them before the rollout cannot + # affect the `recorded == latest` skip logic -- unlike the digest record, + # which MUST stay after a successful `rollout status`, since annotating the + # digest before a failed rollout would freeze the workload on the old image + # (@shujaatTracebloc on #1008). NON-FATAL, like the SKIP_KEY clear above + # (backend#2007): a transient failure on a diagnostic annotation must not + # abort the tick before the re-image; a stale value is re-reconciled next tick. + if [ -n "$stale_pin_args" ]; then + log "updating stale-pin annotations:$stale_pin_args" + # shellcheck disable=SC2086 # word-split stale_pin_args intentional + if ! sp_err="$(kubectl annotate deployment -n "$RELEASE_NAMESPACE" "$DEPLOYMENT_NAME" \ + $stale_pin_args --overwrite --request-timeout=15s 2>&1 >/dev/null)"; then + log " WARNING: could not update stale-pin annotations on deployment/${DEPLOYMENT_NAME}: ${sp_err:-unknown error}. Continuing -- these are diagnostic bookkeeping and a stale value is re-reconciled on the next tick." + fi + fi + # Order matters: rollout FIRST, annotate AFTER `rollout status` # succeeds. Annotating first would let a failed rollout silently # freeze the deployment on the old image (next tick sees diff --git a/client/tests/image_refresh_test.yaml b/client/tests/image_refresh_test.yaml index ad457ebf..488314d3 100644 --- a/client/tests/image_refresh_test.yaml +++ b/client/tests/image_refresh_test.yaml @@ -1020,3 +1020,49 @@ tests: - notMatchRegex: path: data["image-refresh.sh"] pattern: 'recorded="\$\(get_annotation "\$key" \|\| true\)"' + + - it: stale-pin annotations are written BEFORE the restart block, so they survive a latched flap + # Guards #1008 item 1. The #563 flap guard does WARN + FLAP_KEY + + # exit 0 once refresh-attempt >= MAX, BEFORE the final digest-record annotate. + # Batching the stale-pin CLEARS into that final annotate dropped them on a + # tick that is both off-digest and latched -- leaving a FALSE stale-pin + # finding to persist. They now accumulate in their own list and are annotated + # above the restart block. image-refresh-latched-annotate.bats asserts the + # BEHAVIOUR; these lock the code shapes. + template: templates/image-refresh-cronjob.yaml + documentIndex: 0 + asserts: + # ALL FOUR stale-pin writes go to their own accumulator, not annotate_args. + # Every key gets a positive (goes to stale_pin_args) AND a negative (does NOT + # ride annotate_args) -- Bugbot Low + @saadqbal on #1039: a key covered by + # neither could be re-batched into annotate_args and dropped on a latched + # tick while both suites stayed green (this guard's own thesis, one level up). + # The two clears: + - matchRegex: + path: data["image-refresh.sh"] + pattern: 'stale_pin_args="\$stale_pin_args \$\{stale_key\}-"' + - matchRegex: + path: data["image-refresh.sh"] + pattern: 'stale_pin_args="\$stale_pin_args \$\{unpinned_stale_key\}-"' + # the disabled-monitor clear (the third clear -- was uncovered): + - matchRegex: + path: data["image-refresh.sh"] + pattern: 'stale_pin_args="\$stale_pin_args \$\{disabled_stale_key\}-"' + # the finding-SET (a lost set means a real staleness goes unreported): + - matchRegex: + path: data["image-refresh.sh"] + pattern: 'stale_pin_args="\$stale_pin_args \$\{stale_key\}=\$\{pin_latest\}"' + # and that accumulator is annotated BEFORE the restart block (ordering) + - matchRegex: + path: data["image-refresh.sh"] + pattern: '(?s)if \[ -n "\$stale_pin_args" \]; then.*if \[ "\$restart_needed" -eq 1 \]; then' + # NONE of the stale-pin keys may ride the final digest-record annotate_args + - notMatchRegex: + path: data["image-refresh.sh"] + pattern: 'annotate_args="\$annotate_args \$\{stale_key\}' + - notMatchRegex: + path: data["image-refresh.sh"] + pattern: 'annotate_args="\$annotate_args \$\{unpinned_stale_key\}' + - notMatchRegex: + path: data["image-refresh.sh"] + pattern: 'annotate_args="\$annotate_args \$\{disabled_stale_key\}' diff --git a/scripts/tests/image-refresh-latched-annotate.bats b/scripts/tests/image-refresh-latched-annotate.bats new file mode 100644 index 00000000..c7a54ba4 --- /dev/null +++ b/scripts/tests/image-refresh-latched-annotate.bats @@ -0,0 +1,164 @@ +#!/usr/bin/env bats +# image-refresh writes the Pass-0 stale-pin annotations BEFORE the restart block, +# so they survive a tick that is both off-digest (restart_needed=1) and LATCHED +# (refresh-attempt >= MAX_REFRESH_ATTEMPTS). +# +# #1008 item 1. The #563 flap guard does `WARN + FLAP_KEY + exit 0` once +# the attempt counter reaches MAX -- BEFORE the digest-record annotate at the end +# of the tick. When the stale-pin CLEARS were batched into that final annotate, +# a latched tick dropped them, leaving a FALSE "pin is stale" finding to persist +# forever -- and on the exact tick refresh is dead, when the finding matters most. +# The fix moves the stale-pin writes into their own bounded annotate above the +# restart block; the `last-refreshed` digest record deliberately stays BELOW, +# after a successful rollout (@shujaatTracebloc on #1008). +# +# This asserts BEHAVIOUR: it extracts the shipped tail (the stale-pin annotate + +# the restart block + the final digest annotate) from the RENDERED chart and +# drives it with kubectl and the attempt-counter read stubbed, so re-batching the +# stale-pin writes back into the final annotate reddens. + +setup() { + TMP="$(mktemp -d)" + CHART="${BATS_TEST_DIRNAME}/../../client" + helm template t "$CHART" --set clientId=x --set clientPassword=y \ + --set storageClass.create=false > "$TMP/rendered.yaml" + python3 - "$TMP/rendered.yaml" "$TMP/tail.sh" <<'PYX' +import sys + +try: + import yaml +except ImportError: + sys.exit("[ERROR] PyYAML required (pip install pyyaml)") + +MARKER = "already on the pinned digest; no-op" + +def walk(o): + if isinstance(o, str) and MARKER in o: + return o + if isinstance(o, dict): + for v in o.values(): + r = walk(v) + if r: + return r + if isinstance(o, list): + for v in o: + r = walk(v) + if r: + return r + +script = None +for d in yaml.safe_load_all(open(sys.argv[1])): + if not d: + continue + script = walk(d) + if script: + break +assert script, "no rendered image-refresh script found" + +lines = script.splitlines() +start = next(i for i, l in enumerate(lines) + if l.strip() == 'if [ -n "$stale_pin_args" ]; then') +# the LAST `log "tick complete"` -- the flap-guard early exits use the same line, +# so the first match would truncate the region mid-restart-block. +end = max(i for i in range(start, len(lines)) + if lines[i].strip() == 'log "tick complete"') +region = lines[start:end + 1] +indent = min(len(l) - len(l.lstrip()) for l in region if l.strip()) +open(sys.argv[2], "w").write("\n".join(l[indent:] for l in region)) +PYX +} +teardown() { rm -rf "$TMP"; } + +# Drives the shipped tail with kubectl + the ATTEMPT_KEY read stubbed. +# $1 = STUB_ATTEMPT what get_annotation returns for ATTEMPT_KEY (the flap count) +# $2 = JM_SET_ARGS `set image` args (non-empty => a rollout runs, stubbed OK) +# stale_pin_args and annotate_args are always populated so the test can assert +# which of the two landed. +# +# The kubectl stub records EVERY call to "$TMP/calls.log" rather than stdout, +# because the stale-pin annotate is wrapped in a non-fatal handler that discards +# its stdout (`2>&1 >/dev/null`) -- exactly as a real silent-success annotate +# would. The file captures the call regardless of the caller's redirections; +# assert kubectl invocations against "$TMP/calls.log" and log lines against stdout. +run_tail() { + : > "$TMP/calls.log" + cat > "$TMP/harness.sh" <> "\$CALLS"; } +get_annotation() { case "\$1" in "\$ATTEMPT_KEY") printf '%s' "\$STUB_ATTEMPT" ;; esac; } +$(cat "$TMP/tail.sh") +EOF + sh "$TMP/harness.sh" "${1:-0}" "${2:-}" +} + +@test "the harness really extracted the shipped tail (not an empty file)" { + [ -s "$TMP/tail.sh" ] || return 1 + grep -q 'stale_pin_args' "$TMP/tail.sh" || return 1 + grep -q 'restart_needed' "$TMP/tail.sh" || return 1 +} + +@test "LATCHED tick (restart_needed=1, attempt>=MAX): stale-pin clear LANDS, digest record does NOT" { + # The acceptance case (#1008 item 1). attempt=3, MAX=3 -> the flap guard + # WARNs, annotates FLAP_KEY, and exit 0s. The stale-pin clear must already have + # been written (before the restart block); the last-refreshed digest record + # must NOT be (its annotate is after the guard and never runs). + run run_tail "3" + [ "$status" -eq 0 ] || return 1 + calls="$(cat "$TMP/calls.log")" + # stale-pin clear landed, above the restart block + [[ "$calls" == *"annotate deployment"*"tracebloc.io/stale-pin-jobs-manager-"* ]] || return 1 + # the flap guard fired + [[ "$output" == *"FLAP DETECTED"* ]] || return 1 + [[ "$calls" == *"tracebloc.io/refresh-flap-detected=3"* ]] || return 1 + # the digest record did NOT land (dropped by the exit 0, as designed) + [[ "$calls" != *"last-refreshed-jobs-manager-digest=sha256:beef"* ]] || return 1 +} + +@test "NON-latched tick (attempt