-
Notifications
You must be signed in to change notification settings - Fork 899
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.
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
fix(userspace/falco): fixed grpc server shutdown. #2350
Conversation
Signed-off-by: Federico Di Pierro <nierro92@gmail.com>
/milestone 0.34.0 |
Example output:
|
Commented on the issue ticket #2342 (comment) that we are still seeing this error. |
I am still unable to repro :( I cannot repro using latest build from this PR: https://app.circleci.com/pipelines/github/falcosecurity/falco/3569/workflows/dc4db6ae-3f60-4245-9125-cfc21d15949e/jobs/30666/artifacts Did you make a clean build for Falco? |
From master, as soon as i ctrl-C on the falco tab:
On this PR:
EDIT: in both cases, falco-exporter leaves with no error though:
|
The latest build was from master (as of 2 days ago) and pulling this PR ontop of that. Can you clarify what you meant by |
I meant "rm -rf build/ && mkdir build && cmake"! |
Ah; yup -- that is what I did to build + create the package. I can take a look at the build specifically generated from this PR once the workday resumes. |
Not sure if this helps narrow the problem down, but the latest SIGABRTs (after a clean build from master @ 306f9ba and this PR pulled in) occurred on ARM64 nodes. Truth be told, we didn't enable gRPC on for that long to determine if it continues to happen in AMD64 nodes as well. So, to sum up -- we were thinking that this PR resolved the SIGABRTs on AMD64 nodes and unfortunately are continuing on ARM64 nodes? Is there any way we can examine this further or gather more info regarding this? If there are a set of commands and/or conf file we can test would, happy to do so and report our findings. |
That would be so strange! |
Sorry; we are running Some more clarification: we do not think the SIGABRT happens because of falco-exporter but when the falco service is interrupted/needs to restart because of config management software (such as during a salt highstate). |
Uh it's that we do not release falco-exporter for arm :D ok!
Yep this is what i think too |
Just to add some additional context (Fede and I had a quick chat on Slack):
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/approve
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: FedeDP, jasondellaluce, leogr The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
What type of PR is this?
/kind bug
Any specific area of the project related to this PR?
/area engine
What this PR does / why we need it:
This PR fixes the grpc server shutdown.
Nowadays, we support hot-reload by default when ruleset/config is updated; this led to sigabrts when the grpc server was enabled since it was not properly shutdown.
Which issue(s) this PR fixes:
Help with (but probably does not fix) #2342
Special notes for your reviewer:
Does this PR introduce a user-facing change?: