Re: [ovs-discuss] [ovs-dev] [OVN] OVN doesn't work using OVS 2.8.1 on Centos 7.3 using conntrack

[Daniel Alvarez Sanchez]

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  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  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

Re: [ovs-discuss] [ovs-dev] [OVN] OVN doesn't work using OVS 2.8.1 on Centos 7.3 using conntrack

[Numan Siddique]

On Wed, Oct 25, 2017 at 10:51 PM, Ben Pfaff  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  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.
>


Thanks for the review.

Numan
___
discuss mailing list
disc...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-discuss

Re: [ovs-discuss] [ovs-dev] [OVN] OVN doesn't work using OVS 2.8.1 on Centos 7.3 using conntrack

[Ben Pfaff]

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  > wrote:
> 
> >
> >
> > On Tue, Oct 24, 2017 at 11:35 PM, Ben Pfaff  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

Re: [ovs-discuss] [ovs-dev] [OVN] OVN doesn't work using OVS 2.8.1 on Centos 7.3 using conntrack

[Numan Siddique]

On Wed, Oct 25, 2017 at 3:09 PM, Daniel Alvarez Sanchez  wrote:

>
>
> On Tue, Oct 24, 2017 at 11:35 PM, Ben Pfaff  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)

Thanks
Numan



Daniel
>
>
___
discuss mailing list
disc...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-discuss

Re: [ovs-discuss] [ovs-dev] [OVN] OVN doesn't work using OVS 2.8.1 on Centos 7.3 using conntrack

[Daniel Alvarez Sanchez]

On Tue, Oct 24, 2017 at 11:35 PM, Ben Pfaff  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!!
Daniel
___
discuss mailing list
disc...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-discuss

Re: [ovs-discuss] [ovs-dev] [OVN] OVN doesn't work using OVS 2.8.1 on Centos 7.3 using conntrack

[Ben Pfaff]

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.
___
discuss mailing list
disc...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-discuss

Re: [ovs-discuss] [ovs-dev] [OVN] OVN doesn't work using OVS 2.8.1 on Centos 7.3 using conntrack

[Ben Pfaff]

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.
___
discuss mailing list
disc...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-discuss

Re: [ovs-discuss] [ovs-dev] [OVN] OVN doesn't work using OVS 2.8.1 on Centos 7.3 using conntrack

[Daniel Alvarez Sanchez]

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);


Thanks,
Daniel

On Tue, Oct 24, 2017 at 10:38 PM, Ben Pfaff  wrote:

