[chore] Ensure file is closed in Supervisor test #35588
Merged
Conversation
This file contains 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
evan-bradley
requested review from
atoulme,
tigrannajaryan and
a team
as code owners
dpaasman00
approved these changes
Oct 3, 2024
|
Looks like the e2e windows tests got skipped in that last run: https://github.com/open-telemetry/opentelemetry-collector-contrib/actions/runs/11166244550/job/31039792977?pr=35588 I would be surprised if this was fixed by this, the defer should run before anything scheduled t.Cleanup calls happen. I suspect that the zap logger is actually holding the log file open, and not the test code. The fix might be a little more involved if that's the case. |
codeboten
approved these changes
Oct 3, 2024
|
I think the change is still ok to merge even if it might not fix the issue. Would rather we ensure closing the log doesn't return an error |
|
Unfortunately it looks like this did not fix the issue. @BinaryFissionGames I was going to be a little surprised too, but thought that the test scheduler may be doing something strange to clean up tests before deferred statements can be run. Thanks for the pointer to Zap. It looks like Zap is indeed the culprit, since it doesn't keep any handles to close files it opens: https://github.com/uber-go/zap/blob/master/config.go#L316. I'll see if I can come up with another fix. |
evan-bradley
added a commit
that referenced
this pull request
Oct 3, 2024
<!--Ex. Fixing a bug - Describe the bug and how this fixes the issue. Ex. Adding a feature - Explain what this achieves.--> #### Description Follow up to #35588. This skips the test on Windows. The logic should be similar on other operating systems, so it's not a significant loss to skip it. The issue arises because Zap does not close log files itself and provides no handles to close them, so we would need to find a clever way to close the file or would need to simply not clean it up after the test executes.
jriguera
pushed a commit
to springernature/opentelemetry-collector-contrib
that referenced
this pull request
Oct 4, 2024
**Description:** Follow up to open-telemetry#35468. This should fix issues like the following: ``` testing.go:1231: TempDir RemoveAll cleanup: remove C:\Users\RUNNER~1\AppData\Local\Temp\TestSupervisorInfoLoggingLevel3383092187\001\supervisor_log.log: The process cannot access the file because it is being used by another process. ``` I believe this error is caused by the temp directory doing a cleanup before all `defer` statements can run, but I'm not sure. This is how I've seen this error fixed in other tests.
jriguera
pushed a commit
to springernature/opentelemetry-collector-contrib
that referenced
this pull request
Oct 4, 2024
<!--Ex. Fixing a bug - Describe the bug and how this fixes the issue. Ex. Adding a feature - Explain what this achieves.--> #### Description Follow up to open-telemetry#35588. This skips the test on Windows. The logic should be similar on other operating systems, so it's not a significant loss to skip it. The issue arises because Zap does not close log files itself and provides no handles to close them, so we would need to find a clever way to close the file or would need to simply not clean it up after the test executes.
Eromosele-SM
pushed a commit
to sematext/opentelemetry-collector-contrib
that referenced
this pull request
Oct 9, 2024
**Description:** Follow up to open-telemetry#35468. This should fix issues like the following: ``` testing.go:1231: TempDir RemoveAll cleanup: remove C:\Users\RUNNER~1\AppData\Local\Temp\TestSupervisorInfoLoggingLevel3383092187\001\supervisor_log.log: The process cannot access the file because it is being used by another process. ``` I believe this error is caused by the temp directory doing a cleanup before all `defer` statements can run, but I'm not sure. This is how I've seen this error fixed in other tests.
Eromosele-SM
pushed a commit
to sematext/opentelemetry-collector-contrib
that referenced
this pull request
Oct 9, 2024
<!--Ex. Fixing a bug - Describe the bug and how this fixes the issue. Ex. Adding a feature - Explain what this achieves.--> #### Description Follow up to open-telemetry#35588. This skips the test on Windows. The logic should be similar on other operating systems, so it's not a significant loss to skip it. The issue arises because Zap does not close log files itself and provides no handles to close them, so we would need to find a clever way to close the file or would need to simply not clean it up after the test executes.
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.
Description:
Follow up to #35468. This should fix issues like the following:
I believe this error is caused by the temp directory doing a cleanup before all
deferstatements can run, but I'm not sure. This is how I've seen this error fixed in other tests.