diff mbox series

[ovs-dev] northd: do not centralize FIP traffic if redirect-type is set to fixed

Message ID 308a75da796460cc3905e02387c46d57c4c4f529.1663593740.git.lorenzo.bianconi@redhat.com
State Changes Requested
Headers show
Series [ovs-dev] northd: do not centralize FIP traffic if redirect-type is set to fixed | expand

Checks

Context Check Description
ovsrobot/apply-robot success apply and check: success
ovsrobot/github-robot-_Build_and_Test success github build: passed
ovsrobot/github-robot-_ovn-kubernetes success github build: passed

Commit Message

Lorenzo Bianconi Sept. 19, 2022, 1:22 p.m. UTC 
Keep FIP traffic distributed and do not centralize it even if the
CMS sets redirect-type option to bridged for distributed gateway port.

Tested-by: Jianlin Shi <jishi@redhat.com>
Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2007120
Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
---
 northd/northd.c         | 29 +++++++++++++++++++++++++++++
 northd/northd.h         |  2 ++
 northd/ovn-northd.8.xml | 27 +++++++++++++++++++++++++++
 3 files changed, 58 insertions(+)

Comments

Mark Michelson Sept. 20, 2022, 8:05 p.m. UTC | #1 
Hi Lorenzo,

I have a couple of comments below. In addition, please add a test for 
this scenario to the testsuite.

