Repository navigation
fix(e2e): select catch pod logs by the nested release instance label - #3564
Merged
Aleksei Sviridkin (lexfrei) merged 1 commit intoAug 6, 2026
Merged
Conversation
The harbor, openbao and qdrant app charts deploy their workload through a nested HelmRelease named `<release>-system`, and pass `fullnameOverride: <release>` down to it. The inner chart therefore stamps app.kubernetes.io/instance from its own release name, with the suffix, while object names come from the override, without it. The catch blocks selected on the outer release name, so podLogs matched no pods and every failed run collected an empty result. A pod name says nothing about its instance label; the two come from different sources on purpose. In qdrant the unsuffixed value is therefore not retired but kept as a collector of its own: the parent chart's PVC-cleanup Job labels its pod template with the parent release name, and that suite deletes the parent inside a `try:`, so the Job is in scope while the test still runs. The comment there is explicit about how narrow that window is -- the hook always exits 0, so Helm calls it succeeded and `hook-succeeded` takes the pod away; what is left is an uninstall that hangs with Helm still waiting on it. Harbor renders the same hook and does not get the same collector. It has no `delete` op, so its parent release is uninstalled only in Chainsaw's cleanup phase, which no catch can observe: catch is a defer inside the step, while the cleaner runs from t.Cleanup, after the test function returns. That collector would be the same permanently-empty capture this commit removes. A comment records the asymmetry so the next reader does not restore it for symmetry. openbao renders no such hook at all. Each workload value is now also pinned on a resource in an assert the suite already runs, so the chart renaming a label fails a step instead of quietly emptying a capture. The comments state what that does and does not cover: it does not catch a wrong value typed into a selector, and it catches nothing when the run fails before the assert. Harbor's database was reached by no collector at all. It is selected on `cnpg.io/cluster`, CNPG's own primary label, rather than on the instance label CNPG merely derives from the Cluster name -- the primary label has nothing to drift from, and it is what the postgres suite and the WorkloadMonitors already use. Each lookup keeps stderr apart from the value it captures. kubectl exits 0 while logging a klog line about an API group it could not reach, and an aggregated APIService in that state is what a failed run usually looks like; folded into the value, that line makes the "nothing found" branch unreachable and prints as though it were a result. Telling those outcomes apart is one helper in hack/e2e-chainsaw/_lib/ rather than a copy per call site, separated from the kubectl call so it can be pinned without a cluster, the way etcd-probe.sh already is. Both the census and the qdrant cleanup listing report through it, differing only in wording, so the discrimination is implemented and tested once. The tests cover each outcome for both wordings, including the zero exit with a klog line on stderr -- the case a folded stream turned into a row. A collector that matches nothing cannot say so: podLogs is silent, and `kubectl get -l` exits zero on an empty match. Each test-wide catch prints the instance label of every pod in the namespace, and the qdrant cleanup catch reports its three outcomes apart from a single call -- printing what that call returned rather than running it again, which could put a healthy listing under a failure heading. That census runs on a degraded cluster, where kubectl otherwise retries discovery against an unreachable apiserver until something kills the script, so every lookup carries `--request-timeout`. Measured against an unreachable apiserver, a bounded call costs about five times the flag, since kubectl retries discovery five times before it gives up. Each script then carries an explicit op timeout sized above its own calls -- 2m for the single-call censuses, 5m for the qdrant cleanup listing, which makes three. An inner budget is what should fire: a SIGKILL at the op boundary yields nothing, where a kubectl that gives up leaves an error to print. Assisted-By: Claude <[email protected]> Signed-off-by: Aleksei Sviridkin <[email protected]>
Aleksei Sviridkin (lexfrei)
requested a review
from myasnikovdaniil
as a code owner
August 5, 2026 14:19
github-actions
Bot
added
size/L
Contributor
📝 WalkthroughWalkthroughThe PR adds a shared POSIX shell library for pod-label diagnostics, tests its reporting behavior, and integrates bounded label and cleanup diagnostics into Harbor, OpenBao, and Qdrant Chainsaw tests. ChangesPod diagnostics
Estimated code review effort: 3 (Moderate) | ~25 minutes Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@hack/e2e-chainsaw/_lib/pod-label-census.sh`:
- Around line 96-108: Handle failure of _plc_errfile=$(mktemp) in
pod_label_census so set -e cannot exit before the function’s promised return 0;
either guard the temporary-file assignment or add the requested PATH-stub test
covering a failing mktemp and asserting pod_label_census returns zero.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 045f1365-d73a-435f-a445-31765f3c3da7
📒 Files selected for processing (5)
hack/e2e-chainsaw/_lib/pod-label-census.shhack/e2e-chainsaw/harbor/chainsaw-test.yamlhack/e2e-chainsaw/openbao/chainsaw-test.yamlhack/e2e-chainsaw/qdrant/chainsaw-test.yamlhack/pod-label-census_test.bats
Comment on lines
+96
to
+108
| _plc_errfile=$(mktemp) | ||
|
|
||
| if _plc_stdout=$(kubectl -n "$_plc_ns" --request-timeout=15s get pods \ | ||
| -o 'custom-columns=POD:.metadata.name,INSTANCE:.metadata.labels.app\.kubernetes\.io/instance' \ | ||
| --no-headers 2>"$_plc_errfile"); then | ||
| _plc_status=0 | ||
| else | ||
| _plc_status=1 | ||
| fi | ||
|
|
||
| pod_label_census_report "$_plc_status" "$_plc_stdout" "$(cat "$_plc_errfile")" "$_plc_ns" | ||
| rm -f "$_plc_errfile" | ||
| return 0 |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
printf 'Repository files matching pod-label-census:\n'
fd -a 'pod-label-census\.sh' . || true
printf '\nFile outline / contents:\n'
for f in $(fd 'pod-label-census\.sh' .); do
echo "=== $f ==="
wc -l "$f"
cat -n "$f"
done
printf '\nSearch for set -e / errexit / trap usage around lib / chainsaw:\n'
rg -n --hidden --glob '!node_modules' --glob '!dist' --glob '!build' 'set -e|set +e|set -u|set -o pipefail|trap|e2e-chainsaw|catch' hack .github 2>/dev/null | head -200Repository: cozystack/cozystack
Length of output: 22000
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
# Read-only behavioral probe: parse the shell function and model POSIX errexit
# for simple assignment commands and non-control command statements in sequence.
# We do not run repository code.
import re, pathlib
p = next(pathlib.Path('.').rglob('pod-label-census.sh'))
text = p.read_text()
m = re.search(r'^( \|\|\|\|\| \d+:\d+)\npod_label_census\(\) \{.*?^\}', text, re.M|re.S)
if not m:
print("FUNCTION_NOT_FOUND")
raise SystemExit
func = m.group(0)
print("FOUND")
print(func)
# Locate command statements from mktemp assignment through cleanup within the function block.
for i, line in enumerate(func.splitlines(), 1):
s = line.strip()
if s.startswith('if _plc_stderr='):
print(f"if_assignment_line={i}: {s}")
print(" simple_command_in_if_condition=", bool(re.fullmatch(r'if[^;]*\$\([^)]*\).*', s)))
if s.startswith('rm -f'):
print(f"cleanup_line={i}: {s}")
print(" simple_command_statement=", bool(re.fullmatch(r'rm\s+-f\s+["\'\$a-z/_-]*\s*(&&.*|\\|.*|;|;.*|;\s*)(\n|\Z)', s) or re.fullmatch(r'rm\s+-f\s+[^;&|]*(&&.*|\\|.*|;.*|;\s*)$', s)))
PYRepository: cozystack/cozystack
Length of output: 176
🏁 Script executed:
#!/bin/bash
set -eu
printf 'Read existing pod_label_census_test.bats:\n'
wc -l hack/pod-label-census_test.bats
cat -n hack/pod-label-census_test.bats
printf '\nShow similar BATS setups using set -eu -x and sourcing this lib:\n'
sed -n '1,55p' hack/talos-image-cache_test.bats
printf '\n'
sed -n '1,55p' hack/promote-rewrite-tags_test.bats
printf '\nSearch for exact catch context mentioning pod_label_census:\n'
rg -n --hidden --context 8 'pod_label_census|catch|set -eu -x|set -e' docs hack/e2e-chainsaw hack/pod-label-census_test.bats 2>/dev/null | head -250Repository: cozystack/cozystack
Length of output: 50377
🏁 Script executed:
#!/bin/bash
# Read-only POSIX shell errexit probe for the literal control-flow in this helper.
tmpdir="$(mktemp -d)"
export PATH="$tmpdir:$PATH"
cat > "$tmpdir/mktemp" <<'EOF'
exit 1
EOF
set +e
. hack/e2e-chainsaw/_lib/pod-label-census.sh
pod_label_census dummy
echo "exit_status=$?"Repository: cozystack/cozystack
Length of output: 325
Cover temporary-file handling failures.
_plc_errfile=$(mktemp) is a simple assignment command, so set -e can exit before the promised return 0 in a catch path. Add a PATH stub case for a failing mktemp that still asserts pod_label_census returns zero, or guard the assignment in the helper.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@hack/e2e-chainsaw/_lib/pod-label-census.sh` around lines 96 - 108, Handle
failure of _plc_errfile=$(mktemp) in pod_label_census so set -e cannot exit
before the function’s promised return 0; either guard the temporary-file
assignment or add the requested PATH-stub test covering a failing mktemp and
asserting pod_label_census returns zero.
Contributor
Author
|
The red is the exposure the description already names. Run The three suites this changes all passed: That has a cost worth stating. A |
IvanHunters
approved these changes
Aug 6, 2026
IvanHunters
left a comment
IvanHunters
left a comment
Collaborator
There was a problem hiding this comment.
LGTM.
The fix premise holds: the workload ships via a nested <release>-system HelmRelease and the nested chart stamps app.kubernetes.io/instance with the suffix, so the old release-name selector matched nothing. Confirmed for harbor, openbao and qdrant against the charts. All three affected chainsaw suites pass in CI; the E2E failure is an unrelated bring-up flake (node-join in kubernetes-previous/latest). The census unit test is non-vacuous (two mutations turn the targeted tests red; 9/9 offline).
Non-blocking notes:
- hack/e2e-chainsaw/_lib/pod-label-census.sh:
_plc_errfile=$(mktemp)runs underset -euin the three test-wide catch scripts. If mktemp fails (disk pressure on the runner, exactly the state where this catch fires) the census script aborts before any report branch, contradicting the file's "always returns 0" invariant. Cheap fix:set -uwithout-e(as in the qdrant cleanup catch), or guard the mktemp. Test-infra only, does not distort the verdict. - Comment arithmetic in qdrant/chainsaw-test.yaml (~240s vs the ~225s implied by the helper's own model). Harmless.
Caveat worth noting for future readers: the selector value inside the podLogs catch collectors is not exercised in CI (catch only fires on step failure); it is protected indirectly by the three passing steps: asserts and the census backstop. A typo in a collector selector value would not be caught by any test — this is documented in-code.
Aleksei Sviridkin (lexfrei)
deleted the
fix/e2e-chainsaw-instance-selectors
branch
August 6, 2026 12:25
Merged
1 of 11 tasks
2 of 8 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this PR does
The harbor, openbao and qdrant catch blocks selected pod logs on the app release name. Each of these charts deploys its workload through a nested HelmRelease named
<release>-systemand passesfullnameOverride: <release>down to it, so the inner chart stampsapp.kubernetes.io/instancefrom its own release name, with the suffix, while object names come from the override, without it. The selector matched no pods, and a failed run collected nothing.podLogscatch collectors at the-systeminstance labelpodLogscollector for Harbor's CNPG database, which no collector reached before, selected oncnpg.io/clusterrather than on the instance label CNPG merely derives from the Cluster namepodLogscannot report that it matched nothingtry:Harbor renders the same hook and deliberately does not get the same collector: it has no
deleteop, so its parent release is uninstalled only in Chainsaw's cleanup phase, which no catch can observe: catch is a defer inside the step, while the cleaner runs fromt.Cleanup, after the test function returns. A comment records that asymmetry so the next reader does not restore the collector for symmetry.Each comment states what the pin does and does not cover: it does not catch a wrong value typed into a selector, and it catches nothing when a run fails before reaching the assert. The qdrant comment is equally explicit about how narrow its collector's window is, because the hook always exits 0, so Helm calls it succeeded and
hook-succeededtakes the pod away; what remains is an uninstall that hangs with Helm still waiting.Telling a lookup's outcomes apart is one helper under
hack/e2e-chainsaw/_lib/rather than a copy per call site, with the outcome logic split from the kubectl call so it can be pinned without a cluster — the shape_lib/etcd-probe.shalready uses. Both the census and the qdrant cleanup listing report through it, differing only in wording. Each lookup keeps stderr apart from the value it captures: kubectl exits 0 while logging that it could not reach an API group, and an aggregated APIService in that state is what a failed run usually looks like, so folding the streams would make the "nothing found" branch unreachable and print that log line as though it were a result.Note what the extraction costs in CI: touching the shared
_lib/and adding a top-levelhack/*.batsescalates test-impact analysis from the three suites this change edits to the full Chainsaw suite, so this PR runs everything and is exposed to the whole spread of known flakes. A red suite unrelated to harbor, openbao or qdrant is that exposure, not a defect in this diff.Testing
hack/pod-label-census_test.bats, 9 cases over the helper's outcomes, run viahack/cozytest.shand picked up bymake unit-teststhrough the existingBATS_UNIT_FILESglobchainsaw lint test --fileon each of the three suites, andchainsaw test --no-cluster --config hack/e2e-chainsaw/.chainsaw.yaml --include-test-regex '^$'over all three, both with v0.2.15, the version the sandbox image pinsshellcheck --shell=shon the helper and on the catch scripts extracted from the YAMLScreenshots
Not applicable.
Downstream repositories
Release note
Summary by CodeRabbit
New Features
Bug Fixes
Tests