Thanks Ben for the review. I have fixed the tests (basically, now ip or ipv6 is added to the flows) and submitted a v2 of the patch.
Daniel On Wed, Oct 25, 2017 at 7:21 PM, Ben Pfaff <b...@ovn.org> wrote: > On Wed, Oct 25, 2017 at 06:50:29PM +0530, Numan Siddique wrote: > > On Wed, Oct 25, 2017 at 3:09 PM, Daniel Alvarez Sanchez < > dalva...@redhat.com > > > wrote: > > > > > > > > > > > On Tue, Oct 24, 2017 at 11:35 PM, Ben Pfaff <b...@ovn.org> wrote: > > > > > >> On Tue, Oct 24, 2017 at 02:27:59PM -0700, Ben Pfaff wrote: > > >> > On Tue, Oct 24, 2017 at 11:07:58PM +0200, Daniel Alvarez Sanchez > wrote: > > >> > > Hi guys, > > >> > > > > >> > > Great job Numan! > > >> > > As we discussed over IRC, the patch below may make more sense. > > >> > > It essentially sets the dl_type so that when packet comes from the > > >> > > controller, it matches > > >> > > a valid type and OVS_KEY_ATTR_CT_ORIG_TUPLE_IPV4 is not added. > > >> > > Maybe what Numan proposed and this patch could be a good > combination > > >> but I > > >> > > think > > >> > > that we definitely need to set the dl_type as it's later checked > in > > >> > > pkt_metadata_from_flow() > > >> > > and it'll be zero there otherwise. > > >> > > What do you guys think? I have tried the patch below and the > kernel > > >> error > > >> > > is not shown > > >> > > anymore when issuing DHCP requests. > > >> > > > > >> > > > > >> > > diff --git a/lib/flow.c b/lib/flow.c > > >> > > index b2b10aa..62b948f 100644 > > >> > > --- a/lib/flow.c > > >> > > +++ b/lib/flow.c > > >> > > @@ -1073,6 +1073,7 @@ flow_get_metadata(const struct flow *flow, > > >> struct > > >> > > match *flow_metadata) > > >> > > > > >> > > if (flow->ct_state != 0) { > > >> > > match_set_ct_state(flow_metadata, flow->ct_state); > > >> > > + match_set_dl_type(flow_metadata, flow->dl_type); > > >> > > if (is_ct_valid(flow, NULL, NULL) && flow->ct_nw_proto > != 0) > > >> { > > >> > > if (flow->dl_type == htons(ETH_TYPE_IP)) { > > >> > > match_set_ct_nw_src(flow_metadata, > flow->ct_nw_src); > > >> > > > >> > This patch seems reasonable too. > > >> > > > >> > I recommend adding a comment above it to explain what's going on, > > >> > because dl_type is not a metadata field and it's confusing to deal > with > > >> > it in a context that's supposed to be all about metadata. I guess > the > > >> > comment would essentially say that dl_type is essential to the > > >> > interpretation of the conntrack metadata. > > >> > > >> Oh, and for this patch too I'd welcome a formal patch proposal. > > >> > > > > > > Done. Thanks a lot Ben. > > > If this get merged, it would be great if we can get it into 2.8 branch > and > > > add a new tag (2.8.2). > > > > > > Thanks!! > > > > > > > Ben, we have submitted both the patches. The patch from Daniel - ( > > https://patchwork.ozlabs.org/patch/830160/) will fix the issue. > > Not sure if this patch https://patchwork.ozlabs.org/patch/830132/ is > > needed any more. > > > > Request to review these patches if possible as RDO is blocked on these > > patches before we can update from OVS 2.7.2 to OVS 2.8(.2) > > I've reviewed both. I wasn't able to immediately apply either one, but > they're obviously moving in the right direction, so I'd appreciate > follow-up from both of you so that we can get them in. >
_______________________________________________ discuss mailing list disc...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-discuss