attachdetach: wait for the force detach metric in reconciler tests by LiRunGuo · Pull Request #142385 · kubernetes/kubernetes

LiRunGuo · 2026-09-24T12:58:17Z

What type of PR is this?

/kind flake
/sig storage

What this PR does / why we need it:

Two fixes for the force detach metric checks in the reconciler tests.

testForceDetachMetric read the counter once, right after the test saw the
fake detacher called. But the reconciler increments the counter only after
DetachVolume returns, while the detach itself runs in a goroutine started
by the operation executor (nestedpendingoperations). If that goroutine
wins, the test sees the detach before the increment:
```
reconciler_test.go:962: Expected desiredForceDetachMericCounter to be 1, got 0
```
It now polls with the file's existing waitForCondition, like the other
assertions here. For "unchanged" checks it returns on the first read.
The tests start the reconciler with go reconciler.Run(ctx) and only cancel
it on return, so a reconciler can finish one more loop after its test is
done. If that loop force detaches, the global counter changes under the next
test and its "unchanged" check fails (the failure linked in review, from
Test_Run_OneVolumeDetachOnUnhealthyNode). A runReconciler helper now
cancels the reconciler and waits for Run to return in t.Cleanup; all 14
call sites use it. The metric is recorded on the reconciler goroutine, so
nothing can change it after Run returns.

Which issue(s) this PR is related to:

Fixes #142349

Special notes for your reviewer:

Reproduced on an x86 host under CPU contention (processes pinned to a few
cores, GOMAXPROCS=1, -race). "Leaked" counts log lines that ktesting tags
as coming from a reconciler that outlived its test.

setup	master	first commit	both commits
3 tests with metric checks, 4 cores, 45,000 runs	49 failures	0	0
all `Test_Run_*`, 8 cores, 3,000 iterations	0 failures, 70 leaked	1 failure (lines 1281/1302), 63 leaked	0 failures, 0 leaked

make test WHAT=./pkg/controller/volume/attachdetach/... with -count=3
passes.

The same helpers exist on release branches (the issue also lists
ci-kubernetes-unit-1-34), so this may be worth cherry-picking.

AI disclosure: an AI assistant (Claude) helped find the issue, write the
stress harness and the fix, and draft this description. I reviewed the change
and ran the tests.

Does this PR introduce a user-facing change?

NONE

The reconciler records the force detach metric after DetachVolume returns, but the detach itself runs in a goroutine started by the operation executor. The tests wait for the fake detacher to be called and then read the counter once, so on a slow or loaded machine they can observe the detach before the increment. Poll the counter the same way the other assertions in this file do.

kubernetes-prow · 2026-09-24T12:58:26Z

This issue is currently awaiting triage.

If a SIG or subproject determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

kubernetes-prow · 2026-09-24T12:58:28Z

Hi @LiRunGuo. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work.

Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

matteopulcini6 · 2026-09-26T12:31:27Z

This commit is similar to the one I drafted on my local. lgtm

(base) matteo@WuDrew:~/kubernetes/pkg/controller/volume/attachdetach/reconciler$ cd /home/matteo/kubernetes/pkg/controller/volume/attachdetach/reconciler;  timeout 300 ~/go/bi
n/stress -p 32 -o $S/stress-fail- $S/rec.test -test.run '^Test_Run_OneVolumeDetachOnOutOfServiceTaintedNode$'
4m55s: 8231 runs so far, 0 failures, 32 active

Compared to

(base) matteo@WuDrew:~/kubernetes$ wsl.exe -e bash -lc 'export PATH=$PATH:$HOME/go/bin; nproc; cd ~/kubernetes/pkg/controller/volume/attachdetach/reconciler && timeout 300 stress -p 32 ~/rec.test -test.run "^Test_Run_OneVolumeDetachOnOutOfServiceTaintedNode$" 2>&1 | tail -40' 2>&1 | tr -d '\0'
4m55s: 8277 runs so far, 3 failures (0.04%), 32 active

torredil · 2026-09-28T15:07:17Z

+	// The reconciler records the metric after DetachVolume returns, but the
+	// detach itself runs in its own goroutine, so the fake detacher can be
+	// called before the counter is incremented.


