Add new field to ipfix exporter for flowRtt #603
Conversation
|
Hi @bhale. Thanks for your PR. I'm waiting for a netobserv member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
bhale
changed the title
|
I think this pattern is going badly, partly because there isn't a logical place to put the new fields... the previous Kubernetes enrichment section added strings that don't change or contain values, like source namespace. With the new data being produced upstream in the ebpf agent, there are new fields we want to export that are not static data / enrichment, or data that already exists in an IANA data model - such as timeFlowRttNs. Should we be looking to create a third set of fields, or am I missing a better solution? |
|
Hi @bhale , thanks for the contrib! I am not super familiar with the ipfix code, perhaps if @praveingk is around he can better answer that me. That said, it seems indeed wrong to tie agent-specific features such as RTT to the K8s model, as this isn't related to k8s. I'm not sure how to do that (as said I'm not very familiar with this code) but I can take a look and try |
|
@bhale Thanks for addressing this. I had previously added enrichEnterpriseID to support fields that are not supported by IANA registry and a custom registry. In this case, RTT isn't supported by IANA, and it makes sense to add a separate function and not adding as part of K8s enrichment. |
There was a problem hiding this comment.
Hey @bhale , here's some comments.
The ipfix writer here clearly lacks some unit tests, I've started to add some, I'm guessing this will be helpful for you when diving in this code
If you want, I can push my unit test commit to your PR
pkg/pipeline/write/write_ipfix.go
Outdated
| @@ -124,6 +132,11 @@ func loadCustomRegistry(EnterpriseID uint32) error { | |||
| ilog.WithError(err).Errorf("Failed to register element") | |||
| return err | |||
| } | |||
| err = registry.PutInfoElement((*entities.NewInfoElement("timeFlowRttNs", 7740, 4, EnterpriseID, 65535)), EnterpriseID) | |||
There was a problem hiding this comment.
In my understanding, the last arg (len) of NewInfoElement has to be 8, to match the uint64 type here. Also the ID doesn't follow the previous one, I guess it should be 7739
pkg/pipeline/write/write_ipfix.go
Outdated
| @@ -445,6 +466,12 @@ func setKubeIEValue(record config.GenericMap, ieValPtr *entities.InfoElementWith | |||
| } else { | |||
| ieVal.SetStringValue("none") | |||
| } | |||
| case "timeFlowRttNs": | |||
| if record["TimeFlowRttNs"] != nil { | |||
| ieVal.SetUnsigned64Value(record["TimeFlowRttNs"].(uint64)) | |||
There was a problem hiding this comment.
here I believe it should be:
ieVal.SetUnsigned64Value(uint64(record["TimeFlowRttNs"].(int64)))
It's set here: https://github.com/netobserv/netobserv-ebpf-agent/blob/main/pkg/decode/decode_protobuf.go#L122 , this value comes from a duration .Nanoseconds() which return an int64
|
@bhale my PR #606 adds a test with ipfix that should be helpful here |
This is a huge help thank you! I will rebase on these changes tomorrow, it looks like it will simplify things. |
bhale
changed the title
|
Thanks for all your help and advice, @praveingk and @jotak! I incorporated the refactoring and tests from jotak, integrated my changes to export timeFlowRttNs, and added a test for it. The test passes, and I can see valid data for RTT in IBM SevOne for the new RTT field. Regarding the comment on using field id 7739 - my colleague has access to a database at IBM that tracks the fields for our enterpriseId. Someone had already claimed 7739, so I skipped it. |
Nice 👍
ok .. I don't know why exactly it starts at |
|
btw I've merged my PR, this one should be rebased |
bhale
force-pushed
the
new-ipfix-fields
branch
from
1aa0f7e
to
a109f34
Compare
Rebase good to go! |
|
@jotak FYI, yes, between myself & Dale Bowie, we track the field ID allocations. The 7xxx starting number was partly looking for a range that doesn't overlap with other IBM products. We should probably switch it from private to public git at some point. |
jotak
added
the
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=71f5ae6 make set-flp-image |
|
/lgtm |
|
[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 |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #603 +/- ##
==========================================
- Coverage 67.03% 66.95% -0.08%
==========================================
Files 103 103
Lines 7401 7412 +11
==========================================
+ Hits 4961 4963 +2
- Misses 2147 2157 +10
+ Partials 293 292 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Description
I am working on implementing a fix for
#544
There are many new fields added to the pipeline but not yet written via IPFIX. I attempted to start with a single field and get feedback before I added several fields using a bad approach.
Dependencies
None
Checklist
** I am not familiar with the processes **
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.