On 22.09.2026 14:58, Dumitru Ceara wrote: > Hi Alexandra, > > I wanted to share (me, not the AI) that I like the fact that you're > documenting this interesting use case! Looking forward to v2. > > Regards, > Dumitru
Hi Dumitru! Thanks to you and your AI for the review. What do you think about renaming this from "virtual" to "replicated"? I wasn't aware that we had named those gateways "standalone" and "replicated" internally before sending the patch. Thanks! > > On 9/22/26 1:56 PM, Dumitru Ceara via dev wrote: >> NOTE: This review was produced with the help of an AI assistant. >> A human reviewer has examined the feedback below and considered it >> worth sharing. >> --- >> >> Git SHA: 9d8a8aaa3a201f2bdde3b076fdc80068516e1817 >> Author: Alexandra Rukomoinikova <[email protected]> >> Subject: tests: Document the virtual gateway setup. >> >> This patch documents the "virtual gateway" setup where several >> ovn-controller instances share a single chassis name and a single >> anycast encapsulation IP reachable over the underlay through ECMP. >> It adds an RST topic document, a NEWS entry, a note in the >> ovn-controller man page, and a multinode test that deploys two >> gateway nodes as one virtual chassis and verifies north-south >> connectivity and that the controllers do not fight over the >> shared records. >> >>> diff --git a/Documentation/topics/virtual-gateway.rst >>> b/Documentation/topics/virtual-gateway.rst >>> new file mode 100644 >>> index 000000000..7d39bfbe2 >>> --- /dev/null >>> +++ b/Documentation/topics/virtual-gateway.rst >>> [ ... ] >>> +writes that keeps them from fighting each other. >>> +Two consequences follow from this. The columns that OVN derives from the >>> +local node rather than from the configuration must be forced to the same >>> value >>> +on every node, More detailed information on this is provided in the >>> +``Identical OVS and OVN configuration and features`` section below. >> A few issues in this paragraph: >> >> 1. There is no blank line between the preceding paragraph (ending >> with "fighting each other.") and "Two consequences follow from >> this." RST will merge them into one paragraph, which is probably >> not intended. >> >> 2. "Two consequences follow from this" promises two items, but only >> one is stated (the columns must be forced to the same value). >> The second consequence is never mentioned. >> >> 3. "on every node, More detailed information" -- the comma should >> be a period (or the sentence should be restructured). >> >>> [ ... ] >>> +For the same reason, every chassis scoped configuration option has to be >>> +identical on all the nodes, including , for example: >>> ``external_ids:hostname``, >>> +``external_ids:ovn-bridge-mappings`` and all others. >> Nit: there is a spurious space before the comma: "including , for >> example" should be "including, for example". >> >>> [ ... ] >>> +Example >>> +------- >>> + >>> +The ``ovn multinode virtual gateway - anycast encap ip with underlay ECMP`` >>> +test in ``tests/multinode.at`` sets up a complete example of a two node >> The test name referenced here does not match the actual AT_SETUP. >> The test is registered as "ovn multinode virtual gateway" (without >> the "- anycast encap ip with underlay ECMP" suffix). >> >>> diff --git a/tests/multinode-macros.at b/tests/multinode-macros.at >>> index 1c85f79f8..208af60a1 100644 >>> --- a/tests/multinode-macros.at >>> +++ b/tests/multinode-macros.at >>> [ ... ] >>> -# multinode_setup_controller NODE ENCAP_IP REMOTE_IP [ENCAP_TYPE] >>> +# multinode_setup_controller NODE SYSTEM_ID ENCAP_IP REMOTE_IP [ENCAP_TYPE] >>> # >>> -# Sets up controller on specified node. >>> +# Sets up controller on specified node. SYSTEM_ID defaults to NODE; >>> passing a >>> +# different value (or the same value on more than one node) allows a test >>> to >>> +# control the chassis name the node registers with. >> The function now also takes a 6th parameter (start_controller) >> which is used by the new test (passing "no"), but the usage >> comment does not document it. Should be something like: >> >> # multinode_setup_controller NODE SYSTEM_ID ENCAP_IP REMOTE_IP >> # [ENCAP_TYPE] [START_CONTROLLER] >> >>> [ ... ] >>> m_as $c ovs-vsctl set open . >>> external-ids:ovn-bridge-datapath-type=system >>> >>> + >>> # Add back br-ex which was removed when removing ovs conf.db >> Nit: spurious blank line added (two blank lines between the >> external-ids block and the comment). >> >>> diff --git a/tests/multinode.at b/tests/multinode.at >>> index 0c277f5e8..eb6948d5a 100644 >>> --- a/tests/multinode.at >>> +++ b/tests/multinode.at >>> @@ -9,7 +9,7 @@ check_fake_multinode_setup >>> cleanup_multinode_resources >>> >>> # Test East-West switching >>> -check multinode_nbctl ls-add sw0 >>> + >> This removes the "ls-add sw0" from the first test ("ovn multinode >> basic test"), but the very next line is still "lsp-add sw0 >> sw0-port1" which requires sw0 to already exist. Was this deletion >> intentional? It looks like it will break the first test. >> >>> [ ... ] >>> +# On ovn-chassis-2 (aka client) we have client port in public switch for >>> checking >>> +# internal connectivity, public switch is connected to dgp port that >>> claimed by >>> +# virtual chassis. Compute and client node both have underlay route for >>> virtul gw ip (1.1.1.1) >> Nit: "virtul" should be "virtual". >> >>> [ ... ] >>> +# The distributed gateway port is claimed by the virtual gateway chassis. >>> +m_wait_row_count Port_Binding 1 logical_port=cr-lr0-public >>> +vgw_uuid=$(m_central_as ovn-sbctl --bare --columns _uuid find Chassis >>> name=$vgw_name) >>> +m_wait_column "$vgw_uuid" Port_Binding chassis logical_port=cr-lr0-public >>> + >>> +check multinode_nbctl --wait=hv sync >>> +m_wait_for_ports_up sw0-port1 >>> + >>> +# The distributed gateway port is claimed by the virtual gateway chassis. >>> +m_wait_row_count Port_Binding 1 logical_port=cr-lr0-public >>> +vgw_uuid=$(m_central_as ovn-sbctl --bare --columns _uuid find Chassis >>> name=$vgw_name) >>> +m_wait_column "$vgw_uuid" Port_Binding chassis logical_port=cr-lr0-public >> This block is duplicated. The "distributed gateway port is claimed" >> check, the sync, the wait-for-ports-up and the second identical >> block look like an accidental copy-paste. Should one of these be >> removed? >> >>> [ ... ] >>> +for c in $vgw1 $vgw2; do >>> + multinode_setup_controller $c $vgw_name $vgw_encap_ip $central_ip >>> geneve no >>> + on_exit "m_as $c ip addr del $vgw_encap_ip/32 dev lo" >>> + check m_as $c ip addr add $vgw_encap_ip/32 dev lo >>> + check m_as $c ovs-vsctl set open . external-ids:hostname=$vgw_name >>> + check m_as $c ovs-vsctl set open . >>> external-ids:ovn-bridge-mappings=public:br-ex >>> +done >>> + >>> +for c in $vgw1 $vgw2; do >>> + m_as $c /usr/share/ovn/scripts/ovn-ctl start_controller >>> ${CONTROLLER_SSL_ARGS} >>> +done >> The multinode_setup_controller call with start_controller="no" >> sets up OVS and all the external-ids but does not start >> ovn-controller. The controller is then started in a second loop >> after all external-ids (hostname, bridge-mappings) are set. This >> is a good pattern -- it avoids a race where one controller starts >> and writes the Chassis record before the other has finished >> configuring its bridge-mappings. >> >>> [ ... ] >>> +# Restore the original chassis configuration of the nodes used by this >>> test. >>> +# Connect the chassis back to the original northd and remove northd per >>> chassis. >>> +for i in 1 2; do >>> + chassis="ovn-chassis-$i" >>> + ip=$(m_as $chassis ip -4 addr show eth1 | grep inet | awk '{print $2}' >>> | cut -d'/' -f1) >>> + >>> + multinode_cleanup_ic $chassis >>> + multinode_setup_controller $chassis $chassis $ip "170.168.0.2" >>> + multinode_cleanup_northd $chassis >>> +done >> The cleanup restores ovn-chassis-1 and ovn-chassis-2, but the >> two gateway nodes (ovn-gw-1 and ovn-gw-2) that were reconfigured >> with the anycast system-id are not restored to their original >> configuration. Subsequent tests in the same file that rely on the >> gw nodes having their default system-id might be affected. Is >> AT_CLEANUP sufficient to handle this, or should the gw nodes be >> restored here as well? >> >> >> Supplementary findings from full review of 9d8a8aaa3. >> >> The preliminary review noted the deletion of "check multinode_nbctl >> ls-add sw0" from the first test ("ovn multinode basic test") as >> suspicious. Build and closer inspection confirm this is a real bug. >> >> The hunk at the top of tests/multinode.at removes the only ls-add >> for sw0: >> >> -check multinode_nbctl ls-add sw0 >> + >> >> The very next line is: >> >> check multinode_nbctl lsp-add sw0 sw0-port1 >> >> Since cleanup_multinode_resources() deletes the NB database and >> restarts northd, no switch named sw0 exists at this point. The >> lsp-add call requires the switch to already exist and will fail. >> This breaks the first multinode test ("ovn multinode basic test"). >> >> This deletion appears to be an accidental part of the patch; the >> ls-add was not related to the virtual gateway feature and should be >> restored. >> >> _______________________________________________ >> 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
