| Message ID | 20230519181859.1195040-2-odivlad@gmail.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | VTEP lport ARP handling fixes & GARP support | expand |
| 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 | fail | github build: failed |
On 5/19/23 20:18, Vladislav Odintsov wrote: > This patch fixes a situation, where logical flow with incorrect syntax could > be generated. If a logical switch has two attached logical router ports and > one of them has configured gateway chassis, then incorrect flow can have the > match like: > `reg0[14] == 1 && (is_chassis_resident("cr-lrp2") || ` or > `is_chassis_resident("cr-lrp1"))` > > The flow's match was reworked to have at maximum one 'is_chassis_resident()' > part. For each cr-lport a new lflow is created. There should not be many > cr-lports within one datapath (normally there is just one), so the lflows > count shouldn't increase dramatically. > > Now the match looks like: > `reg0[14] == 1 && is_chassis_resident("cr-lrp2")` > > As an additional enhancement, the code became easier and tests were also > simplified. > > Documentation and relevant testcases were updated. > > Fixes: 4e90bcf55c2e ("controller, northd, vtep: support routed networks with HW VTEP") > Signed-off-by: Vladislav Odintsov <odivlad@gmail.com> > --- > northd/northd.c | 35 ++++++++++++++--------------------- > northd/ovn-northd.8.xml | 13 +++++++------ > tests/ovn.at | 17 +++-------------- > 3 files changed, 24 insertions(+), 41 deletions(-) > > diff --git a/northd/northd.c b/northd/northd.c > index 07b127cdf..d6c26735d 100644 > --- a/northd/northd.c > +++ b/northd/northd.c > @@ -7819,37 +7819,30 @@ static void > build_vtep_hairpin(struct ovn_datapath *od, struct hmap *lflows) > { > /* Ingress Pre-ARP flows for VTEP hairpining traffic. Priority 1000: > - * Packets that received from non-VTEP ports should continue processing. */ > - > + * Packets that received from VTEP ports must go directly to L2LKP table. > + */ I changed this to: "Packets received from VTEP ports must go directly to L2LKP table." and applied it to main and backported to all stable branches down to 22.06. Regards, Dumitru > char *action = xasprintf("next(pipeline=ingress, table=%d);", > ovn_stage_get_table(S_SWITCH_IN_L2_LKUP)); > - /* send all traffic from VTEP directly to L2LKP table. */ > ovn_lflow_add(lflows, od, S_SWITCH_IN_HAIRPIN, 1000, > REGBIT_FROM_RAMP" == 1", action); > free(action); > > - struct ds match = DS_EMPTY_INITIALIZER; > - size_t n_ports = od->n_router_ports; > - bool dp_has_l3dgw_ports = false; > - for (int i = 0; i < n_ports; i++) { > - if (is_l3dgw_port(od->router_ports[i]->peer)) { > - ds_put_format(&match, "%sis_chassis_resident(%s)%s", > - i == 0 ? REGBIT_FROM_RAMP" == 1 && (" : "", > - od->router_ports[i]->peer->cr_port->json_key, > - i < n_ports - 1 ? " || " : ")"); > - dp_has_l3dgw_ports = true; > - } > - } > - > /* Ingress pre-arp flow for traffic from VTEP (ramp) switch. > * Priority 2000: Packets, that were received from VTEP (ramp) switch and > * router ports of current datapath are l3dgw ports and they reside on > * current chassis, should be passed to next table for ARP/ND hairpin > - * processing. > - */ > - if (dp_has_l3dgw_ports) { > - ovn_lflow_add(lflows, od, S_SWITCH_IN_HAIRPIN, 2000, ds_cstr(&match), > - "next;"); > + * processing. */ > + struct ds match = DS_EMPTY_INITIALIZER; > + for (int i = 0; i < od->n_router_ports; i++) { > + struct ovn_port *op = od->router_ports[i]->peer; > + if (is_l3dgw_port(op)) { > + ds_clear(&match); > + ds_put_format(&match, > + REGBIT_FROM_RAMP" == 1 && is_chassis_resident(%s)", > + op->cr_port->json_key); > + ovn_lflow_add(lflows, od, S_SWITCH_IN_HAIRPIN, 2000, > + ds_cstr(&match), "next;"); > + } > } > ds_destroy(&match); > } > diff --git a/northd/ovn-northd.8.xml b/northd/ovn-northd.8.xml > index 540fe03bd..a8ef00a28 100644 > --- a/northd/ovn-northd.8.xml > +++ b/northd/ovn-northd.8.xml > @@ -1144,16 +1144,17 @@ > <li> > <p> > For each distributed gateway router port <var>RP</var> attached to > - the logical switch, a priority-2000 flow is added with the match > - <code>reg0[14] == 1 && is_chassis_resident(<var>RP</var>) > - </code> and action <code>next;</code> to pass the traffic to the > - next table to respond to the ARP requests for the router port IPs. > + the logical switch and has chassis redirect port <var>cr-RP</var>, a > + priority-2000 flow is added with the match > + <pre> > +<code>reg0[14] == 1 && is_chassis_resident(<var>cr-RP</var>)</code> > + </pre> > + and action <code>next;</code>. > </p> > > <p> > <code>reg0[14]</code> register bit is set in the ingress L2 port > - security check table for traffic received from HW VTEP (ramp) > - ports. > + security check table for traffic received from HW VTEP (ramp) ports. > </p> > </li> > > diff --git a/tests/ovn.at b/tests/ovn.at > index 9e6e8a14a..53349530b 100644 > --- a/tests/ovn.at > +++ b/tests/ovn.at > @@ -4432,24 +4432,13 @@ response=${sha}${lrpmac}08060001080006040002${lrpmac}${tpa}${sha}${spa} > echo $response >> 3.expected > > # First ensure basic flow contents are as we expect. > -AT_CHECK([ovn-sbctl lflow-list lsw0 | grep 'reg0[\[14\]]' | sort | sed 's/table=../table=??/g' | sed 's/is_chassis_resident([[^)]]*)/is_chassis_resident("??")/g'], [0], [dnl > +AT_CHECK([ovn-sbctl lflow-list lsw0 | grep 'reg0[\[14\]]' | sort | sed 's/table=../table=??/g'], [0], [dnl > table=??(ls_in_check_port_sec), priority=70 , match=(inport == "lp-vtep"), action=(reg0[[14]] = 1; next(pipeline=ingress, table=??);) > table=??(ls_in_hairpin ), priority=1000 , match=(reg0[[14]] == 1), action=(next(pipeline=ingress, table=??);) > - table=??(ls_in_hairpin ), priority=2000 , match=(reg0[[14]] == 1 && (is_chassis_resident("??") || is_chassis_resident("??"))), action=(next;) > + table=??(ls_in_hairpin ), priority=2000 , match=(reg0[[14]] == 1 && is_chassis_resident("cr-lrp1")), action=(next;) > + table=??(ls_in_hairpin ), priority=2000 , match=(reg0[[14]] == 1 && is_chassis_resident("cr-lrp2")), action=(next;) > ]) > > -# We've ensured that the expected hairpin flows are present > -# and that the expected number of "is_chassis_resident" fields are in > -# the flow. Now we need to ensure the contents are correct. > -# Unfortunately, the order of the "is_chassis_resident" fields is > -# unpredictable. Therefore we sort them so the order is predictable. > -actual_chassis=$(ovn-sbctl lflow-list lsw0 | grep 'ls_in_hairpin' | grep 'priority=2000' | grep -o 'is_chassis_resident([[^)]]*)' | sort) > - > -expected_chassis='is_chassis_resident("cr-lrp1") > -is_chassis_resident("cr-lrp2")' > - > -check test "$expected_chassis" = "$actual_chassis" > - > # dump information with counters > echo "------ OVN dump ------" > ovn-nbctl show
diff --git a/northd/northd.c b/northd/northd.c index 07b127cdf..d6c26735d 100644 --- a/northd/northd.c +++ b/northd/northd.c @@ -7819,37 +7819,30 @@ static void build_vtep_hairpin(struct ovn_datapath *od, struct hmap *lflows) { /* Ingress Pre-ARP flows for VTEP hairpining traffic. Priority 1000: - * Packets that received from non-VTEP ports should continue processing. */ - + * Packets that received from VTEP ports must go directly to L2LKP table. + */ char *action = xasprintf("next(pipeline=ingress, table=%d);", ovn_stage_get_table(S_SWITCH_IN_L2_LKUP)); - /* send all traffic from VTEP directly to L2LKP table. */ ovn_lflow_add(lflows, od, S_SWITCH_IN_HAIRPIN, 1000, REGBIT_FROM_RAMP" == 1", action); free(action); - struct ds match = DS_EMPTY_INITIALIZER; - size_t n_ports = od->n_router_ports; - bool dp_has_l3dgw_ports = false; - for (int i = 0; i < n_ports; i++) { - if (is_l3dgw_port(od->router_ports[i]->peer)) { - ds_put_format(&match, "%sis_chassis_resident(%s)%s", - i == 0 ? REGBIT_FROM_RAMP" == 1 && (" : "", - od->router_ports[i]->peer->cr_port->json_key, - i < n_ports - 1 ? " || " : ")"); - dp_has_l3dgw_ports = true; - } - } - /* Ingress pre-arp flow for traffic from VTEP (ramp) switch. * Priority 2000: Packets, that were received from VTEP (ramp) switch and * router ports of current datapath are l3dgw ports and they reside on * current chassis, should be passed to next table for ARP/ND hairpin - * processing. - */ - if (dp_has_l3dgw_ports) { - ovn_lflow_add(lflows, od, S_SWITCH_IN_HAIRPIN, 2000, ds_cstr(&match), - "next;"); + * processing. */ + struct ds match = DS_EMPTY_INITIALIZER; + for (int i = 0; i < od->n_router_ports; i++) { + struct ovn_port *op = od->router_ports[i]->peer; + if (is_l3dgw_port(op)) { + ds_clear(&match); + ds_put_format(&match, + REGBIT_FROM_RAMP" == 1 && is_chassis_resident(%s)", + op->cr_port->json_key); + ovn_lflow_add(lflows, od, S_SWITCH_IN_HAIRPIN, 2000, + ds_cstr(&match), "next;"); + } } ds_destroy(&match); } diff --git a/northd/ovn-northd.8.xml b/northd/ovn-northd.8.xml index 540fe03bd..a8ef00a28 100644 --- a/northd/ovn-northd.8.xml +++ b/northd/ovn-northd.8.xml @@ -1144,16 +1144,17 @@ <li> <p> For each distributed gateway router port <var>RP</var> attached to - the logical switch, a priority-2000 flow is added with the match - <code>reg0[14] == 1 && is_chassis_resident(<var>RP</var>) - </code> and action <code>next;</code> to pass the traffic to the - next table to respond to the ARP requests for the router port IPs. + the logical switch and has chassis redirect port <var>cr-RP</var>, a + priority-2000 flow is added with the match + <pre> +<code>reg0[14] == 1 && is_chassis_resident(<var>cr-RP</var>)</code> + </pre> + and action <code>next;</code>. </p> <p> <code>reg0[14]</code> register bit is set in the ingress L2 port - security check table for traffic received from HW VTEP (ramp) - ports. + security check table for traffic received from HW VTEP (ramp) ports. </p> </li> diff --git a/tests/ovn.at b/tests/ovn.at index 9e6e8a14a..53349530b 100644 --- a/tests/ovn.at +++ b/tests/ovn.at @@ -4432,24 +4432,13 @@ response=${sha}${lrpmac}08060001080006040002${lrpmac}${tpa}${sha}${spa} echo $response >> 3.expected # First ensure basic flow contents are as we expect. -AT_CHECK([ovn-sbctl lflow-list lsw0 | grep 'reg0[\[14\]]' | sort | sed 's/table=../table=??/g' | sed 's/is_chassis_resident([[^)]]*)/is_chassis_resident("??")/g'], [0], [dnl +AT_CHECK([ovn-sbctl lflow-list lsw0 | grep 'reg0[\[14\]]' | sort | sed 's/table=../table=??/g'], [0], [dnl table=??(ls_in_check_port_sec), priority=70 , match=(inport == "lp-vtep"), action=(reg0[[14]] = 1; next(pipeline=ingress, table=??);) table=??(ls_in_hairpin ), priority=1000 , match=(reg0[[14]] == 1), action=(next(pipeline=ingress, table=??);) - table=??(ls_in_hairpin ), priority=2000 , match=(reg0[[14]] == 1 && (is_chassis_resident("??") || is_chassis_resident("??"))), action=(next;) + table=??(ls_in_hairpin ), priority=2000 , match=(reg0[[14]] == 1 && is_chassis_resident("cr-lrp1")), action=(next;) + table=??(ls_in_hairpin ), priority=2000 , match=(reg0[[14]] == 1 && is_chassis_resident("cr-lrp2")), action=(next;) ]) -# We've ensured that the expected hairpin flows are present -# and that the expected number of "is_chassis_resident" fields are in -# the flow. Now we need to ensure the contents are correct. -# Unfortunately, the order of the "is_chassis_resident" fields is -# unpredictable. Therefore we sort them so the order is predictable. -actual_chassis=$(ovn-sbctl lflow-list lsw0 | grep 'ls_in_hairpin' | grep 'priority=2000' | grep -o 'is_chassis_resident([[^)]]*)' | sort) - -expected_chassis='is_chassis_resident("cr-lrp1") -is_chassis_resident("cr-lrp2")' - -check test "$expected_chassis" = "$actual_chassis" - # dump information with counters echo "------ OVN dump ------" ovn-nbctl show
This patch fixes a situation, where logical flow with incorrect syntax could be generated. If a logical switch has two attached logical router ports and one of them has configured gateway chassis, then incorrect flow can have the match like: `reg0[14] == 1 && (is_chassis_resident("cr-lrp2") || ` or `is_chassis_resident("cr-lrp1"))` The flow's match was reworked to have at maximum one 'is_chassis_resident()' part. For each cr-lport a new lflow is created. There should not be many cr-lports within one datapath (normally there is just one), so the lflows count shouldn't increase dramatically. Now the match looks like: `reg0[14] == 1 && is_chassis_resident("cr-lrp2")` As an additional enhancement, the code became easier and tests were also simplified. Documentation and relevant testcases were updated. Fixes: 4e90bcf55c2e ("controller, northd, vtep: support routed networks with HW VTEP") Signed-off-by: Vladislav Odintsov <odivlad@gmail.com> --- northd/northd.c | 35 ++++++++++++++--------------------- northd/ovn-northd.8.xml | 13 +++++++------ tests/ovn.at | 17 +++-------------- 3 files changed, 24 insertions(+), 41 deletions(-)