test(k8sutils): cover the odigos diagnose support bundle - #5852
Draft
cursor[bot] wants to merge 1 commit into
Draft
cursor[bot] wants to merge 1 commit into
cursor[bot] wants to merge 1 commit into
Conversation
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>
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
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 / why we need it:
k8sutils/pkg/diagnosebuilds the support bundle that bothodigos 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
RequestedStagesandRunDiagnose— the headline. The UI sizes its progress bar withtotal: RequestedStages(opts).lengthand ticks it once perStageResult; the CLI prints one line per requested stage and updates it in place via astage -> line indexmap, 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, oneifper toggle. Now pinned for all 32 combinations of the five collection toggles: the multiset of stages reported must equal the stages announced.RunDiagnoseagainst 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/GetConfigMapsDirare three same-shaped calls inRunDiagnoseand swapping any two was previously invisible.IncludeLogsis on. Streaming logs from every instrumented application saturates the shared client-go rate limiter and cancels the in-flight profile lists — the reason thefalseis hardcoded. A refactor would undo it silently.DeploymentConfigreads a flatspec.selectorwhile an Argo Rollout readsspec.selector.matchLabels, distinguished only by aboolargument. 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, andmanagedFieldsmust be stripped from every collected object without mutating the cached object it came from.AbsPath. A?embedded inAbsPathis escaped into the path and never reaches the apiserver, so?seconds=10silently becomes pprof's 30-second default. That fix shipped production-only; the path and the query are now asserted separately, per profile type.(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.notodigos.ioandodigos.io.example.comprove 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.StageResult.Statuswhile every other stage succeeds.FormatBytes,GetRootDir's documentedodigos_debug_ddmmyyyyhhmmssname, and a reflection gate that fails if a collection toggle is added toOptionswithout being classified inDefaultOptions.Test files added
All new, all in
k8sutils/pkg/diagnose(the three existing test files are untouched):helpers_test.goBuilder, a minimal apiserver for the pod-proxy reads, the fake dynamic/odigos clientsdiagnose_test.gorunStage, and the end-to-end bundle layoutworkloads_collect_test.gocrds_test.gometrics_test.goprofiles_capture_test.gologs_test.gobuilder_test.goFormatBytes,DefaultOptionsconfigmaps_test.goWhy 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, andgo vet ./...plusgolangci-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
returnfrom 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:
Jobworkload is never collected.workload.IsValidWorkloadKindhas noJobcase, socategorizeSourcesForDiagnosedrops it — even thoughcollectWorkloadknows perfectly well how to collect a Job, andlistCollectableWorkloadsInNamespacedoes not list Jobs either. This is the fourth place this automation has found a missingJobcase in a kind list; one fix ink8sutils/pkg/workloadcloses several of them.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
k8sutilsand run in thetest-k8sutilsjob, not in the e2e matrix.Changelog entry: Does this PR introduce a user-facing bug fix, feature, dependency update, or breaking change??