Skip to content

test(k8sutils): cover the odigos diagnose support bundle - #5852

Draft
cursor[bot] wants to merge 1 commit into
mainfrom
cursor/missing-test-coverage-8c9f
Draft

cursor[bot] wants to merge 1 commit into
mainfrom
cursor/missing-test-coverage-8c9f

Conversation

@cursor

@cursor cursor Bot commented Sep 19, 2026

Copy link
Copy Markdown
Contributor

What this PR does / why we need it:

k8sutils/pkg/diagnose builds the support bundle that both odigos diagnose (CLI) and the UI's diagnose download produce. It had 3 small test files covering 12 of its 67 functions — the other 55 had no test at all (10.8% of statements), and it is the worst possible place for that: a wrong answer raises no error anywhere. The bundle is written, uploaded and read by a support engineer who has no way to tell that a stage collected the wrong namespace, filed a profile under the wrong component, or skipped a workload entirely.

No production code changed.

Risky behaviour now covered

  • The stage contract between RequestedStages and RunDiagnose — the headline. The UI sizes its progress bar with total: RequestedStages(opts).length and ticks it once per StageResult; the CLI prints one line per requested stage and updates it in place via a stage -> line index map, silently dropping any result that is not in the map. So a stage that runs without being announced overshoots the bar and is invisible in the CLI, and an announced stage that never runs leaves both clients stuck below 100% with no error. Hand-maintained in two places, one if per toggle. Now pinned for all 32 combinations of the five collection toggles: the multiset of stages reported must equal the stages announced.
  • The whole bundle layout, end to end. One full RunDiagnose against a cluster shaped like a real install (odiglet DaemonSet, gateway + UI Deployments, their pods, a ConfigMap, a Destination CR, a Source) asserts the complete list of 33 files. This is the only place the per-stage target directory is checked against the stage that fills it — GetProfileDir/GetMetricsDir/GetConfigMapsDir are three same-shaped calls in RunDiagnose and swapping any two was previously invisible.
  • The deliberate asymmetry that source workloads are collected without logs even when IncludeLogs is on. Streaming logs from every instrumented application saturates the shared client-go rate limiter and cancels the in-flight profile lists — the reason the false is hardcoded. A refactor would undo it silently.
  • Per-workload-kind collection for all seven kinds. Each kind is read through a different client call with a different selector shape; DeploymentConfig reads a flat spec.selector while an Argo Rollout reads spec.selector.matchLabels, distinguished only by a bool argument. Swapping it yields no selector, hence no pods, hence a bundle that looks complete. Also covered: a CronJob/Job with no selector must collect no pods rather than every pod in the namespace, and managedFields must be stripped from every collected object without mutating the cached object it came from.
  • pprof capture sending its query as request parameters rather than in AbsPath. A ? embedded in AbsPath is escaped into the path and never reaches the apiserver, so ?seconds=10 silently becomes pprof's 30-second default. That fix shipped production-only; the path and the query are now asserted separately, per profile type.
  • Which pods each profiled service and each metrics tier reads, and on which port. The odiglet pod carries both the odiglet name label and the node-collector role label because it runs both processes, each with its own pprof port and its own metrics port; (fileSuffix, port) are two same-shaped values one line apart. Every service/tier now has its own fixture and a per-file content assertion, so a selector or port swapped between two of them fails.
  • CRD discovery is a group suffix match. Decoy groups notodigos.io and odigos.io.example.com prove a customer's lookalike CRDs (which may hold secrets) are not swept into the bundle, alongside subresource skipping and the namespaced-list fallback for a caller who cannot list cluster-wide.
  • Partial failure everywhere. A denied list, an unreadable workload, an unreachable scrape target and an unwritable file must each cost only their own item: one denied kind must not cost the other three, and a failing stage must still report its own error through StageResult.Status while every other stage succeeds.
  • Two known gaps pinned as-is (see below), plus FormatBytes, GetRootDir's documented odigos_debug_ddmmyyyyhhmmss name, and a reflection gate that fails if a collection toggle is added to Options without being classified in DefaultOptions.

Test files added

All new, all in k8sutils/pkg/diagnose (the three existing test files are untouched):

file what it covers
helpers_test.go shared fixtures: a recording Builder, a minimal apiserver for the pod-proxy reads, the fake dynamic/odigos clients
diagnose_test.go the stage contract over all 32 toggle combinations, runStage, and the end-to-end bundle layout
workloads_collect_test.go all seven workload kinds, selector extraction, Source expansion and exclusions
crds_test.go CRD discovery, collection and the namespaced fallback
metrics_test.go both collector tiers and the pod-proxy metrics read
profiles_capture_test.go the pprof query contract and per-service selector/port pairing
logs_test.go container, init-container and previous-revision log collection
builder_test.go both builders, the bundle directory layout, FormatBytes, DefaultOptions
configmaps_test.go ConfigMap collection

Why these tests materially reduce regression risk

10.8% -> 97.0% of statements, 55 functions at 0% -> 0, and the suite runs in ~5s (one test deliberately waits out the 5s capture retry pause). It is race-clean under -race, and go vet ./... plus golangci-lint run (v2.10.1, the version CI uses) are clean.

The tests were validated by mutation testing rather than by coverage alone: 102 deliberate faults were injected one at a time into the package and 98 were caught. The four survivors are a deliberate no-op control (which must survive, proving the harness runs the tests) and three provably equivalent mutants — an unparsable API group version that the group filter rejects anyway, a duplicated path slash that client-go normalises, and an early return from a retry pause that a cancelled context already makes return immediately. Designing those mutations found two real holes in the tests, both fixed before this PR: the gzipped-file byte accounting was unasserted, and nothing proved the already-collected key includes the workload kind, so a Deployment and a StatefulSet sharing a name in one namespace would have silently collapsed to one.

Warts found, reported but deliberately not fixed

Both are pinned with the current behaviour and a comment, so fixing them is a deliberate one-line test change rather than a surprise:

  1. A Source for a Job workload is never collected. workload.IsValidWorkloadKind has no Job case, so categorizeSourcesForDiagnose drops it — even though collectWorkload knows perfectly well how to collect a Job, and listCollectableWorkloadsInNamespace does not list Jobs either. This is the fourth place this automation has found a missing Job case in a kind list; one fix in k8sutils/pkg/workload closes several of them.
  2. A StatefulSet in the odigos namespace is never collected. FetchOdigosWorkloads's docstring says it collects "deployments, daemonsets, statefulsets" but it only lists the first two, so a StatefulSet deployed alongside odigos is missing from every bundle.

Additional Kubernetes versions to test:

None — these are unit tests in k8sutils and run in the test-k8sutils job, not in the e2e matrix.

Changelog entry: Does this PR introduce a user-facing bug fix, feature, dependency update, or breaking change??

NONE
Open in Web View Automation 

The diagnose package assembles the support bundle that both `odigos
diagnose` and the UI's diagnose download produce, and 55 of its 67
functions had no test at all. A wrong answer here raises no error: the
bundle is written, uploaded and read by a support engineer who has no way
to tell that a stage collected the wrong namespace, filed a profile under
the wrong component or skipped a workload entirely.

The load-bearing contract is between RequestedStages and the stages
RunDiagnose actually reports. The UI sizes its progress bar from the
former and ticks it from the latter, and the CLI maps stage to output line
the same way, so a divergence leaves both stuck below 100% with no error.
That is now pinned for all 32 combinations of the collection toggles, next
to a full end-to-end collection that asserts the complete bundle layout.

Package statement coverage goes from 10.8% to 97.0% with every function
exercised; no production code changes.

Co-authored-by: Eden Federman <edenf2k@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant