-
Notifications
You must be signed in to change notification settings - Fork 14
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
NETOBSERV-1216 PacketDrop enhancements #362
Conversation
Codecov Report
@@ Coverage Diff @@
## main #362 +/- ##
==========================================
+ Coverage 57.15% 57.18% +0.03%
==========================================
Files 163 167 +4
Lines 7543 7738 +195
Branches 921 913 -8
==========================================
+ Hits 4311 4425 +114
- Misses 2967 3046 +79
- Partials 265 267 +2
Flags with carried forward coverage won't be shown. Click here to find out more.
|
"A _LINUX_TCP_STATES_H TCP name like ESTABLISHED, SYN_SENT, SYN_RECV": "A _LINUX_TCP_STATES_H TCP name like ESTABLISHED, SYN_SENT, SYN_RECV", | ||
"Packet drop latest cause": "Packet drop latest cause", | ||
"Specify a single TCP drop cause.": "Specify a single TCP drop cause.", | ||
"Specify a single TCP drop cause like:": "Specify a single TCP drop cause like:", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this isn't TCP
specific here and below ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry my bad
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -448,6 +449,34 @@ export const getFilterDefinitions = ( | |||
encoder: simpleFiltersEncoder('_HashId'), | |||
overlap: false | |||
}, | |||
{ | |||
id: 'pkt_drop_state', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
well this filter be off when the dropped packet is for UDP as an example ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No specific mechanism is done here. The user can set 2 filters as PktDropLatestState=TCP_XXX
, Proto=UDP
with match any
query option to get both dropped TCP & UDP flows
web/src/utils/filter-options.ts
Outdated
@@ -6,6 +6,7 @@ import { splitResource, SplitStage } from '../model/resource'; | |||
import { autoCompleteCache } from './autocomplete-cache'; | |||
import { DNS_RCODES } from './dns'; | |||
import { getPort, getService } from './port'; | |||
import { DROP_CAUSES, DROP_STATES } from './tcp-drop'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pls rename the pkg to pkt-drop instead ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
web/src/utils/icmp.ts
Outdated
case 5: // ICMP_REDIRECT: | ||
return ICMP_REDIRECT_CODES.find(v => v.value === c); | ||
case 11: // ICMP_TIME_EXCEEDED: | ||
return ICMP_TIME_EXCEEDED_CODES.find(v => v.value === c); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what about icmp6 codes and types they aren't the same as in icmp4 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct:
https://www.iana.org/assignments/icmp-parameters/icmp-parameters.xhtml#icmp-parameters-types
https://www.iana.org/assignments/icmpv6-parameters/icmpv6-parameters.xhtml#icmpv6-parameters-2
I should implement both and switch according to for protocol field
1 | ICMP | Internet Control Message
vs
58 | IPv6-ICMP | ICMP for IPv6
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
web/src/utils/tcp-drop.ts
Outdated
{ value: 72, name: 'SKB_DROP_REASON_IPV6_NDISC_HOP_LIMIT', description: 'invalid hop limit' }, | ||
{ value: 73, name: 'SKB_DROP_REASON_IPV6_NDISC_BAD_CODE', description: 'invalid NDISC icmp6 code' }, | ||
{ value: 74, name: 'SKB_DROP_REASON_IPV6_NDISC_BAD_OPTIONS', description: 'invalid NDISC options' }, | ||
{ value: 75, name: 'SKB_DROP_REASON_IPV6_NDISC_NS_OTHERHOST', description: 'NEIGHBOUR SOLICITATION' } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we have to maintain this translation from number to string in two components ? if it makes more sense to have it here we probably removed from FLP and just keep it as a number to reduce future efforts adding new drops ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a tradeoff to decide. If we want the user to be able to type either number
or name
in the filters, we need these in console plugin.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need to be able to filter on both as u have
child = clickableContent( | ||
flow.fields.PktDropLatestDropCause, | ||
getDropCauseDescription(flow.fields.PktDropLatestDropCause as DROP_CAUSES_NAMES), | ||
'https://elixir.bootlin.com/linux/latest/source/include/net/dropreason-core.h#L88' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there a specific reason to use a URL from elixir.bootlin.com
? (I don't know about this site)
This one sounds more official / primary source: https://github.com/torvalds/linux/blob/master/include/net/dropreason-core.h ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure github sounds better 👍
- ${t('A _LINUX_TCP_STATES_H number like 1, 2, 3')} | ||
- ${t('A _LINUX_TCP_STATES_H TCP name like ESTABLISHED, SYN_SENT, SYN_RECV')}`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Like you put a link to the drop reasons, we may have a link to these states definitions? https://github.com/torvalds/linux/blob/master/include/net/tcp_states.h
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good to me in general (two comments about the external links) |
Currently the new filters are just rejecting empty values I tried to check for the popover suggestions but the |
/lgtm |
/ok-to-test |
New image: It will expire after two weeks. To deploy this build, run from the operator repo, assuming the operator is running: USER=netobserv VERSION=f125544 make set-plugin-image |
Rebased without changes |
New image: It will expire after two weeks. To deploy this build, run from the operator repo, assuming the operator is running: USER=netobserv VERSION=32d3f9d make set-plugin-image |
/lgtm |
New changes are detected. LGTM label has been removed. |
@Amoghrd Addressing feedback from QE doc:
I changed the stat out of
|
While testing I noticed That makes this PR dependant of netobserv/flowlogs-pipeline#470 one. Waiting for feedback about that approach |
New image: It will expire after two weeks. To deploy this build, run from the operator repo, assuming the operator is running: USER=netobserv VERSION=0219ec4 make set-plugin-image |
[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:
Approvers can indicate their approval by writing |
Requires:
netobserv/flowlogs-pipeline#470
netobserv/network-observability-operator#401
This is a followup on NETOBSERV-1064 to improve Packet drop functionnality:
protocol
packets