hubble-relay: implement flows reordering#11397
Merged
Conversation
7 tasks
gandro
requested changes
May 7, 2020
Member
gandro
left a comment
There was a problem hiding this comment.
The implementation looks good to me, one change requested (and one nit).
While I agree with the commit description that we want to test this in CI, it seems to me that this is also something that should be very easily testable by unit tests as well. This probably needs a bit of boiler-plate first (i.e. we want to be able to mock the different peers and the gRPC client), so I'm fine with merging this feature without unit tests, but I do believe that we should add unit tests for this.
| } | ||
| } | ||
| pq.Push(flow) | ||
| case <-time.After(1 * time.Second): // make sure to drain the queue when no new flow responses are received |
Member
There was a problem hiding this comment.
nit: Since we are calling this for each flow and it is presumably creating a channel each time, I wonder if creating a time.NewTimer and calling Reset at the end of the loop might be slightly more efficient. But I actually don't know, just something that stood out to me.
Member
Author
There was a problem hiding this comment.
I could add a benchmark in the tests and actually measure this.
Member
Author
I'll mark this PR as draft while I implement unit tests. We're not in a hurry to merge it so I can dedicate time for that. |
aanm
reviewed
May 7, 2020
99caa70 to
fda1329
Compare
Member
Author
|
@gandro I started to give a go at implementing unit tests but it would require a lot of refactoring as the existing code is not easily mockable. I'll create an issue and implement tests for the whole |
glibsm
reviewed
May 8, 2020
Comment on lines
+1
to
+2
| // Copyright 2019 Authors of Hubble | ||
| // Copyright 2020 Authors of Cilium |
gandro
reviewed
May 11, 2020
Member
gandro
left a comment
There was a problem hiding this comment.
The cancellation logic looks good to me now, I don't see any point which can block forever.
There is one case where the ctx.Done() check will be delayed (see comment) however.
fda1329 to
893ffe5
Compare
gandro
approved these changes
May 11, 2020
Member
Author
|
@glibsm I made these values configurable (see |
Member
Author
|
test-me-please |
Member
I disagree with that. We can provide sane defaults and have documentation for what these values do, but fine tuning is a per-environment thing. There is no way we can provide a default that works for everybody |
glibsm
approved these changes
May 11, 2020
Member
glibsm
left a comment
There was a problem hiding this comment.
Exposing through CLI is not blocking this particular PR though
Member
|
test-me-please |
This priority queue implementation is not yet used and only relevant for hubble-relay. Signed-off-by: Robin Hahling <robin.hahling@gw-computing.net>
Hubble-relay needs a priority queue for GetFlowsResponse objects, not Events. Note that making this priority queue more generic by using an interface instead is not possible as one would have to perform nil checks on an interface type (which would be asking for trouble). Signed-off-by: Robin Hahling <robin.hahling@gw-computing.net>
Signed-off-by: Robin Hahling <robin.hahling@gw-computing.net>
This field is pretty useless if not set. Signed-off-by: Robin Hahling <robin.hahling@gw-computing.net>
This field is pretty useless if not set. Signed-off-by: Robin Hahling <robin.hahling@gw-computing.net>
This commits implements flows reordering for hubble-relay. The goal is to reorder flows based on their timestamp in a "best-effort" basis as waiting for all flows and buffering would have serious drawbacks such as extreme memory usage in some scenarios (and it would not work in follow-mode anyway as the number of flows is virtually infinite in such a case). This implementation uses a priority queue where Pop() returns the oldest element in the queue. In order to ensure that elements are popped out from the queue even when the queue is not full (typically for requests in follow-mode that don't result in a large number of flows), an element is tentatively popped out from the queue at least every second. I have tested this implementation on a 3 nodes cluster and both in follow-mode and non follow-mode. Unit tests need to be implemented but this is true for the relay package in general which is not easily mockable with the current implementation. Signed-off-by: Robin Hahling <robin.hahling@gw-computing.net>
893ffe5 to
faca702
Compare
Member
Author
|
test-me-please |
Contributor
Contributor
|
test-gke |
Member
|
retest-4.9 |
Member
|
retest-runtime |
Member
|
retest-net-next |
Contributor
|
In case of runtime tests, there is something else going on: |
Contributor
|
And in case of |
Contributor
|
No runtime tests are failing with this: And |
Member
Author
@errordeveloper I think this one is #11512 The other one seems to be a transient failure (alpine pkg database could not be reached thus building the docker image failed). |
Member
|
retest-netnext |
Member
|
retest-net-next |
Member
|
retest-runtime |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Please, see individual commits for details (this PR also includes 2 fixes for hubble observer).
Closes #11223.