Conversation
|
cc @cilium/sig-hubble |
e4eaca2 to
a426dda
Compare
|
cc @cilium/sig-datapath |
|
On a very high-level - you're proposing that the datapath matches some specific IP option in the packet (containing some As an alternative thought - afaik the datapath is already punting some(?) packet headers to userspace. If you increase the number of punted bytes (so that it includes the IP options), would that allow you to move all the needed IP option-parsing into hubble? |
We want the This will ensure that subsequent packets in the same connection maintain their traceability even if they do not carry the Also, by pushing this to Hubble, we lose integration with the cilium monitor for events. Considering the future customer use-cases of this feature, we want the ability to see the Is there a specific concern you have about including this in the trace_notify and drop_notify structs? (these fields would only be populated if the feature is enabled and there is an embedded |
bowei
left a comment
There was a problem hiding this comment.
minor comments -- proposal looks great
3b11a51 to
4aa290d
Compare
pchaigno
left a comment
There was a problem hiding this comment.
Thanks for the feature proposal! It's interesting 😃
Just one question below on the verbose debug case.
9915947 to
3809611
Compare
|
CC: @sypakine |
d6cee8b to
5f359e8
Compare
joestringer
left a comment
There was a problem hiding this comment.
Thanks for working on this proposal, it sounds useful. I think this PR mainly just needs the metadata at the top to be updated to reflect that this is now considered "Implementable".
Reading through I did wonder about a couple of points, but I think that those points are probably crossing over from the design aspect and more into the implementation aspect, which I don't think needs to be signed off on in order to merge the PR considering it "Implementable". I figured it's just worth highlighting those below since those aspects could potentially come up during implementation / code review.
|
|
||
| ### Section 4: Event integration | ||
|
|
||
| Include ID as event field for trace and drop structs and the flow message (for Hubble integration): |
There was a problem hiding this comment.
Broadly this makes sense. We probably want to think about whether this uses a new trace_notify_v2 message type (and similarly new message type number + structure for drop). We've done this in the past at least, though it's probably worth digging into the reasoning why we did that to check whether it's still necessary.
There was a problem hiding this comment.
^ This doesn't need to be resolved before merging the CFP as "Implementable". We can figure out those details on the PR.
There was a problem hiding this comment.
Sounds good, I updated the CFP with relevant changes, please let me know if there's anything else necessary before merging. Thank you!
|
|
||
| Mitigation: This feature is a manually enabled debug option, giving users full control over performance overhead of IP option tagged packets. Users can manage overhead and performance concerns related to MTU, GRO compatibility, packet loss, and IP checksum recalculation. By making packet tagging the client's responsibility, Cilium remains unaffected by these concerns, allowing clients to incorporate these considerations into their use of the feature and determine the best way to utilize it according to their specific needs. | ||
|
|
||
| ## Benchmark Testing |
There was a problem hiding this comment.
I appreciate the additional work over-and-beyond a CFP to put benchmarks in here :) Typically for a CFP this is not necessary, but since you've put it together I don't mind having this in here. Usually I'd expect this to go with the corresponding implementation PR rather than the design, but I know that the review for this design was slow this cycle and you've taken the time to develop the solution during that delayed design review.
f9e5108 to
07c78f9
Compare
- Added CFP for packet tracing feature. Signed-off-by: Ben Bigdelle <[email protected]>
07c78f9 to
e106e1c
Compare
|
I think this CFP is in a solid state for implementing. There's a couple of details that could maybe be hashed out a bit more, but I don't think they ultimately detract that much from the overall proposal, and may be impacted by the experience of implementing it. The proposal has been out plenty long enough for @cilium/sig-hubble and @cilium/sig-datapath to provide feedback, and there's now committers endorsing the proposal, so I think this is in a good enough state to merge. As and when the implementation matures and impacts the design, we can always iterate further on this CFP, but we don't have to hold it as a PR during that period. Let's get this merged 🚀 |
Add PR for CFP: Packet Tracing.