On Tue, Mar 29, 2022 at 8:47 PM Ihar Hrachyshka <[email protected]> wrote: > > This version of the series switched to supporting multiple chassis set in > requested-chassis option. This allows for more than two chassis > specified for the same port. > > Also, this series includes a patch that allows to disable tunneling > enforcement for ports with multiple chassis set in requested-chassis. > (This is useful when tunneling network is MTU constrained.) > > v0: initial draft (single patch) > v1: split into pieces > v1: renamed options: migration-destination -> > requested-additional-chassis, > migration-unblocked -> > additional-chassis-activated > v1: introduced options:activation-strategy=rarp to allow for other > strategies / having default no-op strategy > v1: implement in-memory port-activated tracking to avoid races > v1: numerous code cleanup / bug fixes > v1: special handling for localnet attached switches > v2: added ddlog implementation > v2: re-inject RARP packet after vswitch updates flows > v3: re-sent as a single series > v4: redesign to reuse requested-chassis option > v4: support >2 chassis per port > v4: allow to disable tunneling enforcement when n_chassis >= 2 > > Ihar Hrachyshka (15): > Introduce chassis_is_vtep > northd: introduce separate function to look up chassis > northd: separate code for nb->sb port binding chassis update > Pass chassis and encap into get_port_binding_tun > Introduce match_outport_dp_and_port_keys in physical.c > Split code to set zone info into put_zones_ofpacts > Use get_port_binding_tun instead of chassis_tunnel_find > Tag all packets that arrived from a tunnel as LOCAL_ONLY > Update port-up on main chassis only > Support LSP:options:requested-chassis as a list > Clone packets to both port chassis > Implement RARP activation strategy for ports > Reinject RARP packet when activation-strategy=rarp > Allow to disable tunneling enforcement for multi-chassis port > Update NEWS file about new live migration options
Hi Ihar, Thanks for the patch series. I have a few comments. 1) Do you think we really need to support port_binding multiple encaps for this use case ? If you see this commit - https://github.com/ovn-org/ovn/commit/dd527a283cd8b67ca481374302a630f5635de6ac encaps for port binding was added for a specific use case. If CMS desires to configure multiple encaps for a logical port, then I think its Ok to not support multiple requested-chassis for such ports. What do you think ? Do you see any use case for this in openstack ? IMO supporting multiple encaps is complicating the code in binding.c and I'd prefer if we can avoid it. 2) In patch 12 (RARP activation strategy), ovn-controller is programming OF flows in logical flow space (i.e table 8). I think it's better to avoid it. Instead I'd suggest adding these flows in the new physical table - table 73 (see lflow.h). You can modify the flow in table 0 to resubmit to table 73 if the ovs interface port is created on additional chassis. And table 73 can have a controller action for rarp packet and a generic drop flow. 2.a ) In the proposed patch series, pinctrl thread when it receives a packet due to rarp, it deletes the flows. Instead I'd suggest doing the below. In table 73, add a flow with the match = rarp and action = controller(..), learn(...) The first action will send the packet to the controller. When the pinctrl thread receives it, it can tell the main thread to set the option additional-chassis-activated for the port binding. The second OF action "learn" can add a new higher priority flow to resubmit the packets to table 8. This way, pinctrl thread doesn't need to modify or delete any openflows when it receives a rarp activation packet. What do you think ? One use case of learn which ovn-controller uses - https://github.com/ovn-org/ovn/blob/main/controller/lflow.c#L1752 Also I don't think there is a need to set the "pause" flag for the OVN action - activation_strategy_rarp Do you see any problem with this ? 3) Patch 11 (Clone packets to both port chassis) is adding flows in table 38 to tunnel the packet to other requested chassis. Table 38 is meant for local output and I think it should not do this. Table 37 is meant for tunnelling. If suppose a logical port has 2 requested chassis - ch1 and ch2, and if there is ovs port for this logical port on both ch1 and ch2, then I'd suggest adding/modifying the flow in table 37 to tunnel to the other requested chassis and then do a resubmit to 38. These are the comments I had at this point. Let me know what you think. Thanks Numan > > NEWS | 3 + > controller/binding.c | 284 ++++++++-- > controller/binding.h | 5 + > controller/if-status.c | 15 +- > controller/if-status.h | 1 + > controller/lport.c | 42 +- > controller/ovn-controller.c | 4 +- > controller/physical.c | 450 ++++++++++++---- > controller/pinctrl.c | 293 +++++++++- > controller/pinctrl.h | 5 + > include/ovn/actions.h | 9 + > lib/actions.c | 40 +- > northd/northd.c | 109 +++- > northd/ovn-northd.c | 7 +- > ovn-nb.xml | 25 +- > ovn-sb.ovsschema | 19 +- > ovn-sb.xml | 73 ++- > tests/ovn.at | 1000 +++++++++++++++++++++++++++++++++++ > utilities/ovn-trace.c | 3 + > 19 files changed, 2166 insertions(+), 221 deletions(-) > > -- > 2.34.1 > > _______________________________________________ > dev mailing list > [email protected] > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