On 9/19/22 09:22, Lorenzo Bianconi wrote:
> Keep FIP traffic distributed and do not centralize it even if the
> CMS sets redirect-type option to bridged for distributed gateway port.
> 
> Tested-by: Jianlin Shi <jishi@redhat.com>
> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2007120
> Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
> ---
>   northd/northd.c         | 29 +++++++++++++++++++++++++++++
>   northd/northd.h         |  2 ++
>   northd/ovn-northd.8.xml | 27 +++++++++++++++++++++++++++
>   3 files changed, 58 insertions(+)
> 
> diff --git a/northd/northd.c b/northd/northd.c
> index 84440a47f..5c1ddf5c2 100644
> --- a/northd/northd.c
> +++ b/northd/northd.c
> @@ -2616,6 +2616,13 @@ join_logical_ports(struct northd_input *input_data,
>                   op->od = od;
>                   ovs_list_push_back(&od->port_list, &op->dp_node);
>   
> +                const char *redirect_type = smap_get(&nbrp->options,
> +                                                     "redirect-type");
> +                if (!od->redirect_bridged && redirect_type &&
> +                    !strcasecmp(redirect_type, "bridged")) {
> +                    od->redirect_bridged = true;
> +                }

Nit-picky optimization: Only call smap_get() if od->redirect_bridge is 
false.

> +
>                   if (op->nbrp->ha_chassis_group ||
>                       op->nbrp->n_gateway_chassis) {
>                       /* Additional "derived" ovn_port crp represents the
> @@ -13731,6 +13738,28 @@ build_lrouter_nat_defrag_and_lb(struct ovn_datapath *od, struct hmap *lflows,
>                                           100, ds_cstr(match),
>                                           ds_cstr(actions),
>                                           &nat->header_);
> +                if (od->redirect_bridged && distributed) {
> +                    ds_clear(match);
> +                    ds_put_format(
> +                            match,
> +                            "outport == %s && ip%s.src == %s "
> +                            "&& is_chassis_resident(\"%s\")",
> +                            od->l3dgw_ports[0]->json_key,
> +                            is_v6 ? "6" : "4", nat->logical_ip,
> +                            nat->logical_port);
> +                    ds_clear(actions);
> +                    if (is_v6) {
> +                        ds_put_cstr(actions,
> +                            "get_arp(outport, " REG_NEXT_HOP_IPV4 "); next;");
> +                    } else {
> +                        ds_put_cstr(actions,
> +                            "get_nd(outport, " REG_NEXT_HOP_IPV6 "); next;");
> +                    }

It looks like the logic of the if-else statement above is reversed. 
Shouldn't the is_v6 case use get_nd() instead of get_arp()?

> +                    ovn_lflow_add_with_hint(lflows, od,
> +                                            S_ROUTER_IN_ARP_RESOLVE, 90,
> +                                            ds_cstr(match), ds_cstr(actions),
> +                                            &nat->header_);
> +                }
>                   sset_add(&nat_entries, nat->external_ip);
>               }
>           }
> diff --git a/northd/northd.h b/northd/northd.h
> index aa9a3ae6e..60601803f 100644
> --- a/northd/northd.h
> +++ b/northd/northd.h
> @@ -229,6 +229,8 @@ struct ovn_datapath {
>       size_t n_nat_entries;
>   
>       bool has_distributed_nat;
> +    /* router datapath has a logical port with redirect-type set to bridged. */
> +    bool redirect_bridged;
>   
>       /* Set of nat external ips on the router. */
>       struct sset external_ips;
> diff --git a/northd/ovn-northd.8.xml b/northd/ovn-northd.8.xml
> index dae961c87..1d5a46a0d 100644
> --- a/northd/ovn-northd.8.xml
> +++ b/northd/ovn-northd.8.xml
> @@ -4053,6 +4053,33 @@ outport = <var>P</var>
>           </p>
>         </li>
>   
> +      <li>
> +        <p>
> +          If the router datapath runs a port with <code>redirect-type</code>
> +          set to <code>bridged</code>, for each distributed NAT rule with IP
> +          <var>A</var> in the
> +          <ref column="logical_ip" table="NAT" db="OVN_Northbound"/> column
> +          and logical port <var>P</var> in the
> +          <ref column="logical_port" table="NAT" db="OVN_Northbound"/> column
> +          of <ref table="NAT" db="OVN_Northbound"/> table, a priority-90 flow
> +          with the match <code>outport === <var>Q</var> &amp;&amp; ip.src ===

Shouldn't "===" be "=="?

> +          <var>A</var> &amp;&amp; is_chassis_resident(<var>P</var>)</code>,
> +          where <code>Q</code> is the distributed logical router port and
> +          action <code>get_arp(outport, reg0); next;</code> for IPv4 and
> +          <code>get_nd(outport, xxreg0); next;</code> for IPv6.
> +        </p>
> +
> +        <p>
> +          A priority-0 logical flow with match <code>ip4</code> has actions
> +          <code>get_arp(outport, reg0); next;</code>.
> +        </p>
> +
> +        <p>
> +          A priority-0 logical flow with match <code>ip6</code> has actions
> +          <code>get_nd(outport, xxreg0); next;</code>.
> +        </p>

I think you can remove the mention of the priority-0 flows. The patch 
isn't adding new priority-0 flows relating to the bridged redirect-type, 
and the pre-existing priority-0 flows are already documented in the 
"Dynamic MAC bindings" section.

> +      </li>
> +
>         <li>
>           <p>
>             Traffic with IP destination an address owned by the router should be
Lorenzo Bianconi Sept. 28, 2022, 10:18 a.m. UTC | #2 
On Sep 20, Mark Michelson wrote:
> Hi Lorenzo,

Hi Mark,

thx for reviewing the patch.

> 
> I have a couple of comments below. In addition, please add a test for this
> scenario to the testsuite.
> 
> On 9/19/22 09:22, Lorenzo Bianconi wrote:
> > Keep FIP traffic distributed and do not centralize it even if the
> > CMS sets redirect-type option to bridged for distributed gateway port.
> > 
> > Tested-by: Jianlin Shi <jishi@redhat.com>
> > Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2007120
> > Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
> > ---
> >   northd/northd.c         | 29 +++++++++++++++++++++++++++++
> >   northd/northd.h         |  2 ++
> >   northd/ovn-northd.8.xml | 27 +++++++++++++++++++++++++++
> >   3 files changed, 58 insertions(+)
> > 
> > diff --git a/northd/northd.c b/northd/northd.c
> > index 84440a47f..5c1ddf5c2 100644
> > --- a/northd/northd.c
> > +++ b/northd/northd.c
> > @@ -2616,6 +2616,13 @@ join_logical_ports(struct northd_input *input_data,
> >                   op->od = od;
> >                   ovs_list_push_back(&od->port_list, &op->dp_node);
> > +                const char *redirect_type = smap_get(&nbrp->options,
> > +                                                     "redirect-type");
> > +                if (!od->redirect_bridged && redirect_type &&
> > +                    !strcasecmp(redirect_type, "bridged")) {
> > +                    od->redirect_bridged = true;
> > +                }
> 
> Nit-picky optimization: Only call smap_get() if od->redirect_bridge is
> false.

ack, I will fix it in v2.

> 
> > +
> >                   if (op->nbrp->ha_chassis_group ||
> >                       op->nbrp->n_gateway_chassis) {
> >                       /* Additional "derived" ovn_port crp represents the
> > @@ -13731,6 +13738,28 @@ build_lrouter_nat_defrag_and_lb(struct ovn_datapath *od, struct hmap *lflows,
> >                                           100, ds_cstr(match),
> >                                           ds_cstr(actions),
> >                                           &nat->header_);
> > +                if (od->redirect_bridged && distributed) {
> > +                    ds_clear(match);
> > +                    ds_put_format(
> > +                            match,
> > +                            "outport == %s && ip%s.src == %s "
> > +                            "&& is_chassis_resident(\"%s\")",
> > +                            od->l3dgw_ports[0]->json_key,
> > +                            is_v6 ? "6" : "4", nat->logical_ip,
> > +                            nat->logical_port);
> > +                    ds_clear(actions);
> > +                    if (is_v6) {
> > +                        ds_put_cstr(actions,
> > +                            "get_arp(outport, " REG_NEXT_HOP_IPV4 "); next;");
> > +                    } else {
> > +                        ds_put_cstr(actions,
> > +                            "get_nd(outport, " REG_NEXT_HOP_IPV6 "); next;");
> > +                    }
> 
> It looks like the logic of the if-else statement above is reversed.
> Shouldn't the is_v6 case use get_nd() instead of get_arp()?

ack, I will fix it in v2.

> 
> > +                    ovn_lflow_add_with_hint(lflows, od,
> > +                                            S_ROUTER_IN_ARP_RESOLVE, 90,
> > +                                            ds_cstr(match), ds_cstr(actions),
> > +                                            &nat->header_);
> > +                }
> >                   sset_add(&nat_entries, nat->external_ip);
> >               }
> >           }
> > diff --git a/northd/northd.h b/northd/northd.h
> > index aa9a3ae6e..60601803f 100644
> > --- a/northd/northd.h
> > +++ b/northd/northd.h
> > @@ -229,6 +229,8 @@ struct ovn_datapath {
> >       size_t n_nat_entries;
> >       bool has_distributed_nat;
> > +    /* router datapath has a logical port with redirect-type set to bridged. */
> > +    bool redirect_bridged;
> >       /* Set of nat external ips on the router. */
> >       struct sset external_ips;
> > diff --git a/northd/ovn-northd.8.xml b/northd/ovn-northd.8.xml
> > index dae961c87..1d5a46a0d 100644
> > --- a/northd/ovn-northd.8.xml
> > +++ b/northd/ovn-northd.8.xml
> > @@ -4053,6 +4053,33 @@ outport = <var>P</var>
> >           </p>
> >         </li>
> > +      <li>
> > +        <p>
> > +          If the router datapath runs a port with <code>redirect-type</code>
> > +          set to <code>bridged</code>, for each distributed NAT rule with IP
> > +          <var>A</var> in the
> > +          <ref column="logical_ip" table="NAT" db="OVN_Northbound"/> column
> > +          and logical port <var>P</var> in the
> > +          <ref column="logical_port" table="NAT" db="OVN_Northbound"/> column
> > +          of <ref table="NAT" db="OVN_Northbound"/> table, a priority-90 flow
> > +          with the match <code>outport === <var>Q</var> &amp;&amp; ip.src ===
> 
> Shouldn't "===" be "=="?

ack, I will fix it in v2.
> 
> > +          <var>A</var> &amp;&amp; is_chassis_resident(<var>P</var>)</code>,
> > +          where <code>Q</code> is the distributed logical router port and
> > +          action <code>get_arp(outport, reg0); next;</code> for IPv4 and
> > +          <code>get_nd(outport, xxreg0); next;</code> for IPv6.
> > +        </p>
> > +
> > +        <p>
> > +          A priority-0 logical flow with match <code>ip4</code> has actions
> > +          <code>get_arp(outport, reg0); next;</code>.
> > +        </p>
> > +
> > +        <p>
> > +          A priority-0 logical flow with match <code>ip6</code> has actions
> > +          <code>get_nd(outport, xxreg0); next;</code>.
> > +        </p>
> 
> I think you can remove the mention of the priority-0 flows. The patch isn't
> adding new priority-0 flows relating to the bridged redirect-type, and the
> pre-existing priority-0 flows are already documented in the "Dynamic MAC
> bindings" section.

ack, I will fix it in v2.

Regards,
Lorenzo

> 
> > +      </li>
> > +
> >         <li>
> >           <p>
> >             Traffic with IP destination an address owned by the router should be
>
diff mbox series

Patch

diff --git a/northd/northd.c b/northd/northd.c
index 84440a47f..5c1ddf5c2 100644
--- a/northd/northd.c
+++ b/northd/northd.c
@@ -2616,6 +2616,13 @@  join_logical_ports(struct northd_input *input_data,
                 op->od = od;
                 ovs_list_push_back(&od->port_list, &op->dp_node);
 
+                const char *redirect_type = smap_get(&nbrp->options,
+                                                     "redirect-type");
+                if (!od->redirect_bridged && redirect_type &&
+                    !strcasecmp(redirect_type, "bridged")) {
+                    od->redirect_bridged = true;
+                }
+
                 if (op->nbrp->ha_chassis_group ||
                     op->nbrp->n_gateway_chassis) {
                     /* Additional "derived" ovn_port crp represents the
@@ -13731,6 +13738,28 @@  build_lrouter_nat_defrag_and_lb(struct ovn_datapath *od, struct hmap *lflows,
                                         100, ds_cstr(match),
                                         ds_cstr(actions),
                                         &nat->header_);
+                if (od->redirect_bridged && distributed) {
+                    ds_clear(match);
+                    ds_put_format(
+                            match,
+                            "outport == %s && ip%s.src == %s "
+                            "&& is_chassis_resident(\"%s\")",
+                            od->l3dgw_ports[0]->json_key,
+                            is_v6 ? "6" : "4", nat->logical_ip,
+                            nat->logical_port);
+                    ds_clear(actions);
+                    if (is_v6) {
+                        ds_put_cstr(actions,
+                            "get_arp(outport, " REG_NEXT_HOP_IPV4 "); next;");
+                    } else {
+                        ds_put_cstr(actions,
+                            "get_nd(outport, " REG_NEXT_HOP_IPV6 "); next;");
+                    }
+                    ovn_lflow_add_with_hint(lflows, od,
+                                            S_ROUTER_IN_ARP_RESOLVE, 90,
+                                            ds_cstr(match), ds_cstr(actions),
+                                            &nat->header_);
+                }
                 sset_add(&nat_entries, nat->external_ip);
             }
         }
diff --git a/northd/northd.h b/northd/northd.h
index aa9a3ae6e..60601803f 100644
--- a/northd/northd.h
+++ b/northd/northd.h
@@ -229,6 +229,8 @@  struct ovn_datapath {
     size_t n_nat_entries;
 
     bool has_distributed_nat;
+    /* router datapath has a logical port with redirect-type set to bridged. */
+    bool redirect_bridged;
 
     /* Set of nat external ips on the router. */
     struct sset external_ips;
diff --git a/northd/ovn-northd.8.xml b/northd/ovn-northd.8.xml
index dae961c87..1d5a46a0d 100644
--- a/northd/ovn-northd.8.xml
+++ b/northd/ovn-northd.8.xml
@@ -4053,6 +4053,33 @@  outport = <var>P</var>
         </p>
       </li>
 
+      <li>
+        <p>
+          If the router datapath runs a port with <code>redirect-type</code>
+          set to <code>bridged</code>, for each distributed NAT rule with IP
+          <var>A</var> in the
+          <ref column="logical_ip" table="NAT" db="OVN_Northbound"/> column
+          and logical port <var>P</var> in the
+          <ref column="logical_port" table="NAT" db="OVN_Northbound"/> column
+          of <ref table="NAT" db="OVN_Northbound"/> table, a priority-90 flow
+          with the match <code>outport === <var>Q</var> &amp;&amp; ip.src ===
+          <var>A</var> &amp;&amp; is_chassis_resident(<var>P</var>)</code>,
+          where <code>Q</code> is the distributed logical router port and
+          action <code>get_arp(outport, reg0); next;</code> for IPv4 and
+          <code>get_nd(outport, xxreg0); next;</code> for IPv6.
+        </p>
+
+        <p>
+          A priority-0 logical flow with match <code>ip4</code> has actions
+          <code>get_arp(outport, reg0); next;</code>.
+        </p>
+
+        <p>
+          A priority-0 logical flow with match <code>ip6</code> has actions
+          <code>get_nd(outport, xxreg0); next;</code>.
+        </p>
+      </li>
+
       <li>
         <p>
           Traffic with IP destination an address owned by the router should be