NETOBSERV-2118 add missing sampling and exclude interfaces options#209
NETOBSERV-2118 add missing sampling and exclude interfaces options#209openshift-merge-bot[bot] merged 5 commits intonetobserv:mainfrom
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #209 +/- ##
=======================================
Coverage 22.60% 22.60%
=======================================
Files 14 14
Lines 1451 1451
=======================================
Hits 328 328
Misses 1099 1099
Partials 24 24
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
scripts/help.sh
Outdated
| echo " --enable_udn_mapping: enable User Defined Network mapping (default: false)" | ||
| echo " --enable_udn_mapping: enable User Defined Network mapping (default: false)" | ||
| echo " --get-subnets: get subnets information (default: false)" | ||
| echo " --sampling: rate at which packets should be sampled (default: 1)" |
There was a problem hiding this comment.
not sure how much text we can write here without being too verbose, but when referring to sampling I like to be more accurate about what it really does, because it's not obvious for newcomers. E.g: "a value of 50 means one packet every 50 is sampled".
Also: it would be better to talk about sampling ratio rather than rate (but this remark applies to everywhere, not just here/CLI) - see here for the difference. A sampling rate would generally mean how many packets per second are sampled, whereas a sampling ratio is what we want here - the proportion of packets that we keep.
I'll open PRs in other parts to do this change.
There was a problem hiding this comment.
That description should remain short but we can include this option in an example and put the full sentence to explain it 👌
In that case we should write the ratio divisor as the dividend is forced to 1 in our usage ?
There was a problem hiding this comment.
There was a problem hiding this comment.
That description should remain short but we can include this option in an example and put the full sentence to explain it 👌
In that case we should write the
ratio divisoras thedividendis forced to 1 in our usage ?
Technically true but that makes the sentence quite more complicated! I think we can make it easy to understand without being too pedantic (that's why a short example helps IMO). Another way to put it would be: "value that defines the ratio of packets being sampled".
|
/ok-to-test @jpinsonneau tests are failing here, could you PTAL? I am assuming this is ready for testing. |
|
New image: It will expire after two weeks. To use this build, update your commands using: USER=netobserv VERSION=910ce9b make commands |
@memodi I had some issues during rebase |
|
/ok-to-test |
|
New image: It will expire after two weeks. To use this build, update your commands using: USER=netobserv VERSION=b1ce1f3 make commands |
memodi
left a comment
memodi
left a comment
There was a problem hiding this comment.
@jpinsonneau about exclude_interfaces, when I have I exclude certain interface, I still see flows being collected for that interface:
for e.g.: here, I am excluding ens5 interface and there are still flows from that interface.
|
/lgtm |
|
/needs-changes |
|
/label needs-changes |
|
@memodi: The label(s) DetailsIn response to this:
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-sigs/prow repository. |
memodi
added
the
needs-changes
I doubt that's coming from this PR but I will retry as soon as quay is up 😸 |
|
@memodi I'm not reproducing the behavior you describe locally using Could you please ensure the eBPF agents have the exclude interfaces env variable set properly ? Else maybe there is something particular with ens5 interface or the cluster itself |
|
New changes are detected. LGTM label has been removed. |
the configuration happens fine in ebpf-agent, I checked that too, but I was expecting the flows with those interfaces not to appear for any matching Src/Dest. I am not sure if this is broken in ebpf-agent, don't think we have a regression test that's covering it. |
|
/retest |
|
New image: It will expire after two weeks. To use this build, update your commands using: USER=netobserv VERSION=056fe4f make commands |
|
@memodi & @jotak looking at the eBPF logs, I confirm that setting the exclude interface params on However, the collected flows still contains the related interface 🤔 The only exclude interface that truely works is the default one On parallel, I tried back the Results in both This issue happens when the agent is privileged. On CLI, it's always true to keep things simple. |
|
interesting, thanks for opening a bug |
|
/label qe-approved @jpinsonneau thanks for investigating this, to have this feature useful I wonder if it makes sense to introduce auto-detect of privileged setting based on the feature (I know we discussed this for Operator), probably it would be good to introduce in CLI so that |
I can check that in parallel indeed but the best would be to fix the eBPF issue |
Agreed. Filed a story https://issues.redhat.com/browse/NETOBSERV-2262 |
|
[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 DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Rebased without changes |
|
New image: It will expire after two weeks. To use this build, update your commands using: USER=netobserv VERSION=21cae8a make commands |
4 participants
Description
Add sampling and exclude_interfaces options
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.