On 9/22/26 5:59 PM, Rukomoinikova Aleksandra wrote: > 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. >
Hi Alexandra, > 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. > Replicated might be better indeed. It expresses your use case more accurately I guess. Regards, Dumitru > 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
