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

downstream: PR 6882 rework #6906

Merged
merged 10 commits into from
Feb 24, 2023
Merged

downstream: PR 6882 rework #6906

merged 10 commits into from
Feb 24, 2023

Conversation

leonardo-albertovich
Copy link
Collaborator

This pull request addresses the same issues that PR #6882 covered.

@leonardo-albertovich
input: removed unnecessary code from flb_input_downstream_set
18cb334 
input: additionally, changed the stream parameter type to make the
client code simpler.

Signed-off-by: Leonardo Alminana <leonardo@calyptia.com>
@leonardo-albertovich
in_elasticsearch: fixed event loop bugs
3497c66 
Removed a stored reference to the main event loop in favor of obtaining
the current event loop pointer from the TLS which is required to prevent
client sockets from being registered in the main thread when the plugin
instance is running in its own thread.

Additionally, a call to flb_input_downstream_set was added in order to
enable thread safety and move the downstream to the plugin instance
downstream list if needed in order to ensure that the downstream is
processed by the timeout callback runing in the plugins thread instead
of the main thread.

Signed-off-by: Leonardo Alminana <leonardo@calyptia.com>
@leonardo-albertovich
in_forward: fixed event loop bugs
b049abb 
Removed a stored reference to the main event loop in favor of obtaining
the current event loop pointer from the TLS which is required to prevent
client sockets from being registered in the main thread when the plugin
instance is running in its own thread.

Additionally, a call to flb_input_downstream_set was added in order to
enable thread safety and move the downstream to the plugin instance
downstream list if needed in order to ensure that the downstream is
processed by the timeout callback runing in the plugins thread instead
of the main thread.

Signed-off-by: Leonardo Alminana <leonardo@calyptia.com>
@leonardo-albertovich
in_http: fixed event loop bugs
2d7ee90 
Removed a stored reference to the main event loop in favor of obtaining
the current event loop pointer from the TLS which is required to prevent
client sockets from being registered in the main thread when the plugin
instance is running in its own thread.

Additionally, a call to flb_input_downstream_set was added in order to
enable thread safety and move the downstream to the plugin instance
downstream list if needed in order to ensure that the downstream is
processed by the timeout callback runing in the plugins thread instead
of the main thread.

Signed-off-by: Leonardo Alminana <leonardo@calyptia.com>
@leonardo-albertovich
in_mqtt: fixed event loop bugs
1844619 
Removed a stored reference to the main event loop in favor of obtaining
the current event loop pointer from the TLS which is required to prevent
client sockets from being registered in the main thread when the plugin
instance is running in its own thread.

Additionally, a call to flb_input_downstream_set was added in order to
enable thread safety and move the downstream to the plugin instance
downstream list if needed in order to ensure that the downstream is
processed by the timeout callback runing in the plugins thread instead
of the main thread.

Signed-off-by: Leonardo Alminana <leonardo@calyptia.com>
@leonardo-albertovich
in_opentelemetry: fixed event loop bugs
ff26fc7 
Removed a stored reference to the main event loop in favor of obtaining
the current event loop pointer from the TLS which is required to prevent
client sockets from being registered in the main thread when the plugin
instance is running in its own thread.

Additionally, a call to flb_input_downstream_set was added in order to
enable thread safety and move the downstream to the plugin instance
downstream list if needed in order to ensure that the downstream is
processed by the timeout callback runing in the plugins thread instead
of the main thread.

Signed-off-by: Leonardo Alminana <leonardo@calyptia.com>
@leonardo-albertovich
in_syslog: fixed event loop bugs
8506328 
Removed a stored reference to the main event loop in favor of obtaining
the current event loop pointer from the TLS which is required to prevent
client sockets from being registered in the main thread when the plugin
instance is running in its own thread.

Additionally, a call to flb_input_downstream_set was added in order to
enable thread safety and move the downstream to the plugin instance
downstream list if needed in order to ensure that the downstream is
processed by the timeout callback runing in the plugins thread instead
of the main thread.

Signed-off-by: Leonardo Alminana <leonardo@calyptia.com>
@leonardo-albertovich
in_tcp: fixed event loop bugs
0f52ebb 
Removed a stored reference to the main event loop in favor of obtaining
the current event loop pointer from the TLS which is required to prevent
client sockets from being registered in the main thread when the plugin
instance is running in its own thread.

Additionally, a call to flb_input_downstream_set was added in order to
enable thread safety and move the downstream to the plugin instance
downstream list if needed in order to ensure that the downstream is
processed by the timeout callback runing in the plugins thread instead
of the main thread.

Signed-off-by: Leonardo Alminana <leonardo@calyptia.com>
@leonardo-albertovich
in_udp: fixed event loop bugs
05f1ad0 
Removed a stored reference to the main event loop in favor of obtaining
the current event loop pointer from the TLS which is required to prevent
client sockets from being registered in the main thread when the plugin
instance is running in its own thread.

