Skip to content
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-1111: Add support for RoundTripTime in FLP. #446

Merged
merged 2 commits into from
Jul 6, 2023

Conversation

dushyantbehl
Copy link
Contributor

@dushyantbehl dushyantbehl commented Jun 21, 2023

Related PR - netobserv/netobserv-ebpf-agent#117

Since the above pull request makes changes in the ebpf agent proto buf which it exports, this PR is marked as breaking changes as we need same version of agent and flp to work together.

@dushyantbehl dushyantbehl changed the title Add support for RoundTripTime in FLP. NETOBSERV-954: Add support for RoundTripTime in FLP. Jun 21, 2023
@openshift-ci-robot
Copy link
Collaborator

openshift-ci-robot commented Jun 21, 2023

@dushyantbehl: This pull request references NETOBSERV-954 which is a valid jira issue.

In response to this:

Related PR - netobserv/netobserv-ebpf-agent#117

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.

@dushyantbehl
Copy link
Contributor Author

/ok-to-test

@openshift-ci openshift-ci bot added the ok-to-test To set manually when a PR is safe to test. Triggers image build on PR. label Jun 21, 2023
@github-actions
Copy link

New image: quay.io/netobserv/flowlogs-pipeline:54d1bc6. It will expire after two weeks.

@codecov
Copy link

codecov bot commented Jun 21, 2023

Codecov Report

Merging #446 (2c58cc1) into main (a8655fd) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main     #446   +/-   ##
=======================================
  Coverage   62.79%   62.79%           
=======================================
  Files          96       96           
  Lines        6910     6916    +6     
=======================================
+ Hits         4339     4343    +4     
- Misses       2326     2327    +1     
- Partials      245      246    +1     
Flag Coverage Δ
unittests 62.79% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
pkg/pipeline/decode/decode_protobuf.go 26.89% <100.00%> (+0.30%) ⬆️

... and 2 files with indirect coverage changes

