[ovs-dev] [ovs-discuss] [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(&md->ct_orig_tuple, 0, sizeof md->ct_orig_tuple);
>          }
>      } else {
>          memset(&md->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.