On 6/10/20 3:00 PM, Han Zhou wrote:
Support a new logical router option "learn_from_arp_request" that controls
behavior when handling ARP requests or IPv4 ND-NS packets.
"true" - Always learn the MAC/IP binding and add a new MAC_Binding entry
(default behavior)
"false" - If there is a MAC_binding for that IP and the MAC is different, or,
if TPA of ARP request belongs to any router port on this router, then
update/add that MAC/IP binding. Otherwise, don't update/add entries.
Hi Han,
The "learn_from_arp_requests" option is not documented in this patch.
The option values are misleading. As a user, I would expect that setting
"learn_from_arp_requests" to false would mean *never* learning from ARP
requests. Perhaps instead of "true/false" this could be a string such as
"always/local_only" that better indicates what the option does.
Reported-by: Girish Moodalbail <gmoodalb...@nvidia.com>
Reported-at:
https://mail.openvswitch.org/pipermail/ovs-discuss/2020-May/049995.html
Signed-off-by: Han Zhou <hz...@ovn.org>
---
northd/ovn-northd.c | 78 ++++++++++++++++++++++++++++++++++++++++++++---------
tests/ovn.at | 18 +++++++++++++
2 files changed, 84 insertions(+), 12 deletions(-)
diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
index 9a4e884..8a1f490 100644
--- a/northd/ovn-northd.c
+++ b/northd/ovn-northd.c
@@ -219,6 +219,7 @@ enum ovn_stage {
/* Register to store the result of check_pkt_larger action. */
#define REGBIT_PKT_LARGER "reg9[1]"
#define REGBIT_LOOKUP_NEIGHBOR_RESULT "reg9[2]"
+#define REGBIT_LOOKUP_NEIGHBOR_IP_RESULT "reg9[3]"
/* Register for ECMP bucket selection. */
#define REG_ECMP_GROUP_ID "reg8[0..15]"
@@ -7964,18 +7965,33 @@ build_lrouter_flows(struct hmap *datapaths, struct hmap
*ports,
* */
/* Flows for LOOKUP_NEIGHBOR. */
+ bool learn_from_arp_request = smap_get_bool(&od->nbr->options,
+ "learn_from_arp_request",
+ true);
+ ds_clear(&actions);
+ ds_put_format(&actions, REGBIT_LOOKUP_NEIGHBOR_RESULT
+ " = lookup_arp(inport, arp.spa, arp.sha); %snext;",
+ learn_from_arp_request? "":
+ REGBIT_LOOKUP_NEIGHBOR_IP_RESULT" = 1; ");
ovn_lflow_add(lflows, od, S_ROUTER_IN_LOOKUP_NEIGHBOR, 100,
- "arp.op == 2",
- REGBIT_LOOKUP_NEIGHBOR_RESULT" = "
- "lookup_arp(inport, arp.spa, arp.sha); next;");
+ "arp.op == 2", ds_cstr(&actions));
+ ds_clear(&actions);
+ ds_put_format(&actions, REGBIT_LOOKUP_NEIGHBOR_RESULT
+ " = lookup_nd(inport, nd.target, nd.tll); %snext;",
+ learn_from_arp_request? "":
Minor nitpick, but coding guidelines suggest putting a space on either
side of the "?" and ":" operators.
+ REGBIT_LOOKUP_NEIGHBOR_IP_RESULT" = 1; ");
ovn_lflow_add(lflows, od, S_ROUTER_IN_LOOKUP_NEIGHBOR, 100, "nd_na",
- REGBIT_LOOKUP_NEIGHBOR_RESULT" = "
- "lookup_nd(inport, nd.target, nd.tll); next;");
+ ds_cstr(&actions));
+ ds_clear(&actions);
+ ds_put_format(&actions, REGBIT_LOOKUP_NEIGHBOR_RESULT
+ " = lookup_nd(inport, ip6.src, nd.sll); %snext;",
+ learn_from_arp_request? "":
+ REGBIT_LOOKUP_NEIGHBOR_IP_RESULT
+ " = lookup_nd_ip(inport, ip6.src); ");
ovn_lflow_add(lflows, od, S_ROUTER_IN_LOOKUP_NEIGHBOR, 100, "nd_ns",
- REGBIT_LOOKUP_NEIGHBOR_RESULT" = "
- "lookup_nd(inport, ip6.src, nd.sll); next;");
+ ds_cstr(&actions));
/* For other packet types, we can skip neighbor learning.
* So set REGBIT_LOOKUP_NEIGHBOR_RESULT to 1. */
@@ -7984,8 +8000,12 @@ build_lrouter_flows(struct hmap *datapaths, struct hmap
*ports,
/* Flows for LEARN_NEIGHBOR. */
/* Skip Neighbor learning if not required. */
+ ds_clear(&match);
+ ds_put_format(&match, REGBIT_LOOKUP_NEIGHBOR_RESULT" == 1%s",
+ learn_from_arp_request? "":
+ " || "REGBIT_LOOKUP_NEIGHBOR_IP_RESULT" == 0");
ovn_lflow_add(lflows, od, S_ROUTER_IN_LEARN_NEIGHBOR, 100,
- REGBIT_LOOKUP_NEIGHBOR_RESULT" == 1", "next;");
+ ds_cstr(&match), "next;");
ovn_lflow_add(lflows, od, S_ROUTER_IN_LEARN_NEIGHBOR, 90,
"arp", "put_arp(inport, arp.spa, arp.sha); next;");
@@ -8002,8 +8022,38 @@ build_lrouter_flows(struct hmap *datapaths, struct hmap
*ports,
continue;
}
+ bool learn_from_arp_request = smap_get_bool(&op->od->nbr->options,
+ "learn_from_arp_request",
+ true);
+
/* Check if we need to learn mac-binding from ARP requests. */
for (int i = 0; i < op->lrp_networks.n_ipv4_addrs; i++) {
+ if (!learn_from_arp_request) {
+ /* ARP request to this address should always get learned,
+ * so add a priority-110 flow to set
+ * REGBIT_LOOKUP_NEIGHBOR_IP_RESULT to 1. */
+ ds_clear(&match);
+ ds_put_format(&match,
+ "inport == %s && arp.spa == %s/%u && "
+ "arp.tpa == %s && arp.op == 1",
+ op->json_key,
+ op->lrp_networks.ipv4_addrs[i].network_s,
+ op->lrp_networks.ipv4_addrs[i].plen,
+ op->lrp_networks.ipv4_addrs[i].addr_s);
+ if (op->od->l3dgw_port && op == op->od->l3dgw_port
+ && op->od->l3redirect_port) {
+ ds_put_format(&match, " && is_chassis_resident(%s)",
+ op->od->l3redirect_port->json_key);
+ }
+ const char *actions_s = REGBIT_LOOKUP_NEIGHBOR_RESULT
+ " = lookup_arp(inport, arp.spa, arp.sha); "
+ REGBIT_LOOKUP_NEIGHBOR_IP_RESULT" = 1;"
+ " next;";
+ ovn_lflow_add_with_hint(lflows, op->od,
+ S_ROUTER_IN_LOOKUP_NEIGHBOR, 110,
+ ds_cstr(&match), actions_s,
+ &op->nbrp->header_);
+ }
ds_clear(&match);
ds_put_format(&match,
"inport == %s && arp.spa == %s/%u && arp.op == 1",
@@ -8015,12 +8065,16 @@ build_lrouter_flows(struct hmap *datapaths, struct hmap
*ports,
ds_put_format(&match, " && is_chassis_resident(%s)",
op->od->l3redirect_port->json_key);
}
+ ds_clear(&actions);
+ ds_put_format(&actions, REGBIT_LOOKUP_NEIGHBOR_RESULT
+ " = lookup_arp(inport, arp.spa, arp.sha); %snext;",
+ learn_from_arp_request? "":
+ REGBIT_LOOKUP_NEIGHBOR_IP_RESULT
+ " = lookup_arp_ip(inport, arp.spa); ");
ovn_lflow_add_with_hint(lflows, op->od,
S_ROUTER_IN_LOOKUP_NEIGHBOR, 100,
- ds_cstr(&match),
- REGBIT_LOOKUP_NEIGHBOR_RESULT" = "
- "lookup_arp(inport, arp.spa, arp.sha); "
- "next;", &op->nbrp->header_);
+ ds_cstr(&match), ds_cstr(&actions),
+ &op->nbrp->header_);
}
}
diff --git a/tests/ovn.at b/tests/ovn.at
index 12849b4..c7e1cdc 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -3967,6 +3967,18 @@ ip_to_hex() {
sha=f00000000011
spa=`ip_to_hex 192 168 1 100`
tpa=$spa
+
+# When learn_from_arp_request=false, the new mac-binding will not be learned
+# through GARP request.
+ovn-nbctl --wait=hv set logical_router lr0 options:learn_from_arp_request=false
+
+test_arp 11 $sha $spa $tpa
+sleep 1
+AT_CHECK([ovn-sbctl find mac_binding ip="192.168.1.100"], [0], [])
+
+# When learn_from_arp_request=true, the new mac-binding will be learned.
+ovn-nbctl --wait=hv set logical_router lr0 options:learn_from_arp_request=true
+
test_arp 11 $sha $spa $tpa
OVS_WAIT_UNTIL([test `ovn-sbctl find mac_binding ip="192.168.1.100" | wc -l`
-gt 0])
ovn-nbctl --wait=hv sync
@@ -3981,6 +3993,12 @@ test_ip 21 $smac $dmac $sip $dip 11
# lp12 send GARP request to announce ownership of 192.168.1.100.
+# Even when learn_from_arp_request=false, the existing mac-binding should be
+# updated through GARP request.
+ovn-nbctl --wait=hv set logical_router lr0 options:learn_from_arp_request=false
+ovn-sbctl lflow-list
+as hv2 ovs-ofctl dump-flows br-int
+
sha=f00000000012
test_arp 12 $sha $spa $tpa
OVS_WAIT_UNTIL([ovn-sbctl find mac_binding ip="192.168.1.100" | grep
f0:00:00:00:00:12])
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
