On Tue, Nov 19, 2019 at 2:19 PM Russell Bryant <russ...@ovn.org> wrote:
> On Tue, Nov 19, 2019 at 4:44 PM Han Zhou <zhou...@gmail.com> wrote:
> >
> >
> >
> > On Tue, Nov 19, 2019 at 1:38 PM Han Zhou <zhou...@gmail.com> wrote:
> >>
> >>
> >>
> >> On Tue, Nov 19, 2019 at 3:33 AM Numan Siddique <num...@ovn.org> wrote:
> >> >
> >> > On Tue, Nov 19, 2019 at 7:04 AM Russell Bryant <russ...@ovn.org>
> 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 <russ...@ovn.org>
> >> >
> >> > Acked-by: Numan Siddique <num...@ovn.org>
> >> >
> >> > > ---
> >> > > 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
> >
> >>
> >> > >
> >> > >
> >> > > 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
> >> > > d...@openvswitch.org
> >> > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> >> > >
> >> > _______________________________________________
> >> > dev mailing list
> >> > d...@openvswitch.org
> >> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
>
>
> --
> Russell Bryant
>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
