Skip to content

NETOBSERV-2021: Improve UX#146

Merged
msherif1234 merged 21 commits intonetobserv:mainfrom
jpinsonneau:2021
Jan 30, 2025
Merged

NETOBSERV-2021: Improve UX#146
msherif1234 merged 21 commits intonetobserv:mainfrom
jpinsonneau:2021

Conversation

@jpinsonneau
Copy link
Member

@jpinsonneau jpinsonneau commented Jan 8, 2025

Description

  • if one or more filters option is enabled to automatically enable filtering and disable when we have no filtering options and probably in this case we can remove --enable-filter option
  • if filtering require feature specific we can enable the feature and when the filter is removed disable it
  • support multi filter rules
  • add examples
  • List options in alphabetical order
  • List option common separately to both flows and packets to avoid listing them separately for log-level, max-time etc.
  • Use true as default values when option is set, i.e. only check for value == "false" to avoid setting value== true 
  • Have enable_all option to enable all features. 
  • Address OSDOCS-10730 feedback
  • Force having at least one eBPF filter when running packet capture
  • Add missing UDN and pkt xlat features
  • Add Network Name columns

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
Copy link

openshift-ci bot commented Jan 8, 2025

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

function setLastFlowFilter() {
"$YQ_BIN" e --inplace " .spec.template.spec.containers[0].env[] |= select(.name == \"FLOW_FILTER_RULES\").value |=(fromjson | .[-1].$1 = $2 | tostring)" "$3"
}

Copy link
Contributor

Choose a reason for hiding this comment

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

do u have sample output with mutli rule, I have standalone script fo ut this logic maybe we bring this in and intg with ut ci as logic start to become complex ?

Copy link
Member Author

@jpinsonneau jpinsonneau Jan 10, 2025

Choose a reason for hiding this comment

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

Sure. I will add examples and tests !

Copy link
Member Author

Choose a reason for hiding this comment

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

I have added the following scenarios for now:

basic examples:
  netobserv flows --drops         # Capture dropped flows on all nodes
  netobserv packets --port=8080   # Capture packets on port 8080
  netobserv metrics --enable_all  # Capture all cluster metrics with pktDrop, dns, rtt and network events features

advanced examples:
  Capture drops in background and copy output locally
    netobserv flows --background \                            # Capture flows using background mode
    --max-time=15m \                                          # for a maximum of 15 minutes
    --protocol=TCP --port=8080 \                              # either on TCP 8080
    or --protocol=UDP                                         # or UDP
    netobserv follow                                          # Display the progression of the background capture
    netobserv stop                                            # Stop the background capture by deleting eBPF agents
    netobserv copy                                            # Copy the background capture output data
    netobserv cleanup                                         # Cleanup netobserv CLI by removing the remaining collector pod

  Capture packets on specific nodes and port
    netobserv packets                                         # Capture packets
    --node-selector=netobserv:true \                          # on nodes labelled with netobserv=true
    --port=80 \                                               # on port 80 only
    --max-bytes=100000000                                     # for a maximum of 100MB

Please let me know if you feel more would be useful

Copy link
Contributor

Choose a reason for hiding this comment

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

can u pls include example with mutli rules ?

Copy link
Member Author

Choose a reason for hiding this comment

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

You have one above in the advanced examples 😸

@jpinsonneau jpinsonneau changed the title NETOBSERV-2021 Improve filtering UX NETOBSERV-2021 Improve UX Jan 10, 2025
@codecov
Copy link

codecov bot commented Jan 10, 2025

Codecov Report

Attention: Patch coverage is 73.52941% with 9 lines in your changes missing coverage. Please review.

Project coverage is 23.53%. Comparing base (547a146) to head (50187d3).
Report is 28 commits behind head on main.

Files with missing lines Patch % Lines
cmd/map_format.go 66.66% 8 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #146      +/-   ##
==========================================
+ Coverage   22.51%   23.53%   +1.01%     
==========================================
  Files          11       11              
  Lines        1310     1330      +20     