> On Tue, Oct 24, 2017 at 09:04:22PM +0530, Numan Siddique wrote:
> > We did some more investigation. This issue is seen only when OVN  native
> > dhcp is used and with kernel datapath which doesn't support
> > OVS_KEY_ATTR_CT_ORIG_TUPLE_IPV4.  The reason for this failure is because
> > ovs-vswitchd includes the attribute OVS_KEY_ATTR_CT_ORIG_TUPLE_IPV4
> when it
> > sends the packet back to the datapath after the dhcp reply packet is
> > resumed.
> >
> > When the dhcp packet is sent to ovn-controller, the ct_state value is set
> > to 0x21 and dl_type is set to 0 in the flow metadata. When the packet is
> > resumed, the function nxt_resume() calls 'pkt_metadata_from_flow()' which
> > neither sets 'md->ct_orig_tuple' or memsets it [1] because is_ct_valid()
> > returns true and dl_type is 0. And the function odp_key_from_dp_packet()
> > adds OVS_KEY_ATTR_CT_ORIG_TUPLE_IPV4 [2]
> >
> > This issue is not seen in master because of this commit - "f6fabcc624
> > ofproto-dpif: Mark packets as "untracked" after call to ct()" [3]
> >
> > This patch clears the conn track variables after the ct() action.
> >
> > I suppose we cannot apply this patch to OVS 2.8 branch because it was
> > reverted [4] due to some issues.
> >
> > I think we can solve this problem either with the below fixe or by
> setting
> > dl_type to proper value when the packet is sent to controller.
> >
> > ***
> > diff --git a/lib/flow.h b/lib/flow.h
> > index 6ae5a674d..076ce36f1 100644
> > --- a/lib/flow.h
> > +++ b/lib/flow.h
> > @@ -947,6 +947,8 @@ pkt_metadata_from_flow(struct pkt_metadata *md, const
> > struct flow *flow)
> >  flow->ct_tp_dst,
> >  flow->ct_nw_proto,
> >  };
> > +} else {
> > +memset(>ct_orig_tuple, 0, sizeof md->ct_orig_tuple);
> >  }
> >  } else {
> >  memset(>ct_orig_tuple, 0, sizeof md->ct_orig_tuple);
> > **
> >
> > Please let me know if this fix makes sense ? Or if there is a better way
> to
> > solve it ?
>
> I think that is a reasonable patch.  Will you please propose it as a
> formal patch?  Please include a thorough commit message.
>
___
discuss mailing list
disc...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-discuss

Re: [ovs-discuss] [ovs-dev] [OVN] OVN doesn't work using OVS 2.8.1 on Centos 7.3 using conntrack

[Ben Pfaff]

On Tue, Oct 24, 2017 at 09:04:22PM +0530, Numan Siddique wrote:
> We did some more investigation. This issue is seen only when OVN  native
> dhcp is used and with kernel datapath which doesn't support
> OVS_KEY_ATTR_CT_ORIG_TUPLE_IPV4.  The reason for this failure is because
> ovs-vswitchd includes the attribute OVS_KEY_ATTR_CT_ORIG_TUPLE_IPV4 when it
> sends the packet back to the datapath after the dhcp reply packet is
> resumed.
> 
> When the dhcp packet is sent to ovn-controller, the ct_state value is set
> to 0x21 and dl_type is set to 0 in the flow metadata. When the packet is
> resumed, the function nxt_resume() calls 'pkt_metadata_from_flow()' which
> neither sets 'md->ct_orig_tuple' or memsets it [1] because is_ct_valid()
> returns true and dl_type is 0. And the function odp_key_from_dp_packet()
> adds OVS_KEY_ATTR_CT_ORIG_TUPLE_IPV4 [2]
> 
> This issue is not seen in master because of this commit - "f6fabcc624
> ofproto-dpif: Mark packets as "untracked" after call to ct()" [3]
> 
> This patch clears the conn track variables after the ct() action.
> 
> I suppose we cannot apply this patch to OVS 2.8 branch because it was
> reverted [4] due to some issues.
> 
> I think we can solve this problem either with the below fixe or by setting
> dl_type to proper value when the packet is sent to controller.
> 
> ***
> diff --git a/lib/flow.h b/lib/flow.h
> index 6ae5a674d..076ce36f1 100644
> --- a/lib/flow.h
> +++ b/lib/flow.h
> @@ -947,6 +947,8 @@ pkt_metadata_from_flow(struct pkt_metadata *md, const
> struct flow *flow)
>  flow->ct_tp_dst,
>  flow->ct_nw_proto,
>  };
> +} else {
> +memset(>ct_orig_tuple, 0, sizeof md->ct_orig_tuple);
>  }
>  } else {
>  memset(>ct_orig_tuple, 0, sizeof md->ct_orig_tuple);
> **
> 
> Please let me know if this fix makes sense ? Or if there is a better way to
> solve it ?

I think that is a reasonable patch.  Will you please propose it as a
formal patch?  Please include a thorough commit message.
___
discuss mailing list
disc...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-discuss

Re: [ovs-discuss] [ovs-dev] [OVN] OVN doesn't work using OVS 2.8.1 on Centos 7.3 using conntrack

[Numan Siddique]

We did some more investigation. This issue is seen only when OVN  native
dhcp is used and with kernel datapath which doesn't support
OVS_KEY_ATTR_CT_ORIG_TUPLE_IPV4.  The reason for this failure is because
ovs-vswitchd includes the attribute OVS_KEY_ATTR_CT_ORIG_TUPLE_IPV4 when it
sends the packet back to the datapath after the dhcp reply packet is
resumed.

When the dhcp packet is sent to ovn-controller, the ct_state value is set
to 0x21 and dl_type is set to 0 in the flow metadata. When the packet is
resumed, the function nxt_resume() calls 'pkt_metadata_from_flow()' which
neither sets 'md->ct_orig_tuple' or memsets it [1] because is_ct_valid()
returns true and dl_type is 0. And the function odp_key_from_dp_packet()
adds OVS_KEY_ATTR_CT_ORIG_TUPLE_IPV4 [2]

This issue is not seen in master because of this commit - "f6fabcc624
ofproto-dpif: Mark packets as "untracked" after call to ct()" [3]

This patch clears the conn track variables after the ct() action.

I suppose we cannot apply this patch to OVS 2.8 branch because it was
reverted [4] due to some issues.

I think we can solve this problem either with the below fixe or by setting
dl_type to proper value when the packet is sent to controller.

***
diff --git a/lib/flow.h b/lib/flow.h
index 6ae5a674d..076ce36f1 100644
--- a/lib/flow.h
+++ b/lib/flow.h
@@ -947,6 +947,8 @@ pkt_metadata_from_flow(struct pkt_metadata *md, const
struct flow *flow)
 flow->ct_tp_dst,
 flow->ct_nw_proto,
 };
