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.

Sign up for GitHub

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

Jump to bottom

NETOBSERV-588 Add IPv4/6 DSCP field to the exported flow #158

Merged
merged 1 commit into from
Oct 5, 2023
Merged

NETOBSERV-588 Add IPv4/6 DSCP field to the exported flow #158

merged 1 commit into from
Oct 5, 2023

Conversation

msherif1234
Copy link
Contributor

@msherif1234 msherif1234 commented Jul 20, 2023 •
edited
Loading

tested with standalone flow collector for IPv4

ping 1.1.1.1 -Q 0x60
PING 1.1.1.1 (1.1.1.1) 56(84) bytes of data.
64 bytes from 1.1.1.1: icmp_seq=1 ttl=53 time=18.4 ms
64 bytes from 1.1.1.1: icmp_seq=2 ttl=53 time=17.4 ms
64 bytes from 1.1.1.1: icmp_seq=3 ttl=53 time=17.9 ms
64 bytes from 1.1.1.1: icmp_seq=4 ttl=53 time=23.9 ms
64 bytes from 1.1.1.1: icmp_seq=5 ttl=53 time=15.3 ms
64 bytes from 1.1.1.1: icmp_seq=6 ttl=53 time=16.9 ms
64 bytes from 1.1.1.1: icmp_seq=7 ttl=53 time=18.7 ms
64 bytes from 1.1.1.1: icmp_seq=8 ttl=53 time=15.9 ms

ipv4: 09:45:36.374394 enp1s0 IP 192.168.122.69:0 > 1.1.1.1:0: dscp: 0x18 protocol:icmp type: 8 code: 0 dir:1 bytes:490 packets:5 flags:0 ends: 09:45:40.379900 dnsId: 0 dnsFlags: 0x0000 dnsReq: <nil> dnsRsp: <nil> rtt 0

@codecov
Copy link

codecov bot commented Jul 20, 2023 •
edited
Loading

Codecov Report

Patch coverage: 50.00% and project coverage change: +0.03% 🎉

Comparison is base (f87021a) 31.84% compared to head (01a2f17) 31.88%.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main     #158      +/-   ##
==========================================
+ Coverage   31.84%   31.88%   +0.03%     
==========================================
  Files          37       37              
  Lines        3278     3284       +6     
==========================================
+ Hits         1044     1047       +3     
- Misses       2175     2177       +2     
- Partials       59       60       +1
Flag Coverage Δ
unittests 31.88% <50.00%> (+0.03%) ⬆️

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

Files Changed Coverage Δ
pkg/flow/record.go 57.81% <0.00%> (-2.85%) ⬇️
pkg/decode/decode_protobuf.go 29.15% <100.00%> (+0.22%) ⬆️
pkg/exporter/proto.go 95.20% <100.00%> (+0.07%) ⬆️

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

@msherif1234 msherif1234 force-pushed the dscp_support branch 2 times, most recently from ecdf1ef to 654b0d6 Compare July 25, 2023 17:09
@jpinsonneau jpinsonneau mentioned this pull request Sep 22, 2023
Merged
12 tasks
@jpinsonneau jpinsonneau added the ok-to-test To set manually when a PR is safe to test. Triggers image build on PR. label Sep 22, 2023
@github-actions
Copy link

New image:
quay.io/netobserv/netobserv-ebpf-agent:8df3d91

It will expire after two weeks.

To deploy this build, run from the operator repo, assuming the operator is running:

USER=netobserv VERSION=8df3d91 make set-agent-image

@jpinsonneau jpinsonneau mentioned this pull request Sep 22, 2023
Closed
11 tasks
@github-actions github-actions bot removed the ok-to-test To set manually when a PR is safe to test. Triggers image build on PR. label Sep 22, 2023
@msherif1234
Copy link
Contributor Author

/ok-to-test

@openshift-ci openshift-ci bot added the ok-to-test To set manually when a PR is safe to test. Triggers image build on PR. label Sep 22, 2023
@github-actions
Copy link

New image:
quay.io/netobserv/netobserv-ebpf-agent:df1b1dd

It will expire after two weeks.

To deploy this build, run from the operator repo, assuming the operator is running:

USER=netobserv VERSION=df1b1dd make set-agent-image

@github-actions github-actions bot removed the ok-to-test To set manually when a PR is safe to test. Triggers image build on PR. label Sep 25, 2023
@msherif1234
Copy link
Contributor Author

/ok-to-test

@openshift-ci openshift-ci bot added the ok-to-test To set manually when a PR is safe to test. Triggers image build on PR. label Sep 25, 2023
@github-actions
Copy link

New image:
quay.io/netobserv/netobserv-ebpf-agent:75fed66

It will expire after two weeks.

To deploy this build, run from the operator repo, assuming the operator is running:

USER=netobserv VERSION=75fed66 make set-agent-image

jotak
jotak reviewed Sep 29, 2023
View reviewed changes
bpf/flows.c
@@ -86,7 +86,7 @@ static inline int flow_monitor(struct __sk_buff *skb, u8 direction) {
aggregate_flow->start_mono_time_ts = pkt.current_ts;
}
aggregate_flow->flags |= pkt.flags;

aggregate_flow->dscp = pkt.dscp;
Copy link
Member

@jotak jotak Sep 29, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not asking any change here, but just for curiosity, do you know if a single flow might have packets with different dscp values ?
(if that's the case, then only the last one would be taken here; which can be OK I guess

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was reading about this and it sounds the DSCP value is per flow this how they classify flow as higher pri over the others this is where I get that
https://datatracker.ietf.org/doc/html/rfc8837#name-dscp-mappings

@openshift-ci openshift-ci bot added the lgtm label Sep 29, 2023
@msherif1234
Copy link
Contributor Author

@Amoghrd when u can, can we give this is a go Thanks!!

@Amoghrd
Copy link

Amoghrd commented Oct 5, 2023

/label qe-approved

@openshift-ci openshift-ci bot added the qe-approved QE has approved this pull request label Oct 5, 2023
@msherif1234
Copy link
Contributor Author

/approve

@openshift-ci
Copy link

openshift-ci bot commented Oct 5, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: msherif1234

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the approved label Oct 5, 2023
@openshift-ci openshift-ci bot merged commit ca897d5 into netobserv:main Oct 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jotak jotak jotak approved these changes

Assignees

@jotak jotak

Labels
approved lgtm ok-to-test To set manually when a PR is safe to test. Triggers image build on PR. qe-approved QE has approved this pull request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@msherif1234 @Amoghrd @jotak @openshift-merge-robot @jpinsonneau