Add a new exporter type: DirectFLP #223

jotak · 2023-11-16T14:41:21Z

Description

DirectFLP starts the whole flowlogs-pipeline process directly from the
Agent, eliminating the need for a network connection between them.

This can be useful when both Agent and FLP run on the same host, saving
resources. This is mutually exclusive with other exporters such as with
Kafka.

Dependencies

netobserv/flowlogs-pipeline#538

(hold until this is merged => replace directive to remove)

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.

codecov · 2023-11-16T15:32:49Z

Codecov Report

Attention: 17 lines in your changes are missing coverage. Please review.

Comparison is base (a5378d6) 31.83% compared to head (fa1002c) 33.38%.

Files	Patch %	Lines
pkg/agent/agent.go	0.00%	8 Missing ⚠️
pkg/exporter/direct_flp.go	73.91%	4 Missing and 2 partials ⚠️
pkg/exporter/convert_flp.go	95.08%	2 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #223      +/-   ##
==========================================
+ Coverage   31.83%   33.38%   +1.55%     
==========================================
  Files          37       39       +2     
  Lines        3371     3463      +92     
==========================================
+ Hits         1073     1156      +83     
- Misses       2234     2241       +7     
- Partials       64       66       +2

Flag	Coverage Δ
unittests	`33.38% <83.96%> (+1.55%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

jpinsonneau · 2023-11-17T11:38:24Z

pkg/agent/config.go

+	// Flowlogs-pipeline configuration, used when export is "direct-flp". Cf https://github.com/netobserv/flowlogs-pipeline
+	// The "ingest" stage must be omitted from this configuration, since it is handled internally by the agent.
+	FLPConfig string `env:"FLP_CONFIG"`


As soon as FLP_CONFIG is set, EXPORT should be ignored.
The export capabilities from pod point of view are inside FLP config right ?

I guess it depends from where you sit.. In my mind, this new mode is really a new exporter, sitting from the Agent codebase PoV: the agent generates flows and then exports them somewhere, here to FLP; the fact that FLP is in the same process is a detail, it's still an exporter from this codebase PoV.

I guess you have more the "external user point of view" where, once FLP config is set, "export" would mean what happens "after FLP processing", which indeed doesn't really make sense since FLP does the export (to loki, prom, etc.)

idk, I think both approaches can be defended ... @msherif1234 , some preference?

thinking of it as new exporter mode resonate well with me as well

jpinsonneau · 2023-11-17T11:41:01Z

pkg/agent/agent.go

+	case "direct-flp":
+		return buildDirectFLPExporter(cfg)


I would move that outside of cfg.Export as:

if len(cfg.FLPConfig) { return buildDirectFLPExporter(cfg) } switch cfg.Export { ... }

jpinsonneau

Approach sounds good ! Just a small change on config to make it clear for the user
We should also add an example in deployments folder

msherif1234 · 2023-11-20T16:12:45Z

pkg/exporter/direct_flp.go

+	// is screwed up for HTTPClientConfig in github.com/prometheus/common/config (used for Loki)
+	// This is ok as YAML is a superset of JSON.
+	// E.g. try unmarshaling `{"clientConfig":{"authorization":{}}}` as a api.WriteLoki
+	// See also https://github.com/prometheus/prometheus/issues/11816


interesting thanks for referring to the issue in promo its almost 1yr so probably this work around will stay for sometime

Yeah there's even a PR from @OlivierCazade that is older than that :-) prometheus/common#394

msherif1234 · 2023-11-20T16:29:10Z

pkg/exporter/direct_flp.go

+func (d *DirectFLP) ExportFlows(input <-chan []*flow.Record) {
+	for inputRecords := range input {
+		pbRecords := flowsToPBNoChunk(inputRecords)
+		d.Write(pbRecords)


This step can be removed if we let the new FLP pipe process ebpf record directly not protobuf records

done via bcccbb7

jotak · 2023-11-21T11:53:08Z

pkg/decode/decode_protobuf.go

-				out["SrcPort"] = flow.Transport.GetSrcPort()
-				out["DstPort"] = flow.Transport.GetDstPort()
-			}
+	if ethType != ethernet.EtherTypeIPv4 && ethType != ethernet.EtherTypeIPv6 {


@msherif1234 I'm doing this early return here when it's not ipv4/ipv6, could you confirm this is correct?
Previously, in that case (e.g. with ARP), the code would still check for drops/DNS/RTT fields and would add for instance DnsErrno: 0, while now this isn't the case anymore, but I believe our drops/DNS/RTT metrics wouldn't make sense for non-IP packets, right?

If this doesn't sound good to you, I can rollback, but I'd need to adapt also the ConvertToFLP function because I'm enforcing in my tests that the same flow input produces the exact same output regardless if it's using ConvertToFLP or converting via protobuf.

the only thing we could be missing here is l2 drops, also pls test with DNS TCP to be sure DNSError is only none zero for TCP handshake I can help you with this test if u like

jotak · 2023-11-21T11:56:37Z

pkg/exporter/convert_flp_test.go

+	someTime := time.Now()
+	var someDuration time.Duration = 10000000 // 10ms
+
+	tests := []struct {


@msherif1234 FYI the tests here are the same that were previously in decode_protobuf_test except that they're now used to test both pb conversion and direct FLP conversion, making sure they both produce the same result

this test coverting from ebpf record to flp configmap but it seems u deleted from gRPC to configmap in case this mode isn't set ?

cf my next comment below

msherif1234 · 2023-11-21T15:18:36Z

pkg/decode/decode_protobuf_test.go

-		assert.Equalf(t, *tt.expected, out, tt.name)
-	}
-}
+// NOTE: more tests in convert_flp_test.go


did intentionally deleted conversion from gRPC to generic map test cases here ?

yes, like I wrote this test was moved (and sightly changed) to convert_flp_test, but I admit the name is misleading, I'll rename that file to clarify it's testing BOTH grpc and direct-flp conversions

msherif1234 · 2023-11-21T15:22:32Z

pkg/flow/record.go

@@ -17,7 +17,8 @@ const (
 )
 const MacLen = 6

-// IPv6Type value as defined in IEEE 802: https://www.iana.org/assignments/ieee-802-numbers/ieee-802-numbers.xhtml
+// IPv4Type / IPv6Type value as defined in IEEE 802: https://www.iana.org/assignments/ieee-802-numbers/ieee-802-numbers.xhtml
+const IPv4Type = 0x0800


nit: let use ethernet.EtherTypeIPv4 and ethernet.EtherTypeIPv6 instead of hardcoding them ourself

jotak · 2023-11-22T14:33:40Z

pkg/exporter/convert_flp_test.go

+		require.NoError(t, err)
+		assert.Equalf(t, *tt.expected, outDirect, tt.name)
+
+		// Test pb conversion


see here @msherif1234 : I am testing both FLP conversion AND protobuf conversion, with the bonus thatit makes sure the two are producing the same result (they're both compared against the same expected map)

DirectFLP starts the whole flowlogs-pipeline process directly from the Agent, eliminating the need for a network connection between them. This can be useful when both Agent and FLP run on the same host, saving resources. This is mutually exclusive with other exporters such as with Kafka. rollback e2e deps bump Avoid pbuf conversion when using direct-flp Clarify test file name, use existing ipv6 const Restore L2 drops

msherif1234 · 2023-11-23T14:46:54Z

/lgtm

jotak · 2023-11-23T15:39:40Z

/unhold

msherif1234 · 2023-11-23T20:53:27Z

/lgtm

jotak · 2023-11-24T07:11:44Z

thanks @msherif1234 !
/approve

openshift-ci · 2023-11-24T07:11:54Z

[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:

~~OWNERS~~ [jotak]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

jotak added no-doc This PR doesn't require documentation change on the NetObserv operator no-qe This PR doesn't necessitate QE approval labels Nov 16, 2023

jotak requested review from msherif1234 and OlivierCazade November 16, 2023 14:41

jotak added the do-not-merge/hold label Nov 16, 2023

jotak force-pushed the flp-in-process branch from d04eb19 to ebae6ee Compare November 16, 2023 15:28

jotak requested a review from jpinsonneau November 17, 2023 11:02

jpinsonneau reviewed Nov 17, 2023

View reviewed changes

msherif1234 reviewed Nov 20, 2023

View reviewed changes

jotak commented Nov 21, 2023

View reviewed changes

msherif1234 reviewed Nov 21, 2023

View reviewed changes

openshift-merge-robot added the needs-rebase label Nov 22, 2023

jotak commented Nov 22, 2023

View reviewed changes

jotak force-pushed the flp-in-process branch from 2be7d31 to 2a848d8 Compare November 22, 2023 14:50

openshift-merge-robot removed the needs-rebase label Nov 22, 2023

jotak force-pushed the flp-in-process branch from 35398d6 to 8a3894f Compare November 23, 2023 14:46

openshift-ci bot assigned msherif1234 Nov 23, 2023

openshift-ci bot added the lgtm label Nov 23, 2023

update flp dep

fa1002c

openshift-ci bot removed the lgtm label Nov 23, 2023

openshift-ci bot removed the do-not-merge/hold label Nov 23, 2023

openshift-ci bot added the lgtm label Nov 23, 2023

openshift-ci bot added the approved label Nov 24, 2023

openshift-merge-bot bot merged commit 06f4492 into netobserv:main Nov 24, 2023
9 checks passed

jotak deleted the flp-in-process branch November 29, 2023 13:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a new exporter type: DirectFLP #223

Add a new exporter type: DirectFLP #223

jotak commented Nov 16, 2023 •

edited

codecov bot commented Nov 16, 2023 •

edited

jpinsonneau Nov 17, 2023

jotak Nov 17, 2023

msherif1234 Nov 20, 2023

jpinsonneau Nov 17, 2023

jpinsonneau left a comment

msherif1234 Nov 20, 2023

jotak Nov 21, 2023

msherif1234 Nov 20, 2023

jotak Nov 21, 2023

jotak Nov 21, 2023 •

edited

msherif1234 Nov 21, 2023

jotak Nov 21, 2023

msherif1234 Nov 21, 2023

jotak Nov 22, 2023

msherif1234 Nov 21, 2023

jotak Nov 22, 2023

msherif1234 Nov 21, 2023

jotak Nov 22, 2023

msherif1234 commented Nov 23, 2023

jotak commented Nov 23, 2023

msherif1234 commented Nov 23, 2023

jotak commented Nov 24, 2023

openshift-ci bot commented Nov 24, 2023

Add a new exporter type: DirectFLP #223

Add a new exporter type: DirectFLP #223

Conversation

jotak commented Nov 16, 2023 • edited

Description

Dependencies

Checklist

codecov bot commented Nov 16, 2023 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jpinsonneau left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jotak Nov 21, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msherif1234 commented Nov 23, 2023

jotak commented Nov 23, 2023

msherif1234 commented Nov 23, 2023

jotak commented Nov 24, 2023

openshift-ci bot commented Nov 24, 2023

jotak commented Nov 16, 2023 •

edited

codecov bot commented Nov 16, 2023 •

edited

jotak Nov 21, 2023 •

edited