NETOBSERV-1965: Initial intg with UDN interface mapping api #487
Conversation
msherif1234
force-pushed
the
udn-mapping
branch
6 times, most recently
from
148c2ff to
41dd289
Compare
|
/ok-to-test |
openshift-ci
bot
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=6d1822e make set-agent-image |
msherif1234
force-pushed
the
udn-mapping
branch
from
41dd289 to
f6cb719
Compare
github-actions
bot
removed
the
ok-to-test
|
/ok-to-test |
openshift-ci
bot
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=4319c79 make set-agent-image |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #487 +/- ##
==========================================
+ Coverage 28.34% 28.40% +0.06%
==========================================
Files 46 46
Lines 4741 4745 +4
==========================================
+ Hits 1344 1348 +4
Misses 3295 3295
Partials 102 102
Flags with carried forward coverage won't be shown. Click here to find out more.
|
msherif1234
force-pushed
the
udn-mapping
branch
from
f6cb719 to
d625a2e
Compare
github-actions
bot
removed
the
ok-to-test
|
/ok-to-test |
openshift-ci
bot
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=4599f62 make set-agent-image |
msherif1234
force-pushed
the
udn-mapping
branch
from
d625a2e to
bf1b744
Compare
github-actions
bot
removed
the
ok-to-test
|
/ok-to-test |
openshift-ci
bot
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=80a056e make set-agent-image |
msherif1234
changed the title
|
@msherif1234: This pull request references NETOBSERV-1965 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.19.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. |
github-actions
bot
removed
the
ok-to-test
|
/ok-to-test |
openshift-ci
bot
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=b7a449f make set-agent-image |
msherif1234
force-pushed
the
udn-mapping
branch
from
a2e1562 to
a6c3e54
Compare
github-actions
bot
removed
the
ok-to-test
|
/ok-to-test |
openshift-ci
bot
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=096385d make set-agent-image |
msherif1234
force-pushed
the
udn-mapping
branch
from
a6c3e54 to
8c5d63c
Compare
github-actions
bot
removed
the
ok-to-test
|
@msherif1234: The following test failed, say
Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
| ovnmodel "github.com/ovn-org/ovn-kubernetes/go-controller/observability-lib/model" | ||
| ovnobserv "github.com/ovn-org/ovn-kubernetes/go-controller/observability-lib/sampledecoder" |
There was a problem hiding this comment.
I don't want to block this PR further, but I think we should get rid of these dependencies in our model package
So, no blocker - I suggest to add that to a follow-up task - possibly for 1.9.
An option could be to pass an interface, or callbacks, instead of the actual sampledecoder.
(Another option which we briefly discussed is to add a new stage instead of doing this decoration directly in Record, but I'm not sure it's any better)
There was a problem hiding this comment.
yeah we can see how to clean this further in 1.9 I briefly looked at new stage but I quickly realized it adds some overhead and wasn't sure how to correctly wire things
| if err != nil { | ||
| return IntfDirUdn{Interface: intf, Direction: dir, Udn: ""} | ||
| } | ||
| udnsCache = m |
There was a problem hiding this comment.
this cache is never updated? I can add that also as a follow-up if you want
There was a problem hiding this comment.
well the caches is reset every eviction then read from ovnk after that its just ready only no update is needed during the eviction processing
There was a problem hiding this comment.
oh yes I didn't noticed in NewRecord
pkg/pbflow/proto.go
Outdated
| } | ||
| } | ||
|
|
||
| pbflowRecord.NetworkEventsMetadata = make([]*NetworkEvent, 0) |
There was a problem hiding this comment.
nit / follow-up: avoid creating NetworkEventsMetadata if fr.NetworkMonitorEventsMD is empty?
pkg/decode/decode_protobuf.go
Outdated
| for _, intf := range fr.Interfaces { | ||
| directions = append(directions, intf.Direction) | ||
| interfaces = append(interfaces, intf.Interface) | ||
| if intf.Udn != "" { |
There was a problem hiding this comment.
shouldn't we append udns here, regardless of it's presence? It depends how this data is processed in the console plugin. If the udn array is correlated with the interface array, the indexes in udn array must match the one from the interfaces. Not totally sure about that.
There was a problem hiding this comment.
in the console side they are two separate list? u mean when user correlate between the two in the console ui they won't be in sync so maybe its better to show it regardless I will remove this check
There was a problem hiding this comment.
honestly I don't remember how it's done in the console plugin ... For interfaces and direction I know that we have a column that shows both "Interfaces and Directions" so the 2 arrays are correlated; but did we do something similar for udn, I'm not sure
There was a problem hiding this comment.
with my len check we get this
but w/o len check we will get n/a and xlat-test/test-network i.e 2 rows which might be better for readability because now u don't know this tag for which interface so I think better to show "" for ovn-udn1 interface
There was a problem hiding this comment.
/lgtm
thanks @msherif1234
So I created this follow-up for 1.9: https://issues.redhat.com/browse/NETOBSERV-2069 - we can continue discuss here and amend that story accordingly
Signed-off-by: Mohamed Mahmoud <[email protected]>
msherif1234
force-pushed
the
udn-mapping
branch
from
8c5d63c to
8c0a7a8
Compare
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: msherif1234 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 |
openshift-merge-bot
bot
merged commit 5110b52
into
netobserv:main
Description
Dependencies
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.