NETOBSERV-1551: show cross-nodes duplicates in merge mode

[jotak]

Description

This PR currently just shows the cross-nodes duplicates.

Following the reproduce steps described in NETOBSERV-1551, I'm now seeing both interfaces:

But they are displayed as distinct flows. So it raises the question: should we reintroduce the "Show duplicates" checkbox, since there are still the cross-nodes duplicates even in merge mode? Or should we merge them post-query without asking? If we do so, what to put in Node Direction? "both" ?

Dependencies

n/a

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.

• Is this PR backed with a JIRA ticket? If so, make sure it is written as a title prefix (in general, PRs affecting the NetObserv/Network Observability product should be backed with a JIRA ticket - especially if they bring user facing changes).
• Does this PR require product documentation?
  • If so, make sure the JIRA epic is labelled with "documentation" and provides a description relevant for doc writers, such as use cases or scenarios. Any required step to activate or configure the feature should be documented there, such as new CRD knobs.
• Does this PR require a product release notes entry?
  • If so, fill in "Release Note Text" in the JIRA.
• Is there anything else the QE team should know before testing? E.g: configuration changes, environment setup, etc.
  • If so, make sure it is described in the JIRA ticket.
• QE requirements (check 1 from the list):
  • Standard QE validation, with pre-merge tests unless stated otherwise.
  • Regression tests only (e.g. refactoring with no user-facing change).
  • No QE (e.g. trivial change with high reviewer's confidence, or per agreement with the QE team).

[openshift-ci-robot]

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

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the bug to target the "4.16.0" version, but no target version was set.

In response to this:

Description

This PR currently just shows the cross-nodes duplicates.

Following the reproduce steps described in NETOBSERV-1551, I'm now seeing both interfaces:

But they are displayed as distinct flows. So it raises the question: should we reintroduce the "Show duplicates" checkbox, since there are still the cross-nodes duplicates even in merge mode? Or should we merge them post-query without asking? If we do so, what to put in Node Direction? "both" ?

Dependencies

n/a

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.

• Is this PR backed with a JIRA ticket? If so, make sure it is written as a title prefix (in general, PRs affecting the NetObserv/Network Observability product should be backed with a JIRA ticket - especially if they bring user facing changes).
• Does this PR require product documentation?
• If so, make sure the JIRA epic is labelled with "documentation" and provides a description relevant for doc writers, such as use cases or scenarios. Any required step to activate or configure the feature should be documented there, such as new CRD knobs.
• Does this PR require a product release notes entry?
• If so, fill in "Release Note Text" in the JIRA.
• Is there anything else the QE team should know before testing? E.g: configuration changes, environment setup, etc.
• If so, make sure it is described in the JIRA ticket.
• QE requirements (check 1 from the list):
• Standard QE validation, with pre-merge tests unless stated otherwise.
• Regression tests only (e.g. refactoring with no user-facing change).
• No QE (e.g. trivial change with high reviewer's confidence, or per agreement with the QE team).

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 openshift-eng/jira-lifecycle-plugin repository.

[openshift-ci-robot]

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

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the bug to target the "4.16.0" version, but no target version was set.

In response to this:

Description

This PR currently just shows the cross-nodes duplicates.

Following the reproduce steps described in NETOBSERV-1551, I'm now seeing both interfaces:

But they are displayed as distinct flows. So it raises the question: should we reintroduce the "Show duplicates" checkbox, since there are still the cross-nodes duplicates even in merge mode? Or should we merge them post-query without asking? If we do so, what to put in Node Direction? "both" ?

Dependencies

n/a

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.

• Is this PR backed with a JIRA ticket? If so, make sure it is written as a title prefix (in general, PRs affecting the NetObserv/Network Observability product should be backed with a JIRA ticket - especially if they bring user facing changes).
• Does this PR require product documentation?
• If so, make sure the JIRA epic is labelled with "documentation" and provides a description relevant for doc writers, such as use cases or scenarios. Any required step to activate or configure the feature should be documented there, such as new CRD knobs.
• Does this PR require a product release notes entry?
• If so, fill in "Release Note Text" in the JIRA.
• Is there anything else the QE team should know before testing? E.g: configuration changes, environment setup, etc.
• If so, make sure it is described in the JIRA ticket.
• QE requirements (check 1 from the list):
• Standard QE validation, with pre-merge tests unless stated otherwise.
• Regression tests only (e.g. refactoring with no user-facing change).
• No QE (e.g. trivial change with high reviewer's confidence, or per agreement with the QE team).

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 openshift-eng/jira-lifecycle-plugin repository.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 57.49%. Comparing base (d930b2b) to head (248d254).

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #489      +/-   ##
==========================================
+ Coverage   57.39%   57.49%   +0.09%     
==========================================
  Files         167      167              
  Lines        8730     8750      +20     
  Branches     1128     1131       +3     
==========================================
+ Hits         5011     5031      +20     
  Misses       3397     3397              
  Partials      322      322              

Flag	Coverage Δ
uitests	58.30% <100.00%> (+0.12%)	⬆️
unittests	55.17% <ø> (ø)

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]

I'm fine merging these flows without asking (or rename show duplicate to merge and enable by default 😸 )

• NodeDirection should be kept as is since we are going to keep a single flow from a single node.
  • Else it would require to keep node / agent info too ?
• Ingress can be the favorite as same as metrics queries

  network-observability-console-plugin/pkg/model/filters/filters.go

  Lines 67 to 91 in dc83aa0

  	func SplitForReportersMerge(q SingleQuery) (SingleQuery, SingleQuery) {
  	// If FlowDirection is enforced, skip merging both reporters
  	for _, m := range q {
  	if m.Key == fields.FlowDirection {
  	return q, nil
  	}
  	}
  	// The rationale here is that most traffic is duplicated from ingress and egress PoV, except cluster-external traffic.
  	// Merging is done by running a first query with FlowDirection=EGRESS and another with FlowDirection=INGRESS AND SrcOwnerName is empty,
  	// which stands for cluster-external.
  	// (Note that we use SrcOwnerName both as an optimization as it's a Loki index,
  	// and as convenience because looking for empty fields won't work if they aren't indexed)
  	q1 := SingleQuery{
  	NewMatch(fields.FlowDirection, `"`+constants.Egress+`"`),
  	}
  	q2 := SingleQuery{
  	NewMatch(fields.FlowDirection, `"`+constants.Ingress+`"`),
  	NewMatch(fields.SrcOwnerName, `""`),
  	}
  	for _, m := range q {
  	q1 = append(q1, m)
  	q2 = append(q2, m)
  	}
  	return q1, q2
  	}

• Interfaces and directions must be merged keeping the order Egress + Ingress in the array

[jotak]

(rebased)

[github-actions]

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

It will expire after two weeks.

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

USER=netobserv VERSION=c9257af make set-plugin-image

[jotak]

Ok I added a commit that merges without asking.
I haven't changed anything about the Node Direction, ie. this is still whoever has more relevant information (DNS, PktDrops ...) that takes precedence, or else it just pick the first found. This part of the algorithm hasn't changed.

It could be misleading to show Node-direction = (ingress or egress) while it refers to interfaces that relate to the other peer; but perhaps I am nitpicking here

[jpinsonneau]

Looks good in terms of code. Thanks !

[github-actions]

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

It will expire after two weeks.

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

USER=netobserv VERSION=b67f85f make set-plugin-image

[memodi]

@jotak I am seeing the interface directions column have n/a set when there are multiple directions, probably it should list multiple directions just the way interfaces are listed?

Env info:

OCP: 4.16.0-0.nightly-2024-03-13-061822
NetObserv operator: v0.0.0-22a1963
Loki: 0-click-loki
eBPF-agent: main
FLP: main
ConsolePlugin: b67f85f

[jotak]

@memodi I confirm the problem but also having the same problem on main branch (although it's harder to see because there isn't as many merged flows, but scrolling a little bit I can find some)
E.g:

Let me see if I can do a quick fix anyway...

[openshift-ci]

New changes are detected. LGTM label has been removed.

[jotak]

@memodi last commit should fix it
cc @jpinsonneau

[github-actions]

New image:
quay.io/netobserv/network-observability-console-plugin:2dffbd1

It will expire after two weeks.

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

USER=netobserv VERSION=2dffbd1 make set-plugin-image

[memodi]

although it's harder to see because there isn't as many merged flows, but scrolling a little bit I can find some

@jotak - I thought the flows with multiple interfaces and directions are the merged ones, no? (most ones that are shown in your screenshot)

[memodi]

I was able to verify we're not populating Directions column correctly:

/label qe-approved

[openshift-ci-robot]

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

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the bug to target the "4.16.0" version, but no target version was set.

In response to this:

Description

This PR currently just shows the cross-nodes duplicates.

Following the reproduce steps described in NETOBSERV-1551, I'm now seeing both interfaces:

But they are displayed as distinct flows. So it raises the question: should we reintroduce the "Show duplicates" checkbox, since there are still the cross-nodes duplicates even in merge mode? Or should we merge them post-query without asking? If we do so, what to put in Node Direction? "both" ?

Dependencies

n/a

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.

• Is this PR backed with a JIRA ticket? If so, make sure it is written as a title prefix (in general, PRs affecting the NetObserv/Network Observability product should be backed with a JIRA ticket - especially if they bring user facing changes).
• Does this PR require product documentation?
• If so, make sure the JIRA epic is labelled with "documentation" and provides a description relevant for doc writers, such as use cases or scenarios. Any required step to activate or configure the feature should be documented there, such as new CRD knobs.
• Does this PR require a product release notes entry?
• If so, fill in "Release Note Text" in the JIRA.
• Is there anything else the QE team should know before testing? E.g: configuration changes, environment setup, etc.
• If so, make sure it is described in the JIRA ticket.
• QE requirements (check 1 from the list):
• Standard QE validation, with pre-merge tests unless stated otherwise.
• Regression tests only (e.g. refactoring with no user-facing change).
• No QE (e.g. trivial change with high reviewer's confidence, or per agreement with the QE team).

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 openshift-eng/jira-lifecycle-plugin repository.

[jotak]

although it's harder to see because there isn't as many merged flows, but scrolling a little bit I can find some

@jotak - I thought the flows with multiple interfaces and directions are the merged ones, no? (most ones that are shown in your screenshot)

True but there's 2 levels of merge: first in the agent (this was done by the previous "deduper-merge" PR already merged) but it cannot merge everything, so a second merge is done here with this PR.

[jotak]

/approve

[openshift-ci]

[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