eranra
eranra previously approved these changes Jun 21, 2023
@@ -57,6 +57,7 @@ func PBFlowToMap(flow *pbflow.Record) config.GenericMap {
"Flags": flow.Flags,
"IcmpType": flow.Icmp.GetIcmpType(),
"IcmpCode": flow.Icmp.GetIcmpCode(),
"RoundTripTimeMs": flow.TimeFlowRtt.AsDuration().Milliseconds(),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I advice to change to TimeRTTMS to align with other fields like TimeFlowStartMs

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure..will update I think TimeFlowRTTMs will match your example is that fine @eranra

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated - TimeFlowRttMs

KalmanMeth
KalmanMeth previously approved these changes Jun 21, 2023
@dushyantbehl dushyantbehl dismissed stale reviews from KalmanMeth and eranra via b4bd1d1 June 21, 2023 13:27
@github-actions github-actions bot removed the ok-to-test To set manually when a PR is safe to test. Triggers image build on PR. label Jun 21, 2023
@dushyantbehl
Copy link
Contributor Author

/ok-to-test

@openshift-ci openshift-ci bot removed the lgtm label Jun 21, 2023
@dushyantbehl
Copy link
Contributor Author

@KalmanMeth @eranra do we need to hold these changes till the PR:netobserv/netobserv-ebpf-agent#117 gets merged?

@openshift-ci openshift-ci bot added the ok-to-test To set manually when a PR is safe to test. Triggers image build on PR. label Jun 21, 2023
@github-actions
Copy link

New image: quay.io/netobserv/flowlogs-pipeline:a6d3080. It will expire after two weeks.

@dushyantbehl dushyantbehl added the enhancement New feature or request label Jun 21, 2023
jpinsonneau
jpinsonneau previously approved these changes Jun 21, 2023
Copy link
Collaborator

@jpinsonneau jpinsonneau left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thanks @dushyantbehl !

Related PR - netobserv/netobserv-ebpf-agent#117

Signed-off-by: Dushyant Behl <dushyantbehl@users.noreply.github.com>
@dushyantbehl
Copy link
Contributor Author

/ok-to-test

@openshift-ci openshift-ci bot added the ok-to-test To set manually when a PR is safe to test. Triggers image build on PR. label Jun 30, 2023
@github-actions
Copy link

New image: quay.io/netobserv/flowlogs-pipeline:92acfb3. It will expire after two weeks.

@dushyantbehl
Copy link
Contributor Author

Rebased. Please take a look @eranra @jpinsonneau @KalmanMeth

jpinsonneau
jpinsonneau previously approved these changes Jun 30, 2023
Copy link
Collaborator

@jpinsonneau jpinsonneau left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM in terms of code, thanks @dushyantbehl !

@openshift-ci openshift-ci bot added the lgtm label Jun 30, 2023
@dushyantbehl
Copy link
Contributor Author

Thanks @jpinsonneau!!

@dushyantbehl
Copy link
Contributor Author

@jpinsonneau @eranra @KalmanMeth
Can one of you please approve these changes as the PR netobserv/netobserv-ebpf-agent#117 has been merged 😄

@jpinsonneau
Copy link
Collaborator

@jpinsonneau @eranra @KalmanMeth Can one of you please approve these changes as the PR netobserv/netobserv-ebpf-agent#117 has been merged 😄

@dushyantbehl before approving this you need to update your ebpf agent dependency in go.mod

KalmanMeth
KalmanMeth previously approved these changes Jul 3, 2023
@dushyantbehl dushyantbehl added the breaking-change This pull request has breaking changes. They should be described in PR description. label Jul 5, 2023
@openshift-ci-robot
Copy link
Collaborator

openshift-ci-robot commented Jul 5, 2023

@dushyantbehl: This pull request references NETOBSERV-1111 which is a valid jira issue.

In response to this:

Related PR - netobserv/netobserv-ebpf-agent#117

Since the above pull request makes changes in the ebpf agent proto buf which it exports, this PR is marked as breaking changes as we need same version of agent and flp to work together.

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.

Signed-off-by: Dushyant Behl <dushyantbehl@users.noreply.github.com>
@dushyantbehl dushyantbehl dismissed stale reviews from KalmanMeth and jpinsonneau via 2c58cc1 July 5, 2023 08:46
@openshift-ci openshift-ci bot removed the lgtm label Jul 5, 2023
@github-actions github-actions bot removed the ok-to-test To set manually when a PR is safe to test. Triggers image build on PR. label Jul 5, 2023
@dushyantbehl
Copy link
Contributor Author

/ok-to-test

@openshift-ci openshift-ci bot added the ok-to-test To set manually when a PR is safe to test. Triggers image build on PR. label Jul 5, 2023
@github-actions
Copy link

github-actions bot commented Jul 5, 2023

New image: quay.io/netobserv/flowlogs-pipeline:b2a5acd. It will expire after two weeks.

@dushyantbehl
Copy link
Contributor Author

@jpinsonneau @KalmanMeth @eranra Update the go mod dependency to include newer variant of ebpf agent. The tests seem to be fine so i'm assuming we are okay now. Let me know if this lacks anything.

Also, I marked the PR as breaking changes because of the update in protobuf

@dushyantbehl
Copy link
Contributor Author

cc @jotak
missed adding your name before

@jotak
Copy link
Member

jotak commented Jul 6, 2023

thanks @dushyantbehl !
/lgtm
/approve

@openshift-ci openshift-ci bot added the lgtm label Jul 6, 2023
@openshift-ci
Copy link

openshift-ci bot commented Jul 6, 2023

[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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the approved label Jul 6, 2023
@openshift-merge-robot openshift-merge-robot merged commit 7dde343 into netobserv:main Jul 6, 2023
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved breaking-change This pull request has breaking changes. They should be described in PR description. enhancement New feature or request jira/valid-reference lgtm ok-to-test To set manually when a PR is safe to test. Triggers image build on PR.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants