Fix panic when more than 32767 pipeline clients are active #38556

belimawr · 2024-03-22T14:59:56Z

Proposed commit message

The publishing pipeline would panic when more than 32767 clients were active, that happened because each client would add two channels to a slice and an infinity loop would use reflect.Select on this list. reflect.Select supports a maximum of 65536 cases, if there are more it panics.

This PR fixes this by removing the need for this list. The pipeline used an infinity loop to detect if the CloseRef from the pipeline client was closed, if it happened, then it would call client.Close. By analysing the code setting CloseRef we identified there was no need for the pipeline to be responsible for propagating this signal, most of the times the pipeline client was created, there was already a defer to close it, in the few places where it was not present we added one.

Now the pipeline is not responsible for closing any client, whomever creates a client is responsible for correctly closing it.

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.

~~## Author's Checklist~~

How to test this PR locally

This PR fixes a bug by removing a feature that is not needed. That said, it is easy to reproduce the situation that would lead to the crash.

All configuration files and scripts are at the end of this section.

Create at least 33.000 files, you can use ./gen-logs.sh 33000
Start Filebeat with Filestream input reading those files. Use filebeat.yml
Wait until all events are ingested, you can do this by checking the number of lines in the output file: wc -l output*.ndjson, it should report 33.000.000 entries.
Ensure Filebeat does not panic.
Remove all created log files from your system: rm -rf /tmp/too-many-logs

filebeat.yml

