Do not close filestream harvester if an unexpected error is returned when close.on_state_change.* is enabled #26411

kvch · 2021-06-22T14:53:39Z

What does this PR do?

This PR returns early if close.on_state_change.removed is enabled and the opened file no longer exists. Otherwise, it logs an error message and keeps the reader running.

Why is it important?

Previously, a message has been logged on error level and the reader has been stopped if the Stat call returned an error. However, it was not correct because if close.on_state_change.renamed was enabled the reader would have been closed if the file had been removed. Now the reader is not stopped.

Checklist

My code follows the style guidelines of this project
I have commented my code, particularly in hard-to-understand areas
~~- [ ] I have made corresponding changes to the documentation~~
~~- [ ] I have made corresponding change to the default configuration files~~
~~- [ ] I have added tests that prove my fix is effective or that my feature works~~
I have added an entry in CHANGELOG.next.asciidoc or CHANGELOG-developer.next.asciidoc.

elasticmachine · 2021-06-22T14:55:37Z

Pinging @elastic/agent (Team:Agent)

botelastic · 2021-06-22T14:56:11Z

This pull request doesn't have a Team:<team> label.

mergify · 2021-06-22T15:09:30Z

This pull request is now in conflicts. Could you fix it? 🙏
To fixup this pull request, you can check out it locally. See documentation: https://help.github.com/articles/checking-out-pull-requests-locally/

git fetch upstream
git checkout -b feature-filebeat-filestream-decrease-log-level upstream/feature-filebeat-filestream-decrease-log-level
git merge upstream/master
git push upstream feature-filebeat-filestream-decrease-log-level

…am input with close_* options

elasticmachine · 2021-06-22T16:52:13Z

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS

Expand to view the summary

Build stats

Build Cause: Pull request #26411 updated
Start Time: 2021-06-24T11:49:52.843+0000
Duration: 99 min 11 sec
Commit: 8357c74

Test stats 🧪

Test	Results
Failed	0
Passed	14125
Skipped	2311
Total	16436

Trends 🧪

💚 Flaky test report

Tests succeeded.

Expand to view the summary

Test stats 🧪

Test	Results
Failed	0
Passed	14125
Skipped	2311
Total	16436

mergify · 2021-06-23T18:17:45Z

This pull request is now in conflicts. Could you fix it? 🙏
To fixup this pull request, you can check out it locally. See documentation: https://help.github.com/articles/checking-out-pull-requests-locally/

git fetch upstream
git checkout -b feature-filebeat-filestream-decrease-log-level upstream/feature-filebeat-filestream-decrease-log-level
git merge upstream/master
git push upstream feature-filebeat-filestream-decrease-log-level

…evel

urso · 2021-06-24T11:55:31Z

filebeat/input/filestream/filestream.go

+			return true
+		}
+
+		// If an unexpected error happens we keep the reader open hoping once everything will go back to normal.
 		f.log.Errorf("Unexpected error reading from %s; error: %s", f.file.Name(), statErr)


This one still might produce a false-positive log message. The shouldBeClosed method is called by a separate go-routine independent from the reader go-routine. When the reader go-routine shuts down (cancelled event) while shouldBeClosed is run we will see an error telling us that f is invalid because it was already closed.

In order to prevent the false-positive message, we either need a mutex or some ref-counting on f + a wrapper on the reader that allows us to unblock a pending read call if the file gets closed async.

Let's try to solve the race in a separate PR.

