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.

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

Optimize notice framework cluster communication #214

Closed
jsiwek opened this issue Nov 15, 2018 · 4 comments
Closed

Optimize notice framework cluster communication #214

jsiwek opened this issue Nov 15, 2018 · 4 comments
Assignees
Labels
Complexity: Modest A cup of tea and an evening (or two) with Zeek. Type: Enhancement
Milestone

Comments

@jsiwek
Copy link
Contributor

jsiwek commented Nov 15, 2018

This code seems to be a no-op since it runs on the manager and not workers, but the performance problem it intended to address is still real:

https://github.com/bro/bro/blob/3f206cb8a9d90c10f2f9824373604dc0e205956b/scripts/base/frameworks/notice/main.bro#L656-L664

May require non-trivial changes, like moving notice processing to workers, but still having a way to keep de-duplication and suppression functionality.

@jsiwek jsiwek added Complexity: Modest A cup of tea and an evening (or two) with Zeek. Type: Enhancement labels Nov 15, 2018
@jsiwek jsiwek added this to the 2.6.1 milestone Nov 15, 2018
@jsiwek jsiwek modified the milestones: 2.6.1, 2.7 Jan 11, 2019
@rsmmr
Copy link
Member

rsmmr commented May 24, 2019

Not a blocker, but would be a substantial improvement.

@0xxon 0xxon self-assigned this Jun 20, 2019
@0xxon
Copy link
Member

0xxon commented Jun 20, 2019

I might take a stab at this.

After looking at the code for a bit - I think there are two potential ways to solve this.

The first one was already noted by Jon - moving notice processing to workers. I am currently tempted to do this one - however this, in theory, could make it more difficult for users to write notice policies (they will only have local state available for the policies).

The second solution would be to change the datastructures that are being sent over the network - so that they are smaller and do not by default contain the full connection record, etc. This obviously comes with the disadvantage that users won't have as much state available in their policies).

Both approaches break compatibility a bit - at the moment I think that the first one (do the work mostly on the workers) is the more appealing one.

@rsmmr
Copy link
Member

rsmmr commented Jun 21, 2019

I agree that the 1st one seems more appealing; it sounds conceptually right, as it's in line with how other stuff works in the cluster (like log processing). It does raise a major backwards compatibility concern, though; it's just sufficiently convenient to not have to consider cluster issues for notice processing that I'm sure it's been relied on out there somewhere. Not quite sure what to do about it other than warn in NEWS.

0xxon added a commit that referenced this issue Jun 25, 2019
In the past they were processed on the manager - which requires big
records to be sent around.

This has a potential of incompatibilities if someone relied on global
state for notice processing.

GH-214
@0xxon
Copy link
Member

0xxon commented Jun 25, 2019

I tried this and it actually turned out to not rally be very invasive to just move this over to the workers.

Code in branch topic/johanna/gh-214-notice-on-workers or in pull-request #440

One difference is that there now is a chance that notices will not be suppressed immediately - they only will be suppressed once the suppression notice has been distributed over the cluster. I don't really think that is a huge problem though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Complexity: Modest A cup of tea and an evening (or two) with Zeek. Type: Enhancement
Projects
None yet
Development

No branches or pull requests

3 participants