fix(evpn-bridge): make tracer optional #380
Conversation
mardim91
force-pushed
the
make-tracer-optional
branch
2 times, most recently
from
cd01f38 to
6a7df3e
Compare
pkg/utils/netlink.go
Outdated
| @@ -35,6 +36,12 @@ type Netlink interface { | |||
| LinkSetDown(context.Context, netlink.Link) error | |||
| LinkSetMaster(context.Context, netlink.Link, netlink.Link) error | |||
| LinkSetNoMaster(context.Context, netlink.Link) error | |||
| LinkSetNsFd(context.Context, netlink.Link, int) error | |||
There was a problem hiding this comment.
The PR descriptions says that we are making the tracer optional. However, we are adding new methods. Should it be a part of a separate PR?
There was a problem hiding this comment.
Hello @artek-koltun,
I will prefer to have those changes if it is possible to a single PR as it is easier for me to test the CNI PR against this one. Mainly these changes that are introduced in this PR have as purpose to make the netlink and FRR wrapper libraries to be reused by other components like the EVPN GW CNI
I can update the description of the PR if that is ok but I would prefer to not have multiple PRs as it is difficult to test the CNI against this. Hope that is ok.
There was a problem hiding this comment.
What if you test locally, then split and provide one PR and then another one?
There was a problem hiding this comment.
ok let me do the changes on this PR and test them. If they are working I will create two individual ones.
There was a problem hiding this comment.
Hello @artek-koltun,
I have made the changes with the noop tracer and tested them. They are working. I will create the two individuals PRs tommorow as we have discussed above
There was a problem hiding this comment.
Hello @artek-koltun,
I have splitted the changes in two PRs. The first one is this PR here and the second one is this
Please review when you find some time.
There was a problem hiding this comment.
Could I ask you to update the storage bridges as well?
| func NewNetlinkWrapperWithArgs(enableTracer bool) *NetlinkWrapper { | ||
| netlinkWrapper := &NetlinkWrapper{} | ||
| if enableTracer { | ||
| netlinkWrapper.tracer = otel.Tracer("") |
There was a problem hiding this comment.
I see that we need to check a tracer all the time here and there...
Is it possible to use NoopTracerProvider here, so we use that noop object and not checking for nil all the time?
There was a problem hiding this comment.
That is a very good point. Let me take a look
mardim91
changed the title
mardim91
changed the title
mardim91
force-pushed
the
make-tracer-optional
branch
2 times, most recently
from
06a7932 to
b60c09b
Compare
mardim91
changed the title
Signed-off-by: Dimitrios Markou <[email protected]>
mardim91
force-pushed
the
make-tracer-optional
branch
from
b60c09b to
3342b84
Compare
|
Thank you @artek-koltun |
Make tracer optional so we can import the netlinkWrapper library from evpn-gw-cni.
More info here: opiproject/opi-gateway-evpn-cni#14 (comment)