Additionally, a call to flb_input_downstream_set was added in order to
enable thread safety and move the downstream to the plugin instance
downstream list if needed in order to ensure that the downstream is
processed by the timeout callback runing in the plugins thread instead
of the main thread.

Signed-off-by: Leonardo Alminana <leonardo@calyptia.com>
@leonardo-albertovich
in_unix_socket: fixed event loop bugs
8b267d5 
Removed a stored reference to the main event loop in favor of obtaining
the current event loop pointer from the TLS which is required to prevent
client sockets from being registered in the main thread when the plugin
instance is running in its own thread.

Additionally, a call to flb_input_downstream_set was added in order to
enable thread safety and move the downstream to the plugin instance
downstream list if needed in order to ensure that the downstream is
processed by the timeout callback runing in the plugins thread instead
of the main thread.

Signed-off-by: Leonardo Alminana <leonardo@calyptia.com>
@leonardo-albertovich leonardo-albertovich temporarily deployed to pr February 23, 2023 16:42 — with GitHub Actions Inactive
@leonardo-albertovich leonardo-albertovich temporarily deployed to pr February 23, 2023 16:42 — with GitHub Actions Inactive
@leonardo-albertovich leonardo-albertovich temporarily deployed to pr February 23, 2023 16:42 — with GitHub Actions Inactive
@edsiper
Copy link
Member

edsiper commented Feb 23, 2023

the changes makes sense to me, but we need more testing. @nokute78 can you validate the approach proposed here ?

@leonardo-albertovich leonardo-albertovich temporarily deployed to pr February 23, 2023 17:05 — with GitHub Actions Inactive
@leonardo-albertovich
Copy link
Collaborator Author

I just realized that the test case I created for this has already been running for 24 hours when I went to switch branches to backport it to 2.0.

The configuration I used was this :

[SERVICE]
    flush 1
    grace 1
    log_level info

[INPUT]
    name forward
    threaded On

[OUTPUT]
    name stdout
    match *

The clients were eight shells running this line :

while [ 1 ] ; do  telnet 127.0.0.1 24224 <sample_file >/dev/null 2>&1; sleep 0.1 ; done

And the contents of sample_file were these :

{"a": "b"}

Which obviously is wrong considering this is in_forward but what's important in this case is that those 8 clients were constantly establishing new connections, sending a bit of data and closing the connection which combined with the timers and main thread event loop processing caused all "three" issues to trigger originally.

So I am fairly confident that this PR works.

@leonardo-albertovich leonardo-albertovich mentioned this pull request Feb 24, 2023
Merged
@lecaros lecaros added this to the Fluent Bit v2.1.0 milestone Feb 24, 2023
@edsiper edsiper merged commit fc947c5 into master Feb 24, 2023
@edsiper edsiper deleted the leonardo-master-downstream-pr-6882-rework branch February 24, 2023 15:32
@nokute78 nokute78 mentioned this pull request Feb 24, 2023
Closed
3 tasks
@nokute78
Copy link
Collaborator

@leonardo-albertovich Thank you for reworking.
I tested using thread sanitizer and the race was not detected (except #6776)

Note: Side effect of thread sanitizer ?
#6882 (comment)

I tested following configuration and enabled thread sanitizer.
The receiver didn't output and ignored signal (Ctrl+C).

If I disabled thread sanitizer, the receiver worked correctly.

Receiver:

[INPUT]
    Name forward
    Threaded on

[OUTPUT]
    Name stdout

Sender:

fluent-bit -i dummy -p rate=100000 -o forward

Note: Detail of the PR is
#6882 (comment)
#6882 (comment)

@nokute78 nokute78 mentioned this pull request Feb 24, 2023
Closed
@leonardo-albertovich
Copy link
Collaborator Author

I think once we move the atomic operation abstraction layer from cmetrics to cfl we'll be in a much better position to fix that race in a performant way.

I think it's about time we move atomics to cfl, it's not the first time I've been in a situation where I'd like to use them outside of cmetrics and from a purely organizational point of view it would make sense (which I don't think anyone disagrees with).

@cosmo0920 cosmo0920 mentioned this pull request Mar 9, 2023
Closed
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@edsiper edsiper Awaiting requested review from edsiper edsiper is a code owner

@fujimotos fujimotos Awaiting requested review from fujimotos fujimotos is a code owner

@koleini koleini Awaiting requested review from koleini koleini is a code owner

Assignees
No one assigned
Labels
docs-required
Projects
None yet
Milestone
Fluent Bit v2.1.0
Development

Successfully merging this pull request may close these issues.
4 participants
@leonardo-albertovich @edsiper @nokute78 @lecaros