| Message ID | 1447095368-35700-1-git-send-email-joestringer@nicira.com |
|---|---|
| State | Accepted, archived |
| Headers | show |
On 9 November 2015 at 10:56, Joe Stringer <joestringer@nicira.com> wrote: > If conntrack recirculates, it should not stop processing the current > pipeline. The cloned packet will begin with a fresh action set in the > table specified with the current metadata; The current copy of the > packet will continue processing, including to return back to prior > resubmit() calls. > > Reported-by: Russell Bryant <rbryant@redhat.com> > Signed-off-by: Joe Stringer <joestringer@nicira.com> FWIW, the main change in this commit is actually to avoid the following instruction in ctx_trigger_recirculation(): ctx->exit = true; > ofproto/ofproto-dpif-xlate.c | 13 +++++++++++-- > 1 file changed, 11 insertions(+), 2 deletions(-) > > diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c > index 325e308e3340..bc21fa894682 100644 > --- a/ofproto/ofproto-dpif-xlate.c > +++ b/ofproto/ofproto-dpif-xlate.c > @@ -3591,6 +3591,16 @@ compose_recirculate_action(struct xlate_ctx *ctx) > compose_recirculate_action__(ctx, 0); > } > > +/* Fork the pipeline here. The recirculated packet will continue processing in > + * 'table' with an empty action set, and the current packet will continue > + * processing the current action list. */ > +static void > +compose_recirculate_and_fork(struct xlate_ctx *ctx, uint8_t table) > +{ > + ctx->recirc_action_offset = ctx->action_set.size; This is actually indicating that the current action set should be preserved for the recirculated packet. At minimum the documentation/commit message should be updated to reflect this. I also welcome discussion on the implications of preserving the action set across recirculation.
> On Nov 9, 2015, at 12:50 PM, Joe Stringer <joestringer@nicira.com> wrote: > > On 9 November 2015 at 10:56, Joe Stringer <joestringer@nicira.com <mailto:joestringer@nicira.com>> wrote: >> If conntrack recirculates, it should not stop processing the current >> pipeline. The cloned packet will begin with a fresh action set in the >> table specified with the current metadata; The current copy of the >> packet will continue processing, including to return back to prior >> resubmit() calls. >> >> Reported-by: Russell Bryant <rbryant@redhat.com <mailto:rbryant@redhat.com>> >> Signed-off-by: Joe Stringer <joestringer@nicira.com <mailto:joestringer@nicira.com>> > > FWIW, the main change in this commit is actually to avoid the > following instruction in ctx_trigger_recirculation(): > > ctx->exit = true; > >> ofproto/ofproto-dpif-xlate.c | 13 +++++++++++-- >> 1 file changed, 11 insertions(+), 2 deletions(-) >> >> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c >> index 325e308e3340..bc21fa894682 100644 >> --- a/ofproto/ofproto-dpif-xlate.c >> +++ b/ofproto/ofproto-dpif-xlate.c >> @@ -3591,6 +3591,16 @@ compose_recirculate_action(struct xlate_ctx *ctx) >> compose_recirculate_action__(ctx, 0); >> } >> >> +/* Fork the pipeline here. The recirculated packet will continue processing in >> + * 'table' with an empty action set, and the current packet will continue >> + * processing the current action list. */ >> +static void >> +compose_recirculate_and_fork(struct xlate_ctx *ctx, uint8_t table) >> +{ >> + ctx->recirc_action_offset = ctx->action_set.size; > > This is actually indicating that the current action set should be > preserved for the recirculated packet. At minimum the > documentation/commit message should be updated to reflect this. I also > welcome discussion on the implications of preserving the action set > across recirculation. For implicit recirculation, like for pop mpls and then goto/resubmit to another table to match the inner header, we have no other option than to preserve the action set across the recirculation, otherwise we would break behavior standardized for the action set. So the discussion here should pertain only to the explicit recirculation we do with the conntrack action, i.e., should the recirculated packet start with an empty action set, or inherit it from the recirculating flow (at the point of recirculation), as currently happens. The registers and other metadata is inherited in the same fashion, and that can be very useful in that the pipeline processing need not start from an empty state after recirculation. I would say that preserving the action set serves the same purpose, and the flows in the recirculated to table can always clear the action set (clear_actions) if that is desired. Jarno
> On Nov 9, 2015, at 10:56 AM, Joe Stringer <joestringer@nicira.com> wrote: > > If conntrack recirculates, it should not stop processing the current > pipeline. The cloned packet will begin with a fresh action set in the > table specified with the current metadata; The current copy of the As discussed, the action set is forked as well. Assuming the commit message and the comment above the new function is fixed: Acked-by: Jarno Rajahalme <jrajahalme@nicira.com> Jarno > packet will continue processing, including to return back to prior > resubmit() calls. > > Reported-by: Russell Bryant <rbryant@redhat.com> > Signed-off-by: Joe Stringer <joestringer@nicira.com> > --- > ofproto/ofproto-dpif-xlate.c | 13 +++++++++++-- > 1 file changed, 11 insertions(+), 2 deletions(-) > > diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c > index 325e308e3340..bc21fa894682 100644 > --- a/ofproto/ofproto-dpif-xlate.c > +++ b/ofproto/ofproto-dpif-xlate.c > @@ -3591,6 +3591,16 @@ compose_recirculate_action(struct xlate_ctx *ctx) > compose_recirculate_action__(ctx, 0); > } > > +/* Fork the pipeline here. The recirculated packet will continue processing in > + * 'table' with an empty action set, and the current packet will continue > + * processing the current action list. */ > +static void > +compose_recirculate_and_fork(struct xlate_ctx *ctx, uint8_t table) > +{ > + ctx->recirc_action_offset = ctx->action_set.size; > + compose_recirculate_action__(ctx, table); > +} > + > static void > compose_mpls_push_action(struct xlate_ctx *ctx, struct ofpact_push_mpls *mpls) > { > @@ -4239,8 +4249,7 @@ compose_conntrack_action(struct xlate_ctx *ctx, struct ofpact_conntrack *ofc) > } else { > /* Use ct_* fields from datapath during recirculation upcall. */ > ctx->conntracked = true; > - ctx_trigger_recirculation(ctx); > - compose_recirculate_action__(ctx, ofc->recirc_table); > + compose_recirculate_and_fork(ctx, ofc->recirc_table); > } > } > > -- > 2.1.4 >
On 9 November 2015 at 15:33, Jarno Rajahalme <jarno@ovn.org> wrote: > > On Nov 9, 2015, at 12:50 PM, Joe Stringer <joestringer@nicira.com> wrote: > > On 9 November 2015 at 10:56, Joe Stringer <joestringer@nicira.com> wrote: > > If conntrack recirculates, it should not stop processing the current > pipeline. The cloned packet will begin with a fresh action set in the > table specified with the current metadata; The current copy of the > packet will continue processing, including to return back to prior > resubmit() calls. > > Reported-by: Russell Bryant <rbryant@redhat.com> > Signed-off-by: Joe Stringer <joestringer@nicira.com> > > > FWIW, the main change in this commit is actually to avoid the > following instruction in ctx_trigger_recirculation(): > > ctx->exit = true; > > ofproto/ofproto-dpif-xlate.c | 13 +++++++++++-- > 1 file changed, 11 insertions(+), 2 deletions(-) > > diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c > index 325e308e3340..bc21fa894682 100644 > --- a/ofproto/ofproto-dpif-xlate.c > +++ b/ofproto/ofproto-dpif-xlate.c > @@ -3591,6 +3591,16 @@ compose_recirculate_action(struct xlate_ctx *ctx) > compose_recirculate_action__(ctx, 0); > } > > +/* Fork the pipeline here. The recirculated packet will continue processing > in > + * 'table' with an empty action set, and the current packet will continue > + * processing the current action list. */ > +static void > +compose_recirculate_and_fork(struct xlate_ctx *ctx, uint8_t table) > +{ > + ctx->recirc_action_offset = ctx->action_set.size; > > > This is actually indicating that the current action set should be > preserved for the recirculated packet. At minimum the > documentation/commit message should be updated to reflect this. I also > welcome discussion on the implications of preserving the action set > across recirculation. > > > For implicit recirculation, like for pop mpls and then goto/resubmit to > another table to match the inner header, we have no other option than to > preserve the action set across the recirculation, otherwise we would break > behavior standardized for the action set. So the discussion here should > pertain only to the explicit recirculation we do with the conntrack action, > i.e., should the recirculated packet start with an empty action set, or > inherit it from the recirculating flow (at the point of recirculation), as > currently happens. The registers and other metadata is inherited in the same > fashion, and that can be very useful in that the pipeline processing need > not start from an empty state after recirculation. I would say that > preserving the action set serves the same purpose, and the flows in the > recirculated to table can always clear the action set (clear_actions) if > that is desired. Thanks for the feedback, that sounds like a fair approach.
On 9 November 2015 at 15:36, Jarno Rajahalme <jarno@ovn.org> wrote: > >> On Nov 9, 2015, at 10:56 AM, Joe Stringer <joestringer@nicira.com> wrote: >> >> If conntrack recirculates, it should not stop processing the current >> pipeline. The cloned packet will begin with a fresh action set in the >> table specified with the current metadata; The current copy of the > > As discussed, the action set is forked as well. Assuming the commit message and the comment above the new function is fixed: > > Acked-by: Jarno Rajahalme <jrajahalme@nicira.com> Thanks. I'll hold off a little longer in case anyone else wants to chime in.
On 11/10/2015 04:51 PM, Joe Stringer wrote: > On 9 November 2015 at 15:36, Jarno Rajahalme <jarno@ovn.org> wrote: >> >>> On Nov 9, 2015, at 10:56 AM, Joe Stringer <joestringer@nicira.com> wrote: >>> >>> If conntrack recirculates, it should not stop processing the current >>> pipeline. The cloned packet will begin with a fresh action set in the >>> table specified with the current metadata; The current copy of the >> >> As discussed, the action set is forked as well. Assuming the commit message and the comment above the new function is fixed: >> >> Acked-by: Jarno Rajahalme <jrajahalme@nicira.com> > > Thanks. I'll hold off a little longer in case anyone else wants to chime in. > FWIW, I just tested this patch in my OpenStack+OVN environment and it resolved the problem I was having. Thanks!
On 10 November 2015 at 14:01, Russell Bryant <rbryant@redhat.com> wrote: > On 11/10/2015 04:51 PM, Joe Stringer wrote: >> On 9 November 2015 at 15:36, Jarno Rajahalme <jarno@ovn.org> wrote: >>> >>>> On Nov 9, 2015, at 10:56 AM, Joe Stringer <joestringer@nicira.com> wrote: >>>> >>>> If conntrack recirculates, it should not stop processing the current >>>> pipeline. The cloned packet will begin with a fresh action set in the >>>> table specified with the current metadata; The current copy of the >>> >>> As discussed, the action set is forked as well. Assuming the commit message and the comment above the new function is fixed: >>> >>> Acked-by: Jarno Rajahalme <jrajahalme@nicira.com> >> >> Thanks. I'll hold off a little longer in case anyone else wants to chime in. >> > > FWIW, I just tested this patch in my OpenStack+OVN environment and it > resolved the problem I was having. Thanks for the confirmation. I pushed this series to master.
> On Nov 10, 2015, at 2:01 PM, Russell Bryant <rbryant@redhat.com> wrote: > > On 11/10/2015 04:51 PM, Joe Stringer wrote: >> On 9 November 2015 at 15:36, Jarno Rajahalme <jarno@ovn.org> wrote: >>> >>>> On Nov 9, 2015, at 10:56 AM, Joe Stringer <joestringer@nicira.com> wrote: >>>> >>>> If conntrack recirculates, it should not stop processing the current >>>> pipeline. The cloned packet will begin with a fresh action set in the >>>> table specified with the current metadata; The current copy of the >>> >>> As discussed, the action set is forked as well. Assuming the commit message and the comment above the new function is fixed: >>> >>> Acked-by: Jarno Rajahalme <jrajahalme@nicira.com> >> >> Thanks. I'll hold off a little longer in case anyone else wants to chime in. >> > > FWIW, I just tested this patch in my OpenStack+OVN environment and it > resolved the problem I was having. Just to make sure, does this resolve the DHCP issue that you were seeing? Are there are other OVN ACL problems? --Justin
On Wed, Nov 11, 2015 at 1:43 PM, Justin Pettit <jpettit@ovn.org> wrote: > > > On Nov 10, 2015, at 2:01 PM, Russell Bryant <rbryant@redhat.com> wrote: > > > > On 11/10/2015 04:51 PM, Joe Stringer wrote: > >> On 9 November 2015 at 15:36, Jarno Rajahalme <jarno@ovn.org> wrote: > >>> > >>>> On Nov 9, 2015, at 10:56 AM, Joe Stringer <joestringer@nicira.com> > wrote: > >>>> > >>>> If conntrack recirculates, it should not stop processing the current > >>>> pipeline. The cloned packet will begin with a fresh action set in the > >>>> table specified with the current metadata; The current copy of the > >>> > >>> As discussed, the action set is forked as well. Assuming the commit > message and the comment above the new function is fixed: > >>> > >>> Acked-by: Jarno Rajahalme <jrajahalme@nicira.com> > >> > >> Thanks. I'll hold off a little longer in case anyone else wants to > chime in. > >> > > > > FWIW, I just tested this patch in my OpenStack+OVN environment and it > > resolved the problem I was having. > > Just to make sure, does this resolve the DHCP issue that you were seeing? > Are there are other OVN ACL problems? > It does resolve my DHCP issue, or at least part of it. This patch fixes the issue I had where the DHCP request didn't make it to the server in the first place. I have another patch to the Neutron plugin where I create an ACL that allows DHCP responses to reach the VM. It seems conntrack doesn't associate the response with the initial request, so I have to allow the responses explicitly. The logic I have is this, which is roughly: if DHCP is enabled on this network, allow DHCP server-to-client packets through that originated from an IP on the same subnet. + # If DHCP is enabled, allow the DHCP server responses to reach the + # client. + for ip in port['fixed_ips']: + subnet = self.get_subnet(context, ip['subnet_id']) + if subnet['ip_version'] != 4 or not subnet['enable_dhcp']: + continue + txn.add(self._ovn.add_acl( + lswitch=utils.ovn_name(port['network_id']), + lport=port['id'], + priority=ACL_PRIORITY_ALLOW, + action='allow', + log=False, + direction='to-lport', + match=('outport == "%s" && ip4 && ip4.src == %s && ' + 'udp && udp.src == 67 && udp.dst == 68' + ) % (port['id'], subnet['cidr']), + external_ids={'neutron:lport': port['id']})) The combination of Joe's fix and that patch makes DHCP work and seems sane to me. Let me know if you think otherwise. :-)
diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c index 325e308e3340..bc21fa894682 100644 --- a/ofproto/ofproto-dpif-xlate.c +++ b/ofproto/ofproto-dpif-xlate.c @@ -3591,6 +3591,16 @@ compose_recirculate_action(struct xlate_ctx *ctx) compose_recirculate_action__(ctx, 0); } +/* Fork the pipeline here. The recirculated packet will continue processing in + * 'table' with an empty action set, and the current packet will continue + * processing the current action list. */ +static void +compose_recirculate_and_fork(struct xlate_ctx *ctx, uint8_t table) +{ + ctx->recirc_action_offset = ctx->action_set.size; + compose_recirculate_action__(ctx, table); +} + static void compose_mpls_push_action(struct xlate_ctx *ctx, struct ofpact_push_mpls *mpls) { @@ -4239,8 +4249,7 @@ compose_conntrack_action(struct xlate_ctx *ctx, struct ofpact_conntrack *ofc) } else { /* Use ct_* fields from datapath during recirculation upcall. */ ctx->conntracked = true; - ctx_trigger_recirculation(ctx); - compose_recirculate_action__(ctx, ofc->recirc_table); + compose_recirculate_and_fork(ctx, ofc->recirc_table); } }
If conntrack recirculates, it should not stop processing the current pipeline. The cloned packet will begin with a fresh action set in the table specified with the current metadata; The current copy of the packet will continue processing, including to return back to prior resubmit() calls. Reported-by: Russell Bryant <rbryant@redhat.com> Signed-off-by: Joe Stringer <joestringer@nicira.com> --- ofproto/ofproto-dpif-xlate.c | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-)