NETOBSERV-1720: fix issues with topology scopes #550

jotak · 2024-06-25T12:14:50Z

Fixes also NETOBSERV-1721 and NETOBSERV-1722

new func getStepIntoNext that provides the next scope for 'stepInto' and cares about the allowed scopes
fix call to toggleDirElementFilter on stepInto (it was removing the filter instead of adding it), and prevent adding duplicate filters
Do not link groups invalidation to scope via useEffect, as it results in doing 2 queries (first with the invalid group, then without). Make them more strictly tied instead.

Also, this PR changes the stepInto workflow when coming from nodes, it jumps directly to pods. Two reasons for that:

this seems more logical to see pods rather than namespaces when "stepping into a node". Namespaces and Nodes are orthogonal, whereas Pods are contained in Nodes.
more pragmatically, this also is a better workflow in lokiless, since having scope=namespace + filtering on a node generates an error.

The new stepInto workflow is:

                         
 CLUSTER                 
    │                    
    ▼                    
  ZONE                   
    │                    
    ▼                    
  NODE         NAMESPACE 
    │              │     
    │              ▼     
    │            OWNER   
    │              │     
    └──────┬───────┘     
           │             
           ▼             
        RESOURCE

Description

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.

openshift-ci-robot · 2024-06-25T12:14:55Z

codecov · 2024-06-25T12:35:43Z

Codecov Report

Attention: Patch coverage is 8.82353% with 31 lines in your changes missing coverage. Please review.

Project coverage is 56.32%. Comparing base (56ada60) to head (81ae850).

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #550      +/-   ##
==========================================
- Coverage   56.43%   56.32%   -0.12%     
==========================================
  Files         183      183              
  Lines        9079     9095      +16     
  Branches     1182     1185       +3     
==========================================
- Hits         5124     5123       -1     
- Misses       3589     3606      +17     
  Partials      366      366

Flag	Coverage Δ
uitests	`57.85% <8.82%> (-0.16%)`	⬇️
unittests	`52.03% <ø> (ø)`

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

Files	Coverage Δ
...c/components/netflow-topology/netflow-topology.tsx	`65.71% <ø> (ø)`
web/src/components/netflow-traffic.tsx	`57.02% <28.57%> (-0.29%)`	⬇️
...omponents/netflow-topology/2d/topology-content.tsx	`27.63% <0.00%> (+0.41%)`	⬆️
web/src/model/topology.ts	`30.43% <5.00%> (-2.20%)`	⬇️

OlivierCazade

LGTM

openshift-ci-robot · 2024-06-25T13:49:58Z

@jotak: This pull request references NETOBSERV-1720 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.17.0" version, but no target version was set.

In response to this:

Fixes also NETOBSERV-1721 and NETOBSERV-1722

new func getStepIntoNext that provides the next scope for 'stepInto' and cares about the allowed scopes

fix call to toggleDirElementFilter on stepInto (it was removing the filter instead of adding it), and prevent adding duplicate filters

Do not link groups invalidation to scope via useEffect, as it results in doing 2 queries (first with the invalid group, then without). Make them more strictly tied instead.

Also, this PR changes the stepInto workflow when coming from nodes, it jumps directly to pods. Two reasons for that:

this seems more logical to see pods rather than namespaces when "stepping into a node". Namespaces and Nodes are orthogonal, whereas Pods are contained in Nodes.

more pragmatically, this also is a better workflow in lokiless, since having scope=namespace + filtering on a node generates an error.

The new stepInto workflow is:
                        
CLUSTER                 
   │                    
   ▼                    
 ZONE                   
   │                    
   ▼                    
 NODE         NAMESPACE 
   │              │     
   │              ▼     
   │            OWNER   
   │              │     
   └──────┬───────┘     
          │             
          ▼             
       RESOURCE         
                        
Description

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.

memodi · 2024-06-25T17:05:15Z

/ok-to-test

github-actions · 2024-06-25T17:08:31Z

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

It will expire after two weeks.

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

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

jpinsonneau · 2024-06-26T09:20:46Z

web/src/components/netflow-traffic.tsx

