Fix issue with inconsistent GenericMap output #297
Conversation
The FLP GenericMap provided by the agent differs in types depending on the exporter being used, especially with DirectFLP each records size is smaller than when used with protobuf, due to protobuf using uint32 almost everywhere. This is not a problem in most cases as FLP generally converts that in JSON, but it breaks the FLP IPFIX export which expects strict types. This commit brings more consistency between the FLP-direct mode and the protobuf mode, by sharing the same GenericMap encoding function. Which means the protobuf decode is now done in two steps: first proto-to-flow.Record, then flow.Record-to-GenericMap.
jotak
added
the
breaking-change
jotak
added
the
ok-to-test
|
New image: It will expire after two weeks. To deploy this build, run from the operator repo, assuming the operator is running: USER=netobserv VERSION=e1397a0 make set-agent-image |
github-actions
bot
removed
the
ok-to-test
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #297 +/- ##
==========================================
- Coverage 36.74% 33.17% -3.57%
==========================================
Files 42 40 -2
Lines 3813 3587 -226
==========================================
- Hits 1401 1190 -211
+ Misses 2334 2322 -12
+ Partials 78 75 -3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
|
||
| // FlowToPB is an auxiliary function to convert a single flow record, as returned by the eBPF agent, | ||
| // into a protobuf-encoded message ready to be sent to the collector via kafka | ||
| func FlowToPB(fr *flow.Record) *Record { |
There was a problem hiding this comment.
curious why did u move this file from export to here ?
There was a problem hiding this comment.
it's also called from decode package now, and leaving it in export would create a cycle. The other option would be to move it to decode, I can do that if you prefer.
| return net.IP(ip.GetIpv6()) | ||
| } | ||
| n := ip.GetIpv4() | ||
| return net.IPv4( |
There was a problem hiding this comment.
is it any cleaner like this
byteArray := make([]byte, 4)
binary.NativeEndian.PutUint32(byteArray, n)or is it possible to maye it byte type in pb like ipv6 so we can avoid all of this ?
There was a problem hiding this comment.
I can do your first suggestion (I'm not sure about the other, if the way it is was initially done for good reasons or not..)
But I guess I should either use LittleEndian or BigEndian but not Native, as that would make it arch-dependent (and thus fail on BE archs)
|
any cpu impact for the additional processing with ur scale test ? |
|
why this is marked as breaking change ? |
That's a breaking change because the types in the GenericMap output change, as you can see here: https://github.com/netobserv/netobserv-ebpf-agent/pull/297/files#diff-5b98b198dc9b743f2c1c2d470d443d9ec2ed38b4ee15777cb857584ddacce657 So, consumers who would cast explicitly to uint32 would now have to cast to uint8 |
|
/lgtm |
Fix inconsistent maps received from the agent depending on the export mode (grpc / kafka / direct-flp) Depends on netobserv/netobserv-ebpf-agent#297
Slight increase of CPU in one of the tests (0.167 -> 0.172) and memory as well (671MB -> 679 MB) (i picked the one run that seems more relevant as it showed a similar number of flows per seconds). I'd say it's in reasonable bounds... |
jotak
added
no-doc
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jotak 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 |
|
/retest-required |
|
/retest |
Fix inconsistent maps received from the agent depending on the export mode (grpc / kafka / direct-flp) Depends on netobserv/netobserv-ebpf-agent#297
* Fix inconsistent maps Fix inconsistent maps received from the agent depending on the export mode (grpc / kafka / direct-flp) Depends on netobserv/netobserv-ebpf-agent#297 * bump agent protobuf
Description
The FLP GenericMap provided by the agent differs in types depending on the exporter being used, especially with DirectFLP each records size is smaller than when used with protobuf, due to protobuf using uint32 almost everywhere. This is not a problem in most cases as FLP generally converts that in JSON, but it breaks the FLP IPFIX export which expects strict types.
This commit brings more consistency between the FLP-direct mode and the protobuf mode, by sharing the same GenericMap encoding function. Which means the protobuf decode is now done in two steps: first proto-to-flow.Record, then flow.Record-to-GenericMap.
Dependencies
FLP update of the IPFIX exporter: netobserv/flowlogs-pipeline#634
Checklist
If you are not familiar with our processes or don't know what to answer in the list below, let us know in a comment: the maintainers will take care of that.