This fix seems fine, but noting here that we should really also have each test wait for its reconciler to stop before returning (to fix the leak into the next test you described in PR description, which can still happen from tests here that don't check the metric, for example see https://prow.k8s.io/view/gs/kubernetes-ci-logs/pr-logs/pull/141979/pull-kubernetes-unit/2100717770399289344).

Thanks, agreed. The leak you linked is a different path: Test_Run_OneVolumeDetachOnUnhealthyNode's reconciler did one more timeout force detach after the test returned, and the next test's "unchanged" check saw it. I've added a helper that starts the reconciler and, in t.Cleanup, cancels it and waits for Run to return, used at all 14 call sites. The metric is recorded on the reconciler goroutine, so nothing can increment it after a test ends.

Under contention (all Test_Run_*, 3,000 iterations), the first commit alone still hit this once (lines 1281/1302) and logged 63 lines from reconcilers that outlived their test; with the helper, 0 failures and 0 such lines.

torredil · 2026-09-28T15:09:09Z

/ok-to-test

LiRunGuo · 2026-09-29T18:47:36Z

/retest

The tests start the reconciler with "go reconciler.Run(ctx)" and only cancel its context on return, so a reconciler can finish one more loop iteration after its test is done. When that iteration force detaches a volume, the global force detach metric changes under the next test and its "unchanged" check fails. Run the reconciler through a helper that cancels it and waits for Run to return in t.Cleanup. The metric is recorded on the reconciler goroutine, so nothing can change it once Run has returned.

LiRunGuo · 2026-09-29T19:16:24Z

Thanks @matteopulcini6 for checking it with stress, and sorry for the overlap: I hadn't seen your /assign on #142349 before opening this.

torredil

Thanks, latest rev looks great.
/lgtm

kubernetes-prow · 2026-09-30T15:29:33Z

LGTM label has been added.

Details

Git tree hash: 619a8cf08ee9e43c0cbf0474e5096dd2e3427042

torredil · 2026-09-30T15:33:14Z

/cherry-pick release-1.34
/cherry-pick release-1.35

torredil · 2026-09-30T15:35:13Z

/assign @gnufied
for approval

LiRunGuo · 2026-10-05T04:09:10Z

Thanks for the review! Heads-up on the cherry-picks: on release-1.34 and release-1.35 the second commit conflicts (the tests there create their context right above go reconciler.Run(ctx)), so the bot will likely fail; I'll open manual cherry-picks after this merges. release-1.36 and release-1.37 have the same code and this applies cleanly. Should those get cherry-picks too?

LiRunGuo · 2026-10-12T19:51:29Z

Friendly ping @gnufied: this has /lgtm from torredil; could you take a look for approval when you have a moment? Thanks!

jsafrane · 2026-11-03T13:58:35Z

/approve

kubernetes-prow · 2026-11-03T13:58:56Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jsafrane, LiRunGuo

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~pkg/controller/volume/OWNERS~~ [jsafrane]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

kubernetes-prow Bot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Sep 24, 2026

kubernetes-prow Bot added the sig/apps Categorizes an issue or PR as relevant to SIG Apps. label Sep 24, 2026

github-project-automation Bot added this to SIG Apps Sep 24, 2026

github-project-automation Bot moved this to Needs Triage in SIG Apps Sep 24, 2026

kubernetes-prow Bot requested review from gnufied and sunnylovestiramisu September 24, 2026 12:59

torredil reviewed Sep 28, 2026

View reviewed changes

kubernetes-prow Bot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Sep 28, 2026

kubernetes-prow Bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Sep 29, 2026

torredil reviewed Sep 30, 2026

View reviewed changes

kubernetes-prow Bot assigned torredil Sep 30, 2026

kubernetes-prow Bot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 30, 2026

kubernetes-prow Bot assigned gnufied Sep 30, 2026

kubernetes-prow Bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 3, 2026

kubernetes-prow Bot merged commit a8a44c8 into kubernetes:master Nov 3, 2026
17 of 18 checks passed

kubernetes-prow Bot added this to the v1.38 milestone Nov 3, 2026

github-project-automation Bot moved this from Needs Triage to Done in SIG Apps Nov 3, 2026

Conversation

LiRunGuo commented Sep 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR is related to:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Uh oh!

kubernetes-prow Bot commented Sep 24, 2026

Uh oh!

kubernetes-prow Bot commented Sep 24, 2026

Uh oh!

matteopulcini6 commented Sep 26, 2026

Uh oh!

torredil Sep 28, 2026

Choose a reason for hiding this comment

Uh oh!

LiRunGuo Sep 29, 2026

Choose a reason for hiding this comment

Uh oh!

torredil commented Sep 28, 2026

Uh oh!

LiRunGuo commented Sep 29, 2026

Uh oh!

LiRunGuo commented Sep 29, 2026

Uh oh!

torredil left a comment

Choose a reason for hiding this comment

Uh oh!

kubernetes-prow Bot commented Sep 30, 2026

Uh oh!

torredil commented Sep 30, 2026

Uh oh!

torredil commented Sep 30, 2026

Uh oh!

LiRunGuo commented Oct 5, 2026

Uh oh!

LiRunGuo commented Oct 12, 2026

Uh oh!

jsafrane commented Nov 3, 2026

Uh oh!

kubernetes-prow Bot commented Nov 3, 2026

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

LiRunGuo commented Sep 24, 2026 •

edited

Loading