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.

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-1275: Introduce new "INNER" direction for inner-node traffic #378

Merged
merged 5 commits into from
Sep 8, 2023

Conversation

jotak
Copy link
Member

@jotak jotak commented Sep 1, 2023

Related PR: netobserv/flowlogs-pipeline#483

The flows (and duplicates) generated for inner-node traffic differs compared to node-to-node traffic, and reinterpret direction isn't able to decide between ingress or egress. This is causing discrepancies with the dedup mechanism that filters out flows where Duplicate=true and also favors ingress over egress, since the Duplicate flag can be set randomly on ingress or on egress.

To fix that, the proposed solution is to create this new INNER direction specifically for this kind of traffic. Deduping this INNER traffic can then rely solely on the Duplicate flag, since that flag was set from a single Agent (single node) there will always be only one Duplicate=false.

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

github-actions bot commented Sep 1, 2023

New image:
quay.io/netobserv/network-observability-console-plugin:5ea6d76

It will expire after two weeks.

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

USER=netobserv VERSION=5ea6d76 make set-plugin-image

@jotak jotak changed the title Introduce new "INNER" direction for inner-node traffic NETOBSERV-1275: Introduce new "INNER" direction for inner-node traffic Sep 1, 2023
@openshift-ci-robot
Copy link
Collaborator

openshift-ci-robot commented Sep 1, 2023

@jotak: This pull request references NETOBSERV-1275 which is a valid jira issue.

In response to this:

Related PR: netobserv/flowlogs-pipeline#483

The flows (and duplicates) generated for inner-node traffic differs compared to node-to-node traffic, and reinterpret direction isn't able to decide between ingress or egress. This is causing discrepancies with the dedup mechanism that filters out flows where Duplicate=true and also favors ingress over egress, since the Duplicate flag can be set randomly on ingress or on egress.

To fix that, the proposed solution is to create this new INNER direction specifically for this kind of traffic. Deduping this INNER traffic can then rely solely on the Duplicate flag, since that flag was set from a single Agent (single node) there will always be only one Duplicate=false.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@jotak
Copy link
Member Author

jotak commented Sep 1, 2023

In Traffic Flows views, new kind of Direction: Inner for on-node traffic:

image

The "Total bytes" metric on topology (edge labels) now matches the actual bytes count as reported in the Traffic Flows view:

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 1, 2023
@jotak jotak marked this pull request as ready for review September 1, 2023 14:10
@codecov
Copy link

codecov bot commented Sep 1, 2023

Codecov Report

Patch coverage: 90.90% and project coverage change: -0.77% ⚠️

Comparison is base (1ca3b6a) 57.94% compared to head (2fafef4) 57.17%.
Report is 4 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #378      +/-   ##
==========================================
- Coverage   57.94%   57.17%   -0.77%     
==========================================
  Files         166      167       +1     
  Lines        7725     7885     +160     
  Branches      938      959      +21     
==========================================
+ Hits         4476     4508      +32     
- Misses       2981     3101     +120     
- Partials      268      276       +8     
Flag Coverage Δ
uitests 58.03% <88.88%> (-0.31%) ⬇️
unittests 54.79% <100.00%> (-2.03%) ⬇️

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

Files Changed Coverage Δ
...rc/components/dropdowns/query-options-dropdown.tsx 82.35% <ø> (ø)
web/src/utils/columns.ts 66.53% <ø> (+<0.01%) ⬆️
web/src/utils/filter-definitions.ts 66.66% <ø> (ø)
web/src/utils/filter-options.ts 51.85% <ø> (ø)
web/src/components/netflow-record/record-field.tsx 69.56% <50.00%> (ø)
pkg/model/filters/filters.go 82.22% <100.00%> (ø)
web/src/api/ipfix.ts 100.00% <100.00%> (ø)
web/src/utils/flows.ts 100.00% <100.00%> (ø)

... and 24 files with indirect coverage changes

☔ View full report in Codecov by Sentry.

📢 Have feedback on the report? Share it here.

Copy link
Contributor

@jpinsonneau jpinsonneau left a comment

Choose a reason for hiding this comment

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

Approach seems good, thanks @jotak
Have you tried with conversation tracking ? It would be great to solve all at once. See netobserv/flowlogs-pipeline#483 (review)

(
recs.find(
r =>
r.fields.DnsId !== undefined || r.fields.PktDropBytes !== undefined || r.fields.PktDropPackets !== undefined
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
r.fields.DnsId !== undefined || r.fields.PktDropBytes !== undefined || r.fields.PktDropPackets !== undefined
r.fields.DnsId !== undefined || r.fields.PktDropPackets !== undefined

I rely on PktDropPackets for this check. Same on FLP side.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also need to change the Duplicated flows tooltip if you do that change:
Currently:

A flow might be reported from several interfaces, and from both source and destination nodes, making it appear several times. 
By default, duplicates are hidden in favor of Ingress traffic. Showing duplicates is not possible in Overview and Topology tabs to avoid altering metric calculations. 
Use Direction filter to switch between Ingress / Egress traffic.

Copy link
Member Author

Choose a reason for hiding this comment

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

Also in the Direction column tooltip, we should probably clarify that it's direction related to the node

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, I was even thinking about changing the column name to make it more explicit.

Should we also expose IfDirection as filter / column ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Related PR: netobserv/flowlogs-pipeline#483

The flows (and duplicates) generated for inner-node traffic differs compared to node-to-node traffic, and reinterpret direction isn't able to decide between ingress or egress. This is causing discrepancies with the dedup mechanism that filters out flows where Duplicate=true and also favors ingress over egress, since the Duplicate flag can be set randomly on ingress or on egress.

To fix that, the proposed solution is to create this new INNER direction specifically for this kind of traffic. Deduping this INNER traffic can then rely solely on the Duplicate flag, since that flag was set from a single Agent (single node) there will always be only one Duplicate=false.
@jotak
Copy link
Member Author

jotak commented Sep 8, 2023

thanks @jpinsonneau
/approve
will be tested by QE post-merge

@openshift-ci
Copy link

openshift-ci bot commented Sep 8, 2023

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

@openshift-ci openshift-ci bot added the approved label Sep 8, 2023
@openshift-merge-robot openshift-merge-robot merged commit bffb860 into netobserv:main Sep 8, 2023
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants