Skip to content

test(metrics): initialize dirtyPods in TestModule_Reconcile modules - #2667

Closed
Quang Nguyen (nddq) wants to merge 1 commit into
mainfrom
nddq/metrics-test-nil-dirtycache
Closed

test(metrics): initialize dirtyPods in TestModule_Reconcile modules#2667
Quang Nguyen (nddq) wants to merge 1 commit into
mainfrom
nddq/metrics-test-nil-dirtycache

Conversation

@nddq

Copy link
Copy Markdown
Member

Description

TestModule_Reconcile builds four Module literals without dirtyPods — only InitModule initializes that field — and calls Reconcile on them. Reconcile calls run, which starts a ticker on the package constant interval (1 second, metrics_module.go:38) with a context that no test cancels. Each tick calls applyDirtyPods, and the first tick dereferences the nil *DirtyCache:

panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x20 pc=0x21d3fe9]
github.com/microsoft/retina/pkg/common.(*DirtyCache).GetAddList(0x0)
        pkg/common/dirtycache.go:39
github.com/microsoft/retina/pkg/module/metrics.(*Module).applyDirtyPodsAdd(...)
        pkg/module/metrics/metrics_module.go:574

The SIGSEGV kills the whole test binary. Whether it fires is a race between the first tick and process exit: on a machine where the package takes longer than ~2 seconds, go test -tags=unit ./pkg/module/metrics/ fails every run.

This PR sets dirtyPods: common.NewDirtyCache() in the four literals, matching the neighboring TestPodCallBack and TestModule_NamespaceAndPodUpdates. Ticks on an empty cache are no-ops (no filtermanager calls), so the leaked run goroutines stay harmless for the remainder of the process.

Related Issue

N/A — found while validating #2628 locally.

Checklist

  • I have read the contributing documentation.
  • I signed and signed-off the commits (git commit -S -s ...). See this documentation on signing commits.
  • I have correctly attributed the author(s) of the code.
  • I have tested the changes locally.
  • I have followed the project's style guidelines.
  • I have updated the documentation, if necessary.
  • I have added tests, if applicable. (N/A — fixes existing tests)

Screenshots (if applicable) or Testing Completed

At origin/main (792c9324) the panic reproduces on every run of the package on a VM where the suite takes ~2.5 seconds. With this fix, go test -tags=unit -count=10 ./pkg/module/metrics/ passes cleanly.

Additional Notes

go test -race on this package still fails for an unrelated pre-existing reason: TestBaseMetricObject's expire callback writes a test-local variable (basemetricsobject_test.go:54) that the test reads without synchronization (:77). It reproduces at origin/main with -run TestBaseMetricObject alone. CI does not run unit tests with -race, and that fix belongs in its own PR.

Four TestModule_Reconcile cases call Reconcile on Module literals that
leave dirtyPods nil; only InitModule sets it. Reconcile starts run,
whose ticker calls applyDirtyPods every second on a context that no
test cancels. The first tick dereferences the nil DirtyCache and the
SIGSEGV kills the whole test binary.

The crash depends on timing: the binary must stay alive one second
past a Reconcile call. On a machine where the package takes over two
seconds, go test -tags=unit ./pkg/module/metrics/ fails every run.

Initialize dirtyPods in the four literals, matching TestPodCallBack
and TestModule_NamespaceAndPodUpdates. Ticks on an empty cache are
no-ops, so the leaked goroutines stay harmless.

Signed-off-by: Quang Nguyen <28567936+nddq@users.noreply.github.com>
@github-actions

Copy link
Copy Markdown

Retina Code Coverage Report

Total coverage no change

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