Skip to content

fix: make TestWaitExecHandleHooks deterministic for 2-container hook ordering#9944

Open
lubronzhan wants to merge 1 commit into
velero-io:mainfrom
lubronzhan:fix/flaky-wait-exec-hook-handler-test
Open

fix: make TestWaitExecHandleHooks deterministic for 2-container hook ordering#9944
lubronzhan wants to merge 1 commit into
velero-io:mainfrom
lubronzhan:fix/flaky-wait-exec-hook-handler-test

Conversation

@lubronzhan

@lubronzhan lubronzhan commented Jun 24, 2026

Copy link
Copy Markdown
Contributor

Summary

Observed a flaky test from this PR #9943

  • Fixes a flaky test in TestWaitExecHandleHooks: "should return no error with 2 spec hooks in 2 different containers, 1st container starts running after 10ms, 2nd container after 20ms, both succeed"
  • Root cause: the informer's DeltaFIFO queue coalesces rapid source.Modify calls, delivering only the latest pod state to the handler before the first hook fires — so the mock was called with resourceVersion:3 (both containers running) instead of the expected resourceVersion:2 (container1 running, container2 waiting)
  • Fix: introduce waitForSignal chan struct{} on change and onCalled func() on expectedExecution to synchronize the goroutine emitting changes with hook execution, making the test 100% deterministic

Observed failure

CI run: https://github.com/velero-io/velero/actions/runs/28119573625/job/83267758421

mock: Unexpected Method Call
ExecutePodCommand(..., resourceVersion:"3", container2: running, ...)

The closest call I have is:
ExecutePodCommand(..., resourceVersion:"2", container2: waiting, ...)

The comment "Each of these states will be seen by the UpdateFunc handler" was incorrect — under load the FIFO can skip intermediate states.

Test plan

  • Run go test ./internal/hook/... -run TestWaitExecHandleHooks -count=20 — all pass
  • No production code changed, test infrastructure only

🤖 Generated with Claude Code

@netlify

netlify Bot commented Jun 24, 2026

Copy link
Copy Markdown

👷 Deploy request for velero pending review.

Visit the deploys page to approve it

Name Link
🔨 Latest commit f624362

@github-actions github-actions Bot added Dependencies Pull requests that update a dependency file has-changelog labels Jun 24, 2026
lubronzhan added a commit to lubronzhan/velero that referenced this pull request Jun 24, 2026
Signed-off-by: Lubron Zhan <lubron.zhan@broadcom.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@lubronzhan lubronzhan force-pushed the fix/flaky-wait-exec-hook-handler-test branch 3 times, most recently from 784e27d to 80dc42b Compare June 24, 2026 21:07
@kaovilai kaovilai requested a review from Copilot June 24, 2026 21:07
@lubronzhan lubronzhan force-pushed the fix/flaky-wait-exec-hook-handler-test branch from 80dc42b to 8ecc531 Compare June 24, 2026 21:09

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR makes TestWaitExecHandleHooks deterministic by synchronizing pod state transitions with hook execution to avoid informer update coalescing that can skip intermediate pod states, eliminating a known flake in the 2-container hook ordering scenario.

Changes:

  • Adds channel-based synchronization (waitForSignal / onCalled) to control when subsequent fake pod updates are emitted.
  • Updates test comments to reflect informer coalescing behavior.
  • Adds an unreleased changelog entry describing the flaky-test fix.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
internal/hook/wait_exec_hook_handler_test.go Introduces synchronization hooks to make the flaky 2-container ordering test deterministic.
changelogs/unreleased/9944-lubronzhan Documents the test flake fix in the unreleased changelog.
Comments suppressed due to low confidence (1)

internal/hook/wait_exec_hook_handler_test.go:743

  • waitForSignal can deadlock the test indefinitely if the expected hook never fires (e.g., a regression prevents ExecutePodCommand from being invoked), because the pod-update goroutine blocks forever and prevents later state transitions. Add a bounded wait (or tie it to context cancellation) so failures fail fast instead of hanging the entire test run.
				Result(),
			groupResource: "pods",
			byContainer: map[string][]PodExecRestoreHook{
				"container1": {
					{

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@codecov

codecov Bot commented Jun 24, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@lubronzhan lubronzhan force-pushed the fix/flaky-wait-exec-hook-handler-test branch 3 times, most recently from cbe3d90 to 134a54d Compare June 25, 2026 00:19
…ordering

Fixes a flaky test in TestWaitExecHandleHooks:
  "should return no error with 2 spec hooks in 2 different containers,
   1st container starts running after 10ms, 2nd container after 20ms,
   both succeed"

Observed failure (CI run https://github.com/velero-io/velero/actions/runs/28119573625/job/83267758421):

  mock: Unexpected Method Call
  ExecutePodCommand called with resourceVersion:"3" (both containers running),
  but mock was registered expecting resourceVersion:"2" (container1 running,
  container2 still waiting).

Root cause: the two source.Modify calls were 10ms apart. The informer's
DeltaFIFO queue can coalesce rapid updates, delivering only the latest pod
state (resourceVersion:3) to the handler before the hook for container1 fires.
The comment "each of these states will be seen by the UpdateFunc handler" was
incorrect — intermediate states can be silently skipped under load.

Fix: add waitForSignal chan struct{} to the change struct and onCalled func()
to the expectedExecution struct. The goroutine now blocks after sending the
first change until the mock signals that the hook has fired (via close), then
sends the second change. This guarantees the handler observes the intermediate
pod state (resourceVersion:2) when executing container1's hook.

Signed-off-by: Lubron Zhan <lubronzhan@gmail.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@lubronzhan lubronzhan force-pushed the fix/flaky-wait-exec-hook-handler-test branch from 134a54d to f624362 Compare June 25, 2026 00:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Dependencies Pull requests that update a dependency file has-changelog

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants