-
Notifications
You must be signed in to change notification settings - Fork 23
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-1790: Manage enrichment via "k8s.v1.cni.cncf.io/network-status" #674
base: main
Are you sure you want to change the base?
Conversation
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
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=dfe4fdf make set-flp-image |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #674 +/- ##
==========================================
- Coverage 65.36% 65.26% -0.10%
==========================================
Files 107 108 +1
Lines 6874 6953 +79
==========================================
+ Hits 4493 4538 +45
- Misses 2071 2104 +33
- Partials 310 311 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
|
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.
LGTM, thanks @jotak !
err := json.Unmarshal([]byte(statusAnnotationJSON), &networks) | ||
if err == nil { | ||
for _, network := range networks { | ||
allIPs = append(allIPs, network.IPs...) | ||
} | ||
return allIPs, nil | ||
} | ||
|
||
return nil, fmt.Errorf("cannot read annotation %s: %w", statusAnnotation, err) |
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.
Since this part of the code will be run at the pod informer initialisation, it can take a lot of CPU at FLP startup.
We should ensure this works fine on huge clusters.
f39388d |
// TODO: replace this by a new rule input | ||
mac := fmt.Sprintf("%s", outputEntry[strings.Replace(rule.Input, "Addr", "Mac", -1)]) |
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 need to be changed before merge
I'm still discussing about putting this behind a feature gate or not at this stage
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.
mac := fmt.Sprintf("%s", outputEntry[strings.Replace(rule.Input, "Addr", "Mac", -1)]) | ||
var mac string | ||
if len(rule.MacInput) > 0 { | ||
mac = fmt.Sprintf("%s", outputEntry[rule.MacInput]) |
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.
For info, for performance we should avoid using fmt.Sprintf("%s", ...)
whenever possible especially when it's part of the flows processing path - it's not only here but in a bunch of other places too. I've just created a task for that: https://issues.redhat.com/browse/NETOBSERV-1799
thanks @jpinsonneau ! |
@jotak: you cannot LGTM your own PR. In 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. |
@jotak: This pull request references NETOBSERV-1790 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.18.0" version, but no target version was set. In 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 openshift-eng/jira-lifecycle-plugin repository. |
Manage enrichment by extracting pod IPs from the annotation "k8s.v1.cni.cncf.io/network-status", which is used (at least) by multus This allows to correlate Pods with their IPs on secondary interfaces
New changes are detected. LGTM label has been removed. |
@jotak: This pull request references NETOBSERV-1790 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.18.0" version, but no target version was set. In 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 openshift-eng/jira-lifecycle-plugin repository. |
Do not use IP lookup, since now some pods are indexed via MAC and not IPs
/ok-to-test |
PR needs rebase. 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. |
1 similar comment
PR needs rebase. 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. |
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=f457772 make set-flp-image |
Description
Manage enrichment by extracting pod IPs from the annotation "k8s.v1.cni.cncf.io/network-status", which is used (at least) by multus
This allows to correlate Pods with their IPs on secondary interfaces
Breaking change
The API for
add_kubernetes_infra
transform stage has been modified to directly use enriched name & namespace to figure out the layer, instead of doing an informer lookup by IP. The reason is informers can now index by MAC.To migrate, if
add_kubernetes_infra
rule was used such as:it must be changed such as:
where SrcK8S_Namespace, SrcK8S_Name, DstK8S_Namespace and DstK8S_Name match the names of the corresponding enriched fields.
Dependencies
netobserv/network-observability-operator#732 for Mac enrichment
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.