NO-ISSUE: Fix TestMissingImageIsRebuilt goroutine safety issue #5477
Conversation
Replace require.NoError() with assert.NoError() in background goroutines that stream pod logs. require.NoError() calls t.FailNow() which internally calls runtime.Goexit(), terminating only the current goroutine without stopping the test. This causes undefined behavior and test failures. assert.NoError() marks the test as failed without calling Goexit(), making it safe to use in goroutines. This fixes the test failure where log streaming goroutines would silently fail without properly reporting errors.
openshift-ci-robot
added
the
jira/valid-reference
|
@umohnani8: This pull request explicitly references no jira issue. DetailsIn response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
|
/test e2e-gcp-op-ocl |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: umohnani8 The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
|
/test e2e-gcp-op-ocl |
2 similar comments
|
/test e2e-gcp-op-ocl |
|
/test e2e-gcp-op-ocl |
Signed-off-by: Urvashi <umohnani@redhat.com>
|
/test e2e-gcp-op-ocl |
The previous commit (48d070c) fixed the goroutine call sites by changing require.NoError() to assert.NoError(), but missed a require.NoError() call INSIDE streamMachineOSBuilderPodLogsToFile() that is still executed in the goroutine's call stack. When streamMachineOSBuilderPodLogsToFile() is called from a goroutine and encounters an error listing pods, it calls require.NoError(t, err) which internally calls t.FailNow() -> runtime.Goexit(). This terminates only the current goroutine without properly failing the test, causing undefined behavior and test hangs. This commit: 1. Replaces require.NoError() with proper error returns in streamMachineOSBuilderPodLogsToFile() 2. Adds bounds checking for the pods.Items slice 3. Adds debug logging to waitForBuildToComplete() that logs build status every 30 seconds to help diagnose future timeout issues This should resolve the persistent test timeouts that appeared after the original goroutine safety fix.
|
/test e2e-gcp-op-ocl |
|
@umohnani8: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions 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. I understand the commands that are listed here. |
2 participants
- What I did
Replace require.NoError() with assert.NoError() in background goroutines that stream pod logs. require.NoError() calls t.FailNow() which internally calls runtime.Goexit(), terminating only the current goroutine without stopping the test. This causes undefined behavior and test failures.
assert.NoError() marks the test as failed without calling Goexit(), making it safe to use in goroutines.
This fixes the test failure where log streaming goroutines would silently fail without properly reporting errors.
- How to verify it
Failures in the TestMissingImageIsRebuilt should be reported in the logs
- Description for the changelog
Fix logs for TestMissingImageIsRebuilt