Hello Numan Thanks for review and feedback.
On Fri, May 3, 2024 at 7:14 PM Numan Siddique <[email protected]> wrote: > On Mon, Apr 22, 2024 at 2:54 AM Vasyl Saienko <[email protected]> > wrote: > > > > Reply only if target ethernet address is broadcast, if > > address is specified explicitly do noting to let target > > reply by itself. This technique allows to monitor target > > aliveness with arping. > > Thanks for the patch. > > This patch does change the behavior of OVN. Having ARP responder > flows avoids tunnelling the request packet if the destination is on a > different compute node. > But I don't think its a big deal. > > You are totally correct that the ARP responder allows limiting ARP broadcast traffic between nodes. Normally ARP requests are sent to broadcast ff:ff:ff:ff:ff:ff ethernet, as the protocol is designed to identify ethernet addresses for known IP addresses. In this case since traffic is broadcast it will be flooded among all nodes if ARP responder is not applied. At the same time the client may specify the target ethernet address as unicast (in this case a broadcast storm will not happen, as the destination ethernet address is unicast). In OpenStack we use unicast ARP requests as a way to monitor VM aliveness from remote locations to make sure there are no issues with flows/underlying networking infra between nodes. We use ARPs due to several reasons: 1. This protocol is mandatory and can't be disabled on the target machine (which guarantees that the target VM will always reply to ARPs if it is alive) 2. This protocol is not filtered by firewalls/security groups as it is mandatory for network workability 3. We can't use upper layers protocols such as ICMP as they will require specific firewall rules inside VMs, and we do not control VMs in cloud cases, but still need to monitor infrastructure aliveness. If ARP responder replies to requests with broadcast and unicast target ethernet address, we can't use this technique for monitoring, as even if target VM is down (not necessary that ovn port is down, the VM may stuck or be in kernel panic for example, or datapath between monitoring tool and VM is broken) the ARP responder will do reply to our request so we can't identify if VM is really accessible or not as we will always got replies from local ARP responder. At the same time there is no need to set up ARP responder for requests with a unicast ethernet address, as they will not generate broadcast storms by design (they are unicasts). > 1. Accept your patch > 2. Use the NB Global option - "ignore_lsp_down" and set it to false, > so that ovn-northd will install the ARP responder flows only if the > logical port is up. > This option will not allow catching a situation when there is an issue in datapath between monitoring software and VM located on another node, for example issues with flows or some underlying networking problems. Have you considered this approach for your use case ? > 3. Add another global option - "disable_arp_responder" which when set > to true, ovn-northd will not install ARP responder flows. > > Disabling ARP responder for ports completely will allow broadcast storms for ARP requests. Which we would like to avoid, but enable the possibility to monitor infrastructure aliveness. > Given that OpenStack neutron ml2ovs uses ARP responder flows only for > broad cast my preference would be (1). > > I'd like to know other's opinion on this. > > Thanks > Numan > > > > > Closes #239 > > > > Signed-off-by: Vasyl Saienko <[email protected]> > > --- > > northd/northd.c | 11 +++++++++-- > > tests/ovn-northd.at | 4 ++-- > > 2 files changed, 11 insertions(+), 4 deletions(-) > > > > diff --git a/northd/northd.c b/northd/northd.c > > index 37f443e70..e80e1885d 100644 > > --- a/northd/northd.c > > +++ b/northd/northd.c > > @@ -8832,8 +8832,15 @@ build_lswitch_arp_nd_responder_known_ips(struct > ovn_port *op, > > for (size_t i = 0; i < op->n_lsp_addrs; i++) { > > for (size_t j = 0; j < op->lsp_addrs[i].n_ipv4_addrs; j++) { > > ds_clear(match); > > - ds_put_format(match, "arp.tpa == %s && arp.op == 1", > > - op->lsp_addrs[i].ipv4_addrs[j].addr_s); > > + /* NOTE(vsaienko): Do not reply on unicast ARPs, forward > > + * them to the target to have ability to monitor target > > + * aliveness via ARPs. > > + */ > > + ds_put_format(match, > > + "arp.tpa == %s && " > > + "arp.op == 1 && " > > + "eth.dst == ff:ff:ff:ff:ff:ff", > > + op->lsp_addrs[i].ipv4_addrs[j].addr_s); > > ds_clear(actions); > > ds_put_format(actions, > > "eth.dst = eth.src; " > > diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at > > index be006fb32..4162196f4 100644 > > --- a/tests/ovn-northd.at > > +++ b/tests/ovn-northd.at > > @@ -9283,9 +9283,9 @@ AT_CAPTURE_FILE([S1flows]) > > > > AT_CHECK([grep -e "ls_in_arp_rsp" S1flows | ovn_strip_lflows], [0], [dnl > > table=??(ls_in_arp_rsp ), priority=0 , match=(1), > action=(next;) > > - table=??(ls_in_arp_rsp ), priority=100 , match=(arp.tpa == > 192.168.0.10 && arp.op == 1 && inport == "S1-vm1"), action=(next;) > > + table=??(ls_in_arp_rsp ), priority=100 , match=(arp.tpa == > 192.168.0.10 && arp.op == 1 && eth.dst == ff:ff:ff:ff:ff:ff && inport == > "S1-vm1"), action=(next;) > > table=??(ls_in_arp_rsp ), priority=100 , match=(nd_ns && > ip6.dst == {fd00::10, ff02::1:ff00:10} && nd.target == fd00::10 && inport > == "S1-vm1"), action=(next;) > > - table=??(ls_in_arp_rsp ), priority=50 , match=(arp.tpa == > 192.168.0.10 && arp.op == 1), action=(eth.dst = eth.src; eth.src = > 50:54:00:00:00:10; arp.op = 2; /* ARP reply */ arp.tha = arp.sha; arp.sha = > 50:54:00:00:00:10; arp.tpa = arp.spa; arp.spa = 192.168.0.10; outport = > inport; flags.loopback = 1; output;) > > + table=??(ls_in_arp_rsp ), priority=50 , match=(arp.tpa == > 192.168.0.10 && arp.op == 1 && eth.dst == ff:ff:ff:ff:ff:ff), > action=(eth.dst = eth.src; eth.src = 50:54:00:00:00:10; arp.op = 2; /* ARP > reply */ arp.tha = arp.sha; arp.sha = 50:54:00:00:00:10; arp.tpa = arp.spa; > arp.spa = 192.168.0.10; outport = inport; flags.loopback = 1; output;) > > table=??(ls_in_arp_rsp ), priority=50 , match=(nd_ns && > ip6.dst == {fd00::10, ff02::1:ff00:10} && nd.target == fd00::10), > action=(nd_na { eth.src = 50:54:00:00:00:10; ip6.src = fd00::10; nd.target > = fd00::10; nd.tll = 50:54:00:00:00:10; outport = inport; flags.loopback = > 1; output; };) > > ]) > > > > -- > > 2.40.1 > > > > _______________________________________________ > > dev mailing list > > [email protected] > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > > -- <https://www.mirantis.com/> Vasyl Saienko Principal DevOps Engineer [email protected] <[email protected]> +(380) 66 072 07 17 <++1+(650)+564+7038> _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