filebeat.inputs:
  - type: filestream
    id: do-not-panic
    paths:
      - /tmp/too-many-logs/*

output:
  file:
    enabled: true
    codec.json:
      pretty: false
    path: ${path.home}
    filename: "output"
    rotate_on_startup: false
    rotate_every_kb: 1000000

Related issues

Closes Beats panic if there are more than 32767 pipeline clients #38197

~~## Use cases~~
~~## Screenshots~~
~~## Logs~~

mergify · 2024-03-22T15:00:34Z

This pull request does not have a backport label.
If this is a bug or security fix, could you label this PR @belimawr? 🙏.
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

elasticmachine · 2024-03-22T15:03:19Z

⏳ Build in-progress, with failures

Buildkite Build
Commit: bd92336

Failed CI Steps

History

cc @belimawr

elasticmachine · 2024-03-22T15:04:46Z

⏳ Build in-progress, with failures

Buildkite Build
Commit: bd92336

Failed CI Steps

History

cc @belimawr

elasticmachine · 2024-03-22T15:04:46Z

⏳ Build in-progress, with failures

Buildkite Build
Commit: bd92336

Failed CI Steps

❎ Cross compile

cc @belimawr

elasticmachine · 2024-03-22T15:04:53Z

⏳ Build in-progress, with failures

Buildkite Build
Commit: bd92336

Failed CI Steps

History

cc @belimawr

elasticmachine · 2024-03-22T15:05:06Z

⏳ Build in-progress, with failures

Buildkite Build
Commit: bd92336

Failed CI Steps

History

cc @belimawr

elasticmachine · 2024-03-22T15:11:08Z

⏳ Build in-progress, with failures

Buildkite Build
Commit: 4210d81

Failed CI Steps

History

💔 Build #4242 failed bd92336

cc @belimawr

elasticmachine · 2024-03-22T15:11:46Z

💔 Build Failed

Buildkite Build
Commit: 4210d81

Failed CI Steps

:rhel: RHEL-9 Unit Tests

History

💔 Build #1317 failed bd92336

cc @belimawr

elasticmachine · 2024-03-22T15:11:52Z

💚 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

Duration: 150 min 32 sec

❕ Flaky test report

No test was executed to be analysed.

🤖 GitHub comments

Expand to view the GitHub comments

To re-run your PR in the CI, just comment with:

/test : Re-trigger the build.
/package : Generate the packages and run the E2E tests.
/beats-tester : Run the installation tests with beats-tester.
run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

elasticmachine · 2024-03-22T15:23:31Z

💔 Build Failed

Buildkite Build
Commit: 4210d81

Failed CI Steps

History

💔 Build #2593 failed bd92336

cc @belimawr

elasticmachine · 2024-03-22T15:26:38Z

💚 Build Succeeded

Buildkite Build
Commit: 4210d81

History

💔 Build #1359 failed bd92336

cc @belimawr

elasticmachine · 2024-03-27T14:37:55Z

Pinging @elastic/elastic-agent (Team:Elastic-Agent)

rdner

Linter issues need to be fixed but the change looks good to me.

belimawr · 2024-04-11T08:34:24Z

@elastic/obs-cloud-monitoring and @elastic/sec-deployment-and-devices could you take a look at this PR? At least at the files you own?

pierrehilbert · 2024-04-11T14:18:40Z

cc @bturquet & @norrietaylor

x-pack/filebeat/input/entityanalytics/internal/kvstore/input.go

libbeat/publisher/pipeline/pipeline_test.go

kaiyan-sheng

awscloudwatch and awss3 input change looks good to me.

pkoutsovasilis · 2024-04-12T06:15:02Z

@belimawr @pierrehilbert when this one is addressed I am pressing the approve button. Also I would propose to have an entry in CHANGELOG.next.asciidoc to capture the change in ClientConfig

belimawr · 2024-04-12T10:03:50Z

@belimawr @pierrehilbert when this one is addressed I am pressing the approve button. Also I would propose to have an entry in CHANGELOG.next.asciidoc to capture the change in ClientConfig

CHANGELOG.next.asciidoc is the user-facing changelog, I do not believe internal code changes should go there. However I missed the changelog entry.

I added some changelog entries in 85e42dd

Refactor tests so they do not depend on `client.closeRef`

Remove all references to CloseRef, including the ones that were commented out.

The pipeline client used in some tests could not have its Close method called more than once, given the changes introduces in the previous commits, some test scenarios called it more than once. This is fixed by using a sync.Once in ChanClient.Close.

Fix number of clients to close, add some comments and better name a variable.

mergify · 2024-04-12T10:09:14Z

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 fix-pipeline-panic-v3 upstream/fix-pipeline-panic-v3
git merge upstream/main
git push upstream fix-pipeline-panic-v3

belimawr · 2024-04-12T10:11:51Z

Folks, given the PR is pretty much approved, all comments have been addressed and I'm going on PTO, I've enabled auto-merge.

libbeat/publisher/pipeline/pipeline_test.go

belimawr added skip-ci Skip the build in the CI but linting Team:Elastic-Agent Label for the Agent team labels Mar 22, 2024

botelastic bot added needs_team Indicates that the issue/PR needs a Team:* label and removed needs_team Indicates that the issue/PR needs a Team:* label labels Mar 22, 2024

mergify bot assigned belimawr Mar 22, 2024

belimawr force-pushed the fix-pipeline-panic-v3 branch from bd92336 to 4210d81 Compare March 22, 2024 15:08

belimawr added bug and removed skip-ci Skip the build in the CI but linting labels Mar 22, 2024

belimawr requested a review from faec March 22, 2024 16:09

belimawr force-pushed the fix-pipeline-panic-v3 branch 2 times, most recently from dc55d3e to f4e1b38 Compare March 27, 2024 14:32

belimawr marked this pull request as ready for review March 27, 2024 14:37

belimawr requested review from a team as code owners March 27, 2024 14:37

belimawr requested a review from rdner March 27, 2024 14:37

belimawr changed the title ~~[WIP] Fix panic when more than 32767 clients are active~~ Fix panic when more than 32767 clients are active Mar 27, 2024

rdner approved these changes Mar 28, 2024

View reviewed changes

efd6 approved these changes Apr 2, 2024

View reviewed changes

belimawr force-pushed the fix-pipeline-panic-v3 branch 2 times, most recently from 82c201a to 81e6cc7 Compare April 9, 2024 06:59

pkoutsovasilis reviewed Apr 11, 2024

View reviewed changes

x-pack/filebeat/input/entityanalytics/internal/kvstore/input.go Outdated Show resolved Hide resolved

pkoutsovasilis reviewed Apr 11, 2024

View reviewed changes

libbeat/publisher/pipeline/pipeline_test.go Outdated Show resolved Hide resolved

kaiyan-sheng approved these changes Apr 12, 2024

View reviewed changes

belimawr added 8 commits April 12, 2024 12:08

[WIP] Fix panic when more than 32767 clients are active

c25aecf

Refactor tests

a95a3e1

Refactor tests so they do not depend on `client.closeRef`

Remove all references to CloseRef and unused fields

2e4f4ea

Remove all references to CloseRef, including the ones that were commented out.

Fix failing tests

5085fec

The pipeline client used in some tests could not have its Close method called more than once, given the changes introduces in the previous commits, some test scenarios called it more than once. This is fixed by using a sync.Once in ChanClient.Close.

Remove comments

7dad6ba

Improve test

2695d91

Fix number of clients to close, add some comments and better name a variable.

Implement PR review suggestions

a9c1911

Add changelog entries

85e42dd

belimawr force-pushed the fix-pipeline-panic-v3 branch from a1c018c to 85e42dd Compare April 12, 2024 10:09

belimawr requested a review from pkoutsovasilis April 12, 2024 10:09

belimawr enabled auto-merge (squash) April 12, 2024 10:11

belimawr changed the title ~~Fix panic when more than 32767 clients are active~~ Fix panic when more than 32767 pipeline clients are active Apr 12, 2024

pkoutsovasilis reviewed Apr 12, 2024

View reviewed changes

libbeat/publisher/pipeline/pipeline_test.go Outdated Show resolved Hide resolved

Fix after rebase

8852d45

pkoutsovasilis approved these changes Apr 12, 2024

View reviewed changes

belimawr merged commit e993e09 into elastic:main Apr 12, 2024
196 of 202 checks passed

efd6 mentioned this pull request Jun 5, 2024

[Filebeat] Okta Entity Analytics Input panic #39803

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix panic when more than 32767 pipeline clients are active #38556

Fix panic when more than 32767 pipeline clients are active #38556

belimawr commented Mar 22, 2024 •

edited

Loading

mergify bot commented Mar 22, 2024

elasticmachine commented Mar 22, 2024 •

edited

Loading

elasticmachine commented Mar 22, 2024 •

edited

Loading

elasticmachine commented Mar 22, 2024

elasticmachine commented Mar 22, 2024 •

edited

Loading

elasticmachine commented Mar 22, 2024 •

edited

Loading

elasticmachine commented Mar 22, 2024 •

edited

Loading

elasticmachine commented Mar 22, 2024 •

edited

Loading

elasticmachine commented Mar 22, 2024 •

edited

Loading

Build stats

elasticmachine commented Mar 22, 2024 •

edited

Loading

elasticmachine commented Mar 22, 2024

elasticmachine commented Mar 27, 2024

rdner left a comment

belimawr commented Apr 11, 2024

pierrehilbert commented Apr 11, 2024

kaiyan-sheng left a comment

pkoutsovasilis commented Apr 12, 2024

belimawr commented Apr 12, 2024 •

edited

Loading

mergify bot commented Apr 12, 2024

belimawr commented Apr 12, 2024

Fix panic when more than 32767 pipeline clients are active #38556

Fix panic when more than 32767 pipeline clients are active #38556

Conversation

belimawr commented Mar 22, 2024 • edited Loading

Proposed commit message

Checklist

How to test this PR locally

Related issues

mergify bot commented Mar 22, 2024

elasticmachine commented Mar 22, 2024 • edited Loading

⏳ Build in-progress, with failures

Failed CI Steps

History

elasticmachine commented Mar 22, 2024 • edited Loading

⏳ Build in-progress, with failures

Failed CI Steps

History

elasticmachine commented Mar 22, 2024

⏳ Build in-progress, with failures

Failed CI Steps

elasticmachine commented Mar 22, 2024 • edited Loading

⏳ Build in-progress, with failures

Failed CI Steps

History

elasticmachine commented Mar 22, 2024 • edited Loading

⏳ Build in-progress, with failures

Failed CI Steps

History

elasticmachine commented Mar 22, 2024 • edited Loading

⏳ Build in-progress, with failures

Failed CI Steps

History

elasticmachine commented Mar 22, 2024 • edited Loading

💔 Build Failed

Failed CI Steps

History

elasticmachine commented Mar 22, 2024 • edited Loading

💚 Build Succeeded

Build stats

❕ Flaky test report

🤖 GitHub comments

elasticmachine commented Mar 22, 2024 • edited Loading

💔 Build Failed

Failed CI Steps

History

elasticmachine commented Mar 22, 2024

💚 Build Succeeded

History

elasticmachine commented Mar 27, 2024

rdner left a comment

Choose a reason for hiding this comment

belimawr commented Apr 11, 2024

pierrehilbert commented Apr 11, 2024

kaiyan-sheng left a comment

Choose a reason for hiding this comment

pkoutsovasilis commented Apr 12, 2024

belimawr commented Apr 12, 2024 • edited Loading

mergify bot commented Apr 12, 2024

belimawr commented Apr 12, 2024

belimawr commented Mar 22, 2024 •

edited

Loading

elasticmachine commented Mar 22, 2024 •

edited

Loading

elasticmachine commented Mar 22, 2024 •

edited

Loading

elasticmachine commented Mar 22, 2024 •

edited

Loading

elasticmachine commented Mar 22, 2024 •

edited

Loading

elasticmachine commented Mar 22, 2024 •

edited

Loading

elasticmachine commented Mar 22, 2024 •

edited

Loading

elasticmachine commented Mar 22, 2024 •

edited

Loading

elasticmachine commented Mar 22, 2024 •

edited

Loading

belimawr commented Apr 12, 2024 •

edited

Loading