On Tue, Nov 19, 2019 at 5:32 PM Han Zhou <[email protected]> wrote:
>
>
> On Tue, Nov 19, 2019 at 2:19 PM Russell Bryant <[email protected]> wrote:
>
>> On Tue, Nov 19, 2019 at 4:44 PM Han Zhou <[email protected]> wrote:
>> >
>> >
>> >
>> > On Tue, Nov 19, 2019 at 1:38 PM Han Zhou <[email protected]> wrote:
>> >>
>> >>
>> >>
>> >> On Tue, Nov 19, 2019 at 3:33 AM Numan Siddique <[email protected]> wrote:
>> >> >
>> >> > On Tue, Nov 19, 2019 at 7:04 AM Russell Bryant <[email protected]>
>> wrote:
>> >> > >
>> >> > > While debugging some problems in a cluster using ovn-kubernetes, I
>> >> > > noticed that we're creating two conflicting logical flows. These
>> two
>> >> > > flows only matched on the destination MAC address. It was not
>> >> > > deterministic whether you'd hit the IPv4 (ARP) or IPv6 (NS)
>> version.
>> >> > >
>> >> > > This change adds an ip4 or ip6 match to each flow as appropriate.
>> >> > >
>> >> > > Signed-off-by: Russell Bryant <[email protected]>
>> >> >
>> >> > Acked-by: Numan Siddique <[email protected]>
>> >> >
>> >> > > ---
>> >> > > northd/ovn-northd.c | 4 ++--
>> >> > > 1 file changed, 2 insertions(+), 2 deletions(-)
>> >> > >
>> >> > > --- NOTE ---
>> >> > >
>> >> > > I've only tested this by running "make check" and "make
>> check-kernel" so
>> >> > > far, and all tests still pass.
>> >> > >
>> >> > > If I'm reading this code right, I'm really surprised this hasn't
>> come up
>> >> > > sooner? I guess we also don't have adequate test coverage for
>> these
>> >> > > flows?
>> >> >
>> >> > Thanks for the patch. Yeah we don't have much coverage here.
>> >> > We should add system tests for this.
>> >> >
>> >> > Numan
>> >> >
>> >>
>> >> I noticed this when I was testing ddlog which couldn't handle this
>> well initially but later fixed. I thought it was a problem, too, but then
>> figured out it is actually handled by ovn-controller when translating to
>> open-flows. The condition ip4/ip6 is added during the translation
>> automatically.
>> >>
>> > This just explains why "this hasn't come up sooner", but the patch
>> LGTM. It is better to add the condition in logical flows.
>>
>> Interesting - and it works the same even though there are different
>> arguments to arp{} or nd_ns{} in each logical flow?
>>
>> They are two different lflows (since the actions are different), and
> translated in two different OVS flows. During translation by
> ovn-controller, when parsing the actions, the ip4/ip6 is specified as
> prerequisite, and then the prerequisite is added as a match condition, too.
> Please see:
> https://github.com/ovn-org/ovn/blob/master/lib/actions.c#L1169
> https://github.com/ovn-org/ovn/blob/master/lib/actions.c#L1211
>
Ah ha! That explains it. Thank you. :-)
>
>
>> >
>> >>
>> >> > >
>> >> > >
>> >> > > diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
>> >> > > index 41e97f841..f0ab43b27 100644
>> >> > > --- a/northd/ovn-northd.c
>> >> > > +++ b/northd/ovn-northd.c
>> >> > > @@ -9319,7 +9319,7 @@ build_lrouter_flows(struct hmap *datapaths,
>> struct hmap *ports,
>> >> > > }
>> >> > >
>> >> > > ovn_lflow_add(lflows, od, S_ROUTER_IN_ARP_REQUEST, 100,
>> >> > > - "eth.dst == 00:00:00:00:00:00",
>> >> > > + "eth.dst == 00:00:00:00:00:00 && ip4",
>> >> > > "arp { "
>> >> > > "eth.dst = ff:ff:ff:ff:ff:ff; "
>> >> > > "arp.spa = reg1; "
>> >> > > @@ -9328,7 +9328,7 @@ build_lrouter_flows(struct hmap *datapaths,
>> struct hmap *ports,
>> >> > > "output; "
>> >> > > "};");
>> >> > > ovn_lflow_add(lflows, od, S_ROUTER_IN_ARP_REQUEST, 100,
>> >> > > - "eth.dst == 00:00:00:00:00:00",
>> >> > > + "eth.dst == 00:00:00:00:00:00 && ip6",
>> >> > > "nd_ns { "
>> >> > > "nd.target = xxreg0; "
>> >> > > "output; "
>> >> > > --
>> >> > > 2.23.0
>> >> > >
>> >> > > _______________________________________________
>> >> > > dev mailing list
>> >> > > [email protected]
>> >> > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>> >> > >
>> >> > _______________________________________________
>> >> > dev mailing list
>> >> > [email protected]
>> >> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>>
>>
>>
>> --
>> Russell Bryant
>>
>
--
Russell Bryant
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
