Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[flaky test][exporter/prometheusremotewrite] TestExportWithWALEnabled is flaky on Windows #37715

Closed
pjanotti opened this issue Feb 5, 2025 · 6 comments · Fixed by #37719
Closed

[flaky test][exporter/prometheusremotewrite] TestExportWithWALEnabled is flaky on Windows #37715

pjanotti opened this issue Feb 5, 2025 · 6 comments · Fixed by #37719
Assignees
@ArthurSens
Labels
exporter/prometheusremotewrite flaky test a test is flaky os:windows

Comments

@pjanotti
Copy link
Contributor

pjanotti commented Feb 5, 2025

Component(s)

exporter/prometheusremotewrite

Describe the issue you're reporting

This shouldn't be falling on non-Windows since in general they allow deleting a folder even if there are still open handles to files in the directory. Anyway, the test TestExportWithWALEnabled, introduced via #37630, failed on main.

https://github.com/open-telemetry/opentelemetry-collector-contrib/actions/runs/13153338018/job/36704858478#step:8:167

=== FAIL: . TestExportWithWALEnabled (1.59s)
    testing.go:1231: TempDir RemoveAll cleanup: remove C:\Users\RUNNER~1\AppData\Local\Temp\TestExportWithWALEnabled2886325455\001\prom_remotewrite\00000000000000000001: The process cannot access the file because it is being used by another process.
@pjanotti pjanotti added flaky test a test is flaky os:windows labels Feb 5, 2025
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Feb 5, 2025

Pinging code owners:

See Adding Labels via Comments if you do not have permissions to add labels yourself.

@pjanotti
Copy link
Contributor Author

pjanotti commented Feb 5, 2025

Other hits:

@ArthurSens
Copy link
Member

Damn, this Windows thing really doesn't like me 😭. Thanks for raising it

@ArthurSens
Copy link
Member

I noticed that other tests (they all touch the WAL) are being skipped due to the same failure. I wonder if this is really something unfixable and we should skip or if there's anything we can do in all those tests.

@dashpole
Copy link
Contributor

dashpole commented Feb 5, 2025

We should skip for now, and open a tracking issue

@ArthurSens ArthurSens mentioned this issue Feb 5, 2025
Merged
@pjanotti
Copy link
Contributor Author

pjanotti commented Feb 5, 2025

@ArthurSens it is fixable: it is some race with the WAL settings in which some go routine still using files in the test directory. It is common that on shutdown something is missed and left running. Let me try to scan over it to see what I can find.

@songy23 songy23 closed this as completed in 86430c5 Feb 5, 2025
pjanotti added a commit to pjanotti/opentelemetry-service-contrib that referenced this issue Feb 5, 2025
@pjanotti
Remove test skip added for open-telemetry#37715
5cc73c4
@pjanotti pjanotti mentioned this issue Feb 5, 2025
Merged
songy23 pushed a commit that referenced this issue Feb 6, 2025
@pjanotti
[exporter/prometheusremotewrite] Make WAL shutdown wait until gorouti…
c33b2ad 
…ne exits (#37733)

#### Description

When WAL is used the shutdown doesn't wait until the go routine launched
to handle WAL terminates. This makes the shutdown non-deterministic and
cause the test failure issues linked below.

Although a bug on the component it doesn't seem to deserve a changelog
since it was detected via tests and there is no open issue around the
shutdown of the component.

#### Link to tracking issue

Fixes #9124, #37715

#### Testing

Ran the component tests locally multiple times on Windows (more likely
to surface concurrency issues).

#### Documentation

N/A
chengchuanpeng pushed a commit to chengchuanpeng/opentelemetry-collector-contrib that referenced this issue Feb 8, 2025
@ArthurSens @chengchuanpeng
Skip TestExportWithWALEnabled due to flakiness (open-telemetry#37719)
1b90b65 
Fixes open-telemetry#37715

cc @dashpole @pjanotti

Signed-off-by: Arthur Silva Sens <[email protected]>
chengchuanpeng pushed a commit to chengchuanpeng/opentelemetry-collector-contrib that referenced this issue Feb 8, 2025
@pjanotti @chengchuanpeng
[exporter/prometheusremotewrite] Make WAL shutdown wait until gorouti…
be59d39 
…ne exits (open-telemetry#37733)

#### Description

When WAL is used the shutdown doesn't wait until the go routine launched
to handle WAL terminates. This makes the shutdown non-deterministic and
cause the test failure issues linked below.

Although a bug on the component it doesn't seem to deserve a changelog
since it was detected via tests and there is no open issue around the
shutdown of the component.

#### Link to tracking issue

Fixes open-telemetry#9124, open-telemetry#37715

#### Testing

Ran the component tests locally multiple times on Windows (more likely
to surface concurrency issues).

#### Documentation

N/A
@github-actions github-actions bot mentioned this issue Feb 11, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@ArthurSens ArthurSens

Labels
exporter/prometheusremotewrite flaky test a test is flaky os:windows
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Skip TestExportWithWALEnabled due to flakiness ArthurSens/opentelemetry-collector-contrib
3 participants
@dashpole @pjanotti @ArthurSens