On Tue, Jul 4, 2017 at 10:56 PM, Mickey Spiegel <[email protected]> wrote: > > > On Tue, Jun 27, 2017 at 10:42 AM, Han Zhou <[email protected]> wrote: >> >> >> >> On Tue, Jun 27, 2017 at 10:40 AM, Han Zhou <[email protected]> wrote: >> > >> > >> > >> > On Tue, Jun 27, 2017 at 10:12 AM, Mickey Spiegel <[email protected]> wrote: >> > > >> > > >> > > On Tue, Jun 27, 2017 at 1:02 AM, Han Zhou <[email protected]> wrote: >> > >> >> > >> Localnet port was supposed to work for directly connected datapath >> > >> only. However, the recursive local_datapath filling introduced a >> > >> problem in below scenario: >> > >> >> > >> LS A <-> LR <-> LS B, port a@HV1 is on LS A, port b@HV2 is on LS B. >> > >> If B has localnet port, then ovn-controller on HV1 would think port >> > >> b can be reached from HV1 by localnet port on LS B, which is wrong. >> > > >> > > >> > > This scenario is flawed. Logical Routers should only be connected to Logical Switches with localnet ports through gateway routers or distributed gateway ports. Distributed gateway port logic will cause traffic to be emitted from an appropriate hypervisor. It is designed to work with logical switches with localnet ports, whereas normal router ports on distributed logical routers were not. >> > > >> > >> >> > >> This patch fixes it by adding hops information in local datapath >> > >> which can tell if a local-datapath is directly connected to the >> > >> hypervisor. >> > > >> > > >> > > I have not run any tests or tried it, but I think this patch breaks distributed gateway ports. We need to use the localnet port to get to the outside world from the distributed gateway port. There is no problem with the existing localnet port logic, when it is used as designed. >> > > >> > >> > Hi Mickey, thanks for your comments! I agree the scenario is not real use case, but I think it is still a bug which is revealed in such "flawed" scenario, and the result is misleading, regarding the discussion [1]. So I think it is still worth to be fixed. >> > >> It went out too fast: [1] https://mail.openvswitch.org/pipermail/ovs-dev/2017-June/334634.html >> > >> > This patch fixes only the "flaw" scenario, and I don't think it breaks the distributed gateway scenario. > > > I still believe that it breaks distributed gateway port functionality. See more explanation below. > >> >> The patch is just to make sure a remote port can be reached by "localnet" port that is directly connected to the current HV. The logic is in the context of how to reach a *remote port*. > > > I still feel that you are trying to fix an unsupported scenario. Moreover, I don't follow what you are trying to do.
Yes, I am fixing an unsupported scenario, because when user configure it this way, the localnet will be abused, and the behavior could be confusing. This fix is just to make sure the localnet port works in the old and safe way. Maybe this is not the good way to fix, and a better way might be detecting such configuration (localnet port and non-gateway LR port on the same LS) in northd and write a warning log. I was also thinking about abandoning this patch because I think connecting logical switches with localnet ports by distributed LR could be useful in some special case, but since that case would require some more changes and there are open issues, I'd rather discuss it separately, independent of this patch. > It seems like you are making it like OVN does not have any localnets at all, sending a Geneve tunneled packet from one side to the other (since the sender HV1 does not see the localnet for LS B). It is not using the non-OVN VXLAN, unless the underlying transport between HV1 and HV2 uses non-OVN VXLAN, in which case it would be Geneve over VXLAN. > >> And the distributed gateway test cases are passed: >> > >> > OVN end-to-end tests >> > >> > 2336: ovn -- 1 LR with distributed router gateway port ok >> > 2337: ovn -- send gratuitous arp for NAT rules on distributed router ok >> > >> >> Please let me know if I misunderstood anything, or the tests are not complete. > > > The tests are not complete. While there are some multi-node tests in tests/ovn.at in the "1 LR with distributed router gateway port" test case, they do not test the NAT functionality. In that existing test case, without NAT, all north/south traffic goes through the redirect-chassis which will trigger localnet even with your patch. > > Where I think your patch will break things is for distributed NAT rules, where the traffic can go out of the local instance of the distributed gateway port even on a chassis that is not the redirect-chassis. As in the "1 LR with distributed router gateway port" test, the logical topology is: > foo -- R1 -- distributed gateway port -- alice > The physical topology is: > hv1 hosts vif foo1 on LS foo. > hv2 is the redirect-chassis for the distributed gateway port. > There is a distributed NAT rule for foo1. > When foo1 sends traffic to alice1 on hv3, on hv1 it goes > foo -- R1 -- distributed gateway port -- alice -- localnet > The localnet will then get the traffic to hv3 on LS alice, so that it can reach the destination alice1. Sounds right. But I wonder if it is a realistic scenario or just another scenario that we should not support. I think here alice is to model the external physical network which is on the other side of the gateway, so it seems not quite reasonable that a logical port alice1 on alice would be the destination from foo. In the gateway scenario, wouldn't the alice LS be just for connecting the outside world? > Similarly, traffic to the outside world will use > foo -- R1 -- distributed gateway port -- alice -- localnet > all on hv1. Sorry I have different understanding here. For connecting the outside world, it is the port-binding of the localnet@alice that matters. When localnet@alice is processed, the physical flows for that localnet port are added. This patch doesn't change that behavior, because alice still belongs to local-datapath on HV1, and the localnet@alice port-binding will still be processed on HV1. > In both cases the localnet is needed on hv1, but the depth of alice on hv1 is 2. > I was testing these scenarios manually while developing the feature. > > When the distributed gateway port functionality was developed, NAT could only be tested using the kernel module, i.e. "make check-kernel" or "make check-kmod". However, this restricts the tests to be single node tests which will not catch the issue introduced by your patch. Userspace NAT was committed on June 2, but I do not know whether that feature allows for multi-node NAT tests to be developed. If it does, then an automated version of the scenario described above should be developed. It is pretty much the topology of the "1 LR with distributed router gateway port" test case, mixed with the "DNAT and SNAT on distributed router" test cases from tests/ system-ovn.at. > > Mickey > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