==========================================
+ Hits          295      313      +18     
- Misses        999     1000       +1     
- Partials       16       17       +1     
Flag Coverage Δ
unittests 23.53% <73.52%> (+1.01%) ⬆️

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

Files with missing lines Coverage Δ
cmd/flow_capture.go 31.78% <100.00%> (+1.38%) ⬆️
cmd/options.go 25.00% <ø> (ø)
cmd/map_format.go 25.27% <66.66%> (+1.95%) ⬆️

@msherif1234
Copy link
Contributor

BTW if u added the rules in reverse order this will break filtering , rules has to place in the json array in the same order they are configured.

@jpinsonneau
Copy link
Member Author

BTW if u added the rules in reverse order this will break filtering , rules has to place in the json array in the same order they are configured.

The order is kept as I append the whole json from this file: https://github.com/netobserv/network-observability-cli/blob/994f4b4888d67fc72f1a90cd72d4295a50614507/res/flow-filter.json

However we should document this behavior as it's can break easily !

@jpinsonneau
Copy link
Member Author

/retest

1 similar comment
@jpinsonneau
Copy link
Member Author

/retest

Copy link
Contributor

@skrthomas skrthomas left a comment

Choose a reason for hiding this comment

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

Just a few nitties 👼 , and otherwise LGTM! Thanks Julien :)

@memodi
Copy link
Member

memodi commented Jan 21, 2025

/ok-to-test

@github-actions
Copy link

New image:
quay.io/netobserv/network-observability-cli:ac57fb3

It will expire after two weeks.

To use this build, update your commands using:

USER=netobserv VERSION=ac57fb3 make commands

or download the updated commands.

@jpinsonneau
Copy link
Member Author

Rebased without changes

@memodi
Copy link
Member

memodi commented Jan 30, 2025

@Mehul as discussed here are the changes requested: 02698b8 5b706a0

thanks @jpinsonneau , I'll give it a go, hopefully one last one 🤞

@memodi
Copy link
Member

memodi commented Jan 30, 2025

/ok-to-test

@github-actions
Copy link

New image:
quay.io/netobserv/network-observability-cli:1677996

It will expire after two weeks.

To use this build, update your commands using:

USER=netobserv VERSION=1677996 make commands

or download the updated commands.

@memodi
Copy link
Member

memodi commented Jan 30, 2025

@jpinsonneau using the image from last commit I still see toString error:

$ ~/Downloads/kubectl-netobserv flows --icmp_type="0"
Checking dependencies...
'yq' is up to date (version v4.30.7).
'bash' is up to date (version v5.2.37).
Setting up...
Warning: apps.openshift.io/v1 DeploymentConfig is deprecated in v4.14+, unavailable in v4.10000+
NAME       READY   STATUS    RESTARTS   AGE
pod/loki   1/1     Running   0          21m

NAME           TYPE        CLUSTER-IP       EXTERNAL-IP   PORT(S)    AGE
service/loki   ClusterIP   172.30.217.181   <none>        3100/TCP   21m
creating netobserv-cli namespace
namespace/netobserv-cli created
creating service account
serviceaccount/netobserv-cli created
clusterrole.rbac.authorization.k8s.io/netobserv-cli configured
clusterrolebinding.rbac.authorization.k8s.io/netobserv-cli unchanged
creating collector service
service/collector created
creating flow-capture agents:
opt: filter_icmp_type, value: 0
Error: 1:124: invalid input text "tostring)" <----------- here
daemonset.apps/netobserv-cli created

@jpinsonneau
Copy link
Member Author

@Mehul as discussed here are the changes requested:
02698b8
5b706a0

@jpinsonneau using the image from last commit I still see toString error:

$ ~/Downloads/kubectl-netobserv flows --icmp_type="0"
Checking dependencies...
'yq' is up to date (version v4.30.7).
'bash' is up to date (version v5.2.37).
Setting up...
Warning: apps.openshift.io/v1 DeploymentConfig is deprecated in v4.14+, unavailable in v4.10000+
NAME       READY   STATUS    RESTARTS   AGE
pod/loki   1/1     Running   0          21m

