NETOBSERV-1927 fix topology issues

[jpinsonneau]

Description

Followup on #634 to fix topology issues

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).

[github-actions]

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

It will expire after two weeks.

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

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

[jotak]

why do we need to avoid setting owner if it's the same as leaf Type? Curious because it used to work like this even before the scopes refactor PR

[jotak]

(I'm just worrying about potential side effects even if I don't see any right now...)

[jpinsonneau]

There was tons of hardcoded logic for each case (when owner is set / when resource is set and so on), especially for grouping and displaying topology nodes.
Some of this logic is kept but while debugging, I saw that Pods and Services without owners where duplicating their name / type into the owner field.

That's kind of a nonsence and will avoid mistakes if you check the owner field first.

[jpinsonneau]

The same logic was already hardcoded in the display btw:

network-observability-console-plugin/web/src/components/netflow-topology/element-fields.tsx

Line 35 in 72915fe

if (data.peer.owner && data.peer.owner.type !== data.peer.resource?.type) {

[jotak]

/lgtm

[memodi]

@jpinsonneau those 2 tests still fail when I use plugin image with tag: a01a382

  1) (OCP-53591 Network_Observability) Netflow Topology groups features
       (OCP-53591, memodi, Network_Observability) should verify group Nodes+Owners:

      AssertionError: Timed out retrying after 30000ms: Not enough elements found. Found '11', expected '20'.
      + expected - actual

      -11
      +20

      at Context.eval (webpack://cypress-automation/./tests/netobserv/topology_groups.cy.ts:124:42)

  2) (OCP-53591 Network_Observability) Netflow Topology groups features
       (OCP-53591, memodi, Network_Observability) should verify group NS+Owners:

      AssertionError: Timed out retrying after 30000ms: Not enough elements found. Found '9', expected '18'.
      + expected - actual

      -9
      +18

[jpinsonneau]

@jpinsonneau those 2 tests still fail when I use plugin image with tag: a01a382

  1) (OCP-53591 Network_Observability) Netflow Topology groups features
       (OCP-53591, memodi, Network_Observability) should verify group Nodes+Owners:

      AssertionError: Timed out retrying after 30000ms: Not enough elements found. Found '11', expected '20'.
      + expected - actual

      -11
      +20

      at Context.eval (webpack://cypress-automation/./tests/netobserv/topology_groups.cy.ts:124:42)

  2) (OCP-53591 Network_Observability) Netflow Topology groups features
       (OCP-53591, memodi, Network_Observability) should verify group NS+Owners:

      AssertionError: Timed out retrying after 30000ms: Not enough elements found. Found '9', expected '18'.
      + expected - actual

      -9
      +18

@memodi Is there any easy way to get the difference here ? maybe screenshots ?

[memodi]

@memodi Is there any easy way to get the difference here ? maybe screenshots ?

I have the screenshots but not sure how much of those would be helpful, also sharing the links of API data for these dates:

Nodes+Owners:

https://github.com/openshift/openshift-tests-private/blob/master/frontend/fixtures/netobserv/flow_metrics_ghostsOwners.json

NS+Owners:

https://github.com/openshift/openshift-tests-private/blob/master/frontend/fixtures/netobserv/flow_metrics_gNSOwners.json

[jpinsonneau]

Thanks @memodi !

For me the change in the count of groups is valid and is because I removed wrongly set Owners.

For example in a NS+Owners grouping, kubernetes service was showing default as namespace, kubernetes as owner and itself:

Now it displays only default as namespace and itself as there is no owner:

This happens also with pods without owners such as loki pod.

[memodi]

For me the change in the count of groups is valid and is because I removed wrongly set Owners.

@jpinsonneau - SGTM! I'll update these 2 tests. Thanks!

[memodi]

/label qe-approved

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

Approval requirements bypassed by manually added approval.

This pull-request has been approved by:

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

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

[jpinsonneau]

/retest

[codecov]

Codecov Report

Attention: Patch coverage is 0% with 2 lines in your changes missing coverage. Please review.

Project coverage is 56.54%. Comparing base (765ca7c) to head (79b4a01).
Report is 6 commits behind head on main.

Files with missing lines	Patch %	Lines
web/src/model/topology.ts	0.00%	2 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #657   +/-   ##
=======================================
  Coverage   56.54%   56.54%           
=======================================
  Files         196      196           
  Lines       10064    10064           
  Branches     1195     1195           
=======================================
  Hits         5691     5691           
  Misses       4008     4008           
  Partials      365      365           

Flag	Coverage Δ
uitests	59.26% <0.00%> (ø)
unittests	50.58% <ø> (ø)

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

Files with missing lines	Coverage Δ
web/src/utils/metrics.ts	85.86% <ø> (ø)
web/src/model/topology.ts	26.60% <0.00%> (ø)