-
Notifications
You must be signed in to change notification settings - Fork 14
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.
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
NETOBSERV-1235 Reporter option issues #366
Conversation
Codecov Report
@@ Coverage Diff @@
## main #366 +/- ##
==========================================
- Coverage 57.83% 57.24% -0.60%
==========================================
Files 166 167 +1
Lines 7679 7774 +95
Branches 918 924 +6
==========================================
+ Hits 4441 4450 +9
- Misses 2971 3056 +85
- Partials 267 268 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
|
fceb0c5
to
e9f939a
Compare
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=846bd0a make set-plugin-image |
NewMatch(fields.FlowDirection, `"`+constants.Ingress+`"`), | ||
NewMatch(fields.SrcOwnerName, `""`), | ||
NewMatch(fields.FlowDirection, `"`+constants.Egress+`"`), | ||
NewMatch(fields.DstOwnerName, `""`), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why would reversing the logic works now, I think u saw that same issue either way ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DNS response codes are in INGRESS
flows so I want these first and then fill the gap with the EGRESS
ones.
pktDrop seems to be more consistent but I also saw differences
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm investigating both download / upload from pod cases since I have issue with old implementation and new one
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here are my tests for consistency:
- using sampling 1
- Download a ubuntu.iso of 1000.73 MB and observe transferts between the IP and Node
- Upload the downloaded image and see the differences with the new IP
--- download ---
curl https://releases.ubuntu.com/22.04.2/ubuntu-22.04.2-desktop-amd64.iso --output /tmp/ubuntu.iso
reinterpret + Egress (old implementation)
pod 1.2GB
node 1.3GB
=> same results using ingress
no reinterpret + Egress
pod 922.7MB
node 625.1MB
no reinterpret + Ingress (new implementation)
pod 9.2MB ???
node 630.4MB
--- upload ---
curl bashupload.com -T /tmp/ubuntu.iso
reinterpret + Egress (old implementation)
pod 2.8GB
node 4.1GB
=> same results using ingress
no reinterpret + Egress
pod 4.4MB ??
node 2.1GB
no reinterpret + Ingress (new implementation)
pod 2.1GB
node 2.1GB
Note:
-
If you
download
a file from a pod and setDestination Namespace
, you don't see the expected bytes in topology view with my update.
With the original one it worked:
-
If you
upload
a file from a pod and setSource Namespace
, you see your flows with my update.
With the original one it didn't worked:
So in conclusion we can't do that simplification as is if we want to keep Src
/ Dst
consistency and track both download
& upload
amounts !
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see #366 (comment) solution 😸
@@ -1319,7 +1319,7 @@ export const NetflowTraffic: React.FC<{ | |||
setShowDuplicates, | |||
allowFlow: isFlow(), | |||
allowConnection: isConnectionTracking(), | |||
allowShowDuplicates: selectedViewId === 'table', | |||
allowShowDuplicates: selectedViewId === 'table' && recordType !== 'allConnections', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can u explain allconnections check ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On frontend side, you can only switch between 'flowLogs' or 'allConnections'.
However, conntrack events doesn't have the "Duplicate"
field. It actually doesn't make sense to filter on this so I disable the checkbox in the interface.
FYI the connection tracking automatically skip duplicates while doing the aggregates
https://github.com/netobserv/flowlogs-pipeline/blob/main/pkg/pipeline/extract/conntrack/conntrack.go#L71-L73
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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=c9dbdcb make set-plugin-image |
I finally got the expected behavior adding the The numbers are also correct here (using sampling 50):
|
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=27681b6 make set-plugin-image |
Also took the opportunity to manage routing for topology view bd7ffc5 When both |
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=47254cc make set-plugin-image |
/lgtm |
I'm merging this as the impact is quite huge. Let's open followups if needed. |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jpinsonneau 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 |
Followup to fix #311 issues
ingress
by defaultFlowDirection
as filter to let user choose metrics seenSee netobserv/network-observability-operator#402 for metrics consistency