@@ -245,7 +245,7 @@ export const NetflowTraffic: React.FC<NetflowTrafficProps> = ({ forcedFilters, i
  const [lastTop, setLastTop] = useLocalStorage<number>(LOCAL_STORAGE_LAST_TOP_KEY, TOP_VALUES[0]);
  const [range, setRange] = React.useState<number | TimeRange>(getRangeFromURL());
  const [histogramRange, setHistogramRange] = React.useState<TimeRange>();
-  const [metricScope, setMetricScope] = useLocalStorage<FlowScope>(LOCAL_STORAGE_METRIC_SCOPE_KEY, 'namespace');
+  const [metricScope, _setMetricScope] = useLocalStorage<FlowScope>(LOCAL_STORAGE_METRIC_SCOPE_KEY, 'namespace');


We have existing cases like this:

updateTopologyMetricType

updateTableFilters

I think we should decide which approach to use and I can adopt it in NETOBSERV-268

Do you mean only for the naming, or something else? I can rename setMetricScope / updateMetricScope

openshift-ci-robot · 2024-06-26T10:06:01Z

@jotak: This pull request references NETOBSERV-1720 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.17.0" version, but no target version was set.

In response to this:

Fixes also NETOBSERV-1721 and NETOBSERV-1722

new func getStepIntoNext that provides the next scope for 'stepInto' and cares about the allowed scopes

fix call to toggleDirElementFilter on stepInto (it was removing the filter instead of adding it), and prevent adding duplicate filters

Do not link groups invalidation to scope via useEffect, as it results in doing 2 queries (first with the invalid group, then without). Make them more strictly tied instead.

Also, this PR changes the stepInto workflow when coming from nodes, it jumps directly to pods. Two reasons for that:

this seems more logical to see pods rather than namespaces when "stepping into a node". Namespaces and Nodes are orthogonal, whereas Pods are contained in Nodes.

more pragmatically, this also is a better workflow in lokiless, since having scope=namespace + filtering on a node generates an error.

The new stepInto workflow is:
                        
CLUSTER                 
   │                    
   ▼                    
 ZONE                   
   │                    
   ▼                    
 NODE         NAMESPACE 
   │              │     
   │              ▼     
   │            OWNER   
   │              │     
   └──────┬───────┘     
          │             
          ▼             
       RESOURCE         
                        
Description

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.

memodi · 2024-06-26T18:46:42Z

@jotak - couple of findings:
For NETOBSERV-1720, I don't see "step into" button on graph node when Scope=Node:

And I still see bug NETOBSERV-1722

jotak · 2024-06-27T07:38:24Z

@memodi these two are expected:

It's what I mentioned in the PR description, now "stepping into" a Node goes to Pods, which isn't available without Loki, that's why you don't see the icon, it's on purpose. Previously, when stepping into a Node, it would bring you to namespaces, and you would see the error that you report on 2
There's no fix for that, it's what the error says: you need to use Loki to get that query, because you can't mix node and namespace labels with prom metrics. The fix for NETOBSERV-1722 is different: it was sometimes showing this error even though the query should work with prom metrics. cf the reproducing steps in the ticket, they are different.

I also created this enhancement to improve UX on unsupported queries: https://issues.redhat.com//browse/NETOBSERV-1719

Fixes also NETOBSERV-1721 and NETOBSERV-1722 - new func getStepIntoNext that provides the next scope for 'stepInto' and cares about the allowed scopes - fix call to toggleDirElementFilter on stepInto (it was *removing* the filter instead of adding it), and prevent adding duplicate filters - Do not link groups invalidation to scope via useEffect, as it results in doing 2 queries (first with the invalid group, then without). Make them more strictly tied instead.

openshift-ci · 2024-06-27T08:08:27Z

New changes are detected. LGTM label has been removed.

jotak · 2024-06-27T08:08:48Z

(rebased)

github-actions · 2024-06-27T08:11:56Z

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

It will expire after two weeks.

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

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

memodi · 2024-06-27T16:14:00Z

got it, thanks, @jotak

/label qe-approved

openshift-ci-robot · 2024-06-27T16:14:05Z

@jotak: This pull request references NETOBSERV-1720 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.17.0" version, but no target version was set.

In response to this:

Fixes also NETOBSERV-1721 and NETOBSERV-1722

new func getStepIntoNext that provides the next scope for 'stepInto' and cares about the allowed scopes

fix call to toggleDirElementFilter on stepInto (it was removing the filter instead of adding it), and prevent adding duplicate filters

Do not link groups invalidation to scope via useEffect, as it results in doing 2 queries (first with the invalid group, then without). Make them more strictly tied instead.

Also, this PR changes the stepInto workflow when coming from nodes, it jumps directly to pods. Two reasons for that:

this seems more logical to see pods rather than namespaces when "stepping into a node". Namespaces and Nodes are orthogonal, whereas Pods are contained in Nodes.

more pragmatically, this also is a better workflow in lokiless, since having scope=namespace + filtering on a node generates an error.

The new stepInto workflow is:
                        
CLUSTER                 
   │                    
   ▼                    
 ZONE                   
   │                    
   ▼                    
 NODE         NAMESPACE 
   │              │     
   │              ▼     
   │            OWNER   
   │              │     
   └──────┬───────┘     
          │             
          ▼             
       RESOURCE         
                        
Description

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 · 2024-06-27T16:22:19Z

/approve

openshift-ci · 2024-06-27T16:22:24Z

[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

Fixes also NETOBSERV-1721 and NETOBSERV-1722 - new func getStepIntoNext that provides the next scope for 'stepInto' and cares about the allowed scopes - fix call to toggleDirElementFilter on stepInto (it was *removing* the filter instead of adding it), and prevent adding duplicate filters - Do not link groups invalidation to scope via useEffect, as it results in doing 2 queries (first with the invalid group, then without). Make them more strictly tied instead.

jotak requested a review from OlivierCazade June 25, 2024 12:14

openshift-ci-robot added the jira/valid-reference label Jun 25, 2024

OlivierCazade approved these changes Jun 25, 2024

View reviewed changes

openshift-ci bot assigned OlivierCazade Jun 25, 2024

openshift-ci bot added the lgtm label Jun 25, 2024

openshift-ci bot added the ok-to-test To set manually when a PR is safe to test. Triggers image build on PR. label Jun 25, 2024

jpinsonneau reviewed Jun 26, 2024

View reviewed changes

jpinsonneau approved these changes Jun 26, 2024

View reviewed changes

openshift-ci bot assigned jpinsonneau Jun 26, 2024

openshift-merge-robot added the needs-rebase label Jun 27, 2024

jotak force-pushed the scope-issues branch from a9ba656 to 81ae850 Compare June 27, 2024 08:08

openshift-ci bot removed the lgtm label Jun 27, 2024

openshift-merge-robot removed the needs-rebase label Jun 27, 2024

github-actions bot removed the ok-to-test To set manually when a PR is safe to test. Triggers image build on PR. label Jun 27, 2024

jotak added the ok-to-test To set manually when a PR is safe to test. Triggers image build on PR. label Jun 27, 2024

openshift-ci bot added the qe-approved QE has approved this pull request label Jun 27, 2024

openshift-ci bot added the approved label Jun 27, 2024

jotak merged commit 520f3ff into netobserv:main Jun 27, 2024
11 of 12 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

NETOBSERV-1720: fix issues with topology scopes #550

NETOBSERV-1720: fix issues with topology scopes #550

jotak commented Jun 25, 2024 •

edited

Loading

openshift-ci-robot commented Jun 25, 2024 •

edited by openshift-ci bot

Loading

Description

Dependencies

Checklist

codecov bot commented Jun 25, 2024 •

edited

Loading

OlivierCazade left a comment

openshift-ci-robot commented Jun 25, 2024 •

edited by openshift-ci bot

Loading

Description

Dependencies

Checklist

memodi commented Jun 25, 2024

github-actions bot commented Jun 25, 2024

jpinsonneau Jun 26, 2024

jotak Jun 26, 2024

openshift-ci-robot commented Jun 26, 2024 •

edited by openshift-ci bot

Loading

Description

Dependencies

Checklist

memodi commented Jun 26, 2024 •

edited by openshift-ci bot

Loading

jotak commented Jun 27, 2024 •

edited

Loading

openshift-ci bot commented Jun 27, 2024

jotak commented Jun 27, 2024

github-actions bot commented Jun 27, 2024

memodi commented Jun 27, 2024

openshift-ci-robot commented Jun 27, 2024 •

edited by openshift-ci bot

Loading

Description

Dependencies

Checklist

jotak commented Jun 27, 2024

openshift-ci bot commented Jun 27, 2024

NETOBSERV-1720: fix issues with topology scopes #550

NETOBSERV-1720: fix issues with topology scopes #550

Conversation

jotak commented Jun 25, 2024 • edited Loading

Description

Dependencies

Checklist

openshift-ci-robot commented Jun 25, 2024 • edited by openshift-ci bot Loading

Description

Dependencies

Checklist

codecov bot commented Jun 25, 2024 • edited Loading

Codecov Report

OlivierCazade left a comment

Choose a reason for hiding this comment

openshift-ci-robot commented Jun 25, 2024 • edited by openshift-ci bot Loading

Description

Dependencies

Checklist

memodi commented Jun 25, 2024

github-actions bot commented Jun 25, 2024

jpinsonneau Jun 26, 2024

Choose a reason for hiding this comment

jotak Jun 26, 2024

Choose a reason for hiding this comment

openshift-ci-robot commented Jun 26, 2024 • edited by openshift-ci bot Loading

Description

Dependencies

Checklist

memodi commented Jun 26, 2024 • edited by openshift-ci bot Loading

jotak commented Jun 27, 2024 • edited Loading

openshift-ci bot commented Jun 27, 2024

jotak commented Jun 27, 2024

github-actions bot commented Jun 27, 2024

memodi commented Jun 27, 2024

openshift-ci-robot commented Jun 27, 2024 • edited by openshift-ci bot Loading

Description

Dependencies

Checklist

jotak commented Jun 27, 2024

openshift-ci bot commented Jun 27, 2024

jotak commented Jun 25, 2024 •

edited

Loading

openshift-ci-robot commented Jun 25, 2024 •

edited by openshift-ci bot

Loading

codecov bot commented Jun 25, 2024 •

edited

Loading

openshift-ci-robot commented Jun 25, 2024 •

edited by openshift-ci bot

Loading

openshift-ci-robot commented Jun 26, 2024 •

edited by openshift-ci bot

Loading

memodi commented Jun 26, 2024 •

edited by openshift-ci bot

Loading

jotak commented Jun 27, 2024 •

edited

Loading

openshift-ci-robot commented Jun 27, 2024 •

edited by openshift-ci bot

Loading