NAME           TYPE        CLUSTER-IP       EXTERNAL-IP   PORT(S)    AGE
service/loki   ClusterIP   172.30.217.181   <none>        3100/TCP   21m
creating netobserv-cli namespace
namespace/netobserv-cli created
creating service account
serviceaccount/netobserv-cli created
clusterrole.rbac.authorization.k8s.io/netobserv-cli configured
clusterrolebinding.rbac.authorization.k8s.io/netobserv-cli unchanged
creating collector service
service/collector created
creating flow-capture agents:
opt: filter_icmp_type, value: 0
Error: 1:124: invalid input text "tostring)" <----------- here
daemonset.apps/netobserv-cli created

oh you meant on the script side !

I don't reproduce that behavior 🤔

$ oc netobserv flows --icmp_type=0
Checking dependencies... 
'yq' is up to date (version v4.43.1).
'bash' is up to date (version v5.2.26).
Setting up... 
kube:admin
creating netobserv-cli namespace
namespace/netobserv-cli created
creating service account
serviceaccount/netobserv-cli created
clusterrole.rbac.authorization.k8s.io/netobserv-cli unchanged
clusterrolebinding.rbac.authorization.k8s.io/netobserv-cli unchanged
creating collector service
service/collector created
creating flow-capture agents:
opt: filter_icmp_type, value: 0
daemonset.apps/netobserv-cli created

@jpinsonneau
Copy link
Member Author

@memodi 2e86da4 should ensure the toString is correctly applied

@memodi
Copy link
Member

memodi commented Jan 30, 2025

/ok-to-test

@github-actions
Copy link

New image:
quay.io/netobserv/network-observability-cli:b382261

It will expire after two weeks.

To use this build, update your commands using:

USER=netobserv VERSION=b382261 make commands

or download the updated commands.

@github-actions
Copy link

New image:
quay.io/netobserv/network-observability-cli:103f79d

It will expire after two weeks.

To use this build, update your commands using:

USER=netobserv VERSION=103f79d make commands

or download the updated commands.

@memodi
Copy link
Member

memodi commented Jan 30, 2025

thanks @jpinsonneau , last commit fixes all the aforementioned issues.

@memodi
Copy link
Member

memodi commented Jan 30, 2025

/label qe-approved

@jpinsonneau jpinsonneau changed the title NETOBSERV-2021 Improve UX NETOBSERV-2021: Improve UX Jan 30, 2025
@openshift-ci-robot
Copy link
Collaborator

openshift-ci-robot commented Jan 30, 2025

@jpinsonneau: This pull request references NETOBSERV-2021 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 story to target the "4.19.0" version, but no target version was set.

Details

In response to this:

Description

  • if one or more filters option is enabled to automatically enable filtering and disable when we have no filtering options and probably in this case we can remove --enable-filter option
  • if filtering require feature specific we can enable the feature and when the filter is removed disable it
  • support multi filter rules
  • add examples
  • List options in alphabetical order
  • List option common separately to both flows and packets to avoid listing them separately for log-level, max-time etc.
  • Use true as default values when option is set, i.e. only check for value == "false" to avoid setting value== true 
  • Have enable_all option to enable all features. 
  • Address OSDOCS-10730 feedback
  • Force having at least one eBPF filter when running packet capture
  • Add missing UDN and pkt xlat features
  • Add Network Name columns

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
Copy link

openshift-ci bot commented Jan 30, 2025

[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

Details 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

@msherif1234 msherif1234 merged commit 09e656b into netobserv:main Jan 30, 2025
9 of 10 checks passed
@jotak
Copy link
Member

jotak commented Feb 4, 2025

@msherif1234 please again, remember to use "Squash and merge" when not using the openshift bot for merging : there's a lot of unnecessary verbosity in the git history now

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.

7 participants