+} else {
+memset(>ct_orig_tuple, 0, sizeof md->ct_orig_tuple);
 }
 } else {
 memset(>ct_orig_tuple, 0, sizeof md->ct_orig_tuple);
**

Please let me know if this fix makes sense ? Or if there is a better way to
solve it ?



[1] - https://github.com/openvswitch/ovs/blob/master/lib/flow.h#L933
[2] - https://github.com/openvswitch/ovs/blob/master/lib/odp-util.c#L5131
[3] -
https://github.com/openvswitch/ovs/commit/f6fabcc62458d656046c9852ee80fcff3e516e6e#diff-8bbf76f24a1b6618ed3aaaccad70a0a5
[4] - https://patchwork.ozlabs.org/patch/808397/

Thanks
Numan



On Thu, Oct 19, 2017 at 6:07 PM, Daniel Alvarez Sanchez  wrote:

> System information:
> ===
>
> OS: CentOS Linux release 7.3.1611 (Core)
> Kernel version: 3.10.0-693.2.2.el7.x86_64 #1 SMP
> OVS version: v2.8.1  (git tag)
> #ovs-vswitchd --version
> ovs-vswitchd (Open vSwitch) 2.8.1
>
> Bug description:
> 
>
> Right now, OVN doesn't work using OVS 2.8.1 on Centos 7.3 and conntrack.
> Numan Siddique and I have been doing some research on this and we have come
> up with the following conclusions:
>
> When doing a DHCP request on the mentioned system above, the kernel throws
> the following error (see Reproducer section below):
>
> netlink: Key 26 has unexpected len 16 expected 0
>
> Apparently, this commit [0], introduced that key (26
> /OVS_KEY_ATTR_CT_ORIG_TUPLE_IPV4 and looks like the OVS modules in the
> above
> kernel doesn't have that key. When ovs-vswitchd sends those extra bytes,
> the kernel
> module can't find the key and fails with the netlink error above:
>
> 2017-10-18T08:00:18Z|00444|netlink_socket|DBG|nl_sock_transact_multiple__
> (Success): nl(len:496, type=30(ovs_packet), flags=1[REQUEST], seq=6d,
> pid=5939,genl(cmd=3,version=1)
>
> However, if we run OVS master, everything works ok and ovs-vswitchd sends
> 20 bytes less (4 bytes of the header + 16 bytes of data) so it looks like
> it's adapting to the kernel datapath in some way:
>
> 2017-10-18T07:59:03Z|00391|netlink_socket|DBG|nl_sock_transact_multiple__
> (Success): nl(len:476, type=30(ovs_packet), flags=1[REQUEST], seq=32,
> pid=4294962064,genl(cmd=3,version=1)
>
> Note lengths in both cases: 496 vs 476 (working case). In the first case
> (496) the kernel throws the netlink error ("netlink: Key 26 has unexpected
> len 16 expected 0").
>
> I've checked that running an OVS version up to [1] fixes it but can't find
> the exact commit which fixes the current bug.
>
>
> [0]
> https://github.com/openvswitch/ovs/commit/c30b4ceafa235d11a1a9ded5fed11f
> ec86182ee0
> [1]
> https://github.com/openvswitch/ovs/commit/80cee1163e6301dd1c0bd01c5f0323
> fb1a45adf4
>
>
> Reproducer:
> =
>
> ovn-nbctl ls-add sw0
> ovn-nbctl lsp-add sw0 sw0-port1
> ovn-nbctl lsp-set-addresses sw0-port1 "50:54:00:00:00:01 192.168.0.2"
>
> ovn-nbctl --wait=hv acl-add sw0 from-lport 1001 'inport == "sw0-port1" &&
> ip' allow-related
> ovn-nbctl --wait=hv acl-add sw0 to-lport 1001 'outport == "sw0-port1" &&
> ip' drop
> ovn-nbctl acl-list sw0
>
>
> add_phys_port() {
> name=$1
> mac=$2
> ip=$3
> mask=$4
> gw=$5
> iface_id=$6
> ip netns add $name
> ovs-vsctl add-port br-int $name -- set interface $name type=internal
> ip link set $name netns $name
> ip netns exec $name ip link set $name address $mac
> ip netns exec $name ip addr add $ip/$mask dev