…when close.on_state_change.* is enabled (#26411) ## What does this PR do? This PR returns early if `close.on_state_change.removed` is enabled and the opened file no longer exists. Otherwise, it logs an error message and keeps the reader running. ## Why is it important? Previously, a message has been logged on error level and the reader has been stopped if the `Stat` call returned an error. However, it was not correct because if `close.on_state_change.renamed` was enabled the reader would have been closed if the file had been removed. Now the reader is not stopped. (cherry picked from commit 1106449)

…when close.on_state_change.* is enabled (#26411) (#26477) ## What does this PR do? This PR returns early if `close.on_state_change.removed` is enabled and the opened file no longer exists. Otherwise, it logs an error message and keeps the reader running. ## Why is it important? Previously, a message has been logged on error level and the reader has been stopped if the `Stat` call returned an error. However, it was not correct because if `close.on_state_change.renamed` was enabled the reader would have been closed if the file had been removed. Now the reader is not stopped. (cherry picked from commit 1106449) Co-authored-by: Noémi Ványi <kvch@users.noreply.github.com>

…when close.on_state_change.* is enabled (#26411) (#26476) ## What does this PR do? This PR returns early if `close.on_state_change.removed` is enabled and the opened file no longer exists. Otherwise, it logs an error message and keeps the reader running. ## Why is it important? Previously, a message has been logged on error level and the reader has been stopped if the `Stat` call returned an error. However, it was not correct because if `close.on_state_change.renamed` was enabled the reader would have been closed if the file had been removed. Now the reader is not stopped. (cherry picked from commit 1106449) Co-authored-by: Noémi Ványi <kvch@users.noreply.github.com>

* master: (32 commits) [Metricbeat] Change Account ID to Project ID in `gcp.billing` module (elastic#26412) update libbeat fields.ecs.yml file and ecsVersion to 1.10.0 (elastic#26121) [Filebeat] Update AWS ELB ingest pipeline (elastic#26441) [FIlebeat] add strict_date_optional_time_nanos date format to PanOS module (elastic#26158) Fix the irregular and typo on prometheus module. (elastic#25726) [Filebeat] Parse additonal debug data fields for Okta module (elastic#25818) fix: update MSSQL Server linux image's Docker registry (elastic#26440) Update indexing.go godocs (elastic#26408) Do not close filestream harvester if an unexpected error is returned when close.on_state_change.* is enabled (elastic#26411) Add support for copytruncate method when rotating input logs with an external tool in `filestream` input (elastic#23457) Allow fields with ip_range datatype (elastic#26444) Add Anomali ThreatStream support to threatintel module (elastic#26350) fix: use the right param type (elastic#26469) [Automation] Update elastic stack version to 8.0.0-7640093f for testing (elastic#26460) Set SM Filebeat modules as GA (elastic#26226) Fix rfc5464 date parsing in the syslog input (elastic#26419) Add linked account information into billing metricset (elastic#26285) [Filebeat] Update HA Proxy log grok patterns (elastic#25835) disable metricbeat logstash test_node_stats (elastic#26436) chore: pass BEAT_VERSION when running E2E tests (elastic#26291) ...

botelastic bot added the needs_team Indicates that the issue/PR needs a Team:* label label Jun 22, 2021

kvch requested a review from urso June 22, 2021 14:55

kvch added the Team:Elastic-Agent Label for the Agent team label Jun 22, 2021

botelastic bot removed the needs_team Indicates that the issue/PR needs a Team:* label label Jun 22, 2021

kvch added backport-v7.13.0 Automated backport with mergify backport-v7.14.0 Automated backport with mergify needs_team Indicates that the issue/PR needs a Team:* label labels Jun 22, 2021

botelastic bot removed the needs_team Indicates that the issue/PR needs a Team:* label label Jun 22, 2021

kvch added 2 commits June 22, 2021 17:10

Decrease log level from error to debug when closing file inf filestre…

50b77f9

…am input with close_* options

add changelog entry

8ab0707

kvch force-pushed the feature-filebeat-filestream-decrease-log-level branch from 7973b7b to 8ab0707 Compare June 22, 2021 15:10

Merge branch 'master' into feature-filebeat-filestream-decrease-log-l…

8357c74

…evel

urso reviewed Jun 24, 2021

View reviewed changes

urso approved these changes Jun 24, 2021

View reviewed changes

kvch merged commit 1106449 into elastic:master Jun 24, 2021

mergify bot mentioned this pull request Jun 24, 2021

[7.x](backport #26411) Do not close filestream harvester if an unexpected error is returned when close.on_state_change.* is enabled #26476

Merged

mergify bot mentioned this pull request Jun 24, 2021

[7.13](backport #26411) Do not close filestream harvester if an unexpected error is returned when close.on_state_change.* is enabled #26477

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Do not close filestream harvester if an unexpected error is returned when close.on_state_change.* is enabled #26411

Do not close filestream harvester if an unexpected error is returned when close.on_state_change.* is enabled #26411

kvch commented Jun 22, 2021 •

edited

Loading

elasticmachine commented Jun 22, 2021

botelastic bot commented Jun 22, 2021

mergify bot commented Jun 22, 2021

elasticmachine commented Jun 22, 2021 •

edited by jenkins-beats-ci bot

Loading

Build stats

Test stats 🧪

Trends 🧪

Test stats 🧪

mergify bot commented Jun 23, 2021

urso Jun 24, 2021

urso Jun 24, 2021

Do not close filestream harvester if an unexpected error is returned when close.on_state_change.* is enabled #26411

Do not close filestream harvester if an unexpected error is returned when close.on_state_change.* is enabled #26411

Conversation

kvch commented Jun 22, 2021 • edited Loading

What does this PR do?

Why is it important?

Checklist

elasticmachine commented Jun 22, 2021

botelastic bot commented Jun 22, 2021

mergify bot commented Jun 22, 2021

elasticmachine commented Jun 22, 2021 • edited by jenkins-beats-ci bot Loading

💚 Build Succeeded

Build stats

Test stats 🧪

Trends 🧪

💚 Flaky test report

Test stats 🧪

mergify bot commented Jun 23, 2021

urso Jun 24, 2021

Choose a reason for hiding this comment

urso Jun 24, 2021

Choose a reason for hiding this comment

kvch commented Jun 22, 2021 •

edited

Loading

elasticmachine commented Jun 22, 2021 •

edited by jenkins-beats-ci bot

Loading