-
Notifications
You must be signed in to change notification settings - Fork 24
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-1110: Enable support for Flow RTT #394
Conversation
@dushyantbehl: This pull request references NETOBSERV-1110 which is a valid jira issue. 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/test-infra repository. |
/ok-to-test |
New images:
They will expire after two weeks. To deploy this build: # Direct deployment, from operator repo
IMAGE=quay.io/netobserv/network-observability-operator:92a3fee make deploy
# Or using operator-sdk
operator-sdk run bundle quay.io/netobserv/network-observability-operator-bundle:v0.0.0-92a3fee Or as a Catalog Source: apiVersion: operators.coreos.com/v1alpha1
kind: CatalogSource
metadata:
name: netobserv-dev
namespace: openshift-marketplace
spec:
sourceType: grpc
image: quay.io/netobserv/network-observability-operator-catalog:v0.0.0-92a3fee
displayName: NetObserv development catalog
publisher: Me
updateStrategy:
registryPoll:
interval: 1m |
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #394 +/- ##
==========================================
+ Coverage 55.49% 55.82% +0.32%
==========================================
Files 45 45
Lines 5874 5888 +14
==========================================
+ Hits 3260 3287 +27
+ Misses 2393 2380 -13
Partials 221 221
Flags with carried forward coverage won't be shown. Click here to find out more.
☔ View full report in Codecov by Sentry. |
New images:
They will expire after two weeks. To deploy this build: # Direct deployment, from operator repo
IMAGE=quay.io/netobserv/network-observability-operator:e44e1b3 make deploy
# Or using operator-sdk
operator-sdk run bundle quay.io/netobserv/network-observability-operator-bundle:v0.0.0-e44e1b3 Or as a Catalog Source: apiVersion: operators.coreos.com/v1alpha1
kind: CatalogSource
metadata:
name: netobserv-dev
namespace: openshift-marketplace
spec:
sourceType: grpc
image: quay.io/netobserv/network-observability-operator-catalog:v0.0.0-e44e1b3
displayName: NetObserv development catalog
publisher: Me
updateStrategy:
registryPoll:
interval: 1m |
As discussed, the RTT is not present in every flow and the enrichment ratio depends of sampling. We can retreive the ratio of flows containing
We should document that 📜 |
@dushyantbehl can you please rebase your PR ? |
/ok-to-test |
New images:
They will expire after two weeks. To deploy this build: # Direct deployment, from operator repo
IMAGE=quay.io/netobserv/network-observability-operator:251705a make deploy
# Or using operator-sdk
operator-sdk run bundle quay.io/netobserv/network-observability-operator-bundle:v0.0.0-251705a Or as a Catalog Source: apiVersion: operators.coreos.com/v1alpha1
kind: CatalogSource
metadata:
name: netobserv-dev
namespace: openshift-marketplace
spec:
sourceType: grpc
image: quay.io/netobserv/network-observability-operator-catalog:v0.0.0-251705a
displayName: NetObserv development catalog
publisher: Me
updateStrategy:
registryPoll:
interval: 1m |
scc.AllowHostDirVolumePlugin = true | ||
} | ||
if (desired.EnablePktDrop != nil && !*desired.EnablePktDrop) && |
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 was added to handle special case where u start with both features on then u just disable one since we don't compare current vs desired we could endup clearing the AllowHostDirVolumePlugin there was an issue about this by @memodi
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.
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.
@msherif1234 scc
is created a few lines above with AllowHostDirVolumePlugin
not initialized , and since it's a bool
, it means it's false
already . So I don't see the point of setting it to false here?
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.
@msherif1234 if you're talking about the nil-check, it's not relevant anymore now, as the new Features
is a slice, unlike the previous EnablePktDrop
/ EnableDNSTracking
that were bool pointers
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.
can u try the steps mentioned in the bug shared above to be sure, the reason I added the explicit false to make scc desired different than actual anyway I kind of forget the details its been awhile just make sure the steps showing in the above issue are fine
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.
Just tried to reproduce that old ticket, and it works fine: I get no error on the daemonset, pods are updated as expected, flows are flowing with dns/drops info
api/v1beta1/flowcollector_types.go
Outdated
// Agent feature, can be one of:<br> | ||
// - `PKT_DROP`, to track packet drops.<br> | ||
// - `DNS_TRACKING`, to track specific information on DNS traffic.<br> | ||
// - `FLOW_RTT`, to track L4 latency. <i>Unsupported (*)</i><br> |
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 remove
and other weired chars at the end of the above lines and every else in this file
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.
@msherif1234 I guess you are talking about the HTML blocks right ? That's not new to this PR: https://issues.redhat.com/browse/NETOBSERV-1104 ; it's related to asciidocs
Let's keep this as is and solve it in the related ticket
// If the `spec.agent.eBPF.privileged` parameter is not set, an error is reported.<br> | ||
// - `FLOW_RTT` <i>Unsupported (*)</i>: allows enabling flow latency (RTT) calculations in the eBPF agent during TCP handshakes. | ||
// This feature needs both INGRESS and EGRESS direction flow capture and will be disabled if they are not both enabled.<br> | ||
// +optional |
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.
should we add or we need to allow Features: block with no features ?
// +kubebuilder:validation:Required
// +kubebuilder:validation:MinItems:=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.
I don't think we need that check. We could simply initialize it empty []
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.
yeah I don't even think an empty array initialization is needed, we don't do that in other places (e.g. exporters list)
@@ -64,12 +64,12 @@ func (r *FlowCollector) ConvertTo(dstRaw conversion.Hub) error { | |||
|
|||
dst.Spec.Loki.Enable = restored.Spec.Loki.Enable | |||
|
|||
dst.Spec.Agent.EBPF.PktDrop = restored.Spec.Agent.EBPF.PktDrop | |||
dst.Spec.Agent.EBPF.DNSTracking = restored.Spec.Agent.EBPF.DNSTracking | |||
if restored.Spec.Agent.EBPF.Features != nil { |
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 is a slice u need to allocate the space then copy content
dst.Spec.Agent.EBPF.Features = make([]AgentFeature, len(restored.Spec.Agent.EBPF.Features))
copy(dst.Spec.Agent.EBPF.Features, restored.Spec.Agent.EBPF.Features)
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.
done
api/v1beta1/flowcollector_types.go
Outdated
ConfigDisabled FeatureConfigType = "DISABLED" | ||
PktDrop AgentFeature = "PKT_DROP" | ||
DNSTracking AgentFeature = "DNS_TRACKING" | ||
FlowRTT AgentFeature = "FLOW_RTT" |
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.
if I am not mistaken Strings need to use Pascal not snake case ?
PktDrop AgentFeature = "PacketsDrop"
DNSTracking AgentFeature = "DnsTacking"
FlowRTT AgentFeature = "FlowRtt"
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.
You're right, idk why I had in mind the best practice was upper case
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.
done
Co-authored-by: Julien Pinsonneau <91894519+jpinsonneau@users.noreply.github.com>
New images:
They will expire after two weeks. To deploy this build: # Direct deployment, from operator repo
IMAGE=quay.io/netobserv/network-observability-operator:e7c6868 make deploy
# Or using operator-sdk
operator-sdk run bundle quay.io/netobserv/network-observability-operator-bundle:v0.0.0-e7c6868 Or as a Catalog Source: apiVersion: operators.coreos.com/v1alpha1
kind: CatalogSource
metadata:
name: netobserv-dev
namespace: openshift-marketplace
spec:
sourceType: grpc
image: quay.io/netobserv/network-observability-operator-catalog:v0.0.0-e7c6868
displayName: NetObserv development catalog
publisher: Me
updateStrategy:
registryPoll:
interval: 1m |
- Remove mention of INGRESS/EGRESS having to be enabled since it's not configurable - Remove italic text (cf netobserv#407 ) - Update bundle
- Use PascalCase for enums - Copy array in webhook - Update bundle
@dushyantbehl @msherif1234 @jpinsonneau : I updated the PR to address feedback |
New images:
They will expire after two weeks. To deploy this build: # Direct deployment, from operator repo
IMAGE=quay.io/netobserv/network-observability-operator:28a41df make deploy
# Or using operator-sdk
operator-sdk run bundle quay.io/netobserv/network-observability-operator-bundle:v0.0.0-28a41df Or as a Catalog Source: apiVersion: operators.coreos.com/v1alpha1
kind: CatalogSource
metadata:
name: netobserv-dev
namespace: openshift-marketplace
spec:
sourceType: grpc
image: quay.io/netobserv/network-observability-operator-catalog:v0.0.0-28a41df
displayName: NetObserv development catalog
publisher: Me
updateStrategy:
registryPoll:
interval: 1m |
/lgtm |
/approve |
[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:
Approvers can indicate their approval by writing |
Related console PR: netobserv/network-observability-console-plugin#365