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

[bug][filestream] - Fix filebeat GC cleanup bug #40258

Merged
merged 13 commits into from
Jul 17, 2024
Merged

[bug][filestream] - Fix filebeat GC cleanup bug #40258

merged 13 commits into from
Jul 17, 2024

Conversation

VihasMakwana
Copy link
Contributor

@VihasMakwana VihasMakwana commented Jul 16, 2024 •
edited
Loading

Type

  • bugfix

Proposed commit message

From #40178,

Default value for clean_inactive should be 0 (disabled) as explained in the filebeat.reference.yml (no default value indicated in the documentation), but registry entries are removed after 30 min of inactivity.

Fix this behaviour by adding a check to disable gcClean if ttl == -1

  • default clean_interval is changed to -1
  • Current behaviour remains unchanged
    • i.e. clean_inactive: 0 will clean states right after the file is closed

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.

Disruptive User Impact

  • Registries are persisted forever if clean_inactive: -1

Author's Checklist

How to test this PR locally

Related issues

Use cases

Screenshots

Logs

@botelastic botelastic bot added the needs_team Indicates that the issue/PR needs a Team:* label label Jul 16, 2024
@VihasMakwana VihasMakwana added the Team:Elastic-Agent-Data-Plane Label for the Agent Data Plane team label Jul 16, 2024
@botelastic botelastic bot removed the needs_team Indicates that the issue/PR needs a Team:* label label Jul 16, 2024
@mergify Mergify
Copy link
Contributor

mergify bot commented Jul 16, 2024

This pull request does not have a backport label.
If this is a bug or security fix, could you label this PR @VihasMakwana? 🙏.
For such, you'll need to label your PR with:

  • The upcoming major version of the Elastic Stack
  • The upcoming minor version of the Elastic Stack (if you're not pushing a breaking change)

To fixup this pull request, you need to add the backport labels for the needed
branches, such as:

  • backport-v8./d.0 is the label to automatically backport to the 8./d branch. /d is the digit

@VihasMakwana VihasMakwana changed the title DRAFT - [bug][filestream] - Fix GC cleanup bug DRAFT - [bug][filestream] - Fix filebeat registry bug Jul 16, 2024
@VihasMakwana VihasMakwana changed the title DRAFT - [bug][filestream] - Fix filebeat registry bug [bug][filestream] - Fix filebeat GC cleanup bug Jul 16, 2024
@VihasMakwana VihasMakwana marked this pull request as ready for review July 16, 2024 15:21
@VihasMakwana VihasMakwana requested a review from a team as a code owner July 16, 2024 15:21
@elasticmachine
Copy link
Collaborator

Pinging @elastic/elastic-agent-data-plane (Team:Elastic-Agent-Data-Plane)

@VihasMakwana VihasMakwana requested review from rdner and cmacknz and removed request for AndersonQ July 16, 2024 15:23
@pierrehilbert pierrehilbert requested review from faec and removed request for leehinman July 16, 2024 15:50
@cmacknz cmacknz added the backport-8.15 Automated backport to the 8.15 branch with mergify label Jul 16, 2024
faec
faec approved these changes Jul 16, 2024
View reviewed changes
Copy link
Contributor

@faec faec left a comment

Choose a reason for hiding this comment

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

Looks good, just some requests for comments before merge

filebeat/input/filestream/config.go Show resolved Hide resolved
filebeat/input/filestream/internal/input-logfile/clean.go Outdated
@@ -130,5 +130,5 @@ func checkCleanResource(started, now time.Time, resource *resource) bool {
reference = started
}

return reference.Add(ttl).Before(now) && resource.stored
return (ttl >= 0 && reference.Add(ttl).Before(now)) && resource.stored
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add a comment here too, something like "If ttl is negative we never delete it, otherwise check how much time is elapsed."

Nit: the extra parentheses aren't needed since it's just an && of three conditions. Optionally you could also do an immediate if ttl < 0 { return } above so the extra clause isn't needed here.

@VihasMakwana
Copy link
Contributor Author

VihasMakwana commented Jul 17, 2024 •
edited
Loading

@faec Thanks for your quick review! and please let me know if it looks good.

@jlind23
Copy link
Collaborator

jlind23 commented Jul 17, 2024

@kilfoyle the docs-build-pr run is now green according to https://buildkite.com/elastic/docs-build-pr/builds/112412 but it looks like the check is not updated yet, is that a known issue?

@pierrehilbert
Copy link
Collaborator

run docs-build

@pierrehilbert
Copy link
Collaborator

@jlind23 this is now green.

@VihasMakwana VihasMakwana merged commit 1ca6e2f into elastic:main Jul 17, 2024
27 checks passed
mergify bot pushed a commit that referenced this pull request Jul 17, 2024
@VihasMakwana @mergify
[bug][filestream] - Fix filebeat GC cleanup bug (#40258)
d77d110 
* chore: initial commit

* chore: remove default 30mins

* fix: set -1 for default

* fix: typo

* fix: remove validator

* fix: typo

* chore: update reference.yml

* chore: add changelog

* fix: update reference.yml

* fix: address review comments

* fix: typo

(cherry picked from commit 1ca6e2f)
@mergify mergify bot mentioned this pull request Jul 17, 2024
Merged
6 tasks
VihasMakwana added a commit that referenced this pull request Jul 18, 2024
@mergify @VihasMakwana @pierrehilbert
[8.15](backport #40258) [bug][filestream] - Fix filebeat GC cleanup b…
3a9b0f0 
…ug (#40277)

* [bug][filestream] - Fix filebeat GC cleanup bug  (#40258)

* chore: initial commit

* chore: remove default 30mins

* fix: set -1 for default

* fix: typo

* fix: remove validator

* fix: typo

* chore: update reference.yml

* chore: add changelog

* fix: update reference.yml

* fix: address review comments

* fix: typo

(cherry picked from commit 1ca6e2f)

* Update CHANGELOG.next.asciidoc

---------

Co-authored-by: VihasMakwana <121151420+VihasMakwana@users.noreply.github.com>
Co-authored-by: Pierre HILBERT <pierre.hilbert@elastic.co>
@neeej neeej mentioned this pull request Oct 4, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@faec faec faec approved these changes

@rdner rdner Awaiting requested review from rdner

@cmacknz cmacknz Awaiting requested review from cmacknz

Assignees

@VihasMakwana VihasMakwana

Labels
backport-8.15 Automated backport to the 8.15 branch with mergify bugfix Team:Elastic-Agent-Data-Plane Label for the Agent Data Plane team
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Filestream clean_inactive does not work as expected
6 participants
@VihasMakwana @elasticmachine @jlind23 @pierrehilbert @faec @cmacknz