NETOBSERV-1965: Initial intg with UDN interface mapping api #487
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 |
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
force-pushed
the
udn-mapping
branch
from
a37d1ea to
64b226b
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=d0cb66c make set-agent-image |
pkg/pbflow/proto.go
Outdated
| if v != "" { | ||
| entry.Udn = v | ||
| } else { | ||
| entry.Udn = "default-network" |
There was a problem hiding this comment.
Should we use here a name that we are sure cannot be used by the users? Or is "default-network" already a special name (ie. users cannot use that same name) ?
Maybe that would involve adding some special character that can't be retrieved in a udn name?
There was a problem hiding this comment.
I don't think it will ever conflict with udn tag it uses name/namespace syntax but I will double check with ovnk team to be sure
github-actions
bot
removed
the
ok-to-test
msherif1234
force-pushed
the
udn-mapping
branch
from
42a06a7 to
e3a6a54
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=7f0ede5 make set-agent-image |
Signed-off-by: Mohamed Mahmoud <[email protected]>
msherif1234
force-pushed
the
udn-mapping
branch
from
e3a6a54 to
1599849
Compare
github-actions
bot
removed
the
ok-to-test
Signed-off-by: Mohamed Mahmoud <[email protected]>
msherif1234
force-pushed
the
udn-mapping
branch
from
1599849 to
0870aa9
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=c9df929 make set-agent-image |
|
@msherif1234: The following tests 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. |
|
@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. |
| @@ -56,6 +56,7 @@ func TestConversions(t *testing.T) { | |||
| }, | |||
| }, | |||
| Interfaces: []model.IntfDir{model.NewIntfDir("eth0", model.DirectionEgress)}, | |||
| UdnList: []string{""}, | |||
There was a problem hiding this comment.
could you add some non-empty data in some of the tests?
There was a problem hiding this comment.
well actually this will require mock to ovnk decoded to return back strings
| }) | ||
| Udn: "", | ||
| } | ||
| if s != nil { |
There was a problem hiding this comment.
Doing this in protobuf decode will cause the same issue as what I mentioned here: #286 (review)
.. and which made us having this bug: https://issues.redhat.com/browse/NETOBSERV-2009
I think it doesn't make sense to have any enrichment set in protobuf decode. There should be in a common path, not something that may end up not being invoked. IMO we should fix that in a first place rather than adding technical debt.
There was a problem hiding this comment.
I think adding new pipeline might be very risk at this stage given the unstability e have can this be deferred to 1.9 ?
There was a problem hiding this comment.
An option could be to create a new stage (like in this area), or perhaps inject some enrichment callback directly in NewRecord like it's done for interfaceNamer, wdyt?
There was a problem hiding this comment.
We should fix NETOBSERV-2009 in 1.8 so if we fix it for events we could surely fix it as well for udns?
There was a problem hiding this comment.
we still in TP for network events so no urgency for CLI support I think for 1.8 I will try to explode the new stage option
| Udn: "", | ||
| } | ||
| if s != nil { | ||
| m, err := s.GetInterfaceUDNs() |
There was a problem hiding this comment.
Looking at the ovn code, it seems like s.GetInterfaceUDNs() doesn't just returned some cached thing, it calls d.ovsdbClient.List(context.Background(), &ifaces) under the hood, which may involve some i/o ? That would be nice to double check. I think it's better at the very least to move this call out of the interface for loop, but even better, it could be fetched just once for the whole eviction loop or a different place ?
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.