| Message ID | 1482149537-20962-1-git-send-email-guru@ovn.org |
|---|---|
| State | Accepted |
| Headers | show |
On Mon, Dec 19, 2016 at 04:12:17AM -0800, Gurucharan Shetty wrote: > The gateway router was using the ct_next action to > reassemble packets. But ct_next action by default would > use the zone allocated for a logical port and in case of > gateway routers that value was zero. This would make > the flow use the default zone of zero. This had some > unintended consequences as the zone used to track packets > and the zone used to eventually commit it (DNAT zone) > was different. As a result, a packet would never have ct.est set. > > With this commit, when ct_next action is used in a gateway > router, we use the DNAT zone. This is similar to the > strategy used in commit c2e954a117a8 (ovn-controller: Datapath > based conntrack zone for load-balancing.) > > Signed-off-by: Gurucharan Shetty <guru@ovn.org> Seems reasonable. > - ct->zone_src.field = mf_from_id(MFF_LOG_CT_ZONE); > + ct->zone_src.field = ep->is_switch ? mf_from_id(MFF_LOG_CT_ZONE) > + : mf_from_id(MFF_LOG_DNAT_ZONE); You could put the ?: inside the mf_from_id(). Acked-by: Ben Pfaff <blp@ovn.org>
On 19 December 2016 at 21:26, Ben Pfaff <blp@ovn.org> wrote: > On Mon, Dec 19, 2016 at 04:12:17AM -0800, Gurucharan Shetty wrote: > > The gateway router was using the ct_next action to > > reassemble packets. But ct_next action by default would > > use the zone allocated for a logical port and in case of > > gateway routers that value was zero. This would make > > the flow use the default zone of zero. This had some > > unintended consequences as the zone used to track packets > > and the zone used to eventually commit it (DNAT zone) > > was different. As a result, a packet would never have ct.est set. > > > > With this commit, when ct_next action is used in a gateway > > router, we use the DNAT zone. This is similar to the > > strategy used in commit c2e954a117a8 (ovn-controller: Datapath > > based conntrack zone for load-balancing.) > > > > Signed-off-by: Gurucharan Shetty <guru@ovn.org> > > Seems reasonable. > > > - ct->zone_src.field = mf_from_id(MFF_LOG_CT_ZONE); > > + ct->zone_src.field = ep->is_switch ? mf_from_id(MFF_LOG_CT_ZONE) > > + : mf_from_id(MFF_LOG_DNAT_ZONE); > > You could put the ?: inside the mf_from_id(). > > There were a few other places using the above style. So I let it be (I hope there was no inherent advantage with one style over the other here?) > Acked-by: Ben Pfaff <blp@ovn.org> > Thanks, I applied this!
On Tue, Dec 20, 2016 at 08:21:28AM -0800, Guru Shetty wrote: > On 19 December 2016 at 21:26, Ben Pfaff <blp@ovn.org> wrote: > > > - ct->zone_src.field = mf_from_id(MFF_LOG_CT_ZONE); > > > + ct->zone_src.field = ep->is_switch ? mf_from_id(MFF_LOG_CT_ZONE) > > > + : mf_from_id(MFF_LOG_DNAT_ZONE); > > > > You could put the ?: inside the mf_from_id(). > > > There were a few other places using the above style. So I let it be (I > hope there was no inherent advantage with one style over the other here?) The two forms are only syntactically different.
diff --git a/ovn/lib/actions.c b/ovn/lib/actions.c index fa8f175..686ecc5 100644 --- a/ovn/lib/actions.c +++ b/ovn/lib/actions.c @@ -551,7 +551,8 @@ encode_CT_NEXT(const struct ovnact_next *next, { struct ofpact_conntrack *ct = ofpact_put_CT(ofpacts); ct->recirc_table = ep->first_ptable + next->ltable; - ct->zone_src.field = mf_from_id(MFF_LOG_CT_ZONE); + ct->zone_src.field = ep->is_switch ? mf_from_id(MFF_LOG_CT_ZONE) + : mf_from_id(MFF_LOG_DNAT_ZONE); ct->zone_src.ofs = 0; ct->zone_src.n_bits = 16; ofpact_finish(ofpacts, &ct->ofpact); diff --git a/ovn/ovn-sb.xml b/ovn/ovn-sb.xml index 65191ed..6daa8aa 100644 --- a/ovn/ovn-sb.xml +++ b/ovn/ovn-sb.xml @@ -1037,9 +1037,11 @@ As a side effect, IP fragments will be reassembled for matching. If a fragmented packet is output, then it will be sent with any overlapping fragments squashed. The connection tracking state is - scoped by the logical port, so overlapping addresses may be used. - To allow traffic related to the matched flow, execute - <code>ct_commit</code>. + scoped by the logical port when the action is used in a flow for + a logical switch, so overlapping addresses may be used. To allow + traffic related to the matched flow, execute <code>ct_commit + </code>. Connection tracking state is scoped by the logical + topology when the action is used in a flow for a router. </p> <p>
The gateway router was using the ct_next action to reassemble packets. But ct_next action by default would use the zone allocated for a logical port and in case of gateway routers that value was zero. This would make the flow use the default zone of zero. This had some unintended consequences as the zone used to track packets and the zone used to eventually commit it (DNAT zone) was different. As a result, a packet would never have ct.est set. With this commit, when ct_next action is used in a gateway router, we use the DNAT zone. This is similar to the strategy used in commit c2e954a117a8 (ovn-controller: Datapath based conntrack zone for load-balancing.) Signed-off-by: Gurucharan Shetty <guru@ovn.org> --- ovn/lib/actions.c | 3 ++- ovn/ovn-sb.xml | 8 +++++--- 2 files changed, 7 insertions(+), 4